fix: disallow SSRF via remote $ref #2224
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (connection instanceof HttpsURLConnection) { | ||
| final HttpsURLConnection httpsConnection = (HttpsURLConnection) connection; | ||
| httpsConnection.setSSLSocketFactory(sf); | ||
| httpsConnection.setHostnameVerifier(trustAllNames); |
Check warning
Code scanning / CodeQL
Unsafe hostname verification Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
The best way to fix the problem is to remove the insecure HostnameVerifier that returns true and instead use the default hostname verifier provided by the JVM, ensuring proper hostname validation. If you must customize the verifier, it should fully validate the hostname according to RFC 2818 and relevant standards (e.g., by using Java's built-in HttpsURLConnection.getDefaultHostnameVerifier()).
In RemoteUrl.java:
- Remove or replace the
trustAllNamesverifier. - Use
HttpsURLConnection.getDefaultHostnameVerifier()when configuring HTTPS connections, even if theTRUST_ALLsystem property is set. - If certificate trust needs to be loosened for particular use-cases (testing), this should not disable hostname verification.
- The actual code change is in the block that configures HttpsURLConnection in
createConnectionConfigurator()to avoid the insecure verifier.
No changes are required in RemoteUrlTest.java, as the test logic does not reference custom hostname verifiers directly.
| @@ -67,14 +67,14 @@ | ||
| sc.init(null, trustAllCerts, new java.security.SecureRandom()); | ||
| final SSLSocketFactory sf = sc.getSocketFactory(); | ||
|
|
||
| // Create all-trusting host name verifier | ||
| final HostnameVerifier trustAllNames = (hostname, session) -> true; | ||
| // Use JVM default HostnameVerifier to ensure hostnames are properly validated | ||
| final HostnameVerifier defaultHostnameVerifier = HttpsURLConnection.getDefaultHostnameVerifier(); | ||
|
|
||
| return connection -> { | ||
| if (connection instanceof HttpsURLConnection) { | ||
| final HttpsURLConnection httpsConnection = (HttpsURLConnection) connection; | ||
| httpsConnection.setSSLSocketFactory(sf); | ||
| httpsConnection.setHostnameVerifier(trustAllNames); | ||
| httpsConnection.setHostnameVerifier(defaultHostnameVerifier); | ||
| httpsConnection.setConnectTimeout(CONNECTION_TIMEOUT); | ||
| httpsConnection.setReadTimeout(READ_TIMEOUT); | ||
| httpsConnection.setInstanceFollowRedirects(false); |
modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/util/RemoteUrl.java
Outdated
Show resolved
Hide resolved
modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/util/RemoteUrl.java
Show resolved
Hide resolved
modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/util/RemoteUrl.java
Show resolved
Hide resolved
modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/util/RemoteUrl.java
Show resolved
Hide resolved
modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/reference/ReferenceVisitor.java
Outdated
Show resolved
Hide resolved
modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/util/RemoteUrl.java
Show resolved
Hide resolved
|
Looks good :) |
There was a problem hiding this comment.
Pull Request Overview
This PR implements security measures to prevent Server-Side Request Forgery (SSRF) attacks via remote $ref in the Swagger parser by introducing URL validation and restricting access to potentially dangerous network locations.
Key changes:
- Adds
PermittedUrlsCheckerfunctionality to validate URLs before making remote requests - Implements redirect limits and protocol validation to prevent abuse
- Updates test infrastructure to support HTTPS and handle URL security checks
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Removes commented dependencies and adds system property for testing |
| RemoteUrl.java | Refactors URL handling with redirect limits, security checks, and improved error handling |
| RefUtils.java | Updates method signatures to include PermittedUrlsChecker parameter |
| Multiple test files | Updates tests to use HTTPS, adds security validation, and improves test structure |
| PermittedUrlsCheckerAllowLocal.java | New test utility class to allow local connections during testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/util/RemoteUrl.java
Show resolved
Hide resolved
modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/reference/ReferenceVisitor.java
Show resolved
Hide resolved
modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/util/RemoteUrlTest.java
Outdated
Show resolved
Hide resolved
modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/util/RemoteUrlTest.java
Outdated
Show resolved
Hide resolved
|
Fixes #2206 |
|
@rootxjs, thank you for bringing this to our attention! I am closing the issue :) |
The current implementation of the URL validation logic bypasses the security controls of setSafelyResolveURL when an HTTP redirect is encountered. This allows a malicious OpenAPI specification to force the validator's host to connect to internal network resources, creating a significant security risk. This is a fix for this securrity issue.