Conversation
rwinch
force-pushed
the
google-ssrf
branch
2 times, most recently
from
a405f06 to
d82601f
Compare
rwinch
added
in: web
There was a problem hiding this comment.
Looks great, but I did leave a couple of comments and suggestions.
I think the main contract is a good place to provide context and background from a high level perspective, something along the lines of the description of #18498 especially since this is all just a foundational, building block.
web/src/main/java/org/springframework/security/web/util/matcher/InetAddressMatcher.java
Show resolved
Hide resolved
web/src/main/java/org/springframework/security/web/util/matcher/InetAddressMatcher.java
Outdated
Show resolved
Hide resolved
web/src/main/java/org/springframework/security/web/util/matcher/InetAddressMatcher.java
Show resolved
Hide resolved
web/src/main/java/org/springframework/security/web/util/matcher/IpInetAddressMatcher.java
Show resolved
Hide resolved
Thanks I've responded inline and posted updates where I think that it makes sense.
I'm going to avoid documenting anything other than what the API does. It is a general purpose IP address matching API that is used by Spring Security to map server side authorization logic. It can be used by users to do anything that involves matching an IP address (e.g. client side firewall rules -- perhaps to prevent SSRF, MFA logic, etc). |
| if (address == null) { | ||
| return false; | ||
| } | ||
| if (address.isLoopbackAddress()) { |
There was a problem hiding this comment.
Should we do "(address.isLoopbackAddress() || address.isLinkLocalAddress())" instead?
This would include link-local addresses (such as 169.254.169.254) that are commonly targeted for SSRF attacks.
|
I'd suggest using isSiteLocalAddress() which might be more robust. The current manual check for the 172 range only looks for 172.16, missing IPs from 172.17 through 172.31. The built-in method handles the bitmask correctly and keeps the logic much simpler. I'm also a bit concerned about the usage IPv4-Mapped IPv6 addresses as a bypass. Standard methods should handle this inherently. |
| || Character.digit(ipAddress.charAt(0), 16) != -1 | ||
| && ipAddress.indexOf(':') > 0; | ||
| // @formatter:on | ||
| } |
There was a problem hiding this comment.
Should we defensively check whether the IP address to avoid throwing an exception? (e.g if (!StringUtils.hasText(ipAddress)) { return false; })
…back - Use `isLinkLocalAddress()` to correctly identify link-local addresses (e.g., 169.254.x.x) as internal. - Use `isSiteLocalAddress()` to correctly match the entire 172.16.0.0/12 subnet and rely on standard JVM checks for IPv4-mapped IPv6 addresses. - Retain custom matching for Unique Local Addresses (fc00::/7) due to JVM limitations with RFC 4193. - Add defensive blank string checking in `InetAddressParser.isIpAddress()`.
Co-authored-by: Gábor Vaspöri <gabor.vaspori@gmail.com> Co-authored-by: Kian Jamali <kianjamali123@gmail.com> Co-authored-by: Rossen Stoyanchev <rstoyanchev@users.noreply.github.com>
3 participants
Extracts logic for matching
InetAddressfrom theHttpServletRequest. This allows for general reuse of the API. In particular:IpInetAddressMatchernow delegates toInetAddressMatcherIpAddressServerWebExchangeMatchernow delegates toInetAddressMatcherCloses gh-18498