-
Notifications
You must be signed in to change notification settings - Fork 437
# Security: Add URL validation to prevent SSRF in HTTPRequestManager #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| if (hostPart == "localhost" || | ||
| hostPart == "127.0.0.1" || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i hit 127.0.0.2 and the entire function is bypassed
| } | ||
|
|
||
| if (hostPart == "metadata.google.internal" || | ||
| hostPart == "169.254.169.254") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should block the entire /16 range
|
|
||
| if (hostPart == "localhost" || | ||
| hostPart == "127.0.0.1" || | ||
| hostPart == "::1" || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
many many many ways to hit ipv6 loopback without explicitly typing ::1, you want to reverse this and only allow the 2000::/3 range
|
you're not blocking 0.0.0.0/8 either |
Updated error message for blocked URLs in HTTP request validation.
Added utility functions for URL decoding, IP normalization, and host extraction. Improved URL validation by integrating IP checks and refactoring existing logic.
Hey thank you for the comments, added a lot more checks to prevent edge cases. But this is still largely following a blacklisting approach, not ideal. But in this case it is a since it is a framework used by many applications, not a single app. And applications might need to call diverse or unpredictable APIs. So a whitelist would require every application to pre configure allowed domains, which is I don't think is entirely practical. For specific applications with known endpoints they would have to implement their own whitelist approach. But this fix would provide a strong default security by blocking known SSRF vectors. Please let me know if there are any other edge cases that you found. |
Summary
This PR fixes a Server-Side Request Forgery (SSRF) vulnerability in Valdi's HTTP client implementation. The vulnerability allows applications built with Valdi to make HTTP requests to arbitrary URLs, including internal/localhost services, cloud metadata endpoints, and private network resources, without any URL validation.
Vulnerability Details
Type: Server-Side Request Forgery (SSRF)
Severity: High
Component:
HTTPRequestManagerModuleFactory.cppRoot Cause
The vulnerability exists because user-provided URLs from JavaScript/TypeScript are passed directly to native HTTP request managers without any validation. The vulnerable code flow:
TypeScript Layer (
HTTPClient.ts): User input flows intoHTTPClient.perform()methodmakeURL()function:HTTPClient.ts:7-22Native Module Binding (
HTTPRequestManagerModuleFactory.cpp:50-51): URL extracted directly:Request Execution (
HTTPRequestManagerModuleFactory.cpp:94-97): URL passed directly to platform HTTP libraries:Missing Security Controls
The framework lacks:
Impact
Critical Impact: Cloud Metadata Service Access
http://169.254.169.254/latest/meta-data/iam/security-credentials/High Impact: Internal Service Access
http://localhost:8080,http://127.0.0.1:3306http://192.168.1.1/admin,http://10.0.0.1:8080Medium-High Impact: Network Reconnaissance
Affected Applications
All applications built with Valdi that use the
valdi_httpmodule are vulnerable, including:Solution
This PR adds URL validation to prevent SSRF attacks by:
Adding
isUrlAllowed()function inHTTPRequestManagerUtilsthat validates URLs before requests are madeBlocking dangerous URLs:
localhost,127.0.0.1,::1)10.0.0.0/8,172.16.0.0/12,192.168.0.0/16)169.254.169.254,metadata.google.internal)http://andhttps://)Integrating validation into
HTTPRequestManagerModuleFactory::loadModule()to validate URLs before making requestsChanges Made
Files Modified
valdi/src/valdi/runtime/Utils/HTTPRequestManagerUtils.hppisUrlAllowed()static method declarationStringBoxvaldi/src/valdi/runtime/Utils/HTTPRequestManagerUtils.cppisUrlAllowed()function with comprehensive URL validationsrc/valdi_modules/src/cpp/valdi_http/HTTPRequestManagerModuleFactory.cppCode Changes
Before (Vulnerable):
After (Fixed):
Testing
The validation function blocks:
http://localhost:8080- Blockedhttp://127.0.0.1:3306- Blockedhttp://192.168.1.1/admin- Blockedhttp://10.0.0.1:8080- Blockedhttp://169.254.169.254/latest/meta-data/- Blockedhttp://metadata.google.internal/...- Blockedfile:///etc/passwd- Blocked (non-HTTP scheme)https://example.com- Allowedhttp://api.example.com/v1/data- AllowedSecurity Considerations
References
HTTPRequestManagerModuleFactory.cpp:50-97Responsible Disclosure
This vulnerability was discovered during security research. This PR provides a fix while maintaining backward compatibility for legitimate use cases. I reported through hackerone and I was asked to open a PR for this by bugtriage-jay as this is not covered under Snapchat's bug bounty scope.
Note: This is a security fix addressing a framework-level vulnerability that affects all Valdi applications using the HTTP client.
Fixes #63