Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
f479652
commit f7cf283
Showing
4 changed files
with
172 additions
and
1 deletion.
There are no files selected for viewing
47 changes: 47 additions & 0 deletions
47
core/src/main/java/jenkins/security/SuspiciousRequestFilter.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package jenkins.security; | ||
|
||
import jenkins.util.SystemProperties; | ||
import org.kohsuke.accmod.Restricted; | ||
import org.kohsuke.accmod.restrictions.NoExternalUse; | ||
|
||
import javax.servlet.Filter; | ||
import javax.servlet.FilterChain; | ||
import javax.servlet.FilterConfig; | ||
import javax.servlet.ServletException; | ||
import javax.servlet.ServletRequest; | ||
import javax.servlet.ServletResponse; | ||
import javax.servlet.http.HttpServletRequest; | ||
import javax.servlet.http.HttpServletResponse; | ||
import java.io.IOException; | ||
import java.util.logging.Logger; | ||
|
||
@Restricted(NoExternalUse.class) | ||
public class SuspiciousRequestFilter implements Filter { | ||
|
||
/** System property name set to true or false to indicate whether or not semicolons should be allowed in URL paths. */ | ||
public static final String ALLOW_SEMICOLONS_IN_PATH = SuspiciousRequestFilter.class.getName() + ".allowSemicolonsInPath"; | ||
public static boolean allowSemicolonsInPath = SystemProperties.getBoolean(ALLOW_SEMICOLONS_IN_PATH, false); | ||
private static final Logger LOGGER = Logger.getLogger(SuspiciousRequestFilter.class.getName()); | ||
|
||
@Override | ||
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { | ||
HttpServletRequest httpRequest = (HttpServletRequest) request; | ||
HttpServletResponse httpResponse = (HttpServletResponse) response; | ||
if (!allowSemicolonsInPath && httpRequest.getRequestURI().contains(";")) { | ||
LOGGER.warning(() -> "Denying HTTP " + httpRequest.getMethod() + " to " + httpRequest.getRequestURI() + | ||
" as it has an illegal semicolon in the path. This behavior can be overridden by setting the system property " + | ||
ALLOW_SEMICOLONS_IN_PATH + " to true. For more information, see https://jenkins.io/redirect/semicolons-in-urls"); | ||
httpResponse.sendError(HttpServletResponse.SC_BAD_REQUEST, "Semicolons are not allowed in the request URI"); | ||
} else { | ||
chain.doFilter(request, response); | ||
} | ||
} | ||
|
||
@Override | ||
public void init(FilterConfig filterConfig) throws ServletException { | ||
} | ||
|
||
@Override | ||
public void destroy() { | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
102 changes: 102 additions & 0 deletions
102
test/src/test/java/jenkins/security/SuspiciousRequestFilterTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
package jenkins.security; | ||
|
||
import com.gargoylesoftware.htmlunit.WebRequest; | ||
import com.gargoylesoftware.htmlunit.WebResponse; | ||
import hudson.ExtensionList; | ||
import hudson.model.UnprotectedRootAction; | ||
import org.junit.Rule; | ||
import org.junit.Test; | ||
import org.jvnet.hudson.test.Issue; | ||
import org.jvnet.hudson.test.JenkinsRule; | ||
import org.jvnet.hudson.test.TestExtension; | ||
import org.kohsuke.stapler.QueryParameter; | ||
import org.kohsuke.stapler.verb.GET; | ||
|
||
import javax.annotation.CheckForNull; | ||
import javax.servlet.http.HttpServletResponse; | ||
import java.net.URL; | ||
|
||
import static org.hamcrest.MatcherAssert.assertThat; | ||
import static org.hamcrest.Matchers.containsString; | ||
import static org.hamcrest.Matchers.is; | ||
import static org.hamcrest.Matchers.nullValue; | ||
|
||
@Issue("SECURITY-1774") | ||
public class SuspiciousRequestFilterTest { | ||
|
||
@Rule | ||
public JenkinsRule j = new JenkinsRule(); | ||
|
||
private WebResponse get(String path) throws Exception { | ||
return j.createWebClient() | ||
.withThrowExceptionOnFailingStatusCode(false) | ||
.getPage(new WebRequest(new URL(j.getURL(), path))) | ||
.getWebResponse(); | ||
} | ||
|
||
@Test | ||
public void denySemicolonInRequestPathByDefault() throws Exception { | ||
WebResponse response = get("foo/bar/..;/?baz=bruh"); | ||
assertThat(Foo.getInstance().baz, is(nullValue())); | ||
assertThat(response.getStatusCode(), is(HttpServletResponse.SC_BAD_REQUEST)); | ||
assertThat(response.getContentAsString(), containsString("Semicolons are not allowed in the request URI")); | ||
} | ||
|
||
@Test | ||
public void allowSemicolonsInRequestPathWhenEscapeHatchEnabled() throws Exception { | ||
SuspiciousRequestFilter.allowSemicolonsInPath = true; | ||
try { | ||
WebResponse response = get("foo/bar/..;/..;/cli?baz=bruh"); | ||
assertThat(Foo.getInstance().baz, is("bruh")); | ||
assertThat(response.getStatusCode(), is(HttpServletResponse.SC_OK)); | ||
} finally { | ||
SuspiciousRequestFilter.allowSemicolonsInPath = false; | ||
} | ||
} | ||
|
||
@Test | ||
public void allowSemicolonsInQueryParameters() throws Exception { | ||
WebResponse response = get("foo/bar?baz=foo;bar=baz"); | ||
assertThat(Foo.getInstance().baz, is("foo;bar=baz")); | ||
assertThat(response.getStatusCode(), is(HttpServletResponse.SC_OK)); | ||
} | ||
|
||
@TestExtension | ||
public static class Foo implements UnprotectedRootAction { | ||
|
||
private static Foo getInstance() { | ||
return ExtensionList.lookupSingleton(Foo.class); | ||
} | ||
|
||
private String baz; | ||
|
||
@CheckForNull | ||
@Override | ||
public String getIconFileName() { | ||
return null; | ||
} | ||
|
||
@CheckForNull | ||
@Override | ||
public String getDisplayName() { | ||
return "Pitied Foos"; | ||
} | ||
|
||
@CheckForNull | ||
@Override | ||
public String getUrlName() { | ||
return "foo"; | ||
} | ||
|
||
@GET | ||
public void doBar(@QueryParameter String baz) { | ||
this.baz = baz; | ||
} | ||
|
||
@GET | ||
public void doIndex(@QueryParameter String baz) { | ||
this.baz = "index: " + baz; | ||
} | ||
} | ||
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters