This is an automated email from the ASF dual-hosted git repository. dashorst pushed a commit to branch wicket-10.x in repository https://gitbox.apache.org/repos/asf/wicket.git
commit f9b1c0dfc6df4106f6c4de92cbd5379fd98d3ef8 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]> --- .../FileUploadToResourceFieldSecurityTest.java | 138 ++++++++++++++++++++- .../resource/AbstractFileUploadResource.java | 63 +++++++++- 2 files changed, 194 insertions(+), 7 deletions(-) diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java index bb11f70067..c643496685 100644 --- a/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java +++ b/wicket-core-tests/src/test/java/org/apache/wicket/markup/html/form/upload/resource/FileUploadToResourceFieldSecurityTest.java @@ -19,6 +19,7 @@ package org.apache.wicket.markup.html.form.upload.resource; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -29,11 +30,16 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.Comparator; import java.util.List; +import javax.crypto.KeyGenerator; +import javax.crypto.SecretKey; import org.apache.wicket.ajax.AbstractDefaultAjaxBehavior; import org.apache.wicket.ajax.AjaxRequestTarget; +import org.apache.wicket.core.random.ISecureRandomSupplier; +import org.apache.wicket.core.util.crypt.AESCrypt; import org.apache.wicket.markup.html.form.upload.resource.FileUploadToResourceField.UploadInfo; import org.apache.wicket.markup.html.WebPage; import org.apache.wicket.request.mapper.parameter.PageParameters; +import org.apache.wicket.util.crypt.ICrypt; import org.apache.wicket.util.lang.Bytes; import org.apache.wicket.util.file.File; import org.apache.wicket.util.tester.WicketTestCase; @@ -106,6 +112,119 @@ class FileUploadToResourceFieldSecurityTest extends WicketTestCase assertTrue(tester.getLastResponseAsString().contains("uploadFailed")); } + @Test + void acceptsUploadWhenEncryptionIsNotDeterministic() throws Exception + { + SecretKey key = KeyGenerator.getInstance("AES").generateKey(); + ISecureRandomSupplier randomSupplier = tester.getApplication().getSecuritySettings() + .getRandomSupplier(); + tester.getApplication().getSecuritySettings() + .setCryptFactory(() -> new AESCrypt(key, randomSupplier)); + ICrypt crypt = tester.getApplication().getSecuritySettings().getCryptFactory().newCrypt(); + assertNotEquals(crypt.encryptUrlSafe("settings"), crypt.encryptUrlSafe("settings")); + + FolderUploadsFileManager fileManager = newFileManager(); + UploadPage page = new UploadPage(new TestFileUploadResourceReference(fileManager)); + tester.startPage(page); + + File upload = createUploadFile("allowed.txt", 256); + submitUpload(page.field.buildUploadUrl(), upload); + + File storedFile = fileManager.getFile(page.field.getMarkupId(), upload.getName()); + assertTrue(storedFile.exists()); + } + + @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")); + } + @Test void callbackWithoutStoredFileReportsFailure() throws Exception { @@ -301,6 +420,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('=') @@ -309,12 +433,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(); 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 dea29a3494..3bb97ea7eb 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 org.apache.commons.fileupload2.core.FileItem; import org.apache.commons.fileupload2.core.FileUploadException; import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException; import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException; import org.apache.commons.fileupload2.core.FileUploadSizeException; 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; @@ -80,6 +87,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) @@ -90,9 +107,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)); } /** @@ -218,6 +247,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) { @@ -227,7 +274,15 @@ public abstract class AbstractFileUploadResource extends AbstractResource return false; } - String expectedToken = createUploadToken(uploadId, maxSize, fileMaxSize, fileCountMax); + // 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 expectedToken = sign(key, + serializeUploadSettings(uploadId, maxSize, fileMaxSize, fileCountMax)); return MessageDigest.isEqual(expectedToken.getBytes(StandardCharsets.UTF_8), actualToken.getBytes(StandardCharsets.UTF_8)); }
