This is an automated email from the ASF dual-hosted git repository. anmolnar pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/zookeeper.git
commit de2077400fd7b504b8d2bf4e21111d19009466aa Author: Andor Molnar <[email protected]> AuthorDate: Thu Sep 17 14:56:06 2026 -0500 SASL authzid: add extra validation on client port to mirror quorum check --- .../server/auth/SaslServerCallbackHandler.java | 36 ++- .../server/auth/SaslServerCallbackHandlerTest.java | 250 +++++++++++++++++++++ 2 files changed, 282 insertions(+), 4 deletions(-) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java index 79c57e462e..5ab7f70fec 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java @@ -27,6 +27,7 @@ import javax.security.auth.callback.UnsupportedCallbackException; import javax.security.sasl.AuthorizeCallback; import javax.security.sasl.RealmCallback; +import org.apache.zookeeper.common.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -61,7 +62,8 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException { private void handleNameCallback(NameCallback nc) { // check to see if this user is in the user password database. if (credentials.get(nc.getDefaultName()) == null) { - LOG.warn("User '{}' not found in list of DIGEST-MD5 authenticateable users.", nc.getDefaultName()); + LOG.warn("User '{}' not found in list of DIGEST-MD5 authenticateable users.", + StringUtils.sanitizeForLog(nc.getDefaultName())); return; } nc.setName(nc.getDefaultName()); @@ -80,7 +82,7 @@ private void handlePasswordCallback(PasswordCallback pc) { } private void handleRealmCallback(RealmCallback rc) { - LOG.debug("client supplied realm: {}", rc.getDefaultText()); + LOG.debug("client supplied realm: {}", StringUtils.sanitizeForLog(rc.getDefaultText())); rc.setText(rc.getDefaultText()); } @@ -88,8 +90,34 @@ private void handleAuthorizeCallback(AuthorizeCallback ac) { String authenticationID = ac.getAuthenticationID(); String authorizationID = ac.getAuthorizationID(); + // A client must not be allowed to authorize as an identity other than the + // one it authenticated as. The SASL authorizationID (authzid) is an + // attacker-controlled field of the client token. If it differs from the + // authenticated identity we must reject it here: otherwise, whenever the + // identity canonicalization below fails -- which happens for any principal + // not covered by zookeeper.security.auth_to_local rules, i.e. every + // cross-realm principal under the default DEFAULT rule -- the JDK SASL + // server would fall back to returning the client-requested authzid as the + // negotiated identity (AuthorizeCallback.getAuthorizedID()), letting a + // low-privilege client assume any identity (e.g. "super"). This mirrors the + // equality check in SaslQuorumServerCallbackHandler and + // SaslClientCallbackHandler. + if (authenticationID == null || !authenticationID.equals(authorizationID)) { + // authenticationID and authorizationID are unvalidated, client-controlled + // strings from the SASL token; sanitize before logging to prevent log + // injection (CWE-117). WARN, not ERROR: a rejected client is not an + // operator-actionable server fault, and ERROR should stay meaningful for + // alerting since a client can repeat failed attempts at will. + LOG.warn("Client attempted to authorize as a different identity: " + + "authenticationID={}; requested authorizationID={}. Denying authorization.", + StringUtils.sanitizeForLog(authenticationID), + StringUtils.sanitizeForLog(authorizationID)); + ac.setAuthorized(false); + return; + } + LOG.info("Successfully authenticated client: authenticationID={}; authorizationID={}.", - authenticationID, authorizationID); + StringUtils.sanitizeForLog(authenticationID), StringUtils.sanitizeForLog(authorizationID)); ac.setAuthorized(true); // canonicalize authorization id according to system properties: @@ -104,7 +132,7 @@ private void handleAuthorizeCallback(AuthorizeCallback ac) { if (shouldAppendRealm(kerberosName)) { userNameBuilder.append("@").append(kerberosName.getRealm()); } - LOG.info("Setting authorizedID: {}", userNameBuilder); + LOG.info("Setting authorizedID: {}", StringUtils.sanitizeForLog(userNameBuilder.toString())); ac.setAuthorizedID(userNameBuilder.toString()); } catch (IOException e) { LOG.error("Failed to set name based on Kerberos authentication rules.", e); diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandlerTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandlerTest.java new file mode 100644 index 0000000000..d303fc035b --- /dev/null +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/auth/SaslServerCallbackHandlerTest.java @@ -0,0 +1,250 @@ +/* + * 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.zookeeper.server.auth; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import ch.qos.logback.classic.Level; +import java.util.Collections; +import javax.security.auth.callback.Callback; +import javax.security.auth.callback.NameCallback; +import javax.security.sasl.AuthorizeCallback; +import org.apache.zookeeper.test.LoggerTestTool; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for {@link SaslServerCallbackHandler} authorization handling. + * + * <p>These reproduce the client-port SASL authorization-ID (authzid) impersonation + * issue and lock in the fix. The handler runs the same code path a real DIGEST-MD5 + * or GSSAPI negotiation drives via {@link AuthorizeCallback}; the SASL server + * mechanism later returns {@code AuthorizeCallback.getAuthorizedID()} as the + * connection identity, so asserting on it is equivalent to asserting on the + * identity ZooKeeperServer.processSasl would adopt. + * + * <p>No server, KDC, or network is required. + */ +public class SaslServerCallbackHandlerTest { + + private static LoggerTestTool loggerTestTool; + + @BeforeAll + public static void setupBeforeClass() { + loggerTestTool = new LoggerTestTool(SaslServerCallbackHandler.class, Level.INFO); + } + + @AfterAll + public static void tearDownAfterClass() throws Exception { + loggerTestTool.close(); + } + + @BeforeEach + public void resetLog() { + // discard output from earlier tests so each test sees only its own log lines + loggerTestTool.getOutputStream().reset(); + } + + private static SaslServerCallbackHandler newHandler() { + return new SaslServerCallbackHandler(Collections.emptyMap()); + } + + private static AuthorizeCallback authorize(String authnId, String authzId) throws Exception { + SaslServerCallbackHandler handler = newHandler(); + AuthorizeCallback ac = new AuthorizeCallback(authnId, authzId); + handler.handle(new Callback[]{ac}); + return ac; + } + + /** + * The core exploit: a validly authenticated but low-privilege client requests + * authzid "super". Before the fix, canonicalization of the cross-realm principal + * threw NoMatchingRule, the exception was swallowed, authorized stayed true, and + * getAuthorizedID() returned the client-chosen "super". The fix must deny it. + */ + @Test + public void authzidSuperMustBeRejectedWhenItDiffersFromAuthenticationId() throws Exception { + AuthorizeCallback ac = authorize("bob@CROSSREALM", "super"); + + assertFalse(ac.isAuthorized(), + "client must not be authorized to assume a different identity ('super')"); + assertNull(ac.getAuthorizedID(), + "no authorized identity may be granted when authorization is denied"); + } + + /** + * Any authzid different from the authenticated identity is impersonation and + * must be denied, not just the literal "super". + */ + @Test + public void anyMismatchedAuthzidMustBeRejected() throws Exception { + AuthorizeCallback ac = authorize("[email protected]", "[email protected]"); + + assertFalse(ac.isAuthorized()); + assertNull(ac.getAuthorizedID()); + } + + /** + * Legitimate case: the client did not request a distinct authzid, so the SASL + * layer defaults authorizationID to authenticationID. A simple (realm-less) name + * canonicalizes to itself and must be authorized. + */ + @Test + public void matchingSimpleNameIsAuthorized() throws Exception { + AuthorizeCallback ac = authorize("alice", "alice"); + + assertTrue(ac.isAuthorized(), "a client authorizing as itself must be allowed"); + assertEquals("alice", ac.getAuthorizedID()); + } + + /** + * Regression / compatibility guard for cross-realm users. When authzid equals the + * authenticated principal, the client is authorized as itself even though + * canonicalization fails under the default auth_to_local rules. The critical + * property is that the adopted identity is the client's OWN principal and can + * never be an attacker-chosen value. + * + * <p>Assumes the test host's default Kerberos realm is not literally "CROSSREALM" + * (true on any normal build/CI host), so getShortName() throws NoMatchingRule and + * the identity falls back to the authenticated principal. + */ + @Test + public void matchingCrossRealmPrincipalIsAuthorizedAsItselfNotSpoofable() throws Exception { + AuthorizeCallback ac = authorize("bob@CROSSREALM", "bob@CROSSREALM"); + + assertTrue(ac.isAuthorized()); + assertEquals("bob@CROSSREALM", ac.getAuthorizedID(), + "cross-realm client must be authorized as its own principal"); + assertNotEquals("super", ac.getAuthorizedID()); + } + + // --------------------------------------------------------------------- + // Log sanitization (CWE-117). authenticationID and authorizationID are + // client-controlled SASL token fields; control characters must be stripped + // before logging so a client cannot forge adjacent log lines. + // --------------------------------------------------------------------- + + private static final String FORGED_MARKER = "FORGED"; + + /** + * Captures everything the handler logged during this test, then asserts that + * no physical log line begins with the injected marker, i.e. no line was forged. + */ + private static String capturedLogAssertingNoForgedLines() { + String output = loggerTestTool.getOutputStream().toString(); + for (String line : output.split("\\R")) { + assertFalse(line.startsWith(FORGED_MARKER), + "client-controlled input started a new log line (log forgery): " + line); + } + return output; + } + + private static String lineContaining(String output, String needle) { + for (String line : output.split("\\R")) { + if (line.contains(needle)) { + return line; + } + } + return null; + } + + @Test + public void denialLogSanitizesControlCharactersAndLogsAtWarn() throws Exception { + String authnId = "mallory\n" + FORGED_MARKER + " AUTHN\r\t LINE"; + String authzId = "super\n" + FORGED_MARKER + " AUTHZ\r\t LINE"; + + AuthorizeCallback ac = authorize(authnId, authzId); + assertFalse(ac.isAuthorized()); + + String output = capturedLogAssertingNoForgedLines(); + String line = lineContaining(output, "Client attempted to authorize as a different identity"); + assertNotNull(line, "denial was not logged"); + + // CR, LF and TAB are removed; the remaining text stays on the single log line + assertTrue(line.contains("authenticationID=mallory" + FORGED_MARKER + " AUTHN LINE"), + "authenticationID not logged sanitized on one line: " + line); + assertTrue(line.contains("requested authorizationID=super" + FORGED_MARKER + " AUTHZ LINE"), + "authorizationID not logged sanitized on one line: " + line); + + // denial must be WARN, not ERROR + assertTrue(line.contains(" WARN "), "denial should be logged at WARN: " + line); + assertFalse(line.contains(" ERROR "), "denial must not be logged at ERROR: " + line); + } + + @Test + public void successLogSanitizesControlCharacters() throws Exception { + // authcid == authzid, so this takes the success path; the realm-less name + // canonicalizes to itself without throwing + String id = "alice\n" + FORGED_MARKER + " SUCCESS\r\t LINE"; + + AuthorizeCallback ac = authorize(id, id); + assertTrue(ac.isAuthorized()); + + String output = capturedLogAssertingNoForgedLines(); + String line = lineContaining(output, "Successfully authenticated client"); + assertNotNull(line, "successful authorization was not logged"); + assertTrue(line.contains("authorizationID=alice" + FORGED_MARKER + " SUCCESS LINE"), + "authorizationID not logged sanitized on one line: " + line); + + // the canonicalized identity is logged separately; for a realm-less name it + // is the client-supplied string verbatim, so it must be sanitized as well + String setLine = lineContaining(output, "Setting authorizedID"); + assertNotNull(setLine, "canonicalized authorizedID was not logged"); + assertTrue(setLine.contains("Setting authorizedID: alice" + FORGED_MARKER + " SUCCESS LINE"), + "canonicalized authorizedID not logged sanitized on one line: " + setLine); + + // only the log output is sanitized; the identity itself is not altered + assertEquals(id, ac.getAuthorizedID()); + } + + @Test + public void unknownDigestUserLogSanitizesControlCharacters() throws Exception { + // this runs before authentication, so any client can reach it + String userName = "ghost\n" + FORGED_MARKER + " UNKNOWN\r\t USER"; + NameCallback nc = new NameCallback("username: ", userName); + newHandler().handle(new Callback[]{nc}); + + assertNull(nc.getName(), "unknown user must not be accepted"); + + String output = capturedLogAssertingNoForgedLines(); + String line = lineContaining(output, "not found in list of DIGEST-MD5 authenticateable users"); + assertNotNull(line, "unknown user was not logged"); + assertTrue(line.contains("User 'ghost" + FORGED_MARKER + " UNKNOWN USER'"), + "username not logged sanitized on one line: " + line); + } + + @Test + public void denialWithNullAuthenticationIdIsLoggedWithoutError() throws Exception { + AuthorizeCallback ac = authorize(null, "super"); + + assertFalse(ac.isAuthorized()); + assertNull(ac.getAuthorizedID()); + + String line = lineContaining(loggerTestTool.getOutputStream().toString(), + "Client attempted to authorize as a different identity"); + assertNotNull(line, "denial with null authenticationID was not logged"); + assertTrue(line.contains("authenticationID=null"), line); + } +}
