Add InetAddressMatcher #18634
Add InetAddressMatcher #18634
Conversation
Co-authored-by: Gábor Vaspöri <[email protected]> Co-authored-by: Kian Jamali <[email protected]> Co-authored-by: Rossen Stoyanchev <[email protected]>
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
Previously the terminology blurred allow/deny into the matching APIs which was confusing. The API now consistently uses matching logic.
This is more consistent with the matches(String) method and it allows APIs like ServerHttpRequest.getRemoteAddress() to be passed in without needing to check if the value is null.
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). |
IpAddressServerWebExchangeMatcher was used for WebFlux applications which used IpAddressMatcher. This was likely problematic because IpAddressMatcher contains servlet APIs in it's method signatures. Even if those specific methods aren't used by IpAddressServerWebExchangeMatcher it can cause classpath issues for webflux applications. This commit switches to using InetAddressMatcher which does not contain any dependencies on the ServletAPI
| 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; })
3 participants
No description provided.