Skip to content
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

Initial commit of safe url resolver #1910

Merged
merged 4 commits into from
Apr 13, 2023
Merged

Conversation

CalemRoelofsSB
Copy link
Contributor

@CalemRoelofsSB CalemRoelofsSB commented Apr 12, 2023

This PR adds a new module to the swagger parser repo: swagger-parser-safe-url-resolver

What is it?

The main class of the package is the PermittedUrlsChecker which has one method: verify(String url).
This method takes in a string URL and performs the following steps:

  1. Gets the hostname portion from the URL
  2. Resolves the hostname to an IP address
  3. Checks if that IP address is in a private/restricted IP address range (and throws an exception if it is)
  4. Returns a ResolvedUrl object which contains
    4.1. A String url where the original URL has the hostname replaced with the IP address
    4.2. A String hostHeader which contains the hostname from the original URL to be added as a host header

This behavior can also be customized with the allowlist and denylist in the constructor, whereby:

  • An entry in the allowlist will allow the URL to pass even if it resolves to a private/restricted IP address
  • An entry in the denylist will throw an exception even when the URL resolves to a public IP address

Why is it needed?

This lib can be used in services that deal with user-submitted URLs that get fetched (like in swagger-parser resolving external URL $refs) to protect against Server-Side Request Forgery and DNS rebinding attacks

Example usage

List<String> allowlist = List.of("mysite.local");
List<String> denylist = List.of("*.example.com:443");
var checker = new PermittedUrlsChecker(allowlist, denylist);

// Will throw a HostDeniedException as `localhost` resolves to local IP and is not in allowlist
checker.verify("http://localhost/example");

// Will return a ResolvedUrl if `github.com` resolves to a public IP
checker.verify("https://github.com/swagger-api/swagger-parser");

// Will throw a HostDeniedException as `*.example.com` is explicitly deny listed, even if it resolves to public IP
checker.verify("https://subdomain.example.com/somepage");

// Will return a `ResolvedUrl` as `mysite.local` is explicity allowlisted
checker.verify("http://mysite.local/example");

@CalemRoelofsSB CalemRoelofsSB requested a review from frantuma April 12, 2023 12:43
return false;
}

String host = IDN.toASCII(parsed.getHost(), IDN.ALLOW_UNASSIGNED);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does IDN.toASCII handle the wildcards ok? The Javadoc says IllegalArgumentException - if the input string doesn't conform to RFC 3490 specification. I really can't think of a realistic attack vector for this so if it's problematic feel free to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so IDN.toASCII here is only parsing the end urls submitted by the user and seems to work fine with "incorrect" urls like "https://ex*.com/page" as it is using IDN.ALLOW_UNASSIGNED.
This is required to be able to parse emojis as punycode and any other non-standard characters

@bcoughlan
Copy link

Great stuff. LGTM at least. Will there be another PR to enable swagger-parser to be configured to use it?

@CalemRoelofsSB
Copy link
Contributor Author

CalemRoelofsSB commented Apr 12, 2023

Great stuff. LGTM at least. Will there be another PR to enable swagger-parser to be configured to use it?

@bcoughlan Yes indeed, separate tickets created for parser and codegen implementations that will pull this lib in

<dependency>
<groupId>org.testng</groupId>
<artifactId>testng</artifactId>
<version>7.7.0</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go for ${testng-version}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed now

@@ -0,0 +1,97 @@
package io.swagger.parser.v3.urlresolver;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with the project/artifacts package naming this should be io.swagger.v3.parser...
Applicable to all files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated across the packages now

Comment on lines 17 to 18
private final UrlPatternMatcher allowlistMatcher;
private final UrlPatternMatcher denylistMatcher;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe as protected to allow for extensibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, will update presently

return new ResolvedUrl(urlWithIp, hostname);
}

private boolean isRestrictedIpRange(InetAddress ip) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe as protected to allow for extensibility?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants