This is an automated email from the ASF dual-hosted git repository. dashorst pushed a commit to branch wicket-9.x in repository https://gitbox.apache.org/repos/asf/wicket.git
commit 813bc244baea56bc227ee9288df34812e21c61b0 Author: Emond Papegaaij <[email protected]> AuthorDate: Thu Oct 1 14:04:07 2026 +0200 Sign the upload settings with an HMAC instead of the configured crypt The upload token was built with the application's ICrypt. Re-encrypting and comparing only works with a deterministic crypt, so with AESCrypt every upload was rejected; decrypting and comparing instead accepts a token built by the client with any crypt that does not authenticate its ciphertext, such as AESCrypt, whose IV the client controls. Sign the settings with HmacSHA256 under a random key kept in the session. The token no longer depends on the configured crypt, is bound to the session that rendered the field, and an upload from a session without a key is rejected. Co-Authored-By: Claude Opus 5.5 <[email protected]> --- .../resource/AbstractFileUploadResource.java | 71 ++++++++++--- .../FileUploadToResourceFieldSecurityTest.java | 110 ++++++++++++++++++++- 2 files changed, 164 insertions(+), 17 deletions(-) diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/AbstractFileUploadResource.java b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/AbstractFileUploadResource.java index 10d907064e..ff01ad1e4b 100644 --- a/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/AbstractFileUploadResource.java +++ b/wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/AbstractFileUploadResource.java @@ -18,16 +18,23 @@ package org.apache.wicket.markup.html.form.upload.resource; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.security.GeneralSecurityException; import java.security.MessageDigest; import java.util.ArrayList; +import java.util.Base64; import java.util.List; import java.util.Map; +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; import javax.servlet.http.HttpServletResponse; import org.apache.commons.fileupload.FileCountLimitExceededException; import org.apache.commons.fileupload.FileItem; import org.apache.commons.fileupload.FileUploadBase; import org.apache.commons.fileupload.FileUploadException; import org.apache.wicket.Application; +import org.apache.wicket.MetaDataKey; +import org.apache.wicket.Session; +import org.apache.wicket.WicketRuntimeException; import org.apache.wicket.markup.html.form.Form; import org.apache.wicket.markup.html.form.upload.FileUpload; import org.apache.wicket.protocol.http.servlet.MultipartServletWebRequest; @@ -77,6 +84,16 @@ public abstract class AbstractFileUploadResource extends AbstractResource */ public static final String NO_FILE_SELECTED = "wicket.no.files.selected"; + private static final String UPLOAD_TOKEN_ALGORITHM = "HmacSHA256"; + + /** + * The key the upload tokens of a session are signed with. + */ + private static final MetaDataKey<byte[]> UPLOAD_TOKEN_KEY = new MetaDataKey<>() + { + private static final long serialVersionUID = 1L; + }; + private final IUploadsFileManager fileManager; public AbstractFileUploadResource(IUploadsFileManager fileManager) @@ -87,9 +104,21 @@ public abstract class AbstractFileUploadResource extends AbstractResource static String createUploadToken(String uploadId, Bytes maxSize, Bytes fileMaxSize, long fileCountMax) { - String plainText = serializeUploadSettings(uploadId, maxSize, fileMaxSize, fileCountMax); - return Application.get().getSecuritySettings().getCryptFactory().newCrypt() - .encryptUrlSafe(plainText); + Session session = Session.get(); + session.bind(); + + byte[] key; + synchronized (session) + { + key = session.getMetaData(UPLOAD_TOKEN_KEY); + if (key == null) + { + key = Application.get().getSecuritySettings().getRandomSupplier() + .getRandomBytes(32); + session.setMetaData(UPLOAD_TOKEN_KEY, key); + } + } + return sign(key, serializeUploadSettings(uploadId, maxSize, fileMaxSize, fileCountMax)); } /** @@ -215,6 +244,24 @@ public abstract class AbstractFileUploadResource extends AbstractResource return value != null ? Long.toString(value.bytes()) : ""; } + // the settings are signed rather than encrypted with the application's ICrypt: the configured + // crypt may be neither deterministic nor authenticated, and then no check built on it is both + // usable and unforgeable + private static String sign(byte[] key, String settings) + { + try + { + Mac mac = Mac.getInstance(UPLOAD_TOKEN_ALGORITHM); + mac.init(new SecretKeySpec(key, UPLOAD_TOKEN_ALGORITHM)); + return Base64.getUrlEncoder().withoutPadding() + .encodeToString(mac.doFinal(settings.getBytes(StandardCharsets.UTF_8))); + } + catch (GeneralSecurityException e) + { + throw new WicketRuntimeException("Unable to sign the upload settings", e); + } + } + private boolean hasValidUploadToken(ServletWebRequest webRequest, String uploadId, Bytes maxSize, Bytes fileMaxSize, long fileCountMax) { @@ -225,21 +272,17 @@ public abstract class AbstractFileUploadResource extends AbstractResource return false; } - // the token is authenticated but not deterministic - encryption uses a fresh nonce per - // message - so it cannot be validated by re-encrypting and comparing ciphertext. Decrypt - // it instead and compare the settings it carries; the authentication tag is what makes it - // unforgeable, and decryption fails on anything that was tampered with. - String decryptedToken = Application.get().getSecuritySettings().getCryptFactory().newCrypt() - .decryptUrlSafe(actualToken); - if (decryptedToken == null) + // only a session that rendered an upload field has a key; never create one here + byte[] key = Session.get().getMetaData(UPLOAD_TOKEN_KEY); + if (key == null) { return false; } - String expectedSettings = serializeUploadSettings(uploadId, maxSize, fileMaxSize, - fileCountMax); - return MessageDigest.isEqual(expectedSettings.getBytes(StandardCharsets.UTF_8), - decryptedToken.getBytes(StandardCharsets.UTF_8)); + String expectedToken = sign(key, + serializeUploadSettings(uploadId, maxSize, fileMaxSize, fileCountMax)); + return MessageDigest.isEqual(expectedToken.getBytes(StandardCharsets.UTF_8), + actualToken.getBytes(StandardCharsets.UTF_8)); } private ResourceResponse createErrorResponse(ResourceResponse resourceResponse, String errorMessage) diff --git a/wicket-core/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java b/wicket-core/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java index ec62950a7e..3f9580bbbe 100644 --- a/wicket-core/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java +++ b/wicket-core/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java @@ -127,6 +127,97 @@ class FileUploadToResourceFieldSecurityTest extends WicketTestCase assertTrue(tester.getLastResponseAsString().contains("uploadFailed")); } + @Test + void tokenDoesNotDependOnCryptFactory() throws Exception + { + FolderUploadsFileManager fileManager = newFileManager(); + UploadPage page = new UploadPage(new TestFileUploadResourceReference(fileManager)); + tester.startPage(page); + String url = page.field.buildUploadUrl(); + + SecretKey key = KeyGenerator.getInstance("AES").generateKey(); + ISecureRandomSupplier randomSupplier = tester.getApplication().getSecuritySettings() + .getRandomSupplier(); + tester.getApplication().getSecuritySettings() + .setCryptFactory(() -> new AESCrypt(key, randomSupplier)); + + File upload = createUploadFile("allowed.txt", 256); + submitUpload(url, upload); + + File storedFile = fileManager.getFile(page.field.getMarkupId(), upload.getName()); + assertTrue(storedFile.exists()); + } + + @Test + void rejectsUploadWithoutToken() throws Exception + { + FolderUploadsFileManager fileManager = newFileManager(); + UploadPage page = new UploadPage(new TestFileUploadResourceReference(fileManager)); + tester.startPage(page); + + File upload = createUploadFile("allowed.txt", 256); + submitUpload(page.field.buildUploadUrl(null), upload); + + File storedFile = fileManager.getFile(page.field.getMarkupId(), upload.getName()); + assertFalse(storedFile.exists()); + assertTrue(tester.getLastResponseAsString().contains("uploadFailed")); + } + + @Test + void rejectsTamperedToken() throws Exception + { + FolderUploadsFileManager fileManager = newFileManager(); + UploadPage page = new UploadPage(new TestFileUploadResourceReference(fileManager)); + tester.startPage(page); + + String validToken = page.field.createUploadToken(); + String tamperedToken = (validToken.charAt(0) == 'A' ? 'B' : 'A') + validToken.substring(1); + + File upload = createUploadFile("allowed.txt", 256); + submitUpload(page.field.buildUploadUrl(tamperedToken), upload); + + File storedFile = fileManager.getFile(page.field.getMarkupId(), upload.getName()); + assertFalse(storedFile.exists()); + assertTrue(tester.getLastResponseAsString().contains("uploadFailed")); + } + + @Test + void rejectsTokenWithoutSessionKey() throws Exception + { + FolderUploadsFileManager fileManager = newFileManager(); + UploadPage page = new UploadPage(new TestFileUploadResourceReference(fileManager)); + tester.startPage(page); + String url = page.field.buildUploadUrl(); + + tester.getSession().invalidateNow(); + + File upload = createUploadFile("allowed.txt", 256); + submitUpload(url, upload); + + File storedFile = fileManager.getFile(page.field.getMarkupId(), upload.getName()); + assertFalse(storedFile.exists()); + assertTrue(tester.getLastResponseAsString().contains("uploadFailed")); + } + + @Test + void rejectsTokenFromAnotherSession() throws Exception + { + FolderUploadsFileManager fileManager = newFileManager(); + UploadPage page = new UploadPage(new TestFileUploadResourceReference(fileManager)); + tester.startPage(page); + String url = page.field.buildUploadUrl(); + + tester.getSession().invalidateNow(); + tester.startPage(new UploadPage(new TestFileUploadResourceReference(fileManager))); + + File upload = createUploadFile("allowed.txt", 256); + submitUpload(url, upload); + + File storedFile = fileManager.getFile(page.field.getMarkupId(), upload.getName()); + assertFalse(storedFile.exists()); + assertTrue(tester.getLastResponseAsString().contains("uploadFailed")); + } + private FolderUploadsFileManager newFileManager() throws IOException { tempDir = Files.createTempDirectory("wicket-upload-security"); @@ -185,6 +276,11 @@ class FileUploadToResourceFieldSecurityTest extends WicketTestCase } String buildUploadUrl() + { + return buildUploadUrl(createUploadToken()); + } + + String buildUploadUrl(String token) { StringBuilder url = new StringBuilder(getResourceUrl()); url.append('?').append(AbstractFileUploadResource.UPLOAD_ID).append('=') @@ -193,12 +289,20 @@ class FileUploadToResourceFieldSecurityTest extends WicketTestCase .append(getMaxSize().bytes()); url.append('&').append(AbstractFileUploadResource.FILE_COUNT_MAX).append('=') .append(getFileCountMax()); - url.append('&').append(AbstractFileUploadResource.UPLOAD_TOKEN).append('=') - .append(AbstractFileUploadResource.createUploadToken(getMarkupId(), getMaxSize(), - getFileMaxSize(), getFileCountMax())); + if (token != null) + { + url.append('&').append(AbstractFileUploadResource.UPLOAD_TOKEN).append('=') + .append(token); + } return url.toString(); } + String createUploadToken() + { + return AbstractFileUploadResource.createUploadToken(getMarkupId(), getMaxSize(), + getFileMaxSize(), getFileCountMax()); + } + String getResourceUrl() { return urlFor(resourceReference, new PageParameters()).toString();
