From 3ba245c39ce5cc8c26f3fdcad57f93cb09db5366 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Thu, 23 Apr 2026 12:10:48 +0200 Subject: [PATCH] Make acceptable AAGUID ckeck in WebAuthn stricter Closes #48388 Signed-off-by: rmartinc --- .../topics/authentication/webauthn.adoc | 2 +- .../topics/changes/changes-26_6_2.adoc | 12 +++++ .../admin/messages/messages_en.properties | 2 +- .../requiredactions/WebAuthnRegister.java | 31 ++++++----- .../WebAuthnRegisterAndLoginTest.java | 24 +++------ .../webauthn/AbstractWebAuthnVirtualTest.java | 1 + .../WebAuthnOtherSettingsTest.java | 53 +++++++++++++++++-- 7 files changed, 86 insertions(+), 39 deletions(-) create mode 100644 docs/documentation/upgrading/topics/changes/changes-26_6_2.adoc diff --git a/docs/documentation/server_admin/topics/authentication/webauthn.adoc b/docs/documentation/server_admin/topics/authentication/webauthn.adoc index d14a38fe678..3bb2c93d541 100644 --- a/docs/documentation/server_admin/topics/authentication/webauthn.adoc +++ b/docs/documentation/server_admin/topics/authentication/webauthn.adoc @@ -122,7 +122,7 @@ The configurable items and their description are as follows: |If enabled, {project_name} cannot re-register an already registered WebAuthn authenticator. |Acceptable AAGUIDs -|The white list of AAGUIDs which a WebAuthn authenticator must register against. +|The list of allowed AAGUIDs which a WebAuthn authenticator must register against. An AAGUID (Authenticator Attestation Global Unique Identifier) is a 128-bit identifier indicating the authenticator's type (e.g., make and model). This option needs the **Attestation conveyance preference** to be configured (normally `Direct`) to ensure a trusted AAGUID is passed. Default attestation `None` is not reliable, and can anonymize the AAGUID to zero value. If you setup **Acceptable AAGUIDs** only those authenticators are valid to register new WebAuthn credentials. |=== diff --git a/docs/documentation/upgrading/topics/changes/changes-26_6_2.adoc b/docs/documentation/upgrading/topics/changes/changes-26_6_2.adoc new file mode 100644 index 00000000000..e7522d0f579 --- /dev/null +++ b/docs/documentation/upgrading/topics/changes/changes-26_6_2.adoc @@ -0,0 +1,12 @@ +== Notable changes + +Notable changes may include internal behavior changes that prevent common misconfigurations, bugs that are fixed, or changes to simplify running {project_name}. +It also lists significant changes to internal APIs. + +=== WebAuthn acceptable AAGUIDs option restricts authenticators strictly + +The WebAuthn policy presents the option **Acceptable AAGUIDs** to restrict the authenticators that are allowed to register new credentials. The AAGUID (Authenticator Attestation Global Unique Identifier) is an identifier for the authenticator's type (e.g., make and model). This option requires the **Attestation conveyance preference** to be configured too (normally `Direct`), in order to force the authenticator to include the attestation inside the registration data. + +Since this release, when this option is setup, the attestation is required to be present and signed with a valid certificate for the {project_name} trust-store. The `None` attestation format is explicitly not permitted. Previously, there were some corner cases in which a self attestation was accepted. The change is expected to be harmless, but maybe there are combinations of authenticators and WebAuthn policies that can present issues. + +See chapter link:{adminguide_link}#_webauthn-policy[Managing policy] in the {adminguide_name} for more information. \ No newline at end of file diff --git a/js/apps/admin-ui/maven-resources/theme/keycloak.v2/admin/messages/messages_en.properties b/js/apps/admin-ui/maven-resources/theme/keycloak.v2/admin/messages/messages_en.properties index 45f487cea20..7deb15cb3f0 100644 --- a/js/apps/admin-ui/maven-resources/theme/keycloak.v2/admin/messages/messages_en.properties +++ b/js/apps/admin-ui/maven-resources/theme/keycloak.v2/admin/messages/messages_en.properties @@ -1339,7 +1339,7 @@ revoke=Revoke admin=Admin syncUsersError=Could not sync users\: '{{error}}' generatedAccessTokenHelp=See the example access token, which will be generated and sent to the client when the selected user is authenticated. You can see claims and roles that the token will contain based on the effective protocol mappers and role scope mappings and also based on the claims and roles assigned to the actual user. -webAuthnPolicyAcceptableAaguidsHelp=The list of allowed AAGUIDs of which an authenticator can be registered. An AAGUID is a 128-bit identifier indicating the authenticator's type (e.g., make and model). +webAuthnPolicyAcceptableAaguidsHelp=The list of allowed AAGUIDs of which an authenticator can be registered. An AAGUID is a 128-bit identifier indicating the authenticator's type (e.g., make and model). This option needs the Attestation conveyance preference to be configured (normally `Direct`) to ensure a trusted AAGUID is passed. Default attestation `None` is not reliable, and can anonymize the AAGUID to zero value. keyPasswordHelp=Password for the private key frontchannelLogout=Front channel logout logoutConfirmation=Logout confirmation diff --git a/services/src/main/java/org/keycloak/authentication/requiredactions/WebAuthnRegister.java b/services/src/main/java/org/keycloak/authentication/requiredactions/WebAuthnRegister.java index 5bbe0f70dd9..ed79ba039e4 100644 --- a/services/src/main/java/org/keycloak/authentication/requiredactions/WebAuthnRegister.java +++ b/services/src/main/java/org/keycloak/authentication/requiredactions/WebAuthnRegister.java @@ -67,6 +67,7 @@ import com.webauthn4j.data.RegistrationRequest; import com.webauthn4j.data.attestation.authenticator.AttestedCredentialData; import com.webauthn4j.data.attestation.statement.AttestationStatement; import com.webauthn4j.data.attestation.statement.COSEAlgorithmIdentifier; +import com.webauthn4j.data.attestation.statement.NoneAttestationStatement; import com.webauthn4j.data.client.Origin; import com.webauthn4j.data.client.challenge.Challenge; import com.webauthn4j.data.client.challenge.DefaultChallenge; @@ -268,7 +269,7 @@ public class WebAuthnRegister implements RequiredActionProvider, CredentialRegis AuthenticatorUtil.logoutOtherSessions(context); } - WebAuthnRegistrationManager webAuthnRegistrationManager = createWebAuthnRegistrationManager(policy.getAttestationConveyancePreference()); + WebAuthnRegistrationManager webAuthnRegistrationManager = createWebAuthnRegistrationManager(policy); try { // parse RegistrationData registrationData = webAuthnRegistrationManager.parse(registrationRequest); @@ -318,11 +319,12 @@ public class WebAuthnRegister implements RequiredActionProvider, CredentialRegis * Create WebAuthnRegistrationManager instance * Can be overridden in subclasses to customize the used attestation validators * - * @param attestationPreference The attestation selected in the policy + * @param policy The webauthn policy defined * @return webauthn4j WebAuthnRegistrationManager instance */ - protected WebAuthnRegistrationManager createWebAuthnRegistrationManager(String attestationPreference) { + protected WebAuthnRegistrationManager createWebAuthnRegistrationManager(WebAuthnPolicy policy) { List verifiers = new ArrayList<>(6); + final String attestationPreference = policy.getAttestationConveyancePreference(); if (attestationPreference == null || Constants.DEFAULT_WEBAUTHN_POLICY_NOT_SPECIFIED.equals(attestationPreference) || AttestationConveyancePreference.NONE.getValue().equals(attestationPreference)) { @@ -334,10 +336,15 @@ public class WebAuthnRegister implements RequiredActionProvider, CredentialRegis verifiers.add(new AndroidSafetyNetAttestationStatementVerifier()); verifiers.add(new FIDOU2FAttestationStatementVerifier()); + DefaultSelfAttestationTrustworthinessVerifier selfAttestationVerifier = new DefaultSelfAttestationTrustworthinessVerifier(); + final List acceptableAaguids = policy.getAcceptableAaguids(); + // self attestation should be disabled to be sure the AAGUID can be trusted + selfAttestationVerifier.setSelfAttestationAllowed(acceptableAaguids == null || acceptableAaguids.isEmpty()); + return new WebAuthnRegistrationManager( verifiers, this.certPathtrustVerifier, - new DefaultSelfAttestationTrustworthinessVerifier(), + selfAttestationVerifier, Collections.emptyList(), // Custom Registration Verifier is not supported new ObjectConverter() ); @@ -406,20 +413,12 @@ public class WebAuthnRegister implements RequiredActionProvider, CredentialRegis private void checkAcceptedAuthenticator(RegistrationData response, WebAuthnPolicy policy) throws Exception { String aaguid = response.getAttestationObject().getAuthenticatorData().getAttestedCredentialData().getAaguid().toString(); List acceptableAaguids = policy.getAcceptableAaguids(); - boolean isAcceptedAuthenticator = false; if (acceptableAaguids != null && !acceptableAaguids.isEmpty()) { - for(String acceptableAaguid : acceptableAaguids) { - if (aaguid.equals(acceptableAaguid)) { - isAcceptedAuthenticator = true; - break; - } + if (NoneAttestationStatement.FORMAT.equals(response.getAttestationObject().getFormat())) { + throw new WebAuthnException("Acceptable AAGUIDs require an attestation format other than 'none'."); + } else if (acceptableAaguids.stream().noneMatch(acceptableAaguid -> aaguid.equals(acceptableAaguid))) { + throw new WebAuthnException("not acceptable aaguid = " + aaguid); } - } else { - // no accepted authenticators means accepting any kind of authenticator - isAcceptedAuthenticator = true; - } - if (!isAcceptedAuthenticator) { - throw new WebAuthnException("not acceptable aaguid = " + aaguid); } } diff --git a/tests/webauthn/src/test/java/org/keycloak/tests/webauthn/WebAuthnRegisterAndLoginTest.java b/tests/webauthn/src/test/java/org/keycloak/tests/webauthn/WebAuthnRegisterAndLoginTest.java index 54976468101..23b6097a2ed 100644 --- a/tests/webauthn/src/test/java/org/keycloak/tests/webauthn/WebAuthnRegisterAndLoginTest.java +++ b/tests/webauthn/src/test/java/org/keycloak/tests/webauthn/WebAuthnRegisterAndLoginTest.java @@ -17,7 +17,6 @@ package org.keycloak.tests.webauthn; import java.io.IOException; -import java.util.ArrayList; import java.util.List; import java.util.Objects; @@ -53,7 +52,6 @@ import org.keycloak.util.JsonSerialization; import org.hamcrest.Matchers; import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import static org.keycloak.models.AuthenticationExecutionModel.Requirement.ALTERNATIVE; @@ -80,15 +78,6 @@ public class WebAuthnRegisterAndLoginTest extends AbstractWebAuthnVirtualTest { @InjectPage SelectAuthenticatorPage selectAuthenticatorPage; - @BeforeEach - public void customizeWebAuthnTestRealm() { - List acceptableAaguids = new ArrayList<>(); - acceptableAaguids.add("00000000-0000-0000-0000-000000000000"); - acceptableAaguids.add("6d44ba9b-f6ec-2e49-b930-0c8fe920cb73"); - - managedRealm.updateWithCleanup(r -> r.webAuthnPolicyAcceptableAaguids(acceptableAaguids)); - } - @Test public void registerUserSuccess() { String username = "registerUserSuccess"; @@ -481,13 +470,12 @@ public class WebAuthnRegisterAndLoginTest extends AbstractWebAuthnVirtualTest { } private void updateRealmWithDefaultWebAuthnSettings() { - managedRealm.updateWithCleanup(r -> r.webAuthnPolicySignatureAlgorithms(List.of("ES256"))); - managedRealm.updateWithCleanup(r -> r.webAuthnPolicyAttestationConveyancePreference("none")); - managedRealm.updateWithCleanup(r -> r.webAuthnPolicyAuthenticatorAttachment("cross-platform")); - managedRealm.updateWithCleanup(r -> r.webAuthnPolicyRequireResidentKey("No")); - managedRealm.updateWithCleanup(r -> r.webAuthnPolicyRpId(null)); - managedRealm.updateWithCleanup(r -> r.webAuthnPolicyUserVerificationRequirement("preferred")); - managedRealm.updateWithCleanup(r -> r.webAuthnPolicyAcceptableAaguids(List.of(ALL_ZERO_AAGUID))); + managedRealm.updateWithCleanup(r -> r.webAuthnPolicySignatureAlgorithms(List.of("ES256")) + .webAuthnPolicyAttestationConveyancePreference("none") + .webAuthnPolicyAuthenticatorAttachment("cross-platform") + .webAuthnPolicyRequireResidentKey("No") + .webAuthnPolicyRpId(null) + .webAuthnPolicyUserVerificationRequirement("preferred")); } /** diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/AbstractWebAuthnVirtualTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/AbstractWebAuthnVirtualTest.java index ed5aa610ea3..71c16fe83ae 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/AbstractWebAuthnVirtualTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/AbstractWebAuthnVirtualTest.java @@ -121,6 +121,7 @@ public abstract class AbstractWebAuthnVirtualTest extends AbstractChangeImported protected static final String ALL_ZERO_AAGUID = "00000000-0000-0000-0000-000000000000"; protected static final String ALL_ONE_AAGUID = "11111111-1111-1111-1111-111111111111"; + protected static final String CHROME_AAGUID = "01020304-0506-0708-0102-030405060708"; protected static final String USERNAME = "UserWebAuthn"; protected static final String EMAIL = "UserWebAuthn@email"; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/registration/WebAuthnOtherSettingsTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/registration/WebAuthnOtherSettingsTest.java index c76efd24581..2e2d0baf50d 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/registration/WebAuthnOtherSettingsTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/registration/WebAuthnOtherSettingsTest.java @@ -169,19 +169,66 @@ public class WebAuthnOtherSettingsTest extends AbstractWebAuthnVirtualTest { @Test @IgnoreBrowserDriver(FirefoxDriver.class) // See https://github.com/keycloak/keycloak/issues/10368 public void excludeCredentials() throws IOException { - List acceptableAaguids = Collections.singletonList(ALL_ONE_AAGUID); + List acceptableAaguids = Collections.singletonList(ALL_ZERO_AAGUID); + + try (Closeable u = getWebAuthnRealmUpdater() + .setWebAuthnPolicyAcceptableAaguids(acceptableAaguids) + .setWebAuthnPolicyAttestationConveyancePreference(AttestationConveyancePreference.DIRECT.getValue()) + .update()) { + // webauthn virtual emulator in chrome sets a self signed certificate every time, truststore needs to be disabled + testingClient.testing().disableTruststoreSpi(); + + WebAuthnRealmData realmData = new WebAuthnRealmData(managedRealm.admin().toRepresentation(), isPasswordless()); + assertThat(realmData.getAcceptableAaguids(), Matchers.contains(ALL_ZERO_AAGUID)); + + registerDefaultUser(); + + webAuthnErrorPage.assertCurrent(); + assertThat(webAuthnErrorPage.getError(), allOf(containsString("not acceptable aaguid"), containsString(CHROME_AAGUID))); + } finally { + testingClient.testing().reenableTruststoreSpi(); + } + } + + @Test + @IgnoreBrowserDriver(FirefoxDriver.class) // See https://github.com/keycloak/keycloak/issues/10368 + public void excludeCredentialsSuccess() throws IOException { + List acceptableAaguids = Collections.singletonList(CHROME_AAGUID); + + try (Closeable u = getWebAuthnRealmUpdater() + .setWebAuthnPolicyAcceptableAaguids(acceptableAaguids) + .setWebAuthnPolicyAttestationConveyancePreference(AttestationConveyancePreference.DIRECT.getValue()) + .update()) { + // webauthn virtual emulator in chrome sets a self signed certificate every time, truststore needs to be disabled + testingClient.testing().disableTruststoreSpi(); + + WebAuthnRealmData realmData = new WebAuthnRealmData(managedRealm.admin().toRepresentation(), isPasswordless()); + assertThat(realmData.getAcceptableAaguids(), Matchers.contains(CHROME_AAGUID)); + + registerDefaultUser(); + + appPage.assertCurrent(); + } finally { + testingClient.testing().reenableTruststoreSpi(); + } + } + + @Test + @IgnoreBrowserDriver(FirefoxDriver.class) // See https://github.com/keycloak/keycloak/issues/10368 + public void excludeCredentialsUsingNone() throws IOException { + List acceptableAaguids = Collections.singletonList(ALL_ZERO_AAGUID); try (Closeable u = getWebAuthnRealmUpdater() .setWebAuthnPolicyAcceptableAaguids(acceptableAaguids) .update()) { WebAuthnRealmData realmData = new WebAuthnRealmData(managedRealm.admin().toRepresentation(), isPasswordless()); - assertThat(realmData.getAcceptableAaguids(), Matchers.contains(ALL_ONE_AAGUID)); + assertThat(realmData.getAcceptableAaguids(), Matchers.contains(ALL_ZERO_AAGUID)); registerDefaultUser(); webAuthnErrorPage.assertCurrent(); - assertThat(webAuthnErrorPage.getError(), allOf(containsString("not acceptable aaguid"), containsString(ALL_ZERO_AAGUID))); + assertThat(webAuthnErrorPage.getError(), containsString("Acceptable AAGUIDs require an attestation format other than 'none'.")); } } }