From 30e7e3f3d3ffe7d4a3f921acfcdb81c53ada988c Mon Sep 17 00:00:00 2001 From: Cesar Hernandez Date: Thu, 13 Apr 2023 19:34:50 -0600 Subject: [PATCH] Backport for CVE-2023-28708 --- build.properties.default | 2 +- java/org/apache/catalina/Globals.java | 7 ++ .../apache/catalina/connector/Request.java | 14 +++ .../catalina/filters/RemoteIpFilter.java | 6 +- .../catalina/filters/TestRemoteIpFilter.java | 99 ++++++++++++++----- webapps/docs/changelog.xml | 5 + 6 files changed, 104 insertions(+), 29 deletions(-) diff --git a/build.properties.default b/build.properties.default index 9ee1bd3598e3..b37a0a914890 100644 --- a/build.properties.default +++ b/build.properties.default @@ -27,7 +27,7 @@ version.major=7 version.minor=0 version.build=104 version.patch=0 -version.suffix=-SP.7 +version.suffix=-SP.8 # ----- Source control flags ----- git.branch=7.0.x diff --git a/java/org/apache/catalina/Globals.java b/java/org/apache/catalina/Globals.java index 46c30e1ff143..a33286777ec4 100644 --- a/java/org/apache/catalina/Globals.java +++ b/java/org/apache/catalina/Globals.java @@ -222,6 +222,13 @@ public final class Globals { org.apache.coyote.Constants.SENDFILE_SUPPORTED_ATTR; + /** + * The request attribute that is set to the value of {@code Boolean.TRUE} + * if {@link org.apache.catalina.filters.RemoteIpFilter} determines + * that this request was submitted via a secure channel. + */ + public static final String REMOTE_IP_FILTER_SECURE = "org.apache.catalina.filters.RemoteIpFilter.secure"; + /** * The request attribute that can be used by a servlet to pass * to the connector the name of the file that is to be served diff --git a/java/org/apache/catalina/connector/Request.java b/java/org/apache/catalina/connector/Request.java index 2f14ac66a387..2c998a20379e 100644 --- a/java/org/apache/catalina/connector/Request.java +++ b/java/org/apache/catalina/connector/Request.java @@ -3736,6 +3736,20 @@ public void set(Request request, String name, Object value) { // NO-OP } }); + specialAttributes.put(Globals.REMOTE_IP_FILTER_SECURE, + new SpecialAttributeAdapter() { + @Override + public Object get(Request request, String name) { + return Boolean.valueOf(request.isSecure()); + } + + @Override + public void set(Request request, String name, Object value) { + if (value instanceof Boolean) { + request.setSecure(((Boolean) value).booleanValue()); + } + } + }); for (SimpleDateFormat sdf : formatsTemplate) { sdf.setTimeZone(GMT_ZONE); diff --git a/java/org/apache/catalina/filters/RemoteIpFilter.java b/java/org/apache/catalina/filters/RemoteIpFilter.java index 188176d9d504..da8f018c8308 100644 --- a/java/org/apache/catalina/filters/RemoteIpFilter.java +++ b/java/org/apache/catalina/filters/RemoteIpFilter.java @@ -564,10 +564,6 @@ public int getServerPort() { return serverPort; } - @Override - public boolean isSecure() { - return secure; - } public void removeHeader(String name) { Map.Entry> header = getHeaderEntry(name); @@ -608,7 +604,7 @@ public void setScheme(String scheme) { } public void setSecure(boolean secure) { - this.secure = secure; + super.getRequest().setAttribute(Globals.REMOTE_IP_FILTER_SECURE, Boolean.valueOf(secure)); } public void setServerName(String serverName) { diff --git a/test/org/apache/catalina/filters/TestRemoteIpFilter.java b/test/org/apache/catalina/filters/TestRemoteIpFilter.java index 254dde773b76..b50f5f4d7067 100644 --- a/test/org/apache/catalina/filters/TestRemoteIpFilter.java +++ b/test/org/apache/catalina/filters/TestRemoteIpFilter.java @@ -82,15 +82,21 @@ public static class MockHttpServlet extends HttpServlet { private static final long serialVersionUID = 1L; - private transient HttpServletRequest request; - - public HttpServletRequest getRequest() { - return request; - } + public String remoteAddr; + public String remoteHost; + public String scheme; + public String serverName; + public int serverPort; + public boolean isSecure; @Override public void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { - this.request = request; + this.isSecure = request.isSecure(); + this.remoteAddr = request.getRemoteAddr(); + this.remoteHost = request.getRemoteHost(); + this.scheme = request.getScheme(); + this.serverName = request.getServerName(); + this.serverPort = request.getServerPort(); PrintWriter writer = response.getWriter(); writer.println("request.remoteAddr=" + request.getRemoteAddr()); @@ -130,16 +136,6 @@ public void setScheme(String scheme) { getCoyoteRequest().scheme().setString(scheme); } - @Override - public void setAttribute(String name, Object value) { - getCoyoteRequest().getAttributes().put(name, value); - } - - @Override - public Object getAttribute(String name) { - return getCoyoteRequest().getAttributes().get(name); - } - @Override public String getServerName() { return "localhost"; @@ -771,16 +767,73 @@ public void testWithTomcatServer() throws Exception { // VALIDATE Assert.assertEquals(HttpURLConnection.HTTP_OK, httpURLConnection.getResponseCode()); - HttpServletRequest request = mockServlet.getRequest(); - Assert.assertNotNull(request); + //HttpServletRequest request = mockServlet.getRequest(); + //Assert.assertNotNull(request); // VALIDATE X-FORWARDED-FOR - Assert.assertEquals(expectedRemoteAddr, request.getRemoteAddr()); - Assert.assertEquals(expectedRemoteAddr, request.getRemoteHost()); + Assert.assertEquals(expectedRemoteAddr, mockServlet.remoteAddr); + Assert.assertEquals(expectedRemoteAddr, mockServlet.remoteHost); // VALIDATE X-FORWARDED-PROTO - Assert.assertTrue(request.isSecure()); - Assert.assertEquals("https", request.getScheme()); - Assert.assertEquals(443, request.getServerPort()); + Assert.assertTrue(mockServlet.isSecure); + Assert.assertEquals("https", mockServlet.scheme); + Assert.assertEquals(443, mockServlet.serverPort); + } + + @Test + public void testJSessionIdSecureAttributeMissing() throws Exception { + + // mostly default configuration : enable "x-forwarded-proto" + Map remoteIpFilterParameter = new HashMap(); + remoteIpFilterParameter.put("protocolHeader", "x-forwarded-proto"); + + // SETUP + Tomcat tomcat = getTomcatInstance(); + Context root = tomcat.addContext("", TEMP_DIR); + + FilterDef filterDef = new FilterDef(); + filterDef.getParameterMap().putAll(remoteIpFilterParameter); + filterDef.setFilterClass(RemoteIpFilter.class.getName()); + filterDef.setFilterName(RemoteIpFilter.class.getName()); + + root.addFilterDef(filterDef); + + FilterMap filterMap = new FilterMap(); + filterMap.setFilterName(RemoteIpFilter.class.getName()); + filterMap.addURLPattern("*"); + root.addFilterMap(filterMap); + + Bug66471Servlet bug66471Servlet = new Bug66471Servlet(); + + Tomcat.addServlet(root, bug66471Servlet.getClass().getName(), bug66471Servlet); + root.addServletMapping("/test", bug66471Servlet.getClass().getName()); + + getTomcatInstance().start(); + + Map> resHeaders = new HashMap>(); + Map> reqHeaders = new HashMap>(); + String expectedRemoteAddr = "my-remote-addr"; + List forwardedFor = new ArrayList(1); + forwardedFor.add(expectedRemoteAddr); + List forwardedProto = new ArrayList(1); + forwardedProto.add("https"); + reqHeaders.put("x-forwarded-for", forwardedFor); + reqHeaders.put("x-forwarded-proto", forwardedProto); + + getUrl("http://localhost:" + tomcat.getConnector().getLocalPort() + + "/test", null, reqHeaders, resHeaders); + String setCookie = resHeaders.get("Set-Cookie").get(0); + Assert.assertTrue(setCookie.contains("Secure")); + Assert.assertTrue(bug66471Servlet.isSecure.booleanValue()); + } + public static class Bug66471Servlet extends HttpServlet { + private static final long serialVersionUID = 1L; + public Boolean isSecure; + + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { + req.getSession(); + isSecure = (Boolean) req.getAttribute(Globals.REMOTE_IP_FILTER_SECURE); + } } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 84e37febbf3d..aac602d3df4f 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -120,6 +120,11 @@ Examples. Fix CVE-2022-34305, a low severity XSS vulnerability in the Form authentication example. (markt) + + 66471: Fix JSessionId secure attribute missing When + RemoteIpFilter determines that this request was submitted + via a secure channel. (lihan) +