This is an automated email from the ASF dual-hosted git repository. papegaaij pushed a commit to branch wicket-10.x in repository https://gitbox.apache.org/repos/asf/wicket.git
commit 3bea655f74ab8f32f9a1ffa7f7e1e386fac60abd Author: Emond Papegaaij <[email protected]> AuthorDate: Mon Aug 3 15:32:13 2026 +0200 Document the security model and page store crypter trust Add a SECURITY.md covering how to report a vulnerability privately, the supported release lines, and the trust boundaries the framework assumes. Wicket derives its own scheme, host and port from the servlet container and trusts those values as its identity. They back every absolute URL rendered via UrlRenderer, the scheme-switch redirect in HttpsMapper, and the trusted target URI that OriginResourceIsolationPolicy compares incoming Origin and Referer headers against. Nothing in the framework validates the Host header, because only the deployment knows its canonical hostnames, so state that rejecting unexpected hosts is the container's or reverse proxy's job. Also document that page store encryption protects only as far as the configured ICrypter states, which matters because Wicket deserializes whatever it reads back from the store. DefaultCrypter (AES-256-CBC) is unauthenticated and gives confidentiality only; GCMSIVCrypter (AES-256-GCM-SIV) additionally makes the stored bytes tamper-evident. Touches the javadoc of HttpsMapper, ICrypter and its implementations, and StoreSettings, plus the HTTPS and page storing chapters of the user guide. The supported version table follows the download page. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> (cherry picked from commit fa7ab44dde6bc09aaeab3dd420a23291902b765f) --- SECURITY.md | 186 +++++++++++++++++++++ .../wicket/pageStore/crypt/DefaultCrypter.java | 16 +- .../wicket/pageStore/crypt/GCMSIVCrypter.java | 17 +- .../apache/wicket/pageStore/crypt/ICrypter.java | 13 ++ .../apache/wicket/protocol/https/HttpsMapper.java | 18 +- .../org/apache/wicket/settings/StoreSettings.java | 9 +- .../src/main/asciidoc/internals/pagestoring.adoc | 19 ++- .../src/main/asciidoc/security/security_3.adoc | 15 ++ 8 files changed, 284 insertions(+), 9 deletions(-) diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000000..b9b6420b11 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,186 @@ +# Security Policy + +Apache Wicket follows the [Apache Software Foundation security process](https://www.apache.org/security/). + +## Reporting a Vulnerability + +**Please do not report security vulnerabilities through GitHub issues, GitHub +discussions, pull requests, JIRA, or the public mailing lists.** Doing so +discloses the issue publicly before a fix is available. + +Report suspected vulnerabilities privately to: + +- **[email protected]** — the ASF Security Team, who will forward the report + to the Wicket PMC, or +- **[email protected]** — the Wicket PMC directly. + +A useful report includes: + +- the affected Wicket version(s) and the module (e.g. `wicket-core`), +- the affected class and method, ideally with a source reference, +- a concrete description of how an attacker reaches the code, including what + the attacker is assumed to control (see [Security Model](#security-model)), +- the impact you believe follows from that, and +- a reproducer where possible — a failing test is ideal. + +Please state clearly whether you have published anything about the issue, and +whether you are requesting a CVE. + +We ask reporters to keep the issue confidential until a fixed release is +published and the PMC has announced it. In return, we will keep you informed of +our assessment and of the release timeline, and credit you in the announcement +unless you ask us not to. + +Note that reports are assessed against the security model below. A report that +depends on the framework distrusting something this model treats as trusted may +be closed as a deployment or configuration issue rather than a framework +vulnerability. If you believe the model itself is wrong, that is a legitimate +and useful thing to report — please say so explicitly, so we discuss the model +rather than the individual code path. + +Conversely, a demonstrated bypass of a boundary this model does claim — for +example the package resource guard, or an authorization strategy — is a +vulnerability, and we want to hear about it. The boundaries below describe what +Wicket intends to enforce; where the code falls short of them, the code is what +needs fixing. + +## Supported Versions + +Security fixes are applied to the actively maintained release lines. Refer to +the [download page](https://wicket.apache.org/start/download.html) for the +current status and the latest release of each line. + +| Version | Status | +| ------- | ----------------------------------------------- | +| 11.x | In development (`master`) — not yet released | +| 10.x | Current, supported | +| 9.x | Supported | +| 8.x | Security fixes only — upgrade to 9.x or 10.x | +| ≤ 7.x | Discontinued — no security fixes | + +If you are running a discontinued version, the fix is to upgrade. See the +[Migration to Wicket 10.0](https://cwiki.apache.org/confluence/display/WICKET/Migration+to+Wicket+10.0) +guide on our wiki, which links the guides for the earlier lines. + +## Security Model + +Wicket is a framework, not a deployed application. It runs inside a servlet +container, usually behind a reverse proxy, and it inherits its view of the +outside world from that container. This section documents which of those inputs +Wicket treats as trusted, so that operators know what they are responsible for +and reporters know what the framework does and does not claim to defend. + +### Wicket trusts the container-reported host, port and scheme + +Wicket derives its own public identity — the scheme, host and port it believes +it is being served on — from the servlet container, via +`HttpServletRequest#getScheme()`, `#getServerName()` and `#getServerPort()`. +There is no hostname allowlist in the framework and no attempt to verify the +`Host` header, in any of the places this identity is used: + +- `ServletWebRequest#setParameters` sets the host, port and protocol on the + client URL from these three values. That URL backs `UrlRenderer`, and so + every absolute URL Wicket renders. +- `HttpsMapper#createRedirectUrl` builds the scheme-switch redirect for + `@RequireHttps` pages from the same values. +- `OriginResourceIsolationPolicy#getTargetUriFromRequest` builds the **trusted** + target URI that incoming `Origin` and `Referer` headers are compared against. + +This is a deliberate design decision, not an oversight. Only the deployment +knows its own canonical hostnames; the framework cannot infer them. Note in +particular that the third item means the container-reported host is a trusted +input to Wicket's own request-forgery defences — a deployment that lets +arbitrary `Host` values through weakens more than URL rendering. + +**Therefore the deployment is responsible for ensuring that only expected +`Host` values reach the application.** Concretely: + +1. Configure the container or virtual host to reject requests carrying an + unrecognised `Host` — return a 400 or 404 rather than routing them to the + application. Tomcat, Jetty and the common reverse proxies all support this. +2. If TLS is terminated at a proxy, have the proxy set or overwrite `Host` to + the canonical name rather than forwarding whatever the client sent. +3. Do not expose a Wicket application through a catch-all or default virtual + host that accepts any `Host`. +4. Serve the application over HTTPS and enable HSTS, so that plaintext requests + — including the ones `HttpsMapper` exists to upgrade — are not part of the + normal flow. + +A consequence worth stating plainly: on a deployment that accepts arbitrary +`Host` values, absolute URLs and redirects generated by Wicket will contain the +host the client supplied. That is the documented behaviour of trusting the +container. It is not treated as a framework vulnerability, because the host in +such a response is always the same authority the client had already connected +to — it grants an attacker no origin they did not already control. The fix +belongs at the container or proxy, per the points above. + +### `X-Forwarded-*` headers are not trusted by default + +Wicket ignores `X-Forwarded-For` and `X-Forwarded-Proto` unless you explicitly +enable `XForwardedRequestWrapperFactory`. When enabled, it overrides +`getRemoteAddr()`, `getRemoteHost()`, `getScheme()` and `getServerPort()` from +those headers, subject to its `internalProxies` and `trustedProxies` +configuration. + +Only enable it when a trusted proxy in front of the application appends to +these headers and strips any client-supplied copies; otherwise the headers are +attacker-controlled. Wicket does not implement `X-Forwarded-Host` at all, and +`XForwardedRequestWrapper` does not override `getServerName()` — the host always +comes from the container as described above. + +### Client-supplied URLs are not trusted for authority + +For Ajax requests Wicket reads a client-supplied base URL — the +`Wicket-Ajax-BaseURL` header, falling back to the `wicket-ajax-baseurl` request +parameter — in order to resolve relative URLs against the page the client is +actually on. The host, port and protocol of that URL are always overwritten with +the container-reported values before use. The client can influence the path +Wicket renders relative to, never the authority. + +### Deployment configuration is the operator's responsibility + +`RuntimeConfigurationType.DEVELOPMENT` enables debugging aids, verbose error +reporting and development-only components, and disables some caching. It is not +intended for production and is not hardened. Always run production deployments +with the configuration type set to `RuntimeConfigurationType.DEPLOYMENT`. Issues +only reachable in `DEVELOPMENT` mode are treated as configuration errors rather +than vulnerabilities. + +Likewise, `wicket-devutils` is a development aid. Do not deploy it in +production. + +### Serialized data is trusted + +Wicket serializes page instances and session data to its page store. Java +deserialization is not a safe operation on untrusted input, and by default +Wicket's page store does not defend against it. Treat the page store and the +session store as trusted, private storage: do not point them at storage that +untrusted parties can write to, and do not accept externally supplied +serialized page or session data. + +The exception is a page store configured with encryption +(`StoreSettings#setEncrypted(true)`), and then only as far as the configured +`ICrypter` implementation documents. Encryption does not by itself imply +tamper detection: whether the stored bytes are merely confidential or are also +protected against modification depends entirely on the implementation in use, +and the default implementation is not authenticated. Consult the javadoc of the +`ICrypter` you configure and rely on no more than it states. + +### `CryptoMapper` is not an authorization mechanism + +`CryptoMapper` encrypts URLs so that page and component identifiers are not +guessable. It raises the cost of forging a URL, but it is not an access-control +mechanism. Authorization must be enforced with `IAuthorizationStrategy` (or +equivalent) so that it holds regardless of whether a URL was guessed, +replayed, leaked through a referrer, or found in a log. + +## Reporting Something That Is Not a Vulnerability + +Findings that are real but not vulnerabilities are still welcome — please raise +them publicly in [JIRA](https://issues.apache.org/jira/projects/WICKET) or as a +pull request rather than through the private security channel, so they can be +discussed and fixed in the open. Hardening suggestions, defence-in-depth +improvements, and clarifications to this document all fall into that category. + +If you are unsure which channel applies, use the private one — we would rather +receive a non-issue privately than a real issue publicly. diff --git a/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/DefaultCrypter.java b/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/DefaultCrypter.java index 403a4d79a5..bde5d79448 100644 --- a/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/DefaultCrypter.java +++ b/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/DefaultCrypter.java @@ -29,7 +29,21 @@ import javax.crypto.spec.IvParameterSpec; import org.apache.wicket.WicketRuntimeException; /** - * Default encryption and decryption implementation. + * Default encryption and decryption implementation, using AES-256 in CBC mode. + * <p> + * <strong>This implementation is not authenticated and provides confidentiality only.</strong> CBC + * ciphertext is malleable and its integrity is not verified on decryption, so an attacker who can + * write to the underlying store can alter the stored bytes and have the result passed to the + * deserializer. Enabling encryption with this crypter therefore hides the contents of stored pages, + * but does not make the page store safe against tampering - the underlying store must still be + * treated as trusted, private storage. + * <p> + * Use {@link GCMSIVCrypter} instead where the stored bytes need to be tamper-evident as well as + * confidential. It is slower (see its javadoc) and requires Bouncy Castle, which is why this + * implementation remains the default. + * + * @see GCMSIVCrypter + * @see ICrypter */ public class DefaultCrypter implements ICrypter { diff --git a/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/GCMSIVCrypter.java b/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/GCMSIVCrypter.java index e4fff7bcc8..456b3cffe6 100644 --- a/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/GCMSIVCrypter.java +++ b/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/GCMSIVCrypter.java @@ -30,13 +30,24 @@ import org.bouncycastle.jcajce.spec.AEADParameterSpec; /** * Encryption and decryption implementation using AES-256-GCM-SIV authenticated encryption. - * - * This implementation requires Bouncy Castle. It is more secure than the {@link DefaultCrypter}, - * but also more expensive. Simple measurements have shown {@link DefaultCrypter} to be about 10 to + * <p> + * Because GCM-SIV is an AEAD mode, this implementation provides integrity as well as + * confidentiality: modified or substituted ciphertext fails to decrypt instead of being handed to + * the deserializer. It is therefore the implementation to choose when the page store needs to be + * tamper-evident and not merely unreadable - unlike the unauthenticated {@link DefaultCrypter}, + * which is the default. Note that the key is held in the session, so this protects against parties + * who can read or write the stored bytes, not against one who already controls the session. + * <p> + * This implementation requires Bouncy Castle, which is an optional dependency of {@code + * wicket-core} and must be added explicitly. It is more secure than the {@link DefaultCrypter}, but + * also more expensive. Simple measurements have shown {@link DefaultCrypter} to be about 10 to * 15 times faster than this implementation. This is likely caused by not-so-optimal implementation * of the algorithm in Java by BC. When the JDK gets support for GCM-SIV * (https://bugs.openjdk.org/browse/JDK-8256530), this implementation will likely be faster than or * about as fast as CBC. + * + * @see DefaultCrypter + * @see ICrypter */ public class GCMSIVCrypter implements ICrypter { diff --git a/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/ICrypter.java b/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/ICrypter.java index 04ef18b670..3595b9b6da 100644 --- a/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/ICrypter.java +++ b/wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/ICrypter.java @@ -23,6 +23,19 @@ import javax.crypto.SecretKey; /** * An encrypter and decrypter of pages. + * <p> + * Implementations are not required to provide <em>authenticated</em> encryption, and callers must + * not assume that they do. An unauthenticated implementation gives confidentiality only: it hides + * the contents of a serialized page, but it does not detect modification of the stored bytes, so + * tampered ciphertext may still be handed to the deserializer. Of the implementations shipped with + * Wicket, {@link GCMSIVCrypter} is authenticated and {@link DefaultCrypter} - the default - is not. + * <p> + * If you implement this interface and want tamper detection, use an AEAD cipher mode (such as + * GCM, GCM-SIV or CCM) rather than adding encryption on top of an unauthenticated mode. See + * {@code SECURITY.md} for the trust assumptions Wicket makes about the page store. + * + * @see org.apache.wicket.pageStore.CryptingPageStore + * @see org.apache.wicket.settings.StoreSettings#setCrypter(java.util.function.Supplier) */ public interface ICrypter { SecretKey generateKey(SecureRandom random); diff --git a/wicket-core/src/main/java/org/apache/wicket/protocol/https/HttpsMapper.java b/wicket-core/src/main/java/org/apache/wicket/protocol/https/HttpsMapper.java index 25dbd20277..17f4c2dd87 100644 --- a/wicket-core/src/main/java/org/apache/wicket/protocol/https/HttpsMapper.java +++ b/wicket-core/src/main/java/org/apache/wicket/protocol/https/HttpsMapper.java @@ -58,7 +58,16 @@ import org.apache.wicket.util.lang.Args; * * any request to <em>http://hostname:httpPort/secured</em> will be redirected to * <em>https://hostname:httpsPort/secured</em> - * + * <p> + * <strong>Deployment note:</strong> the <em>hostname</em> in the generated redirect is the host + * reported by the servlet container ({@link HttpServletRequest#getServerName()}, i.e. the + * {@code Host} header), which Wicket trusts as its own identity. This mapper deliberately performs + * no hostname validation, because only the deployment knows its canonical names. Configure the + * container, virtual host or reverse proxy to reject requests carrying an unexpected {@code Host} + * rather than routing them to the application; otherwise the redirect - like every absolute URL + * Wicket renders - will echo the host the client supplied. See {@code SECURITY.md} for Wicket's + * trust assumptions about the container-reported host, port and scheme. + * * @author igor */ public class HttpsMapper implements IRequestMapperDelegate @@ -139,7 +148,12 @@ public class HttpsMapper implements IRequestMapperDelegate /** * Constructs a redirect url that should switch the user to the specified {@code scheme} - * + * <p> + * The host is taken from the container-reported server name, so the redirect stays on the + * authority the client already connected to and only the scheme changes. Validating that + * authority is the container's or reverse proxy's responsibility - see the class javadoc. + * Override this method to derive the host from configuration instead. + * * @param handler * request handler being accessed * @param request diff --git a/wicket-core/src/main/java/org/apache/wicket/settings/StoreSettings.java b/wicket-core/src/main/java/org/apache/wicket/settings/StoreSettings.java index a6ab0cb5aa..b25315a5dc 100644 --- a/wicket-core/src/main/java/org/apache/wicket/settings/StoreSettings.java +++ b/wicket-core/src/main/java/org/apache/wicket/settings/StoreSettings.java @@ -212,7 +212,11 @@ public class StoreSettings /** * Sets the supplier for the {@link ICrypter} used by a * {@link org.apache.wicket.pageStore.CryptingPageStore}. - * + * <p> + * The default {@link DefaultCrypter} is not authenticated and gives confidentiality only. Set + * {@link org.apache.wicket.pageStore.crypt.GCMSIVCrypter} here if the stored bytes must also be + * tamper-evident. + * * @param crypter * The new supplier for an {@link ICrypter}. * @return {@code this} object for chaining @@ -226,7 +230,8 @@ public class StoreSettings /** * @return the supplier used to create a {@link ICrypter} for a * {@link org.apache.wicket.pageStore.CryptingPageStore}. The default is - * {@link DefaultCrypter}. + * {@link DefaultCrypter}, which is not authenticated - see + * {@link #setCrypter(Supplier)}. */ public Supplier<ICrypter> getCrypter() { diff --git a/wicket-user-guide/src/main/asciidoc/internals/pagestoring.adoc b/wicket-user-guide/src/main/asciidoc/internals/pagestoring.adoc index 822ce0862e..f98f7d1d4e 100644 --- a/wicket-user-guide/src/main/asciidoc/internals/pagestoring.adoc +++ b/wicket-user-guide/src/main/asciidoc/internals/pagestoring.adoc @@ -38,11 +38,28 @@ i.e. this is the serialized page instance (data) plus additional information nee * *AsynchronousPageStore:* The role of _AsynchronousPageStore_ is to detach the http worker thread from waiting for the write of the page bytes to the disk. To disable it use: _org.apache.wicket.settings.StoreSettings.setAsynchronous(false)_. _AsynchronousPageStore_ can delay the storage of page's bytes for at most _org.apache.wicket.settings.StoreSettings.setAsynchronousQueueCapacity(int)_ pages. If this capacity is exceeded then the page's bytes are written synchronously to the backing [...] -* *CryptingPageStore:* page instances might contain sensible information, therefore it's important to have the chance to encrypt their content before persist them on disk. _CryptingPageStore_ encrypts _SerializedPage_'s with a 256 bit AES before passing them to the underling _DiskPageStore_. Buy default this stage is disabled and not added to the default chain. To change this behavior we can use _org.apache.wicket.settings.StoreSettings.setEncrypted_. +* *CryptingPageStore:* page instances might contain sensible information, therefore it's important to have the chance to encrypt their content before persist them on disk. _CryptingPageStore_ encrypts _SerializedPage_'s with a 256 bit AES key, held in the session, before passing them to the underlying _DiskPageStore_. By default this stage is disabled and not added to the default chain. To change this behavior we can use _org.apache.wicket.settings.StoreSettings.setEncrypted_. What that [...] * *DiskPageStore:* stores _SerializedPage_ on a session-scoped file on disk. The location of the folder where the files are stored is configurable via _org.apache.wicket.settings.StoreSettings.setFileStoreFolder(File)_, by default the web container's work folder is used (ServletContext attribute 'javax.servlet.context.tempdir'). In this folder a sub-folder is created named _'applicationName-filestore'_. This folder contains a sub-folder for each active http session. This session folder contains a single file named 'data' which contains the bytes for the pages. The size of this 'data' file is configurable via _org.apache.wicket.settings.StoreSettings.setMaxSizePerSession(Bytes)_. When this size is exceeded the newly stored files overwrite the oldest ones. +[#pagestore-choosing-a-crypter] +=== Choosing a crypter for CryptingPageStore + +When page store encryption is enabled, the actual encryption is performed by an implementation of _org.apache.wicket.pageStore.crypt.ICrypter_, configurable with _org.apache.wicket.settings.StoreSettings.setCrypter_. Which implementation we pick determines what the encryption protects against, and the difference matters because Wicket deserializes whatever it reads back from the store: + +* _DefaultCrypter_ (AES-256-CBC) is the default. It is *not* authenticated, so it provides confidentiality only: the stored pages are unreadable, but CBC ciphertext is malleable and its integrity is not verified on decryption, so someone able to write to the store can still influence what gets deserialized. +* _GCMSIVCrypter_ (AES-256-GCM-SIV) is authenticated. Modified or substituted ciphertext fails to decrypt instead of reaching the deserializer, which makes the stored bytes tamper-evident as well as confidential. It requires Bouncy Castle - an optional dependency of _wicket-core_ that we must add ourselves - and is noticeably slower, which is why it is not the default. + +So if we enable encryption because the store lives somewhere we do not fully control, rather than merely to keep page contents private, we should select the authenticated crypter explicitly: + +[source,java] +---- +getStoreSettings().setEncrypted(true).setCrypter(GCMSIVCrypter::new); +---- + +In both cases the key is held in the session, so encryption protects the pages against parties who can read or write the stored bytes - not against one who already controls the session. A custom _ICrypter_ only provides tamper detection if the cipher it returns uses an AEAD mode. + === InMemoryPageStore An alternative _IPageStore_ we can use in a custom chain is _org.apache.wicket.pageStore.InMemoryPageStore_. This implementation stores pages in a session-relative variable which can be limited by size or by page instance number. diff --git a/wicket-user-guide/src/main/asciidoc/security/security_3.adoc b/wicket-user-guide/src/main/asciidoc/security/security_3.adoc index 5dd2e0e6d3..b0741c188a 100644 --- a/wicket-user-guide/src/main/asciidoc/security/security_3.adoc +++ b/wicket-user-guide/src/main/asciidoc/security/security_3.adoc @@ -59,3 +59,18 @@ public class HttpsPage extends WebPage implements IMarker { } ---- +=== The redirect host comes from the container + +When _HttpsMapper_ builds the redirect that switches a request to HTTPS, the hostname it uses is the one reported by the servlet container - _HttpServletRequest.getServerName()_, which is the _Host_ header of the incoming request. The same is true of every absolute URL Wicket renders, and of the target URI that Wicket's resource-isolation policies compare incoming _Origin_ and _Referer_ headers against. + +Wicket trusts this value as its own identity and performs no hostname validation of its own: only the deployment knows which hostnames are canonical, so the framework cannot decide this for us. Making sure that only expected _Host_ values ever reach the application is therefore part of deploying a Wicket application, not something the framework does for us: + +* configure the container or virtual host to reject requests carrying an unrecognised _Host_, returning a 400 or 404 instead of routing them to the application; +* when TLS is terminated at a reverse proxy, have the proxy set _Host_ to the canonical name rather than passing on whatever the client sent; +* avoid exposing a Wicket application through a catch-all or default virtual host that accepts any _Host_; +* serve the application over HTTPS and enable HSTS, so that the plain HTTP requests _HttpsMapper_ exists to upgrade are not part of the normal flow. + +On a deployment that does accept arbitrary _Host_ values, redirects and absolute URLs will contain the host the client supplied. Note that this is not a way for an attacker to reach an origin they did not already control - a browser derives the _Host_ header from the URL it is fetching, so the host in the response is always the authority the client had already connected to. It does, however, weaken the _Origin_ and _Referer_ checks mentioned above, which is the better reason to get the c [...] + +Wicket also ignores the _X-Forwarded-For_ and _X-Forwarded-Proto_ headers unless we explicitly install _XForwardedRequestWrapperFactory_, which should only be enabled when a trusted proxy in front of the application strips any client-supplied copies of them. Note that Wicket does not implement _X-Forwarded-Host_ at all; the host always comes from the container. +
