This is an automated email from the ASF dual-hosted git repository.
coheigea pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ws-wss4j.git
The following commit(s) were added to refs/heads/master by this push:
new ebaa5751f Make ReplayCache atomic by adding addIfAbsent (#762)
ebaa5751f is described below
commit ebaa5751f2107daf86c44dc95b264b76a3c50b70
Author: Colm O hEigeartaigh <[email protected]>
AuthorDate: Fri Oct 23 07:20:56 2026 +0100
Make ReplayCache atomic by adding addIfAbsent (#762)
---
.../wss4j/common/cache/EHCacheReplayCache.java | 17 +++
.../wss4j/common/cache/MemoryReplayCache.java | 47 +++++--
.../org/apache/wss4j/common/cache/ReplayCache.java | 33 +++++
.../wss4j/common/util/UsernameTokenUtil.java | 30 +++--
.../apache/wss4j/common/cache/ReplayCacheTest.java | 145 +++++++++++++++++++++
.../wss4j/common/util/UsernameTokenUtilTest.java | 49 +++++++
.../wss4j/dom/message/token/UsernameToken.java | 7 +
.../wss4j/dom/processor/SignatureProcessor.java | 14 +-
.../dom/processor/UsernameTokenProcessor.java | 18 +--
.../wss4j/dom/validate/SamlAssertionValidator.java | 12 +-
.../org/apache/wss4j/dom/message/ReplayTest.java | 40 ++++++
.../processor/input/UsernameTokenInputHandler.java | 12 +-
.../WSSSignatureReferenceVerifyInputProcessor.java | 11 +-
.../stax/validate/SamlTokenValidatorImpl.java | 12 +-
.../org/apache/wss4j/stax/test/ReplayTest.java | 55 ++++++++
15 files changed, 440 insertions(+), 62 deletions(-)
diff --git
a/ws-security-common/src/main/java/org/apache/wss4j/common/cache/EHCacheReplayCache.java
b/ws-security-common/src/main/java/org/apache/wss4j/common/cache/EHCacheReplayCache.java
index ff46c218b..db2a1ede7 100644
---
a/ws-security-common/src/main/java/org/apache/wss4j/common/cache/EHCacheReplayCache.java
+++
b/ws-security-common/src/main/java/org/apache/wss4j/common/cache/EHCacheReplayCache.java
@@ -127,6 +127,23 @@ public class EHCacheReplayCache implements ReplayCache {
cache.put(identifier, new EHCacheValue(identifier, expiry));
}
+ /**
+ * Add the given identifier to the cache if, and only if, it is not
already contained in it, as a
+ * single atomic operation.
+ * @param identifier The identifier to be added
+ * @param expiry A custom expiry time for the identifier. Can be null in
which case, the default expiry is used.
+ * @return true if the identifier was added, false if it was already
contained in the cache or is
+ * null or empty
+ */
+ @Override
+ public boolean addIfAbsent(String identifier, Instant expiry) {
+ if (identifier == null || identifier.length() == 0) {
+ return false;
+ }
+
+ return cache.putIfAbsent(identifier, new EHCacheValue(identifier,
expiry)) == null;
+ }
+
/**
* Return true if the given identifier is contained in the cache
* @param identifier The identifier to check
diff --git
a/ws-security-common/src/main/java/org/apache/wss4j/common/cache/MemoryReplayCache.java
b/ws-security-common/src/main/java/org/apache/wss4j/common/cache/MemoryReplayCache.java
index 9bd0bbfa7..bfe777a96 100644
---
a/ws-security-common/src/main/java/org/apache/wss4j/common/cache/MemoryReplayCache.java
+++
b/ws-security-common/src/main/java/org/apache/wss4j/common/cache/MemoryReplayCache.java
@@ -60,21 +60,48 @@ public class MemoryReplayCache implements ReplayCache {
return;
}
- Instant now = Instant.now();
- Instant maxTTL = now.plusSeconds(MAX_TTL);
- if (expiry == null || expiry.isBefore(now) || expiry.isAfter(maxTTL)) {
- expiry = now.plusSeconds(DEFAULT_TTL);
+ Instant cacheExpiry = getCacheExpiry(expiry);
+ synchronized (cache) {
+ cache.computeIfAbsent(cacheExpiry, k -> new
ArrayList<>(1)).add(identifier);
}
+ ids.add(identifier);
+ }
+ /**
+ * Add the given identifier to the cache if, and only if, it is not
already contained in it, as a
+ * single atomic operation.
+ * @param identifier The identifier to be added
+ * @param expiry A custom expiry time for the identifier
+ * @return true if the identifier was added, false if it was already
contained in the cache or is
+ * null or empty
+ */
+ @Override
+ public boolean addIfAbsent(String identifier, Instant expiry) {
+ if (identifier == null || identifier.length() == 0) {
+ return false;
+ }
+
+ processTokenExpiry();
+
+ Instant cacheExpiry = getCacheExpiry(expiry);
+ // processTokenExpiry removes identifiers from ids while holding the
cache monitor, so the
+ // test-and-set on ids and the matching expiry entry are made under
that monitor too
synchronized (cache) {
- List<String> list = cache.get(expiry);
- if (list == null) {
- list = new ArrayList<>(1);
- cache.put(expiry, list);
+ if (!ids.add(identifier)) {
+ return false;
}
- list.add(identifier);
+ cache.computeIfAbsent(cacheExpiry, k -> new
ArrayList<>(1)).add(identifier);
}
- ids.add(identifier);
+ return true;
+ }
+
+ private static Instant getCacheExpiry(Instant expiry) {
+ Instant now = Instant.now();
+ Instant maxTTL = now.plusSeconds(MAX_TTL);
+ if (expiry == null || expiry.isBefore(now) || expiry.isAfter(maxTTL)) {
+ return now.plusSeconds(DEFAULT_TTL);
+ }
+ return expiry;
}
/**
diff --git
a/ws-security-common/src/main/java/org/apache/wss4j/common/cache/ReplayCache.java
b/ws-security-common/src/main/java/org/apache/wss4j/common/cache/ReplayCache.java
index b41f74450..74ee08c86 100644
---
a/ws-security-common/src/main/java/org/apache/wss4j/common/cache/ReplayCache.java
+++
b/ws-security-common/src/main/java/org/apache/wss4j/common/cache/ReplayCache.java
@@ -47,4 +47,37 @@ public interface ReplayCache extends Closeable {
*/
boolean contains(String identifier);
+ /**
+ * Add the given identifier to the cache if, and only if, it is not
already contained in it, as a
+ * single atomic operation. A replay check must use this method rather
than contains() followed by
+ * add(): two concurrent requests carrying the same identifier could
otherwise both pass contains()
+ * before either reaches add(), and both be accepted.
+ *
+ * The default implementation serializes calls on this cache instance,
which is only atomic with
+ * respect to other calls to this method. Implementations should override
it with an operation that
+ * is atomic in the underlying store.
+ *
+ * @param identifier The identifier to be added
+ * @param expiry A custom expiry time for the identifier. Can be null in
which case, the default
+ * expiry is used.
+ * @return true if the identifier was added, false if it was already
contained in the cache or is
+ * null or empty, in which case the cache is left unchanged
+ */
+ default boolean addIfAbsent(String identifier, Instant expiry) {
+ if (identifier == null || identifier.length() == 0) {
+ return false;
+ }
+ synchronized (this) {
+ if (contains(identifier)) {
+ return false;
+ }
+ if (expiry != null) {
+ add(identifier, expiry);
+ } else {
+ add(identifier);
+ }
+ return true;
+ }
+ }
+
}
diff --git
a/ws-security-common/src/main/java/org/apache/wss4j/common/util/UsernameTokenUtil.java
b/ws-security-common/src/main/java/org/apache/wss4j/common/util/UsernameTokenUtil.java
index c2f0dd9d7..e53a40873 100644
---
a/ws-security-common/src/main/java/org/apache/wss4j/common/util/UsernameTokenUtil.java
+++
b/ws-security-common/src/main/java/org/apache/wss4j/common/util/UsernameTokenUtil.java
@@ -222,19 +222,31 @@ public final class UsernameTokenUtil {
* replay cache on the raw element text would let a replayed message
escape detection simply by
* rewriting its Nonce into an equivalent encoding.
*
- * @param nonce the Nonce exactly as it appeared in the message
+ * An empty Nonce - no text at all, or only characters the decoder ignores
- is rejected: it
+ * cannot be cached, so would otherwise defeat replay detection.
+ *
+ * @param nonce the Nonce exactly as it appeared in the message, or null
if the Nonce element
+ * has no text
* @return the same Nonce re-encoded in canonical base64
- * @throws WSSecurityException if the Nonce is not valid base64
+ * @throws WSSecurityException if the Nonce is empty or not valid base64
*/
public static String getCanonicalNonce(String nonce) throws
WSSecurityException {
- try {
- return Base64.getEncoder().encodeToString(
- org.apache.xml.security.utils.XMLUtils.decode(nonce));
- } catch (IllegalArgumentException ex) {
- LOG.debug(ex.getMessage(), ex);
+ byte[] decodedNonce = null;
+ if (nonce != null) {
+ try {
+ decodedNonce =
org.apache.xml.security.utils.XMLUtils.decode(nonce);
+ } catch (IllegalArgumentException ex) {
+ LOG.debug(ex.getMessage(), ex);
+ throw new WSSecurityException(
+ WSSecurityException.ErrorCode.INVALID_SECURITY, ex,
"badUsernameToken",
+ new Object[] {"The Nonce is not valid Base-64"});
+ }
+ }
+ if (decodedNonce == null || decodedNonce.length == 0) {
throw new WSSecurityException(
- WSSecurityException.ErrorCode.INVALID_SECURITY, ex,
"badUsernameToken",
- new Object[] {"The Nonce is not valid Base-64"});
+ WSSecurityException.ErrorCode.INVALID_SECURITY,
"badUsernameToken",
+ new Object[] {"The Nonce is empty"});
}
+ return Base64.getEncoder().encodeToString(decodedNonce);
}
}
diff --git
a/ws-security-common/src/test/java/org/apache/wss4j/common/cache/ReplayCacheTest.java
b/ws-security-common/src/test/java/org/apache/wss4j/common/cache/ReplayCacheTest.java
index 3bcdb3a9c..dc6bf94e9 100644
---
a/ws-security-common/src/test/java/org/apache/wss4j/common/cache/ReplayCacheTest.java
+++
b/ws-security-common/src/test/java/org/apache/wss4j/common/cache/ReplayCacheTest.java
@@ -23,7 +23,14 @@ import java.io.IOException;
import java.nio.file.Path;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
+import java.util.ArrayList;
+import java.util.List;
import java.util.UUID;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicIntegerArray;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
@@ -180,6 +187,144 @@ public class ReplayCacheTest {
new EHCacheReplayCache("abc", tempDir, 10, 10, false));
}
+ @Test
+ public void testMemoryReplayCacheAddIfAbsent() throws Exception {
+ try (ReplayCache replayCache = new MemoryReplayCache()) {
+ testAddIfAbsent(replayCache);
+ }
+ }
+
+ @Test
+ public void testEhCacheReplayCacheAddIfAbsent() throws Exception {
+ try (ReplayCache replayCache = new EHCacheReplayCache("xyz", tempDir))
{
+ testAddIfAbsent(replayCache);
+ }
+ }
+
+ @Test
+ public void testDefaultAddIfAbsent() throws Exception {
+ try (ReplayCache replayCache = new DelegatingReplayCache(new
MemoryReplayCache())) {
+ testAddIfAbsent(replayCache);
+ }
+ }
+
+ /**
+ * Concurrent requests carrying the same identifier must not all be
accepted: exactly one
+ * addIfAbsent call per identifier may succeed.
+ */
+ @Test
+ public void testMemoryReplayCacheConcurrentAddIfAbsent() throws Exception {
+ try (ReplayCache replayCache = new MemoryReplayCache()) {
+ testConcurrentAddIfAbsent(replayCache);
+ }
+ }
+
+ @Test
+ public void testEhCacheReplayCacheConcurrentAddIfAbsent() throws Exception
{
+ try (ReplayCache replayCache = new EHCacheReplayCache("xyz", tempDir))
{
+ testConcurrentAddIfAbsent(replayCache);
+ }
+ }
+
+ @Test
+ public void testDefaultConcurrentAddIfAbsent() throws Exception {
+ try (ReplayCache replayCache = new DelegatingReplayCache(new
MemoryReplayCache())) {
+ testConcurrentAddIfAbsent(replayCache);
+ }
+ }
+
+ private void testAddIfAbsent(ReplayCache replayCache) throws
InterruptedException {
+ String id = UUID.randomUUID().toString();
+ assertTrue(replayCache.addIfAbsent(id, null));
+ assertTrue(replayCache.contains(id));
+ assertFalse(replayCache.addIfAbsent(id, null));
+ assertFalse(replayCache.addIfAbsent(id,
Instant.now().plusSeconds(100L)));
+
+ // An identifier added with add() is also seen
+ id = UUID.randomUUID().toString();
+ replayCache.add(id);
+ assertFalse(replayCache.addIfAbsent(id, null));
+
+ // An empty identifier cannot be cached, so is never reported as added
+ assertFalse(replayCache.addIfAbsent(null, null));
+ assertFalse(replayCache.addIfAbsent("", null));
+ assertFalse(replayCache.contains(""));
+
+ // Once an identifier has expired, it may be added again
+ id = UUID.randomUUID().toString();
+ assertTrue(replayCache.addIfAbsent(id, Instant.now().plusSeconds(1L)));
+ Thread.sleep(1250L);
+ assertTrue(replayCache.addIfAbsent(id, null));
+ assertFalse(replayCache.addIfAbsent(id, null));
+ }
+
+ private void testConcurrentAddIfAbsent(ReplayCache replayCache) throws
Exception {
+ final int threads = 8;
+ final int identifiers = 500;
+ List<String> ids = new ArrayList<>(identifiers);
+ for (int i = 0; i < identifiers; i++) {
+ ids.add(UUID.randomUUID().toString());
+ }
+ AtomicIntegerArray accepted = new AtomicIntegerArray(identifiers);
+ CyclicBarrier barrier = new CyclicBarrier(threads);
+
+ ExecutorService executor = Executors.newFixedThreadPool(threads);
+ try {
+ List<Future<?>> futures = new ArrayList<>(threads);
+ for (int t = 0; t < threads; t++) {
+ futures.add(executor.submit(() -> {
+ barrier.await();
+ for (int i = 0; i < identifiers; i++) {
+ if (replayCache.addIfAbsent(ids.get(i), null)) {
+ accepted.incrementAndGet(i);
+ }
+ }
+ return null;
+ }));
+ }
+ for (Future<?> future : futures) {
+ future.get();
+ }
+ } finally {
+ executor.shutdown();
+ }
+
+ for (int i = 0; i < identifiers; i++) {
+ assertEquals(1, accepted.get(i), "Identifier " + i + " was
accepted more than once");
+ }
+ }
+
+ /**
+ * A ReplayCache that does not override addIfAbsent, to test the default
implementation
+ */
+ private static final class DelegatingReplayCache implements ReplayCache {
+ private final ReplayCache delegate;
+
+ DelegatingReplayCache(ReplayCache delegate) {
+ this.delegate = delegate;
+ }
+
+ @Override
+ public void add(String identifier) {
+ delegate.add(identifier);
+ }
+
+ @Override
+ public void add(String identifier, Instant expiry) {
+ delegate.add(identifier, expiry);
+ }
+
+ @Override
+ public boolean contains(String identifier) {
+ return delegate.contains(identifier);
+ }
+
+ @Override
+ public void close() throws IOException {
+ delegate.close();
+ }
+ }
+
private void testReplayCacheInstance(ReplayCache replayCache) throws
InterruptedException, IOException {
// Test default TTL caches OK
diff --git
a/ws-security-common/src/test/java/org/apache/wss4j/common/util/UsernameTokenUtilTest.java
b/ws-security-common/src/test/java/org/apache/wss4j/common/util/UsernameTokenUtilTest.java
new file mode 100644
index 000000000..abd10c301
--- /dev/null
+++
b/ws-security-common/src/test/java/org/apache/wss4j/common/util/UsernameTokenUtilTest.java
@@ -0,0 +1,49 @@
+/**
+ * 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.wss4j.common.util;
+
+import org.apache.wss4j.common.ext.WSSecurityException;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class UsernameTokenUtilTest {
+
+ @Test
+ void canonicalNonceIgnoresEncodingDifferences() throws Exception {
+ assertEquals("AAECAwQFBgcICQoLDA0ODw==",
UsernameTokenUtil.getCanonicalNonce("AAECAwQFBgcICQoLDA0ODw=="));
+ assertEquals("AAECAwQFBgcICQoLDA0ODw==",
UsernameTokenUtil.getCanonicalNonce("AAEC\nAwQFBgcICQoLDA0ODw=="));
+ }
+
+ /**
+ * An empty Nonce cannot be cached, so it must be rejected rather than
silently escaping replay
+ * detection, whether the Nonce element has no text at all or only
whitespace.
+ */
+ @Test
+ void rejectsEmptyNonce() {
+ for (String nonce : new String[] {null, "", " ", "\n"}) {
+ WSSecurityException ex =
+ assertThrows(WSSecurityException.class, () ->
UsernameTokenUtil.getCanonicalNonce(nonce));
+ assertEquals(WSSecurityException.ErrorCode.INVALID_SECURITY,
ex.getErrorCode());
+ assertTrue(ex.getMessage().contains("The Nonce is empty"),
ex.getMessage());
+ }
+ }
+}
diff --git
a/ws-security-dom/src/main/java/org/apache/wss4j/dom/message/token/UsernameToken.java
b/ws-security-dom/src/main/java/org/apache/wss4j/dom/message/token/UsernameToken.java
index db47b5335..60cff38e0 100644
---
a/ws-security-dom/src/main/java/org/apache/wss4j/dom/message/token/UsernameToken.java
+++
b/ws-security-dom/src/main/java/org/apache/wss4j/dom/message/token/UsernameToken.java
@@ -426,6 +426,13 @@ public class UsernameToken {
return elementPassword != null;
}
+ /**
+ * Return true if this UsernameToken contains a Nonce element, whether or
not it has any text
+ */
+ public boolean containsNonceElement() {
+ return elementNonce != null;
+ }
+
/**
* Get the Salt value of this UsernameToken.
*
diff --git
a/ws-security-dom/src/main/java/org/apache/wss4j/dom/processor/SignatureProcessor.java
b/ws-security-dom/src/main/java/org/apache/wss4j/dom/processor/SignatureProcessor.java
index d038c3069..ae0213342 100644
---
a/ws-security-dom/src/main/java/org/apache/wss4j/dom/processor/SignatureProcessor.java
+++
b/ws-security-dom/src/main/java/org/apache/wss4j/dom/processor/SignatureProcessor.java
@@ -826,7 +826,8 @@ public class SignatureProcessor implements Processor {
/**
* A pending replay cache addition: the Timestamp/SignatureValue/Key
identifier of the current
* message, to be added to the ReplayCache only once the signature it was
derived from has
- * been successfully validated.
+ * been successfully validated. The addition is atomic, and fails if a
concurrent request
+ * carrying the same identifier has been accepted in the meantime.
*/
private static final class ReplayCacheEntry {
private final ReplayCache replayCache;
@@ -839,11 +840,12 @@ public class SignatureProcessor implements Processor {
this.expires = expires;
}
- void add() {
- if (expires != null) {
- replayCache.add(identifier, expires);
- } else {
- replayCache.add(identifier);
+ void add() throws WSSecurityException {
+ if (!replayCache.addIfAbsent(identifier, expires)) {
+ throw new WSSecurityException(
+ WSSecurityException.ErrorCode.INVALID_SECURITY,
+ "invalidTimestamp",
+ new Object[] {"A replay attack has been detected"});
}
}
}
diff --git
a/ws-security-dom/src/main/java/org/apache/wss4j/dom/processor/UsernameTokenProcessor.java
b/ws-security-dom/src/main/java/org/apache/wss4j/dom/processor/UsernameTokenProcessor.java
index 98713b5e2..074cf87a1 100644
---
a/ws-security-dom/src/main/java/org/apache/wss4j/dom/processor/UsernameTokenProcessor.java
+++
b/ws-security-dom/src/main/java/org/apache/wss4j/dom/processor/UsernameTokenProcessor.java
@@ -151,7 +151,7 @@ public class UsernameTokenProcessor implements Processor {
ReplayCache replayCache = data.getNonceReplayCache(); //NOPMD
String nonceIdentifier = null;
Instant nonceExpiry = null;
- if (replayCache != null && ut.getNonce() != null) {
+ if (replayCache != null && ut.containsNonceElement()) {
nonceIdentifier =
UsernameTokenUtil.getCanonicalNonce(ut.getNonce());
if (replayCache.contains(nonceIdentifier)) {
throw new WSSecurityException(
@@ -178,13 +178,15 @@ public class UsernameTokenProcessor implements Processor {
// Only cache the Nonce once the token has actually been validated.
Caching it beforehand
// lets an attacker poison the cache with the Nonce of a token whose
password does not
- // verify, so that the genuine token is subsequently rejected as a
replay.
- if (nonceIdentifier != null) {
- if (nonceExpiry != null) {
- replayCache.add(nonceIdentifier, nonceExpiry);
- } else {
- replayCache.add(nonceIdentifier);
- }
+ // verify, so that the genuine token is subsequently rejected as a
replay. The Nonce must be
+ // added atomically, as a concurrent request carrying the same Nonce
may also have passed the
+ // check above while this token was being validated.
+ if (nonceIdentifier != null &&
!replayCache.addIfAbsent(nonceIdentifier, nonceExpiry)) {
+ throw new WSSecurityException(
+ WSSecurityException.ErrorCode.INVALID_SECURITY,
+ "badUsernameToken",
+ new Object[] {"A replay attack has been detected"}
+ );
}
return credential;
}
diff --git
a/ws-security-dom/src/main/java/org/apache/wss4j/dom/validate/SamlAssertionValidator.java
b/ws-security-dom/src/main/java/org/apache/wss4j/dom/validate/SamlAssertionValidator.java
index 3487b3121..c4ae14faa 100644
---
a/ws-security-dom/src/main/java/org/apache/wss4j/dom/validate/SamlAssertionValidator.java
+++
b/ws-security-dom/src/main/java/org/apache/wss4j/dom/validate/SamlAssertionValidator.java
@@ -253,19 +253,15 @@ public class SamlAssertionValidator extends
SignatureTrustValidator {
String identifier = samlAssertion.getId();
ReplayCache replayCache = data.getSamlOneTimeUseReplayCache();
//NOPMD
- if (replayCache.contains(identifier)) {
+ // Check and cache the identifier atomically, so that concurrent
requests carrying the
+ // same Assertion cannot all pass the check before any of them has
cached it
+ Instant expires =
samlAssertion.getSaml2().getConditions().getNotOnOrAfter();
+ if (!replayCache.addIfAbsent(identifier, expires)) {
throw new WSSecurityException(
WSSecurityException.ErrorCode.INVALID_SECURITY,
"badSamlToken",
new Object[] {"A replay attack has been detected"});
}
-
- Instant expires =
samlAssertion.getSaml2().getConditions().getNotOnOrAfter();
- if (expires != null) {
- replayCache.add(identifier, expires);
- } else {
- replayCache.add(identifier);
- }
}
}
diff --git
a/ws-security-dom/src/test/java/org/apache/wss4j/dom/message/ReplayTest.java
b/ws-security-dom/src/test/java/org/apache/wss4j/dom/message/ReplayTest.java
index 2a70c2c3d..5ed9233c3 100644
--- a/ws-security-dom/src/test/java/org/apache/wss4j/dom/message/ReplayTest.java
+++ b/ws-security-dom/src/test/java/org/apache/wss4j/dom/message/ReplayTest.java
@@ -610,6 +610,46 @@ public class ReplayTest {
}
}
+ /**
+ * An empty Nonce cannot be cached, so a UsernameToken carrying one must
be rejected rather than
+ * escape replay detection. The check happens before the token is
validated, so the error names
+ * the empty Nonce rather than reporting a replay. An element with no text
and one containing only
+ * whitespace must be treated alike.
+ */
+ @Test
+ public void testUsernameTokenWithEmptyNonce() throws Exception {
+ Document doc = SOAPUtil.toSOAPPart(SOAPUtil.SAMPLE_SOAP_MSG);
+ WSSecHeader secHeader = new WSSecHeader(doc);
+ secHeader.insertSecurityHeader();
+
+ WSSecUsernameToken builder = new WSSecUsernameToken(secHeader);
+ builder.setUserInfo("wernerd", "verySecret");
+
+ String genuineMessage =
XMLUtils.prettyDocumentToString(builder.build());
+
+ Matcher matcher =
Pattern.compile("(<[^>]*Nonce[^>]*>)([^<]*)(</)").matcher(genuineMessage);
+ assertTrue(matcher.find(), "No Nonce element was found");
+
+ for (String emptyNonce : new String[] {"", " "}) {
+ String message = genuineMessage.substring(0, matcher.start(2))
+ + emptyNonce + genuineMessage.substring(matcher.end(2));
+
+ WSSConfig wssConfig = WSSConfig.getNewInstance();
+ RequestData data = new RequestData();
+ data.setCallbackHandler(new UsernamePasswordCallbackHandler());
+ data.setWssConfig(wssConfig);
+ data.setNonceReplayCache(new MemoryReplayCache());
+
+ try {
+ verify(SOAPUtil.toSOAPPart(message), wssConfig, data);
+ fail("Expected failure on an empty Nonce");
+ } catch (WSSecurityException ex) {
+ assertEquals(WSSecurityException.ErrorCode.INVALID_SECURITY,
ex.getErrorCode());
+ assertTrue(ex.getMessage().contains("The Nonce is empty"),
ex.getMessage());
+ }
+ }
+ }
+
/**
* A UsernameToken whose password fails to verify must not leave its Nonce
in the replay cache.
* Otherwise an attacker can replay a genuine token with a corrupted
Password and send that
diff --git
a/ws-security-stax/src/main/java/org/apache/wss4j/stax/impl/processor/input/UsernameTokenInputHandler.java
b/ws-security-stax/src/main/java/org/apache/wss4j/stax/impl/processor/input/UsernameTokenInputHandler.java
index f45a09220..6a03e4a21 100644
---
a/ws-security-stax/src/main/java/org/apache/wss4j/stax/impl/processor/input/UsernameTokenInputHandler.java
+++
b/ws-security-stax/src/main/java/org/apache/wss4j/stax/impl/processor/input/UsernameTokenInputHandler.java
@@ -119,13 +119,11 @@ public class UsernameTokenInputHandler extends
AbstractInputSecurityHeaderHandle
// Only cache the Nonce once the token has actually been validated.
Caching it beforehand
// lets an attacker poison the cache with the Nonce of a token whose
password does not
- // verify, so that the genuine token is subsequently rejected as a
replay.
- if (nonceIdentifier != null) {
- if (nonceExpiry != null) {
- replayCache.add(nonceIdentifier, nonceExpiry);
- } else {
- replayCache.add(nonceIdentifier);
- }
+ // verify, so that the genuine token is subsequently rejected as a
replay. The Nonce must be
+ // added atomically, as a concurrent request carrying the same Nonce
may also have passed the
+ // check above while this token was being validated.
+ if (nonceIdentifier != null &&
!replayCache.addIfAbsent(nonceIdentifier, nonceExpiry)) {
+ throw new
WSSecurityException(WSSecurityException.ErrorCode.FAILED_AUTHENTICATION);
}
SecurityTokenProvider<InboundSecurityToken> securityTokenProvider =
diff --git
a/ws-security-stax/src/main/java/org/apache/wss4j/stax/impl/processor/input/WSSSignatureReferenceVerifyInputProcessor.java
b/ws-security-stax/src/main/java/org/apache/wss4j/stax/impl/processor/input/WSSSignatureReferenceVerifyInputProcessor.java
index d1f35a6b2..eb6aff5e5 100644
---
a/ws-security-stax/src/main/java/org/apache/wss4j/stax/impl/processor/input/WSSSignatureReferenceVerifyInputProcessor.java
+++
b/ws-security-stax/src/main/java/org/apache/wss4j/stax/impl/processor/input/WSSSignatureReferenceVerifyInputProcessor.java
@@ -457,7 +457,8 @@ public class WSSSignatureReferenceVerifyInputProcessor
extends AbstractSignature
/**
* A pending replay cache addition: the Timestamp/SignatureValue
identifier of the current
* message, to be added to the ReplayCache only once the signature it was
derived from has
- * been successfully verified.
+ * been successfully verified. The addition is atomic, and fails if a
concurrent request
+ * carrying the same identifier has been accepted in the meantime.
*/
private static final class ReplayCacheEntry {
private final ReplayCache replayCache;
@@ -470,11 +471,9 @@ public class WSSSignatureReferenceVerifyInputProcessor
extends AbstractSignature
this.expires = expires;
}
- void add() {
- if (expires != null) {
- replayCache.add(identifier, expires);
- } else {
- replayCache.add(identifier);
+ void add() throws WSSecurityException {
+ if (!replayCache.addIfAbsent(identifier, expires)) {
+ throw new
WSSecurityException(WSSecurityException.ErrorCode.MESSAGE_EXPIRED);
}
}
}
diff --git
a/ws-security-stax/src/main/java/org/apache/wss4j/stax/validate/SamlTokenValidatorImpl.java
b/ws-security-stax/src/main/java/org/apache/wss4j/stax/validate/SamlTokenValidatorImpl.java
index 235742cca..7a793e811 100644
---
a/ws-security-stax/src/main/java/org/apache/wss4j/stax/validate/SamlTokenValidatorImpl.java
+++
b/ws-security-stax/src/main/java/org/apache/wss4j/stax/validate/SamlTokenValidatorImpl.java
@@ -251,19 +251,15 @@ public class SamlTokenValidatorImpl extends
SignatureTokenValidatorImpl implemen
&& samlAssertion.getSaml2().getConditions().getOneTimeUse() !=
null) {
String identifier = samlAssertion.getId();
- if (replayCache.contains(identifier)) {
+ // Check and cache the identifier atomically, so that concurrent
requests carrying the
+ // same Assertion cannot all pass the check before any of them has
cached it
+ Instant expires =
samlAssertion.getSaml2().getConditions().getNotOnOrAfter();
+ if (!replayCache.addIfAbsent(identifier, expires)) {
throw new WSSecurityException(
WSSecurityException.ErrorCode.INVALID_SECURITY,
"badSamlToken",
new Object[] {"A replay attack has been detected"});
}
-
- Instant expires =
samlAssertion.getSaml2().getConditions().getNotOnOrAfter();
- if (expires != null) {
- replayCache.add(identifier, expires);
- } else {
- replayCache.add(identifier);
- }
}
}
diff --git
a/ws-security-stax/src/test/java/org/apache/wss4j/stax/test/ReplayTest.java
b/ws-security-stax/src/test/java/org/apache/wss4j/stax/test/ReplayTest.java
index e07ff0ff8..a237504ac 100644
--- a/ws-security-stax/src/test/java/org/apache/wss4j/stax/test/ReplayTest.java
+++ b/ws-security-stax/src/test/java/org/apache/wss4j/stax/test/ReplayTest.java
@@ -349,6 +349,61 @@ public class ReplayTest extends AbstractTestBase {
replayCache.close();
}
+ /**
+ * An empty Nonce cannot be cached, so a UsernameToken carrying one must
be rejected rather than
+ * escape replay detection. The check happens before the token is
validated, so the error names
+ * the empty Nonce. An element with no text and one containing only
whitespace must be treated
+ * alike.
+ */
+ @Test
+ public void testUsernameTokenWithEmptyNonce() throws Exception {
+
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ {
+ InputStream sourceDocument =
this.getClass().getClassLoader().getResourceAsStream("testdata/plain-soap-1.1.xml");
+ String action = WSHandlerConstants.USERNAME_TOKEN;
+ Properties properties = new Properties();
+ Document securedDocument =
doOutboundSecurityWithWSS4J(sourceDocument, action, properties);
+
+ javax.xml.transform.Transformer transformer =
TRANSFORMER_FACTORY.newTransformer();
+ transformer.transform(new DOMSource(securedDocument), new
StreamResult(baos));
+ }
+
+ String genuineMessage = new String(baos.toByteArray(),
StandardCharsets.UTF_8);
+
+ Matcher matcher =
Pattern.compile("(<[^>]*Nonce[^>]*>)([^<]*)(</)").matcher(genuineMessage);
+ assertTrue(matcher.find(), "No Nonce element was found");
+
+ for (String emptyNonce : new String[] {"", " "}) {
+ String message = genuineMessage.substring(0, matcher.start(2))
+ + emptyNonce + genuineMessage.substring(matcher.end(2));
+
+ ReplayCache replayCache = createCache("wss4j.nonce.cache-");
+
+ WSSSecurityProperties securityProperties = new
WSSSecurityProperties();
+ securityProperties.setNonceReplayCache(replayCache);
+ securityProperties.setCallbackHandler(new CallbackHandlerImpl());
+ // Return the underlying security error rather than the generic
masked one, so that the
+ // reason for the rejection can be checked
+ InboundWSSec wsSecIn = WSSec.getInboundWSSec(securityProperties,
false, true);
+ XMLStreamReader xmlStreamReader = wsSecIn.processInMessage(
+ xmlInputFactory.createXMLStreamReader(
+ new
ByteArrayInputStream(message.getBytes(StandardCharsets.UTF_8))));
+
+ try {
+ StAX2DOM.readDoc(documentBuilderFactory.newDocumentBuilder(),
xmlStreamReader);
+ fail("Exception expected");
+ } catch (XMLStreamException e) {
+ assertTrue(e.getCause() instanceof WSSecurityException);
+ WSSecurityException ex = (WSSecurityException) e.getCause();
+ assertEquals(WSSecurityException.ErrorCode.INVALID_SECURITY,
ex.getErrorCode());
+ assertTrue(ex.getMessage().contains("The Nonce is empty"),
ex.getMessage());
+ }
+
+ replayCache.close();
+ }
+ }
+
/**
* Test that creates, sends and processes an unsigned SAML 2
authentication assertion. This
* is just a sanity test to make sure that it is possible to send the SAML
token twice, as
