WICKET-7190 Unify and harden the cryptography subsystem - #1536
Draft
papegaaij wants to merge 7 commits into
Draft
Conversation
Replace the two disconnected crypt stacks (util.crypt.ICrypt and pageStore.crypt.ICrypter) with a single authenticated, self-describing crypto system in org.apache.wicket.core.util.crypt: - ICryptScheme with AesGcmCryptScheme (JDK-native AES-256-GCM, the default) and AesGcmSivCryptScheme (AES-256-GCM-SIV, Bouncy Castle, optional). Ciphertext is prefixed with a one-byte scheme marker that is bound as AEAD associated data. - SchemeCrypt selects the scheme by its marker against a configurable whitelist (downgrade protection), always encrypts with the strongest configured scheme, and returns null on any decryption failure. - Key-source factories: KeyInSessionCryptFactory (per-session key, the default) and ApplicationKeyCryptFactory (application-wide key, used by remember-me). - SecuritySettings owns the crypto policy (crypt scheme + whitelist); the page store, CryptoMapper and the remember-me cookie all use it. This fixes the reported page-store weakness (unauthenticated AES-CBC, malleable via CBC bit-flipping and vulnerable to a padding oracle) by making all encryption authenticated, and removes the weak PBEWithMD5AndDES default that was used for URLs and the authentication cookie. Authenticated encryption uses a random nonce and is therefore non-deterministic. CryptoMapper reuses, within a request, the ciphertext it decrypted so that a regenerated URL matches the requested one; this avoids Wicket's URL-normalisation infinite redirect while keeping random nonces (a nonce is only ever reused for the identical plaintext). Removes the obsolete implementations (SunJceCrypt, AbstractCrypt, TrivialCrypt, CryptFactoryCachingDecorator, AESCrypt, AbstractJceCrypt, KeyInSessionSunJceCryptFactory, AbstractKeyInSessionCryptFactory, DefaultCrypter, GCMSIVCrypter, ICrypter) and adds a MigrateToWicket11 OpenRewrite recipe. Data encrypted by Wicket 10 cannot be decrypted by Wicket 11 (clean break); the marker/whitelist mechanism enables future migrations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… data Expose associated data (AAD) in the crypt API and use it to bind each encrypted page to its id: - ICrypt gains encrypt(byte[], byte[] associatedData) and decrypt(byte[], byte[] associatedData); the no-arg overloads delegate with no associated data. SchemeCrypt authenticates marker || associatedData, so the no-associated-data path is unchanged and URL/cookie ciphertext is unaffected. - CryptingPageStore passes the page id (4 big-endian bytes) as associated data, using the trusted id parameter on getPage and the server-assigned id on addPage. A blob stored for one id can no longer be substituted into another slot by a tamper-capable storage adversary: it fails authentication and is treated as a cache miss. URLs (CryptoMapper) and the remember-me cookie bind no associated data: there is no useful context to add (the session is redundant under a per-session key and breaks URL sharing under an application-wide key; request-specific context would break bookmarkability). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A factory cannot know what key material an encryption scheme needs, so key
generation is moved from the factories onto the scheme:
- ICryptScheme gains an abstract generateKey(SecureRandom). The scheme owns the
key material; a factory only decides where the key lives (per session, global,
externally supplied). All schemes sharing one SchemeCrypt must produce
compatible keys, since existing ciphertext is decrypted with the current key
during migration.
- New AbstractAesGcmCryptScheme carries everything the AES-256 GCM-family schemes
share: the 256-bit AES key generation and the encrypt/decrypt flow (12-byte
nonce, 128-bit tag, nonce||ciphertext||tag layout, marker authenticated as
associated data). AesGcmCryptScheme and AesGcmSivCryptScheme now only supply
id(), the Cipher and the AlgorithmParameterSpec, removing the duplication that
existed between them.
All four call sites that previously hardcoded
CipherUtils.generateKey("AES", 256, ...) now delegate to the configured scheme:
- AbstractCryptFactory gains a protected generateKey(random) helper resolving the
scheme from SecuritySettings; KeyInSessionCryptFactory uses it.
- ApplicationKeyCryptFactory(SecureRandom) generates its key lazily on first use
(so the scheme is configured by then) and caches it; the (SecretKey)
external-key constructor is unchanged.
- SecuritySettings.getAuthenticationStrategy() and CryptingPageStore both ask the
scheme for the key.
Tests: SchemeCryptTest verifies generateKey yields a usable 256-bit AES key that
round-trips; CryptFactoryTest covers the lazy application-random-key path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "URLs encryption in detail" section still described the Wicket 10 crypt API (some of it already stale before this work). Bring it in line with the redesign: - ICrypt is shown from its real package org.apache.wicket.core.util.crypt with the byte-array + URL-safe-String signature and the null-on-failure contract. - The default implementation is SchemeCrypt (self-describing, authenticated ciphertext), not SunJceCrypt/PBEWithMD5AndDES. Document the pluggable ICryptScheme, the default AesGcmCryptScheme (JDK-native AES-256-GCM), the scheme whitelist / downgrade protection and migration, and the AES-256-GCM-SIV opt-in (requires Bouncy Castle). - The default factory is KeyInSessionCryptFactory (was KeyInSessionSunJceCrypt- Factory); the stateless note now points to ApplicationKeyCryptFactory. Also drop the outdated warning in the URL chapter that the default cipher "might not be strong enough for production": the default is now authenticated AES-256-GCM with a per-session key. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The signed upload settings introduced in 483b83c were validated by re-encrypting the expected settings and comparing the resulting ciphertext to the token supplied by the client. That only worked because the default crypt at the time was deterministic PBE: the same plaintext always produced the same ciphertext. SchemeCrypt encrypts with a fresh random nonce per message, as an AEAD mode must, so no two encryptions of the same settings agree and the comparison never succeeds - every upload through FileUploadToResourceField was rejected. Validate the token by decrypting it and comparing the settings it carries instead. The authentication tag is what makes the token unforgeable, and decryption returns null for anything tampered with, so the check keeps rejecting a client that alters the limits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The redesigned crypt classes document what they guarantee, but not the limits of that guarantee. Both points were made in the javadoc of the ICrypter implementations this branch removes, and until now survived only in StoreSettings#setEncrypted, the user guide and SECURITY.md - none of which a reader lands on when they open CryptingPageStore or implement ICryptScheme. Record on CryptingPageStore that the session-held key protects the stored pages against a party who can read or write the underlying store, but not against one who already controls the session, so the store is still trusted storage. Note on ICryptScheme that a custom scheme returning unverified plaintext instead of null removes the tamper detection callers rely on, and point both at SECURITY.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Authenticated encryption draws a random nonce, so re-encrypting the same URL produced different ciphertext every time. CryptoMapper worked around that with a per-RequestCycle plaintext -> ciphertext memo: enough to keep URL normalisation from redirecting endlessly, but it died with the request. Every page view therefore handed the browser brand-new URLs for every JavaScript, CSS and image resource. Those responses carry a one-year cache duration, and because the URL itself changed the browser could not even issue a conditional request, so each page view re-downloaded every resource; through CSS, whose body embeds resource URLs, the effect compounded. Wicket 10 did not have this problem, as SunJceCrypt derived its IV from a per-session salt and was thereby deterministic. Add an explicit deterministic encryption path to the crypt API and use it for URLs: - ICrypt.encryptDeterministic and ICryptScheme.encryptDeterministic. Required rather than an opt-in with a random-nonce fallback, so that a scheme cannot silently reintroduce the redirect hazard. Both write the existing ciphertext format, so decryption is unchanged. - AbstractAesGcmCryptScheme derives the nonce as HMAC(HMAC(key, "wicket-deterministic-nonce"), len(aad) || aad || plaintext) truncated to 96 bits, covering AES-256-GCM and AES-256-GCM-SIV alike. - CryptoMapper encrypts with the deterministic path, which lets the entire memo go: ENCRYPTED_URL_CACHE, encryptString, rememberEncryption and getEncryptionMemo, along with the verification decrypt they required. Determinism reveals that two ciphertexts encrypt equal plaintexts, and lets a key holder confirm a guessed URL. With the default per-session key that stays confined to a single session, and it is what Wicket 10 already did. The page store, the "remember me" cookie and the upload token keep randomized nonces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wicket had three overlapping crypt abstractions —
org.apache.wicket.util.crypt.ICrypt(URL encryption, "remember me"),org.apache.wicket.pageStore.crypt.ICrypter(page store), and theICryptFactoryhierarchy — with different guarantees. Notably the page store default,DefaultCrypter, was AES-256-CBC: unauthenticated, so stored bytes were confidential but malleable, and tampered ciphertext still reached the deserializer.This replaces all of it with one authenticated abstraction.
What changes
One interface.
org.apache.wicket.core.util.crypt.ICryptis now the single abstraction, used by the page store,CryptoMapperand the authentication cookie alike. It operates onbyte[]with an optional associated-data parameter, and provides the URL-safe Base64Stringlayer as default methods.decryptreturnsnullon any failure — unknown scheme, failed authentication, malformed input — so callers uniformly treat undecryptable data as absent rather than catching exceptions.Authenticated by default, and self-describing.
SchemeCryptwritesmarker(1) || scheme-payload, where the marker is the id of theICryptSchemethat produced the payload. Encryption always uses the configured scheme; decryption looks the marker up in a whitelist and refuses anything not on it. The marker is authenticated as associated data, so it cannot be altered to force a weaker scheme — together that gives downgrade protection, and a documented migration path (whitelist the old scheme while the new one becomes the encryption scheme, then drop it once data is rewritten).Two shipped schemes, both AEAD:
AesGcmCryptScheme— JDK-native AES-256-GCM. The new default; no extra dependencies.AesGcmSivCryptScheme— AES-256-GCM-SIV, nonce-misuse resistant. Requires Bouncy Castle.Scheme and key source are separated. An
ICryptSchemedecides how data is encrypted and generates the keys it consumes; anICryptFactorydecides only where the key lives —KeyInSessionCryptFactory(per session, the default) orApplicationKeyCryptFactory(application-wide, for stateless deployments).Encrypted pages are bound to their page id.
CryptingPageStorepasses the page id as associated data, so a stored page cannot be replayed as a different one.Behavioural fix outside the crypt package
AbstractFileUploadResourcevalidated its signed upload settings by re-encrypting the expected values and comparing the resulting ciphertext. That only worked because the previous default was deterministic PBE.SchemeCryptuses a fresh nonce per message, as an AEAD mode must, so the comparison could never succeed and everyFileUploadToResourceFieldupload would have been rejected. It now decrypts the token and compares the settings it carries; the authentication tag is what makes the token unforgeable, andFileUploadToResourceFieldSecurityTeststill rejects tampered limits.Compatibility — this is a Wicket 11 API break
Removed with no replacement:
SunJceCrypt,AbstractCrypt,TrivialCrypt,CryptFactoryCachingDecorator,AESCrypt,AbstractJceCrypt,KeyInSessionSunJceCryptFactory,AbstractKeyInSessionCryptFactory,DefaultCrypter,GCMSIVCrypter,ICrypter.NoCrypt,NoCryptFactoryandICryptFactorymove toorg.apache.wicket.core.util.crypt. OpenRewrite recipes for the moves and notes on the removals are inwicket-migration.Existing ciphertext does not decrypt under the new schemes. In practice that means encrypted URLs and page store entries from a previous version are treated as expired, and "remember me" cookies are invalidated.
Testing
wicket-core-testssuite: 2331 tests, 0 failures.clean test-compilepasses.SchemeCryptTest(19),CryptFactoryTest(3), plus extendedCryptoMapperTestandCryptingPageStoreTest— round-trips, associated-data mismatch, whitelist refusal, marker tampering.Known red:
japicmpwill fail on the removals above and needs a Wicket 11 baseline before CI is green.🤖 Generated with Claude Code