This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new b9fafd4cd9 Add best efforts protection for a mis-configured Valve
b9fafd4cd9 is described below
commit b9fafd4cd9d29f7a685d572bf3bb43a1d692d48b
Author: Mark Thomas <[email protected]>
AuthorDate: Fri Aug 28 09:48:43 2026 +0100
Add best efforts protection for a mis-configured Valve
---
.../valves/CrawlerSessionManagerValve.java | 42 ++++---
.../apache/catalina/valves/LocalStrings.properties | 2 +
.../valves/TestCrawlerSessionManagerValve.java | 49 ++++----
.../TestCrawlerSessionManagerValveIntegration.java | 131 +++++++++++++++++++++
webapps/docs/changelog.xml | 5 +
webapps/docs/config/valve.xml | 6 +
6 files changed, 199 insertions(+), 36 deletions(-)
diff --git a/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java
b/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java
index 553588e7b8..437e9e1e19 100644
--- a/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java
+++ b/java/org/apache/catalina/valves/CrawlerSessionManagerValve.java
@@ -26,13 +26,13 @@ import java.util.function.Function;
import java.util.regex.Pattern;
import jakarta.servlet.ServletException;
-import jakarta.servlet.http.HttpSession;
import jakarta.servlet.http.HttpSessionBindingEvent;
import jakarta.servlet.http.HttpSessionBindingListener;
import org.apache.catalina.Context;
import org.apache.catalina.Host;
import org.apache.catalina.LifecycleException;
+import org.apache.catalina.Session;
import org.apache.catalina.connector.Request;
import org.apache.catalina.connector.Response;
import org.apache.juli.logging.Log;
@@ -284,24 +284,36 @@ public class CrawlerSessionManagerValve extends ValveBase
{
getNext().invoke(request, response);
if (isBot) {
- if (sessionId == null) {
- // Has bot just created a session, if so make a note of it
- HttpSession s = request.getSession(false);
- if (s != null) {
- clientIdSessionId.put(clientIdentifier, s.getId());
- // #valueUnbound() will be called on session expiration
- s.setAttribute(this.getClass().getName(),
- new
CrawlerHttpSessionBindingListener(clientIdSessionId, clientIdentifier));
- s.setMaxInactiveInterval(sessionInactiveInterval);
-
+ Session s = request.getSessionInternal(false);
+ if (s == null || s.getPrincipal() == null) {
+ if (sessionId == null) {
+ // Has bot just created a session, if so make a note of it
+ if (s != null) {
+ clientIdSessionId.put(clientIdentifier, s.getId());
+ // #valueUnbound() will be called on session expiration
+ s.getSession().setAttribute(this.getClass().getName(),
+ new
CrawlerHttpSessionBindingListener(clientIdSessionId, clientIdentifier));
+ s.setMaxInactiveInterval(sessionInactiveInterval);
+
+ if (log.isTraceEnabled()) {
+ log.trace(request.hashCode() + ": New bot session.
SessionID=" + s.getId());
+ }
+ }
+ } else {
if (log.isTraceEnabled()) {
- log.trace(request.hashCode() + ": New bot session.
SessionID=" + s.getId());
+ log.trace(request.hashCode() + ": Bot session
accessed. SessionID=" + sessionId);
}
}
} else {
- if (log.isTraceEnabled()) {
- log.trace(request.hashCode() + ": Bot session accessed.
SessionID=" + sessionId);
- }
+ /*
+ * The session is authenticated. That shouldn't happen and
indicates some form of mis-configuration.
+ * Make a best efforts (i.e. this is hardening against
mis-configuration, NOT vulnerability mitigation)
+ * attempt to protect against the authenticated session being
shared.
+ */
+ s.expire();
+ clientIdSessionId.remove(clientIdentifier, s.getIdInternal());
+ log.warn(sm.getString("crawlerSessionManagerValve.principal",
clientIdentifier,
+ request.getHeader("User-Agent"), s.getPrincipal()));
}
}
}
diff --git a/java/org/apache/catalina/valves/LocalStrings.properties
b/java/org/apache/catalina/valves/LocalStrings.properties
index 0cb4d4545b..4a9383a247 100644
--- a/java/org/apache/catalina/valves/LocalStrings.properties
+++ b/java/org/apache/catalina/valves/LocalStrings.properties
@@ -26,6 +26,8 @@ accessLogValve.rotateFail=Failed to rotate access log
accessLogValve.unsupportedEncoding=Failed to set encoding to [{0}], will use
the system default character set.
accessLogValve.writeFail=Failed to write log message [{0}]
+crawlerSessionManagerValve.principal=The crawler session being used by client
[{0}] with user agent [{1}] was authenticated as [{2}] so the session was
expired.
+
# Default error page should not have '[' ']' symbols around substituted text
fragments.
# https://bz.apache.org/bugzilla/show_bug.cgi?id=61134
errorReportValve.contentTypeFail=Failure to set the content-type of response
diff --git
a/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java
b/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java
index c8131564c5..64c42d40e5 100644
--- a/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java
+++ b/test/org/apache/catalina/valves/TestCrawlerSessionManagerValve.java
@@ -33,6 +33,7 @@ import org.junit.Test;
import org.apache.catalina.Context;
import org.apache.catalina.Host;
import org.apache.catalina.Manager;
+import org.apache.catalina.Session;
import org.apache.catalina.Valve;
import org.apache.catalina.connector.Request;
import org.apache.catalina.connector.Response;
@@ -40,7 +41,6 @@ import org.apache.catalina.core.StandardContext;
import org.apache.catalina.session.StandardManager;
import org.apache.catalina.session.StandardSession;
import org.easymock.EasyMock;
-import org.easymock.IExpectationSetters;
public class TestCrawlerSessionManagerValve {
@@ -58,14 +58,15 @@ public class TestCrawlerSessionManagerValve {
valve.setCrawlerIps("216\\.58\\.206\\.174");
valve.setCrawlerUserAgents(valve.getCrawlerUserAgents());
valve.setNext(EasyMock.createMock(Valve.class));
- HttpSession session = createSessionExpectations(valve, true);
+ HttpSession httpSession = EasyMock.createMock(HttpSession.class);
+ Session session = createSessionExpectations(valve, httpSession, true);
Request request = createRequestExpectations("216.58.206.174", session,
true);
- EasyMock.replay(request, session);
+ EasyMock.replay(request, session, httpSession);
valve.invoke(request, EasyMock.createMock(Response.class));
- EasyMock.verify(request, session);
+ EasyMock.verify(request, session, httpSession);
}
@Test
@@ -74,14 +75,15 @@ public class TestCrawlerSessionManagerValve {
valve.setCrawlerIps("216\\.58\\.206\\.174");
valve.setCrawlerUserAgents(valve.getCrawlerUserAgents());
valve.setNext(EasyMock.createMock(Valve.class));
- HttpSession session = createSessionExpectations(valve, false);
+ HttpSession httpSession = EasyMock.createMock(HttpSession.class);
+ Session session = createSessionExpectations(valve, httpSession, false);
Request request = createRequestExpectations("127.0.0.1", session,
false);
- EasyMock.replay(request, session);
+ EasyMock.replay(request, session, httpSession);
valve.invoke(request, EasyMock.createMock(Response.class));
- EasyMock.verify(request, session);
+ EasyMock.verify(request, session, httpSession);
}
@Test
@@ -137,36 +139,41 @@ public class TestCrawlerSessionManagerValve {
private void verifyCrawlingLocalhost(CrawlerSessionManagerValve valve,
String hostname)
throws IOException, ServletException {
- HttpSession session = createSessionExpectations(valve, true);
+ HttpSession httpSession = EasyMock.createMock(HttpSession.class);
+ Session session = createSessionExpectations(valve, httpSession, true);
Request request = createRequestExpectations("127.0.0.1", session,
true, hostname, "/examples", "tomcatBot 1.0");
- EasyMock.replay(request, session);
+ EasyMock.replay(request, session, httpSession);
valve.invoke(request, EasyMock.createMock(Response.class));
- EasyMock.verify(request, session);
+ EasyMock.verify(request, session, httpSession);
}
private void verifyCrawlingContext(CrawlerSessionManagerValve valve,
String contextPath)
throws IOException, ServletException {
- HttpSession session = createSessionExpectations(valve, true);
+ HttpSession httpSession = EasyMock.createMock(HttpSession.class);
+ Session session = createSessionExpectations(valve, httpSession, true);
Request request = createRequestExpectations("127.0.0.1", session,
true, "localhost", contextPath,
"tomcatBot 1.0");
- EasyMock.replay(request, session);
+ EasyMock.replay(request, session, httpSession);
valve.invoke(request, EasyMock.createMock(Response.class));
- EasyMock.verify(request, session);
+ EasyMock.verify(request, session, httpSession);
}
- private HttpSession createSessionExpectations(CrawlerSessionManagerValve
valve, boolean isBot) {
- HttpSession session = EasyMock.createMock(HttpSession.class);
+ private Session createSessionExpectations(CrawlerSessionManagerValve
valve, HttpSession httpSession,
+ boolean isBot) {
+ Session session = EasyMock.createMock(Session.class);
if (isBot) {
- EasyMock.expect(session.getId()).andReturn("id").times(1);
- session.setAttribute(EasyMock.eq(valve.getClass().getName()),
+ EasyMock.expect(session.getPrincipal()).andReturn(null);
+ EasyMock.expect(session.getId()).andReturn("id");
+ EasyMock.expect(session.getSession()).andReturn(httpSession);
+ httpSession.setAttribute(EasyMock.eq(valve.getClass().getName()),
EasyMock.anyObject(HttpSessionBindingListener.class));
EasyMock.expectLastCall();
session.setMaxInactiveInterval(60);
@@ -176,11 +183,11 @@ public class TestCrawlerSessionManagerValve {
}
- private Request createRequestExpectations(String ip, HttpSession session,
boolean isBot) {
+ private Request createRequestExpectations(String ip, Session session,
boolean isBot) {
return createRequestExpectations(ip, session, isBot, "localhost",
"/examples", "something 1.0");
}
- private Request createRequestExpectations(String ip, HttpSession session,
boolean isBot, String hostname,
+ private Request createRequestExpectations(String ip, Session session,
boolean isBot, String hostname,
String contextPath, String userAgent) {
Request request = EasyMock.createMock(Request.class);
EasyMock.expect(request.getRemoteAddr()).andReturn(ip);
@@ -188,9 +195,9 @@ public class TestCrawlerSessionManagerValve {
EasyMock.expect(request.getHost()).andReturn(simpleHostWithName(hostname));
EasyMock.expect(request.getHost()).andReturn(simpleHostWithName(hostname));
EasyMock.expect(request.getContext()).andReturn(simpleContextWithName(contextPath));
- IExpectationSetters<HttpSession> setter =
EasyMock.expect(request.getSession(false)).andReturn(null);
+ EasyMock.expect(request.getSession(false)).andReturn(null);
if (isBot) {
- setter.andReturn(session);
+
EasyMock.expect(request.getSessionInternal(false)).andReturn(session);
}
EasyMock.expect(request.getHeaders("user-agent"))
.andAnswer(() ->
Collections.enumeration(Arrays.asList(userAgent)));
diff --git
a/test/org/apache/catalina/valves/TestCrawlerSessionManagerValveIntegration.java
b/test/org/apache/catalina/valves/TestCrawlerSessionManagerValveIntegration.java
new file mode 100644
index 0000000000..82c5f667c5
--- /dev/null
+++
b/test/org/apache/catalina/valves/TestCrawlerSessionManagerValveIntegration.java
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina.valves;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.security.Principal;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.Context;
+import org.apache.catalina.authenticator.BasicAuthenticator;
+import org.apache.catalina.startup.TesterMapRealm;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.util.buf.ByteChunk;
+import org.apache.tomcat.util.descriptor.web.LoginConfig;
+import org.apache.tomcat.util.descriptor.web.SecurityCollection;
+import org.apache.tomcat.util.descriptor.web.SecurityConstraint;
+
+public class TestCrawlerSessionManagerValveIntegration extends TomcatBaseTest {
+
+ private static final String USER = "alice";
+ private static final String PASSWORD = "alice-password";
+ private static final String ROLE = "account-holder";
+ private static final String SECRET = "alice-private-token";
+
+ /**
+ * Test for handling of Valve mis-configuration that results in one of the
clients identified as a crawler creating
+ * an authenticated session. The Valve tries (but does not guarantee) to
protect against this.
+ * <p>
+ * Alice authenticates with a crawler-classified User-Agent. A separate
HTTP request with no Authorization or Cookie
+ * header uses a different crawler-classified User-Agent from the same
apparent address.
+ * <p>
+ * The separate request should not see Alice's authenticated session.
+ *
+ * @throws Exception If the test experiences an unexpected error
+ */
+ @Test
+ public void testUnauthenticatedCrawlerMustNotReuseAuthenticatedSession()
throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+ Context context = tomcat.addContext("", null);
+
+ TesterMapRealm realm = new TesterMapRealm();
+ realm.addUser(USER, PASSWORD);
+ realm.addUserRole(USER, ROLE);
+ context.setRealm(realm);
+ context.setLoginConfig(new LoginConfig(HttpServletRequest.BASIC_AUTH,
"crawler-test", null, null));
+
+ context.getPipeline().addValve(new CrawlerSessionManagerValve());
+ BasicAuthenticator authenticator = new BasicAuthenticator();
+ authenticator.setAlwaysUseSession(true);
+ context.getPipeline().addValve(authenticator);
+
+ Tomcat.addServlet(context, "account", new HttpServlet() {
+ @Override
+ protected void doGet(HttpServletRequest request,
HttpServletResponse response) throws IOException {
+ Principal principal = request.getUserPrincipal();
+ response.setContentType("text/plain");
+ response.getWriter().print("principal=" + principal.getName()
+ " secret=" + SECRET + " session=" +
+ request.getSession(true).getId());
+ }
+ });
+ context.addServletMapping("/account", "account");
+ context.addSecurityRole(ROLE);
+ SecurityCollection collection = new SecurityCollection();
+ collection.addPattern("/account");
+ SecurityConstraint constraint = new SecurityConstraint();
+ constraint.addCollection(collection);
+ constraint.addAuthRole(ROLE);
+ context.addConstraint(constraint);
+
+ tomcat.start();
+
+ ByteChunk baselineBody = new ByteChunk();
+ int baselineStatus = getUrl(url(), baselineBody,
headers("ordinary-browser/1.0", null), null);
+ Assert.assertEquals("protected resource must reject an unauthenticated
ordinary client", 401, baselineStatus);
+
+ String credentials = "Basic " +
+ Base64.getEncoder().encodeToString((USER + ":" +
PASSWORD).getBytes(StandardCharsets.ISO_8859_1));
+ ByteChunk aliceBody = new ByteChunk();
+ int aliceStatus = getUrl(url(), aliceBody, headers("aliceBot/1.0",
credentials), null);
+ Assert.assertEquals(200, aliceStatus);
+ Assert.assertTrue(aliceBody.toString().contains("principal=alice
secret=" + SECRET));
+
+ // New client request: no Cookie and no Authorization. It only shares
+ // the server-observed address/Host/Context and matches the bot
pattern.
+ ByteChunk malloryBody = new ByteChunk();
+ int malloryStatus = getUrl(url(), malloryBody,
headers("malloryBot/9.9", null), null);
+
+ Assert.assertEquals("unauthenticated crawler inherited Alice's session
and response: " + malloryBody, 401,
+ malloryStatus);
+ Assert.assertFalse(malloryBody.toString().contains(SECRET));
+ }
+
+ private String url() {
+ return "http://localhost:"; + getPort() + "/account";
+ }
+
+ private static Map<String,List<String>> headers(String userAgent, String
authorization) {
+ Map<String,List<String>> headers = new HashMap<>();
+ headers.put("user-agent", List.of(userAgent));
+ if (authorization != null) {
+ headers.put("authorization", List.of(authorization));
+ }
+ return headers;
+ }
+}
\ No newline at end of file
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 81865b721a..1cb95ee48a 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -289,6 +289,11 @@
Resolve null or missing rewrite substitutions as an empty string, to
align with the mod_rewrite behavior. (remm)
</fix>
+ <add>
+ Add a best efforts protection in the
+ <code>CrawlerSessionManagerValve</code> against crawlers being
+ associated with an authenticated session. (markt)
+ </add>
</changelog>
</subsection>
<subsection name="Coyote">
diff --git a/webapps/docs/config/valve.xml b/webapps/docs/config/valve.xml
index 6e6ba1f1dc..08435424f3 100644
--- a/webapps/docs/config/valve.xml
+++ b/webapps/docs/config/valve.xml
@@ -2435,6 +2435,12 @@ RequestHeader unset SSL_CLIENT_ESCAPED_CERT</source>
should be defined before this valve to ensure that the correct client IP
address is presented to this valve.</p>
+ <p>Care should be taken when configuring this Valve to ensure that crawlers
+ are not associated with the authenticated session of a genuine user. In
+ particular, the <code>crawlerUserAgents</code> must be chosen with care.
The
+ Valve will attempt (but does NOT guarantee) to detect this situation and
+ will expire the authenticated session if detected.</p>
+
</subsection>
<subsection name="Attributes">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
