diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 76a97425..fd860f42 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -2,3 +2,8 @@ **Vulnerability:** Untrusted paths from API responses were directly assigned to `a.href` and used in `iframe` generation, which allows execution of malicious URIs like `javascript:` or `data:`. **Learning:** Even when avoiding `innerHTML`, directly setting URL-like strings to DOM attributes without protocol validation introduces XSS vectors. The payload can be executed when the link is clicked or the iframe is loaded. **Prevention:** Implement an `isSafeUrl` verification function to ensure the protocol is strictly `http:` or `https:` (using `new URL()`) before assigning untrusted inputs to DOM attributes like `href` or `src`. + +## 2026-07-02 - Cryptographic Signature Verification Bypass in Policy Override +**Vulnerability:** The document validation service logged the presence of policy override parameters (approverId, approvalToken) but failed to actually verify the cryptographic signature of the token against a shared secret. This allowed an attacker to bypass file extension restrictions (e.g., uploading blocked `.hwp` files) by sending any arbitrary token. +**Learning:** Checking for the presence of security tokens is insufficient if the token payload and signature are not cryptographically validated against a trusted secret. The absence of this check created a critical authorization bypass. +**Prevention:** Always verify cryptographic signatures (using constant-time comparison like `MessageDigest.isEqual`) for any policy override or authorization token before granting the elevated privilege or bypassing a security control. diff --git a/src/main/java/com/clearfolio/viewer/config/ConversionProperties.java b/src/main/java/com/clearfolio/viewer/config/ConversionProperties.java index d8ad3d8d..220f380d 100644 --- a/src/main/java/com/clearfolio/viewer/config/ConversionProperties.java +++ b/src/main/java/com/clearfolio/viewer/config/ConversionProperties.java @@ -20,6 +20,7 @@ public class ConversionProperties { private long retryMaxDelayMs = 5_000L; private double retryBackoffMultiplier = 2.0; private long maxUploadSizeBytes = 5 * 1024 * 1024L; + private String policyOverrideSecret = ""; /** * Returns file extensions that are blocked from upload. @@ -179,4 +180,22 @@ public long getMaxUploadSizeBytes() { public void setMaxUploadSizeBytes(long maxUploadSizeBytes) { this.maxUploadSizeBytes = Math.max(1L, maxUploadSizeBytes); } + + /** + * Returns the policy override secret used to verify blocked extension uploads. + * + * @return policy override secret + */ + public String getPolicyOverrideSecret() { + return policyOverrideSecret; + } + + /** + * Sets the policy override secret used to verify blocked extension uploads. + * + * @param policyOverrideSecret policy override secret + */ + public void setPolicyOverrideSecret(String policyOverrideSecret) { + this.policyOverrideSecret = policyOverrideSecret == null ? "" : policyOverrideSecret; + } } diff --git a/src/main/java/com/clearfolio/viewer/service/DefaultDocumentValidationService.java b/src/main/java/com/clearfolio/viewer/service/DefaultDocumentValidationService.java index b898f7ac..7bbda524 100644 --- a/src/main/java/com/clearfolio/viewer/service/DefaultDocumentValidationService.java +++ b/src/main/java/com/clearfolio/viewer/service/DefaultDocumentValidationService.java @@ -2,11 +2,15 @@ import java.nio.charset.StandardCharsets; import java.security.MessageDigest; +import java.security.InvalidKeyException; import java.security.NoSuchAlgorithmException; import java.util.HexFormat; import java.util.Locale; import java.util.Set; +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; @@ -26,6 +30,7 @@ public class DefaultDocumentValidationService implements DocumentValidationServi private final Set blockedExtensions; private final long maxUploadSizeBytes; + private final String policyOverrideSecret; /** * Creates the validation service from conversion configuration values. @@ -35,6 +40,7 @@ public class DefaultDocumentValidationService implements DocumentValidationServi public DefaultDocumentValidationService(ConversionProperties conversionProperties) { this.blockedExtensions = conversionProperties.getBlockedExtensions(); this.maxUploadSizeBytes = conversionProperties.getMaxUploadSizeBytes(); + this.policyOverrideSecret = conversionProperties.getPolicyOverrideSecret(); } /** @@ -79,6 +85,23 @@ public void validateOrThrow(MultipartFile file, PolicyOverrideRequest overrideRe effectiveOverride.approverId(), PolicyOverrideRequest.APPROVER_ID_HEADER + " is required when policy override is true." ); + + if (policyOverrideSecret == null || policyOverrideSecret.isBlank()) { + throw new IllegalStateException("Policy override secret is not configured."); + } + + byte[] providedBytes; + try { + providedBytes = HexFormat.of().parseHex(approvalToken); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Invalid policy override signature.", e); + } + + byte[] expectedBytes = computeSignature(approverId, extension, policyOverrideSecret); + if (!MessageDigest.isEqual(providedBytes, expectedBytes)) { + throw new IllegalArgumentException("Invalid policy override signature."); + } + overrideApproverIdForAudit = approverId; overrideTokenForAudit = approvalToken; } @@ -136,6 +159,17 @@ private String requireNonBlank(String value, String message) { return value.trim(); } + private byte[] computeSignature(String approverId, String extension, String secret) { + try { + Mac mac = Mac.getInstance("HmacSHA256"); + mac.init(new SecretKeySpec(secret.getBytes(StandardCharsets.UTF_8), "HmacSHA256")); + String payload = approverId + ":" + extension; + return mac.doFinal(payload.getBytes(StandardCharsets.UTF_8)); + } catch (NoSuchAlgorithmException | InvalidKeyException ex) { + throw new IllegalStateException("HmacSHA256 unavailable or key invalid", ex); + } + } + private String tokenFingerprint(String approvalToken) { try { MessageDigest digest = MessageDigest.getInstance("SHA-256"); diff --git a/src/main/resources/application.yml b/src/main/resources/application.yml index b98dcbe2..f01287f0 100644 --- a/src/main/resources/application.yml +++ b/src/main/resources/application.yml @@ -9,6 +9,7 @@ conversion: - hwp - hwpx worker-threads: 4 + policy-override-secret: "${CONVERSION_POLICY_OVERRIDE_SECRET:}" queue-capacity: 200 max-retry-attempts: 3 retry-initial-delay-ms: 500 diff --git a/src/test/java/com/clearfolio/viewer/controller/ConversionControllerMultipartLimitTest.java b/src/test/java/com/clearfolio/viewer/controller/ConversionControllerMultipartLimitTest.java index 931cb526..c759e740 100644 --- a/src/test/java/com/clearfolio/viewer/controller/ConversionControllerMultipartLimitTest.java +++ b/src/test/java/com/clearfolio/viewer/controller/ConversionControllerMultipartLimitTest.java @@ -14,6 +14,11 @@ import org.springframework.http.MediaType; import org.springframework.http.HttpHeaders; import org.springframework.http.client.MultipartBodyBuilder; +import java.nio.charset.StandardCharsets; +import java.util.HexFormat; +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; + import org.springframework.test.context.TestPropertySource; import org.springframework.test.web.reactive.server.WebTestClient; import org.springframework.web.reactive.function.BodyInserters; @@ -135,9 +140,22 @@ void submitReturnsUnsupportedFormatForBlockedExtensionWithServiceValidation() { .jsonPath("$.traceId").value(ConversionControllerMultipartLimitTest::assertNonBlankTraceId); } + private String generateSignature(String approverId, String extension, String secret) { + try { + Mac mac = Mac.getInstance("HmacSHA256"); + mac.init(new SecretKeySpec(secret.getBytes(StandardCharsets.UTF_8), "HmacSHA256")); + String payload = approverId + ":" + extension; + byte[] hashed = mac.doFinal(payload.getBytes(StandardCharsets.UTF_8)); + return HexFormat.of().formatHex(hashed); + } catch (Exception ex) { + throw new RuntimeException(ex); + } + } + @Test void submitAcceptsBlockedExtensionWhenPolicyOverrideHeadersAreValid() { - submit("contract.hwp", "hello".getBytes(), "true", "token-xyz", "approver-99") + String validSignature = generateSignature("approver-99", "hwp", "test-secret"); + submit("contract.hwp", "hello".getBytes(), "true", validSignature, "approver-99") .expectStatus().isAccepted() .expectBody() .jsonPath("$.status").isEqualTo("ACCEPTED") diff --git a/src/test/java/com/clearfolio/viewer/service/DefaultDocumentValidationServiceTest.java b/src/test/java/com/clearfolio/viewer/service/DefaultDocumentValidationServiceTest.java index dc1df793..4ecb694e 100644 --- a/src/test/java/com/clearfolio/viewer/service/DefaultDocumentValidationServiceTest.java +++ b/src/test/java/com/clearfolio/viewer/service/DefaultDocumentValidationServiceTest.java @@ -7,10 +7,15 @@ 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.HexFormat; import java.util.Set; +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; + import org.junit.jupiter.api.Test; import org.springframework.mock.web.MockMultipartFile; import org.springframework.web.multipart.MultipartFile; @@ -38,18 +43,90 @@ void rejectsHwpAndHwpxByDefault() { assertEquals("hwp", ex.getExtension()); } + private String generateSignature(String approverId, String extension, String secret) { + try { + Mac mac = Mac.getInstance("HmacSHA256"); + mac.init(new SecretKeySpec(secret.getBytes(StandardCharsets.UTF_8), "HmacSHA256")); + String payload = approverId + ":" + extension; + byte[] hashed = mac.doFinal(payload.getBytes(StandardCharsets.UTF_8)); + return HexFormat.of().formatHex(hashed); + } catch (Exception ex) { + throw new RuntimeException(ex); + } + } + @Test void allowsBlockedExtensionWhenOverrideHeadersAreValid() { ConversionProperties conversionProperties = new ConversionProperties(); conversionProperties.setBlockedExtensions(Set.of("hwp", "hwpx")); + conversionProperties.setPolicyOverrideSecret("test-secret"); DefaultDocumentValidationService validationService = new DefaultDocumentValidationService(conversionProperties); + String validSignature = generateSignature("approver-1", "hwp", "test-secret"); assertDoesNotThrow(() -> validationService.validateOrThrow( new MockMultipartFile("file", "contract.hwp", "application/octet-stream", new byte[] {1}), - PolicyOverrideRequest.of("true", "token-123", "approver-1") + PolicyOverrideRequest.of("true", validSignature, "approver-1") )); } + @Test + void rejectsBlockedExtensionWhenOverrideSignatureIsInvalid() { + ConversionProperties conversionProperties = new ConversionProperties(); + conversionProperties.setBlockedExtensions(Set.of("hwp", "hwpx")); + conversionProperties.setPolicyOverrideSecret("test-secret"); + DefaultDocumentValidationService validationService = new DefaultDocumentValidationService(conversionProperties); + + IllegalArgumentException ex = assertThrows( + IllegalArgumentException.class, + () -> validationService.validateOrThrow( + new MockMultipartFile("file", "contract.hwp", "application/octet-stream", new byte[] {1}), + PolicyOverrideRequest.of("true", "invalid-token", "approver-1") + ) + ); + + assertEquals("Invalid policy override signature.", ex.getMessage()); + } + + @Test + void rejectsBlockedExtensionWhenSignatureIsWellFormedButDoesNotMatch() { + ConversionProperties conversionProperties = new ConversionProperties(); + conversionProperties.setBlockedExtensions(Set.of("hwp", "hwpx")); + conversionProperties.setPolicyOverrideSecret("test-secret"); + DefaultDocumentValidationService validationService = new DefaultDocumentValidationService(conversionProperties); + + // Valid hex of the correct length, but computed with the wrong secret, so the + // constant-time MessageDigest.isEqual comparison must reject it. + String wrongSignature = generateSignature("approver-1", "hwp", "attacker-secret"); + + IllegalArgumentException ex = assertThrows( + IllegalArgumentException.class, + () -> validationService.validateOrThrow( + new MockMultipartFile("file", "contract.hwp", "application/octet-stream", new byte[] {1}), + PolicyOverrideRequest.of("true", wrongSignature, "approver-1") + ) + ); + + assertEquals("Invalid policy override signature.", ex.getMessage()); + } + + @Test + void rejectsBlockedExtensionWhenSecretIsNotConfigured() { + ConversionProperties conversionProperties = new ConversionProperties(); + conversionProperties.setBlockedExtensions(Set.of("hwp", "hwpx")); + conversionProperties.setPolicyOverrideSecret(""); + DefaultDocumentValidationService validationService = new DefaultDocumentValidationService(conversionProperties); + + IllegalStateException ex = assertThrows( + IllegalStateException.class, + () -> validationService.validateOrThrow( + new MockMultipartFile("file", "contract.hwp", "application/octet-stream", new byte[] {1}), + PolicyOverrideRequest.of("true", "any-token", "approver-1") + ) + ); + + assertEquals("Policy override secret is not configured.", ex.getMessage()); + } + @Test void rejectsBlockedExtensionWhenOverrideFlagIsInvalid() { ConversionProperties conversionProperties = new ConversionProperties(); @@ -376,8 +453,12 @@ void sanitizeForLogReplacesTabCharacter() throws Exception { void throwsWhenSha256DigestIsUnavailableForOverrideAuditFingerprint() { ConversionProperties conversionProperties = new ConversionProperties(); conversionProperties.setBlockedExtensions(Set.of("hwp", "hwpx")); + conversionProperties.setPolicyOverrideSecret("test-secret"); DefaultDocumentValidationService validationService = new DefaultDocumentValidationService(conversionProperties); + // Generate the signature BEFORE removing security providers + String validSignature = generateSignature("approver-1", "hwp", "test-secret"); + synchronized (SECURITY_PROVIDERS_LOCK) { Provider[] providers = Security.getProviders(); for (Provider provider : providers) { @@ -389,11 +470,11 @@ void throwsWhenSha256DigestIsUnavailableForOverrideAuditFingerprint() { IllegalStateException.class, () -> validationService.validateOrThrow( new MockMultipartFile("file", "contract.hwp", "application/octet-stream", new byte[] {1}), - PolicyOverrideRequest.of("true", "token-123", "approver-1") + PolicyOverrideRequest.of("true", validSignature, "approver-1") ) ); - assertEquals("SHA-256 digest unavailable", ex.getMessage()); + assertEquals("HmacSHA256 unavailable or key invalid", ex.getMessage()); } finally { for (int index = 0; index < providers.length; index++) { Security.insertProviderAt(providers[index], index + 1); diff --git a/src/test/resources/application.yml b/src/test/resources/application.yml new file mode 100644 index 00000000..04e1f76e --- /dev/null +++ b/src/test/resources/application.yml @@ -0,0 +1,2 @@ +conversion: + policy-override-secret: "test-secret"