Skip to content

Commit ccbad61

Browse files
rwinchrwinch
committed
Change blacklist to blocklist
Closes gh-8676
1 parent ca1252b commit ccbad61

File tree

3 files changed

+108
-41
lines changed

3 files changed

+108
-41
lines changed

‎web/src/main/java/org/springframework/security/web/firewall/FirewalledResponse.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ class FirewalledResponse extends HttpServletResponseWrapper {
3737

3838
@Override
3939
public void sendRedirect(String location) throws IOException {
40-
// TODO: implement pluggable validation, instead of simple blacklisting.
40+
// TODO: implement pluggable validation, instead of simple blocklist.
4141
// SEC-1790. Prevent redirects containing CRLF
4242
validateCrlf(LOCATION_HEADER, location);
4343
super.sendRedirect(location);

‎web/src/main/java/org/springframework/security/web/firewall/StrictHttpFirewall.java

Lines changed: 61 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -98,23 +98,23 @@ public class StrictHttpFirewall implements HttpFirewall {
9898

9999
private static final List<String> FORBIDDEN_BACKSLASH = Collections.unmodifiableList(Arrays.asList("\\", "%5c", "%5C"));
100100

101-
private Set<String> encodedUrlBlacklist = new HashSet<>();
101+
private Set<String> encodedUrlBlocklist = new HashSet<>();
102102

103-
private Set<String> decodedUrlBlacklist = new HashSet<>();
103+
private Set<String> decodedUrlBlocklist = new HashSet<>();
104104

105105
private Set<String> allowedHttpMethods = createDefaultAllowedHttpMethods();
106106

107107
private Predicate<String> allowedHostnames = hostname -> true;
108108

109109
public StrictHttpFirewall() {
110-
urlBlacklistsAddAll(FORBIDDEN_SEMICOLON);
111-
urlBlacklistsAddAll(FORBIDDEN_FORWARDSLASH);
112-
urlBlacklistsAddAll(FORBIDDEN_DOUBLE_FORWARDSLASH);
113-
urlBlacklistsAddAll(FORBIDDEN_BACKSLASH);
114-
115-
this.encodedUrlBlacklist.add(ENCODED_PERCENT);
116-
this.encodedUrlBlacklist.addAll(FORBIDDEN_ENCODED_PERIOD);
117-
this.decodedUrlBlacklist.add(PERCENT);
110+
urlBlocklistsAddAll(FORBIDDEN_SEMICOLON);
111+
urlBlocklistsAddAll(FORBIDDEN_FORWARDSLASH);
112+
urlBlocklistsAddAll(FORBIDDEN_DOUBLE_FORWARDSLASH);
113+
urlBlocklistsAddAll(FORBIDDEN_BACKSLASH);
114+
115+
this.encodedUrlBlocklist.add(ENCODED_PERCENT);
116+
this.encodedUrlBlocklist.addAll(FORBIDDEN_ENCODED_PERIOD);
117+
this.decodedUrlBlocklist.add(PERCENT);
118118
}
119119

120120
/**
@@ -185,9 +185,9 @@ public void setAllowedHttpMethods(Collection<String> allowedHttpMethods) {
185185
*/
186186
public void setAllowSemicolon(boolean allowSemicolon) {
187187
if (allowSemicolon) {
188-
urlBlacklistsRemoveAll(FORBIDDEN_SEMICOLON);
188+
urlBlocklistsRemoveAll(FORBIDDEN_SEMICOLON);
189189
} else {
190-
urlBlacklistsAddAll(FORBIDDEN_SEMICOLON);
190+
urlBlocklistsAddAll(FORBIDDEN_SEMICOLON);
191191
}
192192
}
193193

@@ -208,9 +208,9 @@ public void setAllowSemicolon(boolean allowSemicolon) {
208208
*/
209209
public void setAllowUrlEncodedSlash(boolean allowUrlEncodedSlash) {
210210
if (allowUrlEncodedSlash) {
211-
urlBlacklistsRemoveAll(FORBIDDEN_FORWARDSLASH);
211+
urlBlocklistsRemoveAll(FORBIDDEN_FORWARDSLASH);
212212
} else {
213-
urlBlacklistsAddAll(FORBIDDEN_FORWARDSLASH);
213+
urlBlocklistsAddAll(FORBIDDEN_FORWARDSLASH);
214214
}
215215
}
216216

@@ -225,9 +225,9 @@ public void setAllowUrlEncodedSlash(boolean allowUrlEncodedSlash) {
225225
*/
226226
public void setAllowUrlEncodedDoubleSlash(boolean allowUrlEncodedDoubleSlash) {
227227
if (allowUrlEncodedDoubleSlash) {
228-
urlBlacklistsRemoveAll(FORBIDDEN_DOUBLE_FORWARDSLASH);
228+
urlBlocklistsRemoveAll(FORBIDDEN_DOUBLE_FORWARDSLASH);
229229
} else {
230-
urlBlacklistsAddAll(FORBIDDEN_DOUBLE_FORWARDSLASH);
230+
urlBlocklistsAddAll(FORBIDDEN_DOUBLE_FORWARDSLASH);
231231
}
232232
}
233233

@@ -250,9 +250,9 @@ public void setAllowUrlEncodedDoubleSlash(boolean allowUrlEncodedDoubleSlash) {
250250
*/
251251
public void setAllowUrlEncodedPeriod(boolean allowUrlEncodedPeriod) {
252252
if (allowUrlEncodedPeriod) {
253-
this.encodedUrlBlacklist.removeAll(FORBIDDEN_ENCODED_PERIOD);
253+
this.encodedUrlBlocklist.removeAll(FORBIDDEN_ENCODED_PERIOD);
254254
} else {
255-
this.encodedUrlBlacklist.addAll(FORBIDDEN_ENCODED_PERIOD);
255+
this.encodedUrlBlocklist.addAll(FORBIDDEN_ENCODED_PERIOD);
256256
}
257257
}
258258

@@ -275,9 +275,9 @@ public void setAllowUrlEncodedPeriod(boolean allowUrlEncodedPeriod) {
275275
*/
276276
public void setAllowBackSlash(boolean allowBackSlash) {
277277
if (allowBackSlash) {
278-
urlBlacklistsRemoveAll(FORBIDDEN_BACKSLASH);
278+
urlBlocklistsRemoveAll(FORBIDDEN_BACKSLASH);
279279
} else {
280-
urlBlacklistsAddAll(FORBIDDEN_BACKSLASH);
280+
urlBlocklistsAddAll(FORBIDDEN_BACKSLASH);
281281
}
282282
}
283283

@@ -297,11 +297,11 @@ public void setAllowBackSlash(boolean allowBackSlash) {
297297
*/
298298
public void setAllowUrlEncodedPercent(boolean allowUrlEncodedPercent) {
299299
if (allowUrlEncodedPercent) {
300-
this.encodedUrlBlacklist.remove(ENCODED_PERCENT);
301-
this.decodedUrlBlacklist.remove(PERCENT);
300+
this.encodedUrlBlocklist.remove(ENCODED_PERCENT);
301+
this.decodedUrlBlocklist.remove(PERCENT);
302302
} else {
303-
this.encodedUrlBlacklist.add(ENCODED_PERCENT);
304-
this.decodedUrlBlacklist.add(PERCENT);
303+
this.encodedUrlBlocklist.add(ENCODED_PERCENT);
304+
this.decodedUrlBlocklist.add(PERCENT);
305305
}
306306
}
307307

@@ -320,20 +320,20 @@ public void setAllowedHostnames(Predicate<String> allowedHostnames) {
320320
this.allowedHostnames = allowedHostnames;
321321
}
322322

323-
private void urlBlacklistsAddAll(Collection<String> values) {
324-
this.encodedUrlBlacklist.addAll(values);
325-
this.decodedUrlBlacklist.addAll(values);
323+
private void urlBlocklistsAddAll(Collection<String> values) {
324+
this.encodedUrlBlocklist.addAll(values);
325+
this.decodedUrlBlocklist.addAll(values);
326326
}
327327

328-
private void urlBlacklistsRemoveAll(Collection<String> values) {
329-
this.encodedUrlBlacklist.removeAll(values);
330-
this.decodedUrlBlacklist.removeAll(values);
328+
private void urlBlocklistsRemoveAll(Collection<String> values) {
329+
this.encodedUrlBlocklist.removeAll(values);
330+
this.decodedUrlBlocklist.removeAll(values);
331331
}
332332

333333
@Override
334334
public FirewalledRequest getFirewalledRequest(HttpServletRequest request) throws RequestRejectedException {
335335
rejectForbiddenHttpMethod(request);
336-
rejectedBlacklistedUrls(request);
336+
rejectedBlocklistedUrls(request);
337337
rejectedUntrustedHosts(request);
338338

339339
if (!isNormalized(request)) {
@@ -363,13 +363,13 @@ private void rejectForbiddenHttpMethod(HttpServletRequest request) {
363363
}
364364
}
365365

366-
private void rejectedBlacklistedUrls(HttpServletRequest request) {
367-
for (String forbidden : this.encodedUrlBlacklist) {
366+
private void rejectedBlocklistedUrls(HttpServletRequest request) {
367+
for (String forbidden : this.encodedUrlBlocklist) {
368368
if (encodedUrlContains(request, forbidden)) {
369369
throw new RequestRejectedException("The request was rejected because the URL contained a potentially malicious String \"" + forbidden + "\"");
370370
}
371371
}
372-
for (String forbidden : this.decodedUrlBlacklist) {
372+
for (String forbidden : this.decodedUrlBlocklist) {
373373
if (decodedUrlContains(request, forbidden)) {
374374
throw new RequestRejectedException("The request was rejected because the URL contained a potentially malicious String \"" + forbidden + "\"");
375375
}
@@ -481,20 +481,41 @@ private static boolean isNormalized(String path) {
481481
}
482482

483483
/**
484-
* Provides the existing encoded url blacklist which can add/remove entries from
484+
* Provides the existing encoded url blocklist which can add/remove entries from
485485
*
486-
* @return the existing encoded url blacklist, never null
486+
* @return the existing encoded url blocklist, never null
487487
*/
488+
public Set<String> getEncodedUrlBlocklist() {
489+
return this.encodedUrlBlocklist;
490+
}
491+
492+
/**
493+
* Provides the existing decoded url blocklist which can add/remove entries from
494+
*
495+
* @return the existing decoded url blocklist, never null
496+
*/
497+
public Set<String> getDecodedUrlBlocklist() {
498+
return this.decodedUrlBlocklist;
499+
}
500+
501+
/**
502+
* Provides the existing encoded url blocklist which can add/remove entries from
503+
*
504+
* @return the existing encoded url blocklist, never null
505+
* @deprecated Use {@link #getEncodedUrlBlocklist()} instead
506+
*/
507+
@Deprecated
488508
public Set<String> getEncodedUrlBlacklist() {
489-
return encodedUrlBlacklist;
509+
return getEncodedUrlBlocklist();
490510
}
491511

492512
/**
493-
* Provides the existing decoded url blacklist which can add/remove entries from
513+
* Provides the existing decoded url blocklist which can add/remove entries from
514+
*
515+
* @return the existing decoded url blocklist, never null
494516
*
495-
* @return the existing decoded url blacklist, never null
496517
*/
497518
public Set<String> getDecodedUrlBlacklist() {
498-
return decodedUrlBlacklist;
519+
return getDecodedUrlBlocklist();
499520
}
500521
}

‎web/src/test/java/org/springframework/security/web/firewall/StrictHttpFirewallTests.java

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -522,6 +522,52 @@ public void getFirewalledRequestWhenRemoveFromDecodedUrlBlacklistThenNoException
522522
assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException();
523523
}
524524

525+
// blocklist
526+
527+
@Test
528+
public void getFirewalledRequestWhenRemoveFromUpperCaseEncodedUrlBlocklistThenNoException() {
529+
this.firewall.setAllowUrlEncodedSlash(true);
530+
MockHttpServletRequest request = new MockHttpServletRequest("GET", "");
531+
request.setRequestURI("/context-root/a/b%2F%2Fc");
532+
this.firewall.getEncodedUrlBlocklist().removeAll(Arrays.asList("%2F%2F"));
533+
assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException();
534+
}
535+
536+
@Test
537+
public void getFirewalledRequestWhenRemoveFromLowerCaseEncodedUrlBlocklistThenNoException() {
538+
this.firewall.setAllowUrlEncodedSlash(true);
539+
MockHttpServletRequest request = new MockHttpServletRequest("GET", "");
540+
request.setRequestURI("/context-root/a/b%2f%2fc");
541+
this.firewall.getEncodedUrlBlocklist().removeAll(Arrays.asList("%2f%2f"));
542+
assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException();
543+
}
544+
545+
@Test
546+
public void getFirewalledRequestWhenRemoveFromLowerCaseAndUpperCaseEncodedUrlBlocklistThenNoException() {
547+
this.firewall.setAllowUrlEncodedSlash(true);
548+
MockHttpServletRequest request = new MockHttpServletRequest("GET", "");
549+
request.setRequestURI("/context-root/a/b%2f%2Fc");
550+
this.firewall.getEncodedUrlBlocklist().removeAll(Arrays.asList("%2f%2F"));
551+
assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException();
552+
}
553+
554+
@Test
555+
public void getFirewalledRequestWhenRemoveFromUpperCaseAndLowerCaseEncodedUrlBlocklistThenNoException() {
556+
this.firewall.setAllowUrlEncodedSlash(true);
557+
MockHttpServletRequest request = new MockHttpServletRequest("GET", "");
558+
request.setRequestURI("/context-root/a/b%2F%2fc");
559+
this.firewall.getEncodedUrlBlocklist().removeAll(Arrays.asList("%2F%2f"));
560+
assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException();
561+
}
562+
563+
@Test
564+
public void getFirewalledRequestWhenRemoveFromDecodedUrlBlocklistThenNoException() {
565+
MockHttpServletRequest request = new MockHttpServletRequest("GET", "");
566+
request.setPathInfo("/a/b//c");
567+
this.firewall.getDecodedUrlBlocklist().removeAll(Arrays.asList("//"));
568+
assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException();
569+
}
570+
525571
@Test
526572
public void getFirewalledRequestWhenTrustedDomainThenNoException() {
527573
this.request.addHeader("Host", "example.org");

0 commit comments

Comments
 (0)