Throws exception when passed IP address with too long mask #7084

clementkng · 2019-07-05T21:01:56Z

Originally, matches(String address) would first call parse on an address, when I believe the correct behavior is to first check for the validity of the subnet mask (an invalid mask for an IP address is invalid regardless of what method it's a part of). This is why invalid masks could be passed into the constructor but then throw exceptions on valid calls to matches. So I changed both the constructor and the matches method to first validate that the mask in the IP address passed in is not too long.

Draft b/c I'm not sure if my assumption fully holds for all cases.

Fixes gh-2790

jzheaux

@clementkng, thanks for the PR! I left some feedback inline.

jzheaux · 2019-07-09T11:56:43Z

web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java

 	}

 	public boolean matches(HttpServletRequest request) {
 		return matches(request.getRemoteAddr());
 	}

 	public boolean matches(String address) {
-		InetAddress remoteAddress = parseAddress(address);
+		String [] addressAndMask = preprocessAddressAndMask(address);


The address passed into this method doesn't have a mask, so the preprocessing is unnecessary here.

As such, the preprocessAddressAndMask method is probably also unnecessary for now.

So does this mean that we don't have to check remote address anymore, since there's no mask that will be assigned there?

Because of this line:

if ((remAddr[i] & mask[i]) != (reqAddr[i] & mask[i])) {

The code needs to ensure that both remAddr and reqAddr are not too short. It makes sense to me that the code would verify the required address in the constructor and the remote address in the matches method.

I assume you're referring to this line. I can add a comparison that the remAddr.length * 8 >= nMaskBits, but I'm having trouble coming up w/ a too short IP address. Stuff like 192.168.1 and shorter variants seem to be matching just fine.

Oops, I was behind a version. I'm referring to this line.

I wonder if it would break if you configured to match against an IPv6 address and called matches with an IPv4 address?

@jzheaux In that case this line will realize that the classes of the IP addresses are different (java.net.Inet6Address vs java.net.Inet4Address) and then simply return False. IMO this feels more correct than an IllegalArgumentException stating that a mask length is too long, which isn't indicative that the addresses you're comparing are of different types. Unless it is valid to try to mix and match ipv4 and ipv6 addresses?

No, it's not a valid use case -- was just thinking out loud about how you might make the remote address byte array too long.

I agree that an IPv4Address instance will always be 4 bytes long and an IPv6Address will always be 16 bytes long, so there's no need to check the length of remote address.

Thanks @jzheaux, any other feedback?

jzheaux · 2019-07-13T04:20:41Z

web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java

@@ -46,15 +47,18 @@
 	 */
 	public IpAddressMatcher(String ipAddress) {

+		String ipAddr = ipAddress;


While this may be an improvement, @clementkng, let's please leave renaming the variable for another PR, unless there is a reason it's necessary?

@jzheaux I changed it because ipAddress is overwritten if we parse a mask, meaning that I can’t use it in a meaningful error message (to show the ip and mask together).

I guess an alternative would be to rewrite the message in a way i.e. IP address <address> too short for bitmask <mask>. Let me know what you think.

I agree that a message with placeholders is a good solution. I like it because of the smaller change footprint.

@jzheaux I changed the message and changed the variables back.

clementkng · 2019-07-17T23:31:06Z

@jzheaux Thanks for the review. Is there any other feedback?

jzheaux · 2019-07-18T10:27:20Z

web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java

@@ -76,6 +80,8 @@ public boolean matches(String address) {
 		byte[] reqAddr = requiredAddress.getAddress();

 		int nMaskFullBytes = nMaskBits / 8;
+		Assert.isTrue(remAddr.length >= nMaskFullBytes,


Actually, this check can be removed, correct @clementkng? You and I could not find a use case where this check would fail.

I'd recommend taking it out; however, if it stays, then let's make sure that the error messages align to both include the ip address and the bitmask length.

@jzheaux good catch, forgot about that! I removed the check.

Fixes spring-projectsgh-2790

jzheaux · 2019-07-19T10:26:17Z

Thanks, @clementkng! This is now merged into master.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 5, 2019

clementkng marked this pull request as ready for review July 5, 2019 22:38

jzheaux requested changes Jul 9, 2019

View reviewed changes

jzheaux self-assigned this Jul 9, 2019

jzheaux added in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue status: waiting-for-feedback We need additional information before we can continue type: improvement and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 9, 2019

jzheaux added this to the 5.2.0.RC1 milestone Jul 9, 2019

jzheaux added type: enhancement A general enhancement and removed type: improvement labels Jul 9, 2019

clementkng force-pushed the gh-2790-ip-address-matcher-array-length-checks branch from dc38ca4 to be57e8f Compare July 9, 2019 18:38

jzheaux reviewed Jul 13, 2019

View reviewed changes

clementkng force-pushed the gh-2790-ip-address-matcher-array-length-checks branch from be57e8f to 6d917c1 Compare July 13, 2019 17:47

spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 17, 2019

jzheaux reviewed Jul 18, 2019

View reviewed changes

Throws exception when passed IP address with too long mask

a80944e

Fixes spring-projectsgh-2790

clementkng force-pushed the gh-2790-ip-address-matcher-array-length-checks branch from 6d917c1 to a80944e Compare July 18, 2019 14:20

jzheaux approved these changes Jul 19, 2019

View reviewed changes

jzheaux merged commit ab6440d into spring-projects:master Jul 19, 2019

Throws exception when passed IP address with too long mask #7084

Throws exception when passed IP address with too long mask #7084

Uh oh!

Conversation

clementkng commented Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jzheaux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clementkng Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clementkng commented Jul 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzheaux commented Jul 19, 2019

Uh oh!

Uh oh!

clementkng commented Jul 5, 2019 •

edited

Loading

clementkng Jul 13, 2019 •

edited

Loading