From 554df6fab629fdf1c72829e55f45ebe1fc2a3d84 Mon Sep 17 00:00:00 2001 From: Steve Riesenberg <5248162+sjohnr@users.noreply.github.com> Date: Thu, 14 Nov 2024 16:32:04 -0600 Subject: [PATCH 1/3] Fix NPE in IpAddressMatcher Closes gh-15527 (cherry picked from commit 52de894c3c0a812562d6822db30f5c6c88526181) --- .../web/util/matcher/IpAddressMatcher.java | 5 +++++ .../web/util/matcher/IpAddressMatcherTests.java | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java b/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java index e7a4fdab037..d4ce76d50a8 100644 --- a/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java +++ b/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java @@ -71,6 +71,11 @@ public boolean matches(HttpServletRequest request) { } public boolean matches(String address) { + // Do not match null or blank address + if (!StringUtils.hasText(address)) { + return false; + } + assertNotHostName(address); InetAddress remoteAddress = parseAddress(address); if (!this.requiredAddress.getClass().equals(remoteAddress.getClass())) { diff --git a/web/src/test/java/org/springframework/security/web/util/matcher/IpAddressMatcherTests.java b/web/src/test/java/org/springframework/security/web/util/matcher/IpAddressMatcherTests.java index eebd8fa9450..9329eed69a0 100644 --- a/web/src/test/java/org/springframework/security/web/util/matcher/IpAddressMatcherTests.java +++ b/web/src/test/java/org/springframework/security/web/util/matcher/IpAddressMatcherTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -126,4 +126,17 @@ public void numericDomainNameThenIllegalArgumentException() { .withMessage("ipAddress 123.156.7.18.org doesn't look like an IP Address. Is it a host name?"); } + // gh-15527 + @Test + public void matchesWhenIpAddressIsLoopbackAndAddressIsNullThenFalse() { + IpAddressMatcher ipAddressMatcher = new IpAddressMatcher("127.0.0.1"); + assertThat(ipAddressMatcher.matches((String) null)).isFalse(); + } + + // gh-15527 + @Test + public void matchesWhenAddressIsNullThenFalse() { + assertThat(this.v4matcher.matches((String) null)).isFalse(); + } + } From ddf4542a9ecfa41986451d44cfa6e476a20e22b0 Mon Sep 17 00:00:00 2001 From: Steve Riesenberg <5248162+sjohnr@users.noreply.github.com> Date: Thu, 14 Nov 2024 23:12:51 -0600 Subject: [PATCH 2/3] Add hasText assertion to IpAddressMatcher constructor Issue gh-15527 (cherry picked from commit 3a298196512de5f3002707e2af8298d650033df7) --- .../web/util/matcher/IpAddressMatcher.java | 1 + .../web/util/matcher/IpAddressMatcherTests.java | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java b/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java index d4ce76d50a8..788d107b768 100644 --- a/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java +++ b/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java @@ -50,6 +50,7 @@ public final class IpAddressMatcher implements RequestMatcher { * come. */ public IpAddressMatcher(String ipAddress) { + Assert.hasText(ipAddress, "ipAddress cannot be empty"); assertNotHostName(ipAddress); if (ipAddress.indexOf('/') > 0) { String[] addressAndMask = StringUtils.split(ipAddress, "/"); diff --git a/web/src/test/java/org/springframework/security/web/util/matcher/IpAddressMatcherTests.java b/web/src/test/java/org/springframework/security/web/util/matcher/IpAddressMatcherTests.java index 9329eed69a0..ce702bfbebb 100644 --- a/web/src/test/java/org/springframework/security/web/util/matcher/IpAddressMatcherTests.java +++ b/web/src/test/java/org/springframework/security/web/util/matcher/IpAddressMatcherTests.java @@ -139,4 +139,18 @@ public void matchesWhenAddressIsNullThenFalse() { assertThat(this.v4matcher.matches((String) null)).isFalse(); } + // gh-15527 + @Test + public void constructorWhenRequiredAddressIsNullThenThrowsIllegalArgumentException() { + assertThatIllegalArgumentException().isThrownBy(() -> new IpAddressMatcher(null)) + .withMessage("ipAddress cannot be empty"); + } + + // gh-15527 + @Test + public void constructorWhenRequiredAddressIsEmptyThenThrowsIllegalArgumentException() { + assertThatIllegalArgumentException().isThrownBy(() -> new IpAddressMatcher("")) + .withMessage("ipAddress cannot be empty"); + } + } From 285d16b0461dc3273c8385d4cd18b5ff15dfc092 Mon Sep 17 00:00:00 2001 From: Steve Riesenberg <5248162+sjohnr@users.noreply.github.com> Date: Thu, 14 Nov 2024 23:13:24 -0600 Subject: [PATCH 3/3] Polish IpAddressMatcher (cherry picked from commit 83a79159b81d3ee9f15f91cf9384f0267aafed4a) --- .../web/util/matcher/IpAddressMatcher.java | 55 +++++++++++-------- 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java b/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java index 788d107b768..a235b59dd99 100644 --- a/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java +++ b/web/src/main/java/org/springframework/security/web/util/matcher/IpAddressMatcher.java @@ -18,6 +18,7 @@ import java.net.InetAddress; import java.net.UnknownHostException; +import java.util.Objects; import java.util.regex.Pattern; import jakarta.servlet.http.HttpServletRequest; @@ -33,16 +34,17 @@ * IPv4 address will never match a request which returns an IPv6 address, and vice-versa. * * @author Luke Taylor + * @author Steve Riesenberg * @since 3.0.2 */ public final class IpAddressMatcher implements RequestMatcher { private static Pattern IPV4 = Pattern.compile("\\d{0,3}.\\d{0,3}.\\d{0,3}.\\d{0,3}(/\\d{0,3})?"); - private final int nMaskBits; - private final InetAddress requiredAddress; + private final int nMaskBits; + /** * Takes a specific IP address or a range specified using the IP/Netmask (e.g. * 192.168.1.0/24 or 202.24.0.0/14). @@ -52,18 +54,22 @@ public final class IpAddressMatcher implements RequestMatcher { public IpAddressMatcher(String ipAddress) { Assert.hasText(ipAddress, "ipAddress cannot be empty"); assertNotHostName(ipAddress); + + String requiredAddress; + int nMaskBits; if (ipAddress.indexOf('/') > 0) { - String[] addressAndMask = StringUtils.split(ipAddress, "/"); - ipAddress = addressAndMask[0]; - this.nMaskBits = Integer.parseInt(addressAndMask[1]); + String[] parts = Objects.requireNonNull(StringUtils.split(ipAddress, "/")); + requiredAddress = parts[0]; + nMaskBits = Integer.parseInt(parts[1]); } else { - this.nMaskBits = -1; + requiredAddress = ipAddress; + nMaskBits = -1; } - this.requiredAddress = parseAddress(ipAddress); - String finalIpAddress = ipAddress; + this.requiredAddress = parseAddress(requiredAddress); + this.nMaskBits = nMaskBits; Assert.isTrue(this.requiredAddress.getAddress().length * 8 >= this.nMaskBits, () -> String - .format("IP address %s is too short for bitmask of length %d", finalIpAddress, this.nMaskBits)); + .format("IP address %s is too short for bitmask of length %d", requiredAddress, this.nMaskBits)); } @Override @@ -71,14 +77,14 @@ public boolean matches(HttpServletRequest request) { return matches(request.getRemoteAddr()); } - public boolean matches(String address) { + public boolean matches(String ipAddress) { // Do not match null or blank address - if (!StringUtils.hasText(address)) { + if (!StringUtils.hasText(ipAddress)) { return false; } - assertNotHostName(address); - InetAddress remoteAddress = parseAddress(address); + assertNotHostName(ipAddress); + InetAddress remoteAddress = parseAddress(ipAddress); if (!this.requiredAddress.getClass().equals(remoteAddress.getClass())) { return false; } @@ -88,26 +94,31 @@ public boolean matches(String address) { byte[] remAddr = remoteAddress.getAddress(); byte[] reqAddr = this.requiredAddress.getAddress(); int nMaskFullBytes = this.nMaskBits / 8; - byte finalByte = (byte) (0xFF00 >> (this.nMaskBits & 0x07)); for (int i = 0; i < nMaskFullBytes; i++) { if (remAddr[i] != reqAddr[i]) { return false; } } + byte finalByte = (byte) (0xFF00 >> (this.nMaskBits & 0x07)); if (finalByte != 0) { return (remAddr[nMaskFullBytes] & finalByte) == (reqAddr[nMaskFullBytes] & finalByte); } return true; } - private void assertNotHostName(String ipAddress) { - boolean isIpv4 = IPV4.matcher(ipAddress).matches(); - if (isIpv4) { - return; - } - String error = "ipAddress " + ipAddress + " doesn't look like an IP Address. Is it a host name?"; - Assert.isTrue(ipAddress.charAt(0) == '[' || ipAddress.charAt(0) == ':' - || (Character.digit(ipAddress.charAt(0), 16) != -1 && ipAddress.contains(":")), error); + private static void assertNotHostName(String ipAddress) { + Assert.isTrue(isIpAddress(ipAddress), + () -> String.format("ipAddress %s doesn't look like an IP Address. Is it a host name?", ipAddress)); + } + + private static boolean isIpAddress(String ipAddress) { + // @formatter:off + return IPV4.matcher(ipAddress).matches() + || ipAddress.charAt(0) == '[' + || ipAddress.charAt(0) == ':' + || Character.digit(ipAddress.charAt(0), 16) != -1 + && ipAddress.indexOf(':') > 0; + // @formatter:on } private InetAddress parseAddress(String address) {