This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch coheigea/ReplayCache in repository https://gitbox.apache.org/repos/asf/ws-wss4j.git
commit 5c551025a8dc0e93d59364bc418e046a6e085961 Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Thu Oct 22 08:17:41 2026 +0100 Make ReplayCache atomic by adding addIfAbsent --- .../wss4j/common/cache/EHCacheReplayCache.java | 16 +++ .../wss4j/common/cache/MemoryReplayCache.java | 46 +++++-- .../org/apache/wss4j/common/cache/ReplayCache.java | 33 +++++ .../apache/wss4j/common/cache/ReplayCacheTest.java | 145 +++++++++++++++++++++ .../wss4j/dom/processor/SignatureProcessor.java | 14 +- .../dom/processor/UsernameTokenProcessor.java | 16 ++- .../wss4j/dom/validate/SamlAssertionValidator.java | 12 +- .../processor/input/UsernameTokenInputHandler.java | 12 +- .../WSSSignatureReferenceVerifyInputProcessor.java | 11 +- .../stax/validate/SamlTokenValidatorImpl.java | 12 +- 10 files changed, 265 insertions(+), 52 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..91443144b 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,22 @@ 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 false if the identifier was already contained in the cache, true otherwise + */ + @Override + public boolean addIfAbsent(String identifier, Instant expiry) { + if (identifier == null || identifier.length() == 0) { + return true; + } + + 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..f23c80e68 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,47 @@ 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 false if the identifier was already contained in the cache, true otherwise + */ + @Override + public boolean addIfAbsent(String identifier, Instant expiry) { + if (identifier == null || identifier.length() == 0) { + return true; + } + + 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..969a9dc1c 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 false if the identifier was already contained in the cache, in which case the cache is + * left unchanged, true otherwise. An empty identifier is never cached, so true is returned. + */ + default boolean addIfAbsent(String identifier, Instant expiry) { + if (identifier == null || identifier.length() == 0) { + return true; + } + 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/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..0acbbf6f9 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 is never cached + assertTrue(replayCache.addIfAbsent(null, null)); + assertTrue(replayCache.addIfAbsent("", null)); + assertTrue(replayCache.addIfAbsent("", null)); + + // 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-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..e1a08da4e 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 @@ -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-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); - } } }
