diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 0000000..8dd7600 --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,4 @@ +## 2026-06-28 - [Null Byte Injection Bypass in File Extension Validation] +**Vulnerability:** A file named `malicious.hwp\0.pdf` bypasses the `blockedExtensions` check (which verifies against `.pdf`) in `DefaultDocumentValidationService`, but could be subsequently processed as `.hwp` by systems or libraries written in C/C++ that terminate strings at the null byte. +**Learning:** Checking the extension after the last dot does not secure the system if the file name contains a null byte, which is ignored in Java but terminates strings in C/C++. +**Prevention:** Explicitly validate all uploaded file names against null bytes before performing extension extraction or other validation checks. diff --git a/src/main/java/com/clearfolio/viewer/service/DefaultDocumentValidationService.java b/src/main/java/com/clearfolio/viewer/service/DefaultDocumentValidationService.java index b898f7a..710577b 100644 --- a/src/main/java/com/clearfolio/viewer/service/DefaultDocumentValidationService.java +++ b/src/main/java/com/clearfolio/viewer/service/DefaultDocumentValidationService.java @@ -1,5 +1,7 @@ package com.clearfolio.viewer.service; +import java.io.IOException; +import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; @@ -16,13 +18,25 @@ import com.clearfolio.viewer.exception.UnsupportedDocumentFormatException; /** - * Default document validator that enforces extension and size constraints. + * Default document validator that enforces extension, size, and content signature constraints. */ @Service public class DefaultDocumentValidationService implements DocumentValidationService { private static final Logger LOGGER = LoggerFactory.getLogger(DefaultDocumentValidationService.class); private static final int FINGERPRINT_TRUNCATE_BYTES = 8; + private static final int SIGNATURE_BYTES = 8; + private static final byte[] PDF_SIGNATURE = "%PDF-".getBytes(StandardCharsets.US_ASCII); + private static final byte[] ZIP_LOCAL_FILE_SIGNATURE = new byte[] {0x50, 0x4B, 0x03, 0x04}; + private static final byte[] ZIP_EMPTY_ARCHIVE_SIGNATURE = new byte[] {0x50, 0x4B, 0x05, 0x06}; + private static final byte[] ZIP_SPANNED_ARCHIVE_SIGNATURE = new byte[] {0x50, 0x4B, 0x07, 0x08}; + private static final byte[] OLE_COMPOUND_FILE_SIGNATURE = new byte[] { + (byte) 0xD0, (byte) 0xCF, 0x11, (byte) 0xE0, (byte) 0xA1, (byte) 0xB1, 0x1A, (byte) 0xE1 + }; + private static final byte[] RTF_SIGNATURE = "{\\rtf".getBytes(StandardCharsets.US_ASCII); + private static final Set ZIP_DOCUMENT_EXTENSIONS = Set.of("docx", "pptx", "xlsx", "odt", "odp", "ods"); + private static final Set OLE_DOCUMENT_EXTENSIONS = Set.of("doc", "ppt", "xls"); + private static final Set TEXT_DOCUMENT_EXTENSIONS = Set.of("csv", "md", "txt"); private final Set blockedExtensions; private final long maxUploadSizeBytes; @@ -55,6 +69,10 @@ public void validateOrThrow(MultipartFile file, PolicyOverrideRequest overrideRe } String fileName = file.getOriginalFilename(); + if (fileName != null && fileName.indexOf('\0') != -1) { + throw new IllegalArgumentException("File name is invalid."); + } + String extension = extensionOf(fileName); if (extension.isEmpty()) { throw new IllegalArgumentException("File extension is required."); @@ -87,6 +105,10 @@ public void validateOrThrow(MultipartFile file, PolicyOverrideRequest overrideRe throw new IllegalArgumentException("File is too large."); } + if (!blockedExtension) { + validateContentSignature(file, extension); + } + if (blockedExtension) { LOGGER.info( "Blocked-format override accepted extension={} approverId={} tokenFingerprint={}", @@ -97,6 +119,70 @@ public void validateOrThrow(MultipartFile file, PolicyOverrideRequest overrideRe } } + private void validateContentSignature(MultipartFile file, String extension) { + byte[] header = readHeader(file); + boolean valid = switch (extension) { + case "pdf" -> startsWith(header, PDF_SIGNATURE); + case "rtf" -> startsWith(header, RTF_SIGNATURE); + default -> { + if (ZIP_DOCUMENT_EXTENSIONS.contains(extension)) { + yield startsWith(header, ZIP_LOCAL_FILE_SIGNATURE) + || startsWith(header, ZIP_EMPTY_ARCHIVE_SIGNATURE) + || startsWith(header, ZIP_SPANNED_ARCHIVE_SIGNATURE); + } + if (OLE_DOCUMENT_EXTENSIONS.contains(extension)) { + yield startsWith(header, OLE_COMPOUND_FILE_SIGNATURE); + } + if (TEXT_DOCUMENT_EXTENSIONS.contains(extension)) { + yield isTextLike(header); + } + throw new IllegalArgumentException("File extension is not supported."); + } + }; + + if (!valid) { + throw new IllegalArgumentException("File content does not match file extension."); + } + } + + private byte[] readHeader(MultipartFile file) { + byte[] header = new byte[SIGNATURE_BYTES]; + try (InputStream inputStream = file.getInputStream()) { + int bytesRead = inputStream.readNBytes(header, 0, header.length); + if (bytesRead == header.length) { + return header; + } + + byte[] truncated = new byte[bytesRead]; + System.arraycopy(header, 0, truncated, 0, bytesRead); + return truncated; + } catch (IOException ex) { + throw new IllegalArgumentException("File content could not be read.", ex); + } + } + + private boolean startsWith(byte[] actual, byte[] expected) { + if (actual.length < expected.length) { + return false; + } + for (int index = 0; index < expected.length; index++) { + if (actual[index] != expected[index]) { + return false; + } + } + return true; + } + + private boolean isTextLike(byte[] header) { + for (byte value : header) { + int unsigned = Byte.toUnsignedInt(value); + if (unsigned == 0 || unsigned < 0x09 || (unsigned > 0x0D && unsigned < 0x20)) { + return false; + } + } + return true; + } + private String extensionOf(String fileName) { if (fileName == null || fileName.isBlank()) { return ""; diff --git a/src/test/java/com/clearfolio/viewer/controller/ConversionControllerMultipartLimitTest.java b/src/test/java/com/clearfolio/viewer/controller/ConversionControllerMultipartLimitTest.java index 46687e1..2a58629 100644 --- a/src/test/java/com/clearfolio/viewer/controller/ConversionControllerMultipartLimitTest.java +++ b/src/test/java/com/clearfolio/viewer/controller/ConversionControllerMultipartLimitTest.java @@ -122,7 +122,7 @@ void submitReturnsBadRequestWhenServiceUploadLimitIsExceeded() { @Test void submitAcceptsDocumentAtServiceUploadLimitBoundary() { - byte[] payload = new byte[1024]; + byte[] payload = docxBytes(1024); submit("report.docx", payload) .expectStatus().isAccepted() @@ -173,4 +173,13 @@ private static void assertNonBlankTraceId(Object value) { String traceId = (String) value; assertFalse(traceId.isBlank()); } + + private static byte[] docxBytes(int size) { + byte[] bytes = new byte[size]; + bytes[0] = 0x50; + bytes[1] = 0x4B; + bytes[2] = 0x03; + bytes[3] = 0x04; + return bytes; + } } diff --git a/src/test/java/com/clearfolio/viewer/service/DefaultDocumentConversionServiceTest.java b/src/test/java/com/clearfolio/viewer/service/DefaultDocumentConversionServiceTest.java index 7d07d6b..a1faba8 100644 --- a/src/test/java/com/clearfolio/viewer/service/DefaultDocumentConversionServiceTest.java +++ b/src/test/java/com/clearfolio/viewer/service/DefaultDocumentConversionServiceTest.java @@ -108,7 +108,7 @@ public void validateOrThrow(MultipartFile file, PolicyOverrideRequest overrideRe "file", "contract.docx", "application/octet-stream", - "hello-viewer".getBytes() + docxBytes("hello-viewer") ); UUID jobId = service.submit(file, null); @@ -133,7 +133,7 @@ void returnsSameJobIdForDuplicatePayloads() { "file", "contract.docx", "application/octet-stream", - "hello-viewer".getBytes() + docxBytes("hello-viewer") ); UUID first = service.submit(file); @@ -158,13 +158,13 @@ void returnsDifferentJobIdsForDifferentPayloads() { "file", "contract.docx", "application/octet-stream", - "hello-viewer".getBytes() + docxBytes("hello-viewer") ); MockMultipartFile secondFile = new MockMultipartFile( "file", "contract.docx", "application/octet-stream", - "different-content".getBytes() + docxBytes("different-content") ); UUID first = service.submit(firstFile); @@ -189,7 +189,7 @@ void returnsSameJobForConcurrentDuplicatePayloads() throws Exception { "file", "contract.docx", "application/octet-stream", - "hello-viewer-concurrent".getBytes() + docxBytes("hello-viewer-concurrent") ); ExecutorService executor = Executors.newFixedThreadPool(8); @@ -552,6 +552,17 @@ UUID lastEnqueuedJobId() { } } + private static byte[] docxBytes(String payload) { + byte[] suffix = payload.getBytes(StandardCharsets.UTF_8); + byte[] bytes = new byte[suffix.length + 4]; + bytes[0] = 0x50; + bytes[1] = 0x4B; + bytes[2] = 0x03; + bytes[3] = 0x04; + System.arraycopy(suffix, 0, bytes, 4, suffix.length); + return bytes; + } + private static class FindOrStoreOnlyRepository implements ConversionJobRepository { private final java.util.function.Function finder; diff --git a/src/test/java/com/clearfolio/viewer/service/DefaultDocumentValidationServiceTest.java b/src/test/java/com/clearfolio/viewer/service/DefaultDocumentValidationServiceTest.java index dc1df79..d73e479 100644 --- a/src/test/java/com/clearfolio/viewer/service/DefaultDocumentValidationServiceTest.java +++ b/src/test/java/com/clearfolio/viewer/service/DefaultDocumentValidationServiceTest.java @@ -7,6 +7,7 @@ import static org.mockito.Mockito.when; import java.lang.reflect.Method; +import java.nio.charset.StandardCharsets; import java.security.Provider; import java.security.Security; import java.util.Set; @@ -159,7 +160,7 @@ void ignoresInvalidOverrideFlagForSupportedExtension() { DefaultDocumentValidationService validationService = new DefaultDocumentValidationService(conversionProperties); assertDoesNotThrow(() -> validationService.validateOrThrow( - new MockMultipartFile("file", "contract.docx", "application/octet-stream", new byte[] {1}), + new MockMultipartFile("file", "contract.docx", "application/octet-stream", docxBytes("override")), PolicyOverrideRequest.of("invalid", null, null) )); } @@ -171,7 +172,70 @@ void allowsSupportedExtensions() { DefaultDocumentValidationService validationService = new DefaultDocumentValidationService(conversionProperties); assertDoesNotThrow(() -> validationService.validateOrThrow( - new MockMultipartFile("file", "contract.docx", "application/vnd.openxmlformats-officedocument.wordprocessingml.document", new byte[] {1}) + new MockMultipartFile( + "file", + "contract.docx", + "application/vnd.openxmlformats-officedocument.wordprocessingml.document", + docxBytes("contract") + ) + )); + } + + @Test + void rejectsSupportedExtensionWhenContentSignatureDoesNotMatch() { + ConversionProperties conversionProperties = new ConversionProperties(); + conversionProperties.setBlockedExtensions(Set.of("hwp", "hwpx")); + DefaultDocumentValidationService validationService = new DefaultDocumentValidationService(conversionProperties); + + IllegalArgumentException ex = assertThrows( + IllegalArgumentException.class, + () -> validationService.validateOrThrow( + new MockMultipartFile( + "file", + "malicious.php.docx", + "application/vnd.openxmlformats-officedocument.wordprocessingml.document", + "".getBytes(StandardCharsets.UTF_8) + ) + ) + ); + + assertEquals("File content does not match file extension.", ex.getMessage()); + } + + @Test + void rejectsUnsupportedDoubleExtension() { + ConversionProperties conversionProperties = new ConversionProperties(); + conversionProperties.setBlockedExtensions(Set.of("hwp", "hwpx")); + DefaultDocumentValidationService validationService = new DefaultDocumentValidationService(conversionProperties); + + IllegalArgumentException ex = assertThrows( + IllegalArgumentException.class, + () -> validationService.validateOrThrow( + new MockMultipartFile( + "file", + "malicious.php.jpg", + "image/jpeg", + " validationService.validateOrThrow( + new MockMultipartFile( + "file", + "report.pdf", + "application/pdf", + "%PDF-1.7\n".getBytes(StandardCharsets.UTF_8) + ) )); } @@ -191,6 +255,22 @@ void rejectsMissingExtension() { assertEquals("File extension is required.", ex.getMessage()); } + @Test + void rejectsFilenameWithNullByte() { + ConversionProperties conversionProperties = new ConversionProperties(); + conversionProperties.setBlockedExtensions(Set.of("hwp", "hwpx")); + DefaultDocumentValidationService validationService = new DefaultDocumentValidationService(conversionProperties); + + IllegalArgumentException ex = assertThrows( + IllegalArgumentException.class, + () -> validationService.validateOrThrow( + new MockMultipartFile("file", "contract.hwp\0.pdf", "application/octet-stream", new byte[] {1}) + ) + ); + + assertEquals("File name is invalid.", ex.getMessage()); + } + @Test void rejectsBlankFilenameOrMissingName() { ConversionProperties conversionProperties = new ConversionProperties(); @@ -401,4 +481,15 @@ void throwsWhenSha256DigestIsUnavailableForOverrideAuditFingerprint() { } } } + + private static byte[] docxBytes(String payload) { + byte[] suffix = payload.getBytes(StandardCharsets.UTF_8); + byte[] bytes = new byte[suffix.length + 4]; + bytes[0] = 0x50; + bytes[1] = 0x4B; + bytes[2] = 0x03; + bytes[3] = 0x04; + System.arraycopy(suffix, 0, bytes, 4, suffix.length); + return bytes; + } }