Document the security model and Wicket's trust assumptions

SECURITY.md

@@ -0,0 +1,211 @@
+# 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:
+
+- **security@apache.org** — the ASF Security Team, who will forward the report
+  to the Wicket PMC, or
+- **private@wicket.apache.org** — 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.
+
+### Another origin may not invoke a listener
+
+Where `ResourceIsolationRequestCycleListener` is registered, a request originating
+from another origin must not be able to invoke a listener on a page — a
+`Link.onClick()`, a `Form.onSubmit()`, or an AJAX behaviour. A demonstrated way
+for another origin to reach one is a vulnerability.
+
+Two things sit deliberately outside that boundary:
+
+- **Rendering a page is allowed.** A page may be reached by a simple top-level
+  navigation from anywhere, so that pages remain linkable from other sites. Only
+  the invocation of a listener is refused. Requests that are not top-level
+  navigations — subresource loads, `fetch`, `<object>` and `<embed>` — are
+  refused for renders too.
+- **Sibling origins may be trusted explicitly.** `Sec-Fetch-Site: same-site`
+  means a different origin on the same registrable domain and scheme, such as
+  another subdomain, and is refused by default. A deployment that trusts every
+  origin on its own site can allow it; sibling-origin actions are then that
+  deployment's decision rather than a framework vulnerability.
+
+This listener is opt-in and is not registered by default. Without it Wicket
+enforces no cross-origin boundary on listener invocation at all. `CryptoMapper`
+raises the cost of forging a URL but is not a substitute for it, for the reason
+below.
+
+### `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.

wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/DefaultCrypter.java

@@ -29,7 +29,21 @@
 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
 {

wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/GCMSIVCrypter.java

@@ -30,13 +30,24 @@
 
 /**
  * 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
 {

wicket-core/src/main/java/org/apache/wicket/pageStore/crypt/ICrypter.java

@@ -23,6 +23,19 @@
 
 /**
  * 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);

wicket-core/src/main/java/org/apache/wicket/protocol/https/HttpsMapper.java

@@ -58,7 +58,16 @@
  * 
  * 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 @@ protected IRequestHandler createRedirectHandler(String url)
 
 	/**
 	 * 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

wicket-core/src/main/java/org/apache/wicket/settings/StoreSettings.java

@@ -212,7 +212,11 @@ public boolean isEncrypted()
 	/**
 	 * 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 StoreSettings setCrypter(Supplier<ICrypter> crypter)
 	/**
 	 * @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()
 	{