diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java index 338710d..15e7b65 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java @@ -12,6 +12,7 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.env.Environment; +import org.springframework.http.HttpMethod; import org.springframework.security.config.ObjectPostProcessor; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.core.session.SessionRegistry; @@ -168,6 +169,18 @@ public SecurityFilterChain buildSecurityFilterChain(HttpSecurity http, SessionRe setupMfa(http); } + // Close Spring Security's built-in WebAuthn credential-delete endpoint (DELETE /webauthn/register/{id}), + // registered by http.webAuthn(...). It deletes a passkey after checking only credential ownership, bypassing + // the framework's own DELETE /user/webauthn/credentials/{id} safeguards (last-credential lockout protection, + // current-password re-authentication, and audit logging). See GHSA-3cv9-vgqh-jwpm. This deny rule must be + // registered before the anyRequest() rule below (Spring rejects matchers added after anyRequest()) and applies + // regardless of defaultAction, since allow mode's anyRequest().permitAll() would otherwise expose it too. + // Consumers relying on this endpoint should migrate to DELETE /user/webauthn/credentials/{id}. + if (webAuthnConfigProperties.isEnabled()) { + http.authorizeHttpRequests( + (authorize) -> authorize.requestMatchers(HttpMethod.DELETE, "/webauthn/register/**").denyAll()); + } + // Configure authorization rules based on the default action String defaultAction = userSecurityConfig.getDefaultAction(); if (DEFAULT_ACTION_DENY.equals(defaultAction)) { diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java index 99ea55c..a03b07f 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java @@ -181,4 +181,16 @@ void shouldRejectRenameWhenCurrentPasswordIncorrectForPasswordAccount() throws E .isPresent() .hasValueSatisfying(c -> assertThat(c.getLabel()).isEqualTo("My Device")); } + + @Test + @DisplayName("should deny Spring Security's built-in DELETE /webauthn/register/{id} endpoint (GHSA-3cv9-vgqh-jwpm)") + void shouldDenySpringWebAuthnRegistrationDeleteEndpoint() throws Exception { + // http.webAuthn(...) registers WebAuthnRegistrationFilter, whose DELETE /webauthn/register/{id} deletes a + // passkey after checking only credential ownership, bypassing the framework's lockout protection, + // current-password re-authentication, and audit logging. The filter chain must deny it outright. + mockMvc.perform(delete("/webauthn/register/cred-1").with(user(TEST_EMAIL).roles("USER")).with(csrf())) + .andExpect(status().isForbidden()); + + assertThat(webAuthnCredentialRepository.findByIdWithUser("cred-1")).isPresent(); + } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnRegistrationDeleteAllowModeTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnRegistrationDeleteAllowModeTest.java new file mode 100644 index 0000000..133b598 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnRegistrationDeleteAllowModeTest.java @@ -0,0 +1,52 @@ +package com.digitalsanctuary.spring.user.security; + +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.user; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.web.servlet.MockMvc; +import com.digitalsanctuary.spring.user.test.annotations.SecurityTest; + +/** + * Regression test for GHSA-3cv9-vgqh-jwpm under {@code user.security.defaultAction=allow}. + *

+ * {@link WebAuthnFeatureEnabledIntegrationTest} already locks in the deny under the test profile's default + * {@code defaultAction=deny}. That mode alone would not have exposed the vulnerability: under {@code deny}, an + * unlisted URI merely requires authentication, so an authenticated attacker could still have reached the endpoint. + * The scenario the fix specifically targets is {@code allow} mode, where {@code anyRequest().permitAll()} would + * otherwise expose the built-in {@code DELETE /webauthn/register/{id}} endpoint to anonymous callers entirely. This + * test proves the deny rule is registered ahead of both {@code defaultAction} branches, not just the {@code deny} + * one. + *

+ */ +@SecurityTest +@TestPropertySource(properties = {"user.security.defaultAction=allow", "user.webauthn.enabled=true"}) +@DisplayName("WebAuthn built-in delete endpoint - defaultAction=allow (GHSA-3cv9-vgqh-jwpm)") +class WebAuthnRegistrationDeleteAllowModeTest { + + @Autowired + private MockMvc mockMvc; + + @Test + @DisplayName("should deny an anonymous DELETE to the built-in WebAuthn registration endpoint when defaultAction is allow") + void shouldDenyAnonymousDeleteWhenAllow() throws Exception { + // Under allow mode, anyRequest().permitAll() is the fallback for anything not explicitly matched. Without + // the fix, an unlisted DELETE /webauthn/register/{id} would fall through to that permitAll() and be + // reachable by anyone. denyAll() still produces an AccessDeniedException for an anonymous principal, but + // ExceptionTranslationFilter routes an unauthenticated caller to the login entry point (302) rather than a + // bare 403 -- matching WebSecurityAuthorizationAllowTest's anonymous-denied assertions. Either way the + // delete never executes. + mockMvc.perform(delete("/webauthn/register/cred-1").with(csrf())).andExpect(status().is3xxRedirection()); + } + + @Test + @DisplayName("should deny an authenticated DELETE to the built-in WebAuthn registration endpoint when defaultAction is allow") + void shouldDenyAuthenticatedDeleteWhenAllow() throws Exception { + mockMvc.perform(delete("/webauthn/register/cred-1").with(user("user@test.com").roles("USER")).with(csrf())) + .andExpect(status().isForbidden()); + } +}