Skip to content

Commit e9e5e3f

Browse files
fpaponfpapon
authored
Merge pull request #1026 from haster/change-pathtraversal-blockmode
[#1025] - Shiro's InvalidRequestFilter blocks valid paths with encoded slashes
2 parents 1ceab51 + f83aede commit e9e5e3f

3 files changed

Lines changed: 103 additions & 118 deletions

File tree

‎src/suppressions.xml‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,22 +19,25 @@
1919

2020
<!-- See https://github.com/hazelcast/hazelcast/blob/master/checkstyle/suppressions.xml for examples -->
2121
<suppressions>
22+
<suppress checks=".*" files="target/generated-sources/.*"/>
23+
<suppress checks=".*" files="target/generated-test-sources/.*"/>
2224
<!-- do not require package-info.java -->
23-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]ee[\\/]*"/>
24-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]testing[\\/]*"/>
25-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]cdi[\\/]*"/>
26-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]spring[\\/]*"/>
27-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]guice[\\/]*"/>
28-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]aop[\\/]*"/>
29-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]*"/>
30-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]authc[\\/]*"/>
31-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]web[\\/]jaxrs[\\/]*"/>
32-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]lang[\\/]*"/>
33-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]tools[\\/]hasher[\\/]*"/>
34-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]config[\\/]ogdl[\\/]*"/>
35-
<suppress checks="JavadocPackage" files="org[\\/]apache[\\/]shiro[\\/]cache[\\/]ehcache[\\/]*"/>
25+
<suppress checks="JavadocPackage" files="org.apache.shiro.ee.*"/>
26+
<suppress checks="JavadocPackage" files="org.apache.shiro.testing.*"/>
27+
<suppress checks="JavadocPackage" files="org.apache.shiro.cdi.*"/>
28+
<suppress checks="JavadocPackage" files="org.apache.shiro.spring.*"/>
29+
<suppress checks="JavadocPackage" files="org.apache.shiro.guice.*"/>
30+
<suppress checks="JavadocPackage" files="org.apache.shiro.aop.*"/>
31+
<suppress checks="JavadocPackage" files="org.apache.shiro.*"/>
32+
<suppress checks="JavadocPackage" files="org.apache.shiro.authc.*"/>
33+
<suppress checks="JavadocPackage" files="org.apache.shiro.web.jaxrs.*"/>
34+
<suppress checks="JavadocPackage" files="org.apache.shiro.lang.*"/>
35+
<suppress checks="JavadocPackage" files="org.apache.shiro.tools.hasher.*"/>
36+
<suppress checks="JavadocPackage" files="org.apache.shiro.config.ogdl.*"/>
37+
<suppress checks="JavadocPackage" files="org.apache.shiro.cache.ehcache.*"/>
3638
<suppress checks="MethodName" files=".*Test\.java"/>
3739
<suppress checks="MethodName" files=".*IT\.java"/>
3840
<suppress checks=".*" files="Quickstart.*.java"/>
3941
<suppress checks=".*" files="application.properties"/>
42+
<suppress checks="LineLength" lines="45" files="org.apache.shiro.web.filter.InvalidRequestFilter"/>
4043
</suppressions>

‎web/src/main/java/org/apache/shiro/web/filter/InvalidRequestFilter.java‎

Lines changed: 45 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,7 @@
2929
import java.util.Arrays;
3030
import java.util.Collections;
3131
import java.util.List;
32-
import java.util.stream.Stream;
3332

34-
@SuppressWarnings("checkstyle:LineLength")
3533
/**
3634
* A request filter that blocks malicious requests. Invalid request will respond with a 400 response code.
3735
* <p>
@@ -40,7 +38,7 @@
4038
* <li>Semicolon - can be disabled by setting {@code blockSemicolon = false}</li>
4139
* <li>Backslash - can be disabled by setting {@code blockBackslash = false}</li>
4240
* <li>Non-ASCII characters - can be disabled by setting {@code blockNonAscii = false},
43-
* the ability to disable this check will be removed in future version.</li>
41+
* the ability to disable this check will be removed in future version.</li>
4442
* <li>Path traversals - can be disabled by setting {@code blockTraversal = false}</li>
4543
* </ul>
4644
*
@@ -49,6 +47,11 @@
4947
* @since 1.6
5048
*/
5149
public class InvalidRequestFilter extends AccessControlFilter {
50+
public enum PathTraversalBlockMode {
51+
STRICT,
52+
NORMAL,
53+
NO_BLOCK;
54+
}
5255

5356
private static final List<String> SEMICOLON = Collections.unmodifiableList(Arrays.asList(";", "%3b", "%3B"));
5457

@@ -64,35 +67,27 @@ public class InvalidRequestFilter extends AccessControlFilter {
6467

6568
private boolean blockNonAscii = true;
6669

67-
private boolean blockTraversal = true;
68-
69-
private boolean blockEncodedPeriod = true;
70-
71-
private boolean blockEncodedForwardSlash = true;
72-
73-
private boolean blockRewriteTraversal = true;
70+
private PathTraversalBlockMode pathTraversalBlockMode = PathTraversalBlockMode.NORMAL;
7471

7572
@Override
7673
protected boolean isAccessAllowed(ServletRequest req, ServletResponse response, Object mappedValue) throws Exception {
7774
HttpServletRequest request = WebUtils.toHttp(req);
7875
// check the original and decoded values
76+
7977
// user request string (not decoded)
8078
return isValid(request.getRequestURI())
8179
// decoded servlet part
8280
&& isValid(request.getServletPath())
83-
// decoded path info (may be null)
81+
// decoded path info (maybe null)
8482
&& isValid(request.getPathInfo());
8583
}
8684

87-
@SuppressWarnings("checkstyle:BooleanExpressionComplexity")
8885
private boolean isValid(String uri) {
8986
return !StringUtils.hasText(uri)
90-
|| (!containsSemicolon(uri)
91-
&& !containsBackslash(uri)
92-
&& !containsNonAsciiCharacters(uri)
93-
&& !containsTraversal(uri)
94-
&& !containsEncodedPeriods(uri)
95-
&& !containsEncodedForwardSlash(uri));
87+
|| (!containsSemicolon(uri)
88+
&& !containsBackslash(uri)
89+
&& !containsNonAsciiCharacters(uri))
90+
&& !containsTraversal(uri);
9691
}
9792

9893
@Override
@@ -134,23 +129,13 @@ private static boolean containsOnlyPrintableAsciiCharacters(String uri) {
134129
}
135130

136131
private boolean containsTraversal(String uri) {
137-
if (isBlockTraversal()) {
138-
return !isNormalized(uri)
139-
|| (isBlockRewriteTraversal() && Stream.of("/..;", "/.;").anyMatch(uri::contains));
140-
}
141-
return false;
142-
}
143-
144-
private boolean containsEncodedPeriods(String uri) {
145-
if (isBlockEncodedPeriod()) {
146-
return PERIOD.stream().anyMatch(uri::contains);
132+
if (pathTraversalBlockMode == PathTraversalBlockMode.NORMAL) {
133+
return !(isNormalized(uri));
147134
}
148-
return false;
149-
}
150-
151-
private boolean containsEncodedForwardSlash(String uri) {
152-
if (isBlockEncodedForwardSlash()) {
153-
return FORWARDSLASH.stream().anyMatch(uri::contains);
135+
if (pathTraversalBlockMode == PathTraversalBlockMode.STRICT) {
136+
return !(isNormalized(uri)
137+
&& PERIOD.stream().noneMatch(uri::contains)
138+
&& FORWARDSLASH.stream().noneMatch(uri::contains));
154139
}
155140
return false;
156141
}
@@ -205,35 +190,51 @@ public void setBlockNonAscii(boolean blockNonAscii) {
205190
this.blockNonAscii = blockNonAscii;
206191
}
207192

208-
public boolean isBlockTraversal() {
209-
return blockTraversal;
193+
public PathTraversalBlockMode getPathTraversalBlockMode() {
194+
return pathTraversalBlockMode;
210195
}
211196

212-
public void setBlockTraversal(boolean blockTraversal) {
213-
this.blockTraversal = blockTraversal;
197+
public void setBlockPathTraversal(PathTraversalBlockMode mode) {
198+
this.pathTraversalBlockMode = mode;
214199
}
215200

216201
public boolean isBlockEncodedPeriod() {
217-
return blockEncodedPeriod;
202+
return pathTraversalBlockMode == PathTraversalBlockMode.STRICT;
218203
}
219204

220205
public void setBlockEncodedPeriod(boolean blockEncodedPeriod) {
221-
this.blockEncodedPeriod = blockEncodedPeriod;
206+
setBlockPathTraversal(blockEncodedPeriod ? PathTraversalBlockMode.STRICT : PathTraversalBlockMode.NORMAL);
222207
}
223208

224209
public boolean isBlockEncodedForwardSlash() {
225-
return blockEncodedForwardSlash;
210+
return pathTraversalBlockMode == PathTraversalBlockMode.STRICT;
226211
}
227212

228213
public void setBlockEncodedForwardSlash(boolean blockEncodedForwardSlash) {
229-
this.blockEncodedForwardSlash = blockEncodedForwardSlash;
214+
setBlockPathTraversal(blockEncodedForwardSlash ? PathTraversalBlockMode.STRICT : PathTraversalBlockMode.NORMAL);
230215
}
231216

232217
public boolean isBlockRewriteTraversal() {
233-
return blockRewriteTraversal;
218+
return pathTraversalBlockMode == PathTraversalBlockMode.NORMAL;
234219
}
235220

236221
public void setBlockRewriteTraversal(boolean blockRewriteTraversal) {
237-
this.blockRewriteTraversal = blockRewriteTraversal;
222+
setBlockPathTraversal(blockRewriteTraversal ? PathTraversalBlockMode.NORMAL : PathTraversalBlockMode.NO_BLOCK);
223+
}
224+
225+
/**
226+
* @deprecated use {@link #getPathTraversalBlockMode()} instead
227+
*/
228+
@Deprecated
229+
public boolean isBlockTraversal() {
230+
return pathTraversalBlockMode != PathTraversalBlockMode.NO_BLOCK;
231+
}
232+
233+
/**
234+
* @deprecated Use {@link #setBlockPathTraversal(PathTraversalBlockMode)}
235+
*/
236+
@Deprecated
237+
public void setBlockTraversal(boolean blockTraversal) {
238+
this.pathTraversalBlockMode = blockTraversal ? PathTraversalBlockMode.NORMAL : PathTraversalBlockMode.NO_BLOCK;
238239
}
239240
}

‎web/src/test/groovy/org/apache/shiro/web/filter/InvalidRequestFilterTest.groovy‎

Lines changed: 42 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,9 @@ class InvalidRequestFilterTest {
3939
assertThat "filter.blockBackslash expected to be true", filter.isBlockBackslash()
4040
assertThat "filter.blockNonAscii expected to be true", filter.isBlockNonAscii()
4141
assertThat "filter.blockSemicolon expected to be true", filter.isBlockSemicolon()
42-
assertThat "filter.blockTraversal expected to be true", filter.isBlockTraversal()
42+
assertThat "filter.blockTraversal expected to be NORMAL",
43+
filter.getPathTraversalBlockMode() == InvalidRequestFilter.PathTraversalBlockMode.NORMAL
4344
assertThat "filter.blockRewriteTraversal expected to be true", filter.isBlockRewriteTraversal()
44-
assertThat "filter.blockEncodedPeriod expected to be true", filter.isBlockEncodedPeriod()
45-
assertThat "filter.blockEncodedForwardSlash expected to be true", filter.isBlockEncodedForwardSlash()
4645
}
4746

4847
@Test
@@ -76,11 +75,10 @@ class InvalidRequestFilterTest {
7675

7776
assertPathBlocked(filter, "/something", "/;something")
7877
assertPathBlocked(filter, "/something", "/something", "/;")
79-
assertPathBlocked(filter, "/something", "/something", "/.;")
8078
}
8179

8280
@Test
83-
void testBlocksTraversal() {
81+
void testBlocksTraversalNormal() {
8482
InvalidRequestFilter filter = new InvalidRequestFilter()
8583
assertPathBlocked(filter, "/something/../")
8684
assertPathBlocked(filter, "/something/../bar")
@@ -89,77 +87,52 @@ class InvalidRequestFilterTest {
8987
assertPathBlocked(filter, "/..")
9088
assertPathBlocked(filter, "..")
9189
assertPathBlocked(filter, "../")
92-
assertPathBlocked(filter, "%2F./")
9390
assertPathBlocked(filter, "/something/./")
9491
assertPathBlocked(filter, "/something/./bar")
9592
assertPathBlocked(filter, "/something/\u002e/bar")
9693
assertPathBlocked(filter, "/something/./bar/")
9794
assertPathBlocked(filter, "/something/.")
9895
assertPathBlocked(filter, "/.")
9996
assertPathBlocked(filter, "/something/../something/.")
100-
assertPathBlocked(filter, "/something/../something/.")
101-
assertPathBlocked(filter, "/something/.;")
102-
assertPathBlocked(filter, "/something/%2e%3b")
103-
104-
assertPathAllowed(filter, "/something/.bar")
105-
assertPathAllowed(filter, "/.something")
106-
assertPathAllowed(filter, ".something")
107-
}
108-
109-
@Test
110-
void testBlocksEncodedPeriod() {
111-
InvalidRequestFilter filter = new InvalidRequestFilter()
112-
assertPathBlocked(filter, "/%2esomething")
113-
assertPathBlocked(filter, "%2esomething")
114-
assertPathBlocked(filter, "%2E./")
115-
assertPathBlocked(filter, "%2F./")
116-
assertPathBlocked(filter, "/something/%2e;")
117-
assertPathBlocked(filter, "/something/%2e%3b")
118-
assertPathBlocked(filter, "/something/%2e%2E/bar/")
119-
assertPathBlocked(filter, "/something/%2e/bar/")
120-
}
12197

122-
@Test
123-
void testAllowsEncodedPeriod() {
124-
InvalidRequestFilter filter = new InvalidRequestFilter()
125-
filter.setBlockEncodedPeriod(false)
126-
assertPathAllowed(filter, "/%2esomething")
127-
assertPathAllowed(filter, "%2esomething")
12898
assertPathAllowed(filter, "%2E./")
129-
assertPathAllowed(filter, "/something/%2e%2E/bar/")
130-
assertPathAllowed(filter, "/something/%2e/bar/")
131-
}
132-
133-
@Test
134-
void testBlocksEncodedForwardSlash() {
135-
InvalidRequestFilter filter = new InvalidRequestFilter()
136-
assertPathBlocked(filter, "%2F./")
137-
assertPathBlocked(filter, "/something/%2f/bar/")
138-
}
139-
140-
@Test
141-
void testAllowsEncodedForwardSlash() {
142-
InvalidRequestFilter filter = new InvalidRequestFilter()
143-
filter.setBlockEncodedForwardSlash(false)
14499
assertPathAllowed(filter, "%2F./")
100+
assertPathAllowed(filter, "/something/%2e/bar/")
145101
assertPathAllowed(filter, "/something/%2f/bar/")
102+
assertPathAllowed(filter, "/something/http:%2f%2fmydomain.example.com%2foidc/bar/")
103+
assertPathAllowed(filter, "/something/%2e%2E/bar/")
104+
assertPathAllowed(filter, "/something/http:%2f%2fmydomain%2eexample%2ecom%2foidc/bar/")
146105
}
147106

148107
@Test
149-
void testBlocksRewriteTraversal() {
108+
void testBlocksTraversalStrict() {
150109
InvalidRequestFilter filter = new InvalidRequestFilter()
151-
filter.setBlockSemicolon(false)
152-
assertPathBlocked(filter, "/something/..;jsessionid=foobar")
153-
assertPathBlocked(filter, "/something/.;jsessionid=foobar")
154-
}
110+
filter.setBlockPathTraversal(InvalidRequestFilter.PathTraversalBlockMode.STRICT)
111+
assertThat "filter.blockEncodedPeriod expected to be true", filter.isBlockEncodedPeriod()
112+
assertThat "filter.blockEncodedForwardSlash expected to be true", filter.isBlockEncodedForwardSlash()
155113

156-
@Test
157-
void testAllowRewriteTraversal() {
158-
InvalidRequestFilter filter = new InvalidRequestFilter()
159-
filter.setBlockSemicolon(false)
160-
filter.setBlockRewriteTraversal(false)
161-
assertPathAllowed(filter, "/something/..;jsessionid=foobar")
162-
assertPathAllowed(filter, "/something/.;jsessionid=foobar")
114+
assertPathBlocked(filter, "/something/../")
115+
assertPathBlocked(filter, "/something/../bar")
116+
assertPathBlocked(filter, "/something/../bar/")
117+
assertPathBlocked(filter, "/something/..")
118+
assertPathBlocked(filter, "/..")
119+
assertPathBlocked(filter, "..")
120+
assertPathBlocked(filter, "../")
121+
assertPathBlocked(filter, "/something/./")
122+
assertPathBlocked(filter, "/something/./bar")
123+
assertPathBlocked(filter, "/something/\u002e/bar")
124+
assertPathBlocked(filter, "/something/./bar/")
125+
assertPathBlocked(filter, "/something/.")
126+
assertPathBlocked(filter, "/.")
127+
assertPathBlocked(filter, "/something/../something/.")
128+
129+
assertPathBlocked(filter, "%2E./")
130+
assertPathBlocked(filter, "%2F./")
131+
assertPathBlocked(filter, "/something/%2e/bar/")
132+
assertPathBlocked(filter, "/something/%2f/bar/")
133+
assertPathBlocked(filter, "/something/http:%2f%2fmydomain.example.com%2foidc/bar/")
134+
assertPathBlocked(filter, "/something/%2e%2E/bar/")
135+
assertPathBlocked(filter, "/something/http:%2f%2fmydomain%2eexample%2ecom%2foidc/bar/")
163136
}
164137

165138
@Test
@@ -213,7 +186,7 @@ class InvalidRequestFilterTest {
213186
@Test
214187
void testAllowTraversal() {
215188
InvalidRequestFilter filter = new InvalidRequestFilter()
216-
filter.setBlockTraversal(false)
189+
filter.setBlockPathTraversal(InvalidRequestFilter.PathTraversalBlockMode.NO_BLOCK);
217190

218191
assertPathAllowed(filter, "/something/../")
219192
assertPathAllowed(filter, "/something/../bar")
@@ -230,6 +203,14 @@ class InvalidRequestFilterTest {
230203
assertPathAllowed(filter, "/something/.")
231204
assertPathAllowed(filter, "/.")
232205
assertPathAllowed(filter, "/something/../something/.")
206+
207+
assertPathAllowed(filter, "%2E./")
208+
assertPathAllowed(filter, "%2F./")
209+
assertPathAllowed(filter, "/something/%2e/bar/")
210+
assertPathAllowed(filter, "/something/%2f/bar/")
211+
assertPathAllowed(filter, "/something/http:%2f%2fmydomain.example.com%2foidc/bar/")
212+
assertPathAllowed(filter, "/something/%2e%2E/bar/")
213+
assertPathAllowed(filter, "/something/http:%2f%2fmydomain%2eexample%2ecom%2foidc/bar/")
233214
}
234215

235216
static void assertPathBlocked(InvalidRequestFilter filter, String requestUri, String servletPath = requestUri, String pathInfo = null) {

0 commit comments

Comments
 (0)