Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Original file line number Diff line number Diff line change
@@ -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}.
* <p>
* {@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.
* </p>
*/
@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());
}
}
Loading