diff --git a/app/assets/javascripts/discourse/app/lib/webauthn.js b/app/assets/javascripts/discourse/app/lib/webauthn.js index 9b1af621c23..3fe71998481 100644 --- a/app/assets/javascripts/discourse/app/lib/webauthn.js +++ b/app/assets/javascripts/discourse/app/lib/webauthn.js @@ -1,4 +1,5 @@ import I18n from "I18n"; + export function stringToBuffer(str) { let buffer = new ArrayBuffer(str.length); let byteView = new Uint8Array(buffer); @@ -33,7 +34,10 @@ export function getWebauthnCredential( type: "public-key", }; }); + // See https://w3c.github.io/webauthn/#sctn-verifying-assertion for the steps followed here. + // 1. Let options be a new PublicKeyCredentialRequestOptions structure configured to the Relying Party's needs + // 2. Call navigator.credentials.get() and pass options as the publicKey option. navigator.credentials .get({ publicKey: { @@ -48,8 +52,19 @@ export function getWebauthnCredential( }, }) .then((credential) => { - // 1. if there is a credential, check if the raw ID base64 matches - // any of the allowed credential ids + // 3. If credential.response is not an instance of AuthenticatorAssertionResponse, abort the ceremony. + // eslint-disable-next-line no-undef + if (!(credential.response instanceof AuthenticatorAssertionResponse)) { + return errorCallback( + I18n.t("login.security_key_invalid_response_error") + ); + } + + // 4. Let clientExtensionResults be the result of calling credential.getClientExtensionResults(). + // We are not using this + + // 5. If options.allowCredentials is not empty, verify that credential.id identifies one of the public key + // credentials listed in options.allowCredentials. if ( !allowedCredentialIds.some( (credentialId) => bufferToBase64(credential.rawId) === credentialId @@ -68,7 +83,9 @@ export function getWebauthnCredential( ), credentialId: bufferToBase64(credential.rawId), }; + successCallback(credentialData); + // steps 6+ of this flow are handled by lib/discourse_webauthn/authentication_service.rb }) .catch((err) => { if (err.name === "NotAllowedError") { diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 9c5fcf33355..823f24208f9 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2169,6 +2169,7 @@ en: security_key_not_allowed_error: "The security key authentication process either timed out or was cancelled." security_key_no_matching_credential_error: "No matching credentials could be found in the provided security key." security_key_support_missing_error: "Your current device or browser does not support the use of security keys. Please use a different method." + security_key_invalid_response_error: "The security key authentication process failed due to an invalid response." email_placeholder: "Email / Username" caps_lock_warning: "Caps Lock is on" error: "Unknown error" diff --git a/lib/discourse_webauthn/authentication_service.rb b/lib/discourse_webauthn/authentication_service.rb index 8c18c6f9220..15ed1236979 100644 --- a/lib/discourse_webauthn/authentication_service.rb +++ b/lib/discourse_webauthn/authentication_service.rb @@ -8,6 +8,7 @@ module DiscourseWebauthn # the steps followed here. Memoized methods are called in their # place in the step flow to make the process clearer. def authenticate_security_key + # Steps 1-5 of this authentication flow are in the frontend at lib/webauthn.js if @params.blank? || (!@params.is_a?(Hash) && !@params.is_a?(ActionController::Parameters)) raise( MalformedPublicKeyCredentialError, @@ -15,63 +16,75 @@ module DiscourseWebauthn ) end + # 6. Identify the user being authenticated and verify that this user is the + # owner of the public key credential source credentialSource identified by credential.id: + + # 6a. If the user was identified before the authentication ceremony was initiated, + # verify that the identified user account contains a credential record whose id equals credential.rawId. security_key = UserSecurityKey.find_by(credential_id: @params[:credentialId]) raise(KeyNotFoundError, I18n.t("webauthn.validation.not_found_error")) if security_key.blank? - # 3. Identify the user being authenticated and verify that this user is the - # owner of the public key credential source credentialSource identified by credential.id: if @factor_type == UserSecurityKey.factor_types[:second_factor] && (@current_user == nil || security_key.user == nil || security_key.user != @current_user) raise(OwnershipError, I18n.t("webauthn.validation.ownership_error")) end - # 4. Using credential.id (or credential.rawId, if base64url encoding is inappropriate for your use case), - # look up the corresponding credential public key and let credentialPublicKey be that credential public key. - public_key = security_key.public_key + # 6b. If the user was not identified before the authentication ceremony was initiated, + # verify that response.userHandle is present. Verify that the user account identified by response.userHandle + # contains a credential record whose id equals credential.rawId + if @factor_type == UserSecurityKey.factor_types[:first_factor] && + Base64.decode64(@params[:userHandle]) != @current_user.secure_identifier + raise(OwnershipError, I18n.t("webauthn.validation.ownership_error")) + end - # 5. Let cData, authData and sig denote the value of credential’s response's clientDataJSON, authenticatorData, and signature respectively. - # 6. Let JSONtext be the result of running UTF-8 decode on the value of cData. - # 7. Let C, the client data claimed as used for the signature, be the result of running an implementation-specific JSON parser on JSONtext. + # 7. No upstream step + # 8. No upstream step + + # 9. Let cData, authData and sig denote the value of credential’s response's clientDataJSON, authenticatorData, and signature respectively. + # 10. Let JSONtext be the result of running UTF-8 decode on the value of cData. + # 11. Let C, the client data claimed as used for the signature, be the result of running an implementation-specific JSON parser on JSONtext. client_data - # 8. Verify that the value of C.type is the string webauthn.get. + # 12. Verify that the value of C.type is the string webauthn.get. validate_webauthn_type(::DiscourseWebauthn::ACCEPTABLE_AUTHENTICATION_TYPE) - # 9. Verify that the value of C.challenge equals the base64url encoding of options.challenge. + # 13. Verify that the value of C.challenge equals the base64url encoding of options.challenge. validate_challenge - # 10. Verify that the value of C.origin matches the Relying Party's origin. + # 14. Verify that the value of C.origin matches the Relying Party's origin. validate_origin - # 11. Verify that the value of C.tokenBinding.status matches the state of Token Binding for the TLS connection - # over which the attestation was obtained. If Token Binding was used on that TLS connection, also verify - # that C.tokenBinding.id matches the base64url encoding of the Token Binding ID for the connection. - # Not using this right now. + # 15. If C.topOrigin is present: + # - Verify that the Relying Party expects this credential to be used within an iframe that is not same-origin with its ancestors. + # - Verify that the value of C.topOrigin matches the origin of a page that the Relying Party expects to be sub-framed within. + # We are not using this. - # 12. Verify that the rpIdHash in authData is the SHA-256 hash of the RP ID expected by the Relying Party. + # 16. Verify that the rpIdHash in authData is the SHA-256 hash of the RP ID expected by the Relying Party. validate_rp_id_hash - # 13. Verify that the User Present bit of the flags in authData is set. + # 17. Verify that the User Present bit of the flags in authData is set. # https://blog.bigbinary.com/2011/07/20/ruby-pack-unpack.html # validate_user_presence # - # 14. If user verification is required for this registration, verify that - # the User Verified bit of the flags in authData is set. + # 18. Determine whether user verification is required for this assertion. + # User verification SHOULD be required if, and only if, options.userVerification is set to required. + # If user verification was determined to be required, verify that the UV bit of the flags in authData is set. + # Otherwise, ignore the value of the UV flag. validate_user_verification if @factor_type == UserSecurityKey.factor_types[:first_factor] - # 15. Verify that the values of the client extension outputs in clientExtensionResults and the authenticator - # extension outputs in the extensions in authData are as expected, considering the client extension input - # values that were given in options.extensions and any specific policy of the Relying Party regarding - # unsolicited extensions, i.e., those that were not specified as part of options.extensions. In the - # general case, the meaning of "are as expected" is specific to the Relying Party and which extensions are in use. + # 19. If the BE bit of the flags in authData is not set, verify that the BS bit is not set. + # Not using this right now. + # 20. If the credential backup state is used as part of Relying Party business logic or policy... + # Not using this right now. + # 21. Verify that the values of the client extension outputs in clientExtensionResults... # Not using this right now. - # 16. Let hash be the result of computing a hash over response.clientDataJSON using SHA-256. + # 22. Let hash be the result of computing a hash over response.clientDataJSON using SHA-256. client_data_hash - # 17. Using credentialPublicKey, verify that sig is a valid signature over the binary concatenation of authData and hash. + # 23. Using credentialPublicKey, verify that sig is a valid signature over the binary concatenation of authData and hash. cose_key = COSE::Key.deserialize(Base64.decode64(security_key.public_key)) cose_algorithm = COSE::Algorithm.find(cose_key.alg) @@ -90,7 +103,13 @@ module DiscourseWebauthn raise(PublicKeyError, I18n.t("webauthn.validation.public_key_error")) end - # Success! Update the last used at time for the key. + # 24. If authData.signCount is nonzero or credentialRecord.signCount is nonzero... + # Not using this right now. + + # 25. If response.attestationObject is present and the Relying Party wishes to verify the attestation... + # Not using this right now. + + # 26. Success! Update the last used at time for the key (credentialRecord). security_key.update(last_used: Time.zone.now) # Return security key record so controller can use it to update the session diff --git a/spec/lib/discourse_webauthn/authentication_service_spec.rb b/spec/lib/discourse_webauthn/authentication_service_spec.rb index 2f0a58a2862..01df15769ec 100644 --- a/spec/lib/discourse_webauthn/authentication_service_spec.rb +++ b/spec/lib/discourse_webauthn/authentication_service_spec.rb @@ -269,7 +269,7 @@ RSpec.describe DiscourseWebauthn::AuthenticationService do end end - describe "authenticating a valid passkey" do + describe "authenticating passkeys" do let(:options) do { factor_type: UserSecurityKey.factor_types[:first_factor], session: secure_session } end @@ -297,10 +297,20 @@ RSpec.describe DiscourseWebauthn::AuthenticationService do ) end - it "works and returns the correct key credential" do - key = service.authenticate_security_key - expect(key).to eq(security_key) - expect(key.factor_type).to eq(UserSecurityKey.factor_types[:first_factor]) + before do + # this essentially bypasses the user handle check for this key + # a real test would need to go through the full registration/authentication flow in one go + params[:userHandle] = Base64.strict_encode64( + security_key_user.create_or_fetch_secure_identifier, + ) + end + + context "with a valid passkey" do + it "works and returns the correct key credential" do + key = service.authenticate_security_key + expect(key).to eq(security_key) + expect(key.factor_type).to eq(UserSecurityKey.factor_types[:first_factor]) + end end context "when the user verification flag in the key is false" do @@ -319,5 +329,16 @@ RSpec.describe DiscourseWebauthn::AuthenticationService do ) end end + + context "when the user handle does not match" do + it "raises an OwnershipError" do + params[:userHandle] = Base64.strict_encode64(SecureRandom.hex(20)) + + expect { service.authenticate_security_key }.to raise_error( + DiscourseWebauthn::OwnershipError, + I18n.t("webauthn.validation.ownership_error"), + ) + end + end end end