This is an automated email from the ASF dual-hosted git repository. robertlazarski pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/axis-axis2-java-rampart.git
commit d0d8ff88a1e5701da46b9ac97bf6337ceb2b5942 Author: Robert Lazarski <[email protected]> AuthorDate: Wed Sep 2 18:04:54 2026 -1000 Compare plaintext UsernameToken passwords and stop trusting bare BSTs verifyPlaintextPassword handed the wire password to the callback and returned without comparing anything, so any password authenticated any user. Pass null, as the callback contract expects, and compare what comes back. Separately, RahasData took the SubjectDN of any BinarySecurityToken as the STS principal and its certificate as the proof-key binding, though nothing validates a bare BST; identity now comes only from verified signature and UsernameToken results. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../handler/RampartUsernameTokenValidator.java | 26 ++++- .../handler/RampartUsernameTokenValidatorTest.java | 117 +++++++++++++++++++++ .../src/main/java/org/apache/rahas/RahasData.java | 17 +-- 3 files changed, 153 insertions(+), 7 deletions(-) diff --git a/modules/rampart-core/src/main/java/org/apache/rampart/handler/RampartUsernameTokenValidator.java b/modules/rampart-core/src/main/java/org/apache/rampart/handler/RampartUsernameTokenValidator.java index 3b497a94..804aa49e 100644 --- a/modules/rampart-core/src/main/java/org/apache/rampart/handler/RampartUsernameTokenValidator.java +++ b/modules/rampart-core/src/main/java/org/apache/rampart/handler/RampartUsernameTokenValidator.java @@ -17,6 +17,8 @@ package org.apache.rampart.handler; import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; import javax.security.auth.callback.Callback; import javax.security.auth.callback.UnsupportedCallbackException; @@ -45,12 +47,24 @@ public class RampartUsernameTokenValidator extends UsernameTokenValidator { /** * Verify a UsernameToken containing a plaintext password. + * <p> + * The callback is handed a <code>null</code> password and is expected to set the + * password it holds for the user, which is then compared against the one received + * on the wire. Passing the received password into the callback instead, and + * returning without comparing anything, means every password authenticates every + * user: a callback that follows the documented set-password contract overwrites + * nothing and reports nothing, so there is no check left anywhere on this path. + * Do not put <code>usernameToken.getPassword()</code> back into the callback. */ @Override protected void verifyPlaintextPassword(UsernameToken usernameToken, RequestData data ) throws WSSecurityException { + if (data.getCallbackHandler() == null) { + throw new WSSecurityException(WSSecurityException.ErrorCode.FAILURE, "noCallback"); + } + WSPasswordCallback pwCb = new WSPasswordCallback(usernameToken.getName(), - usernameToken.getPassword(), + null, usernameToken.getPasswordType(), WSPasswordCallback.USERNAME_TOKEN); try { @@ -59,6 +73,16 @@ public class RampartUsernameTokenValidator extends UsernameTokenValidator { throw new WSSecurityException(WSSecurityException.ErrorCode.FAILED_AUTHENTICATION, e); } + String expectedPassword = pwCb.getPassword(); + String receivedPassword = usernameToken.getPassword(); + // A callback that neither set a password nor threw has authenticated nobody. + if (expectedPassword == null || receivedPassword == null) { + throw new WSSecurityException(WSSecurityException.ErrorCode.FAILED_AUTHENTICATION); + } + if (!MessageDigest.isEqual(expectedPassword.getBytes(StandardCharsets.UTF_8), + receivedPassword.getBytes(StandardCharsets.UTF_8))) { + throw new WSSecurityException(WSSecurityException.ErrorCode.FAILED_AUTHENTICATION); + } } } diff --git a/modules/rampart-core/src/test/java/org/apache/rampart/handler/RampartUsernameTokenValidatorTest.java b/modules/rampart-core/src/test/java/org/apache/rampart/handler/RampartUsernameTokenValidatorTest.java new file mode 100644 index 00000000..a852e132 --- /dev/null +++ b/modules/rampart-core/src/test/java/org/apache/rampart/handler/RampartUsernameTokenValidatorTest.java @@ -0,0 +1,117 @@ +/* + * 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.rampart.handler; + +import javax.security.auth.callback.Callback; +import javax.security.auth.callback.CallbackHandler; +import javax.xml.parsers.DocumentBuilderFactory; + +import junit.framework.TestCase; + +import org.apache.wss4j.common.ext.WSPasswordCallback; +import org.apache.wss4j.common.ext.WSSecurityException; +import org.apache.wss4j.dom.WSConstants; +import org.apache.wss4j.dom.handler.RequestData; +import org.apache.wss4j.dom.message.token.UsernameToken; +import org.w3c.dom.Document; + +/** + * Tests that a plaintext UsernameToken password is actually compared. + * + * <p>The validator used to hand the password received on the wire to the callback + * and return, comparing nothing, so any password authenticated any user. These + * tests pin the comparison: the callback is asked what the password should be, and + * a token that does not carry that password is rejected. + */ +public class RampartUsernameTokenValidatorTest extends TestCase { + + private static final String USER = "alice"; + private static final String CORRECT_PASSWORD = "correct-horse"; + + /** Follows the documented contract: sets the password it holds for the user. */ + private static class SetPasswordCallbackHandler implements CallbackHandler { + public void handle(Callback[] callbacks) { + for (Callback callback : callbacks) { + WSPasswordCallback pwcb = (WSPasswordCallback) callback; + if (USER.equals(pwcb.getIdentifier())) { + pwcb.setPassword(CORRECT_PASSWORD); + } + } + } + } + + /** Sets nothing and throws nothing, so it has authenticated nobody. */ + private static class SilentCallbackHandler implements CallbackHandler { + public void handle(Callback[] callbacks) { + } + } + + private UsernameToken usernameToken(String password) throws Exception { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + Document doc = factory.newDocumentBuilder().newDocument(); + UsernameToken token = new UsernameToken(true, doc, WSConstants.PASSWORD_TEXT); + token.setName(USER); + token.setPassword(password); + return token; + } + + private void verify(UsernameToken token, CallbackHandler handler) throws WSSecurityException { + RequestData data = new RequestData(); + data.setCallbackHandler(handler); + new RampartUsernameTokenValidator().verifyPlaintextPassword(token, data); + } + + public void testCorrectPasswordIsAccepted() throws Exception { + verify(usernameToken(CORRECT_PASSWORD), new SetPasswordCallbackHandler()); + } + + public void testWrongPasswordIsRejected() throws Exception { + try { + verify(usernameToken("anything-else"), new SetPasswordCallbackHandler()); + fail("a UsernameToken carrying the wrong password must not authenticate"); + } catch (WSSecurityException expected) { + assertEquals(WSSecurityException.ErrorCode.FAILED_AUTHENTICATION, + expected.getErrorCode()); + } + } + + /** + * The case that made every password work: the callback overwrites nothing, so + * whatever arrived on the wire must not be treated as verified. + */ + public void testCallbackThatSetsNoPasswordIsRejected() throws Exception { + try { + verify(usernameToken("anything-at-all"), new SilentCallbackHandler()); + fail("a callback that set no password must not authenticate anyone"); + } catch (WSSecurityException expected) { + assertEquals(WSSecurityException.ErrorCode.FAILED_AUTHENTICATION, + expected.getErrorCode()); + } + } + + public void testMissingCallbackHandlerIsRejected() throws Exception { + try { + verify(usernameToken(CORRECT_PASSWORD), null); + fail("no callback handler means nothing can verify the password"); + } catch (WSSecurityException expected) { + // expected + } + } +} diff --git a/modules/rampart-trust/src/main/java/org/apache/rahas/RahasData.java b/modules/rampart-trust/src/main/java/org/apache/rahas/RahasData.java index ecb7f2a1..3d070b65 100644 --- a/modules/rampart-trust/src/main/java/org/apache/rahas/RahasData.java +++ b/modules/rampart-trust/src/main/java/org/apache/rahas/RahasData.java @@ -183,12 +183,17 @@ public class RahasData { this.principal = (Principal) principalObject; } else if (act == WSConstants.UT && principalObject != null) { this.principal = (Principal) principalObject; - } else if (act == WSConstants.BST) { - final X509Certificate[] certificates = - (X509Certificate[]) wser - .get(WSSecurityEngineResult.TAG_X509_CERTIFICATES); - this.clientCert = certificates[0]; - this.principal = this.clientCert.getSubjectDN(); + // A BinarySecurityToken is deliberately not a principal source. + // Nothing validates a bare BST: it need not be trusted, and the + // sender need not hold its private key, so taking its SubjectDN as + // the principal and its certificate as the proof-key binding let a + // requestor be issued a signed assertion for any identity, bound to + // a certificate of its choosing. Worse, results are walked in + // attacker-controlled order, so a smuggled BST could overwrite a + // principal already established by a verified signature. Identity + // here comes only from the SIGN and UT results above; where no + // client certificate is established the token issuers look one up + // by principal name in the configured keystore, which is trusted. } else if (act == WSConstants.ST_UNSIGNED) { this.assertion = (Assertion) wser .get(WSSecurityEngineResult.TAG_SAML_ASSERTION);
