From e01d8393b642bc7ca95a85018e8f7de907230d44 Mon Sep 17 00:00:00 2001 From: cnauroth Date: Fri, 5 Feb 2016 14:38:21 -0800 Subject: [PATCH] HADOOP-12758. Extend CSRF Filter with UserAgent Checks. Contributed by Larry McCay. (cherry picked from commit a37e423e8407c42988577d87907d13ce0432dda1) (cherry picked from commit 5752df23622495cd288747e61b3644c4137251db) --- .../hadoop-common/CHANGES.txt | 3 + .../http/RestCsrfPreventionFilter.java | 54 ++++++++++- .../http/TestRestCsrfPreventionFilter.java | 95 +++++++++++++++++-- 3 files changed, 143 insertions(+), 9 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index 9956fdeeb3a..a756b4d72e0 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -425,6 +425,9 @@ Release 2.8.0 - UNRELEASED HADOOP-12450. UserGroupInformation should not log at WARN level if no groups are found. (Elliott Clark via stevel) + HADOOP-12758. Extend CSRF Filter with UserAgent Checks + (Larry McCay via cnauroth) + BUG FIXES HADOOP-12617. SPNEGO authentication request to non-default realm gets diff --git a/hadoop-common/src/main/java/org/apache/hadoop/security/http/RestCsrfPreventionFilter.java b/hadoop-common/src/main/java/org/apache/hadoop/security/http/RestCsrfPreventionFilter.java index 50f95adea70..4f7f5bbdf29 100644 --- a/hadoop-common/src/main/java/org/apache/hadoop/security/http/RestCsrfPreventionFilter.java +++ b/hadoop-common/src/main/java/org/apache/hadoop/security/http/RestCsrfPreventionFilter.java @@ -19,6 +19,8 @@ package org.apache.hadoop.security.http; import java.io.IOException; import java.util.HashSet; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.Set; import javax.servlet.Filter; @@ -38,13 +40,18 @@ import javax.servlet.http.HttpServletResponse; * attempt as a bad request. */ public class RestCsrfPreventionFilter implements Filter { + public static final String HEADER_USER_AGENT = "User-Agent"; + public static final String BROWSER_USER_AGENT_PARAM = + "browser-useragents-regex"; public static final String CUSTOM_HEADER_PARAM = "custom-header"; public static final String CUSTOM_METHODS_TO_IGNORE_PARAM = "methods-to-ignore"; + static final String BROWSER_USER_AGENTS_DEFAULT = "^Mozilla.*,^Opera.*"; static final String HEADER_DEFAULT = "X-XSRF-HEADER"; static final String METHODS_TO_IGNORE_DEFAULT = "GET,OPTIONS,HEAD,TRACE"; private String headerName = HEADER_DEFAULT; private Set methodsToIgnore = null; + private Set browserUserAgents; @Override public void init(FilterConfig filterConfig) throws ServletException { @@ -59,6 +66,20 @@ public class RestCsrfPreventionFilter implements Filter { } else { parseMethodsToIgnore(METHODS_TO_IGNORE_DEFAULT); } + + String agents = filterConfig.getInitParameter(BROWSER_USER_AGENT_PARAM); + if (agents == null) { + agents = BROWSER_USER_AGENTS_DEFAULT; + } + parseBrowserUserAgents(agents); + } + + void parseBrowserUserAgents(String userAgents) { + String[] agentsArray = userAgents.split(","); + browserUserAgents = new HashSet(); + for (String patternString : agentsArray) { + browserUserAgents.add(Pattern.compile(patternString)); + } } void parseMethodsToIgnore(String mti) { @@ -69,17 +90,46 @@ public class RestCsrfPreventionFilter implements Filter { } } + /** + * This method interrogates the User-Agent String and returns whether it + * refers to a browser. If its not a browser, then the requirement for the + * CSRF header will not be enforced; if it is a browser, the requirement will + * be enforced. + *

+ * A User-Agent String is considered to be a browser if it matches + * any of the regex patterns from browser-useragent-regex; the default + * behavior is to consider everything a browser that matches the following: + * "^Mozilla.*,^Opera.*". Subclasses can optionally override + * this method to use different behavior. + * + * @param userAgent The User-Agent String, or null if there isn't one + * @return true if the User-Agent String refers to a browser, false if not + */ + protected boolean isBrowser(String userAgent) { + if (userAgent == null) { + return false; + } + for (Pattern pattern : browserUserAgents) { + Matcher matcher = pattern.matcher(userAgent); + if (matcher.matches()) { + return true; + } + } + return false; + } + @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { HttpServletRequest httpRequest = (HttpServletRequest)request; - if (methodsToIgnore.contains(httpRequest.getMethod()) || + if (!isBrowser(httpRequest.getHeader(HEADER_USER_AGENT)) || + methodsToIgnore.contains(httpRequest.getMethod()) || httpRequest.getHeader(headerName) != null) { chain.doFilter(request, response); } else { ((HttpServletResponse)response).sendError( HttpServletResponse.SC_BAD_REQUEST, - "Missing Required Header for Vulnerability Protection"); + "Missing Required Header for CSRF Vulnerability Protection"); } } diff --git a/hadoop-common/src/test/java/org/apache/hadoop/security/http/TestRestCsrfPreventionFilter.java b/hadoop-common/src/test/java/org/apache/hadoop/security/http/TestRestCsrfPreventionFilter.java index adf89f5b44d..29dccd3a200 100644 --- a/hadoop-common/src/test/java/org/apache/hadoop/security/http/TestRestCsrfPreventionFilter.java +++ b/hadoop-common/src/test/java/org/apache/hadoop/security/http/TestRestCsrfPreventionFilter.java @@ -34,8 +34,12 @@ import org.mockito.Mockito; public class TestRestCsrfPreventionFilter { + private static final String NON_BROWSER = "java"; + private static final String BROWSER_AGENT = + "Mozilla/5.0 (compatible; U; ABrowse 0.6; Syllable)" + + " AppleWebKit/420+ (KHTML, like Gecko)"; private static final String EXPECTED_MESSAGE = - "Missing Required Header for Vulnerability Protection"; + "Missing Required Header for CSRF Vulnerability Protection"; private static final String X_CUSTOM_HEADER = "X-CUSTOM_HEADER"; @Test @@ -52,7 +56,9 @@ public class TestRestCsrfPreventionFilter { // CSRF has not been sent HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_DEFAULT)). - thenReturn(null); + thenReturn(null); + Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_USER_AGENT)). + thenReturn(BROWSER_AGENT); // Objects to verify interactions based on request HttpServletResponse mockRes = Mockito.mock(HttpServletResponse.class); @@ -68,6 +74,71 @@ public class TestRestCsrfPreventionFilter { Mockito.verifyZeroInteractions(mockChain); } + @Test + public void testNoHeaderCustomAgentConfig_badRequest() + throws ServletException, IOException { + // Setup the configuration settings of the server + FilterConfig filterConfig = Mockito.mock(FilterConfig.class); + Mockito.when(filterConfig.getInitParameter( + RestCsrfPreventionFilter.CUSTOM_HEADER_PARAM)).thenReturn(null); + Mockito.when(filterConfig.getInitParameter( + RestCsrfPreventionFilter.CUSTOM_METHODS_TO_IGNORE_PARAM)). + thenReturn(null); + Mockito.when(filterConfig.getInitParameter( + RestCsrfPreventionFilter.BROWSER_USER_AGENT_PARAM)). + thenReturn("^Mozilla.*,^Opera.*,curl"); + + // CSRF has not been sent + HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); + Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_DEFAULT)). + thenReturn(null); + Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_USER_AGENT)). + thenReturn("curl"); + + // Objects to verify interactions based on request + HttpServletResponse mockRes = Mockito.mock(HttpServletResponse.class); + FilterChain mockChain = Mockito.mock(FilterChain.class); + + // Object under test + RestCsrfPreventionFilter filter = new RestCsrfPreventionFilter(); + filter.init(filterConfig); + filter.doFilter(mockReq, mockRes, mockChain); + + verify(mockRes, atLeastOnce()).sendError( + HttpServletResponse.SC_BAD_REQUEST, EXPECTED_MESSAGE); + Mockito.verifyZeroInteractions(mockChain); + } + + @Test + public void testNoHeaderDefaultConfigNonBrowser_goodRequest() + throws ServletException, IOException { + // Setup the configuration settings of the server + FilterConfig filterConfig = Mockito.mock(FilterConfig.class); + Mockito.when(filterConfig.getInitParameter( + RestCsrfPreventionFilter.CUSTOM_HEADER_PARAM)).thenReturn(null); + Mockito.when(filterConfig.getInitParameter( + RestCsrfPreventionFilter.CUSTOM_METHODS_TO_IGNORE_PARAM)). + thenReturn(null); + + // CSRF has not been sent + HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); + Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_DEFAULT)). + thenReturn(null); + Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_USER_AGENT)). + thenReturn(NON_BROWSER); + + // Objects to verify interactions based on request + HttpServletResponse mockRes = Mockito.mock(HttpServletResponse.class); + FilterChain mockChain = Mockito.mock(FilterChain.class); + + // Object under test + RestCsrfPreventionFilter filter = new RestCsrfPreventionFilter(); + filter.init(filterConfig); + filter.doFilter(mockReq, mockRes, mockChain); + + Mockito.verify(mockChain).doFilter(mockReq, mockRes); + } + @Test public void testHeaderPresentDefaultConfig_goodRequest() throws ServletException, IOException { @@ -136,9 +207,11 @@ public class TestRestCsrfPreventionFilter { Mockito.when(filterConfig.getInitParameter( RestCsrfPreventionFilter.CUSTOM_METHODS_TO_IGNORE_PARAM)). thenReturn(null); + HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); + Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_USER_AGENT)). + thenReturn(BROWSER_AGENT); // CSRF has not been sent - HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_DEFAULT)). thenReturn(null); @@ -164,9 +237,11 @@ public class TestRestCsrfPreventionFilter { Mockito.when(filterConfig.getInitParameter( RestCsrfPreventionFilter.CUSTOM_METHODS_TO_IGNORE_PARAM)). thenReturn(""); + HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); + Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_USER_AGENT)). + thenReturn(BROWSER_AGENT); // CSRF has not been sent - HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_DEFAULT)). thenReturn(null); Mockito.when(mockReq.getMethod()). @@ -194,9 +269,11 @@ public class TestRestCsrfPreventionFilter { Mockito.when(filterConfig.getInitParameter( RestCsrfPreventionFilter.CUSTOM_METHODS_TO_IGNORE_PARAM)). thenReturn("GET"); + HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); + Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_USER_AGENT)). + thenReturn(BROWSER_AGENT); // CSRF has not been sent - HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_DEFAULT)). thenReturn(null); Mockito.when(mockReq.getMethod()). @@ -224,9 +301,11 @@ public class TestRestCsrfPreventionFilter { Mockito.when(filterConfig.getInitParameter( RestCsrfPreventionFilter.CUSTOM_METHODS_TO_IGNORE_PARAM)). thenReturn("GET,OPTIONS"); + HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); + Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_USER_AGENT)). + thenReturn(BROWSER_AGENT); // CSRF has not been sent - HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_DEFAULT)). thenReturn(null); Mockito.when(mockReq.getMethod()). @@ -254,9 +333,11 @@ public class TestRestCsrfPreventionFilter { Mockito.when(filterConfig.getInitParameter( RestCsrfPreventionFilter.CUSTOM_METHODS_TO_IGNORE_PARAM)). thenReturn("GET,OPTIONS"); + HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); + Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_USER_AGENT)). + thenReturn(BROWSER_AGENT); // CSRF has not been sent - HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class); Mockito.when(mockReq.getHeader(RestCsrfPreventionFilter.HEADER_DEFAULT)). thenReturn(null); Mockito.when(mockReq.getMethod()).