Handle errors when registration fails

Previously we would raise a 500 error in a variety of cases:

- If a second key was being registered simultaneously (e.g. in a
separate tab), which means the registration state could be missing
after the first registration completes. That smells like an attack.

- If the server-side verification failed e.g. origin verification,
challenge verification, etc. The library seems to use 'ValueError'
for all such errors [1] (after auditing its 'raise' statements, and
excluding AttestationError [2], since we're not doing that).

- If a key is used that attempts to sign with an unsupported
algorithm. This would normally raise a NotImplemented error as part
of verifying attestation [3], but we don't do that, so we need to
verify the algorithm is supported by the library manually.

This adds error handling to return a 400 response and error message
in these cases, since the error is not unexpected (i.e. not a 500).
A 400 seems more appropriate than a 403, since in many cases it's
not clear if the request data is valid.

I've used CBOR for the transport encoding, to match the successful
request / response encoding. Note that the ordering of then/catch
matters in JS - we don't want to catch our own throws!

[1]: 142587b3e6/fido2/server.py (L255)
[2]: c42d9628a4/fido2/attestation/base.py (L39)
[3]: c42d9628a4/fido2/cose.py (L92)

tests/app/main/views/test_webauthn_credentials.py

@@ -3,6 +3,7 @@ from fido2 import cbor
 from flask import url_for
 
 from app import webauthn_server
+from app.models.webauthn_credential import RegistrationError
 
 
 @pytest.mark.parametrize('endpoint', [
@@ -132,3 +133,37 @@ def test_complete_register_clears_session(
 
     with platform_admin_client.session_transaction() as session:
         assert 'webauthn_registration_state' not in session
+
+
+def test_complete_register_handles_library_errors(
+    platform_admin_client,
+    mocker,
+):
+    with platform_admin_client.session_transaction() as session:
+        session['webauthn_registration_state'] = 'state'
+
+    mocker.patch(
+        'app.models.webauthn_credential.WebAuthnCredential.from_registration',
+        side_effect=RegistrationError('error')
+    )
+
+    response = platform_admin_client.post(
+        url_for('main.webauthn_complete_register'),
+        data=cbor.encode('public_key_credential'),
+    )
+
+    assert response.status_code == 400
+    assert cbor.decode(response.data) == 'error'
+
+
+def test_complete_register_handles_missing_state(
+    platform_admin_client,
+    mocker,
+):
+    response = platform_admin_client.post(
+        url_for('main.webauthn_complete_register'),
+        data=cbor.encode('public_key_credential'),
+    )
+
+    assert response.status_code == 400
+    assert cbor.decode(response.data) == 'No registration in progress'

tests/app/models/test_webauthn_credential.py

@@ -5,7 +5,7 @@ from fido2 import cbor
 from fido2.cose import ES256
 
 from app import webauthn_server
-from app.models.webauthn_credential import WebAuthnCredential
+from app.models.webauthn_credential import RegistrationError, WebAuthnCredential
 
 # noqa adapted from https://github.com/duo-labs/py_webauthn/blob/90e3d97e0182899a35a70fc510280b4082cce19b/tests/test_webauthn.py#L14-L24
 SESSION_STATE = {'challenge': 'bPzpX3hHQtsp9evyKYkaZtVc9UN07PUdJ22vZUdDp94', 'user_verification': 'discouraged'}
@@ -14,6 +14,9 @@ CLIENT_DATA_JSON = b'{"type": "webauthn.create", "clientExtensions": {}, "challe
 # had to use the cbor2 library to re-encode the attestationObject due to implementation differences
 ATTESTATION_OBJECT = base64.b64decode(b'o2NmbXRoZmlkby11MmZnYXR0U3RtdKJjc2lnWEgwRgIhAI1qbvWibQos/t3zsTU05IXw1Ek3SDApATok09uc4UBwAiEAv0fB/lgb5Ot3zJ691Vje6iQLAtLhJDiA8zDxaGjcE3hjeDVjgVkCUzCCAk8wggE3oAMCAQICBDxoKU0wDQYJKoZIhvcNAQELBQAwLjEsMCoGA1UEAxMjWXViaWNvIFUyRiBSb290IENBIFNlcmlhbCA0NTcyMDA2MzEwIBcNMTQwODAxMDAwMDAwWhgPMjA1MDA5MDQwMDAwMDBaMDExLzAtBgNVBAMMJll1YmljbyBVMkYgRUUgU2VyaWFsIDIzOTI1NzM0ODExMTE3OTAxMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEvd9nk9t3lMNQMXHtLE1FStlzZnUaSLql2fm1ajoggXlrTt8rzXuSehSTEPvEaEdv/FeSqX22L6Aoa8ajIAIOY6M7MDkwIgYJKwYBBAGCxAoCBBUxLjMuNi4xLjQuMS40MTQ4Mi4xLjUwEwYLKwYBBAGC5RwCAQEEBAMCBSAwDQYJKoZIhvcNAQELBQADggEBAKrADVEJfuwVpIazebzEg0D4Z9OXLs5qZ/ukcONgxkRZ8K04QtP/CB5x6olTlxsj+SXArQDCRzEYUgbws6kZKfuRt2a1P+EzUiqDWLjRILSr+3/o7yR7ZP/GpiFKwdm+czb94POoGD+TS1IYdfXj94mAr5cKWx4EKjh210uovu/pLdLjc8xkQciUrXzZpPR9rT2k/q9HkZhHU+NaCJzky+PTyDbq0KKnzqVhWtfkSBCGw3ezZkTS+5lrvOKbIa24lfeTgu7FST5OwTPCFn8HcfWZMXMSD/KNU+iBqJdAwTLPPDRoLLvPTl29weCAIh+HUpmBQd0UltcPOrA/LFvAf61oYXV0aERhdGFYwnSm6pITyZwvdLIkkrMgz0AmKpTBqVCgOX8pJQtghB7wQQAAAAAAAAAAAAAAAAAAAAAAAAAAAECKU1ppjl9gmhHWyDkgHsUvZmhr6oF3/lD3llzLE2SaOSgOGIsIuAQqgp8JQSUu3r/oOaP8RS44dlQjrH+ALfYtpAECAyYhWCAxnqAfESXOYjKUc2WACuXZ3ch0JHxV0VFrrTyjyjIHXCJYIFnx8H87L4bApR4M+hPcV+fHehEOeW+KCyd0H+WGY8s6')  # noqa
 
+# manually adapted by working out which character in the encoded CBOR corresponds to the public key algorithm ID
+UNSUPPORTED_ATTESTATION_OBJECT = base64.b64decode(b'o2NmbXRoZmlkby11MmZnYXR0U3RtdKJjc2lnWEgwRgIhAI1qbvWibQos/t3zsTU05IXw1Ek3SDApATok09uc4UBwAiEAv0fB/lgb5Ot3zJ691Vje6iQLAtLhJDiA8zDxaGjcE3hjeDVjgVkCUzCCAk8wggE3oAMCAQICBDxoKU0wDQYJKoZIhvcNAQELBQAwLjEsMCoGA1UEAxMjWXViaWNvIFUyRiBSb290IENBIFNlcmlhbCA0NTcyMDA2MzEwIBcNMTQwODAxMDAwMDAwWhgPMjA1MDA5MDQwMDAwMDBaMDExLzAtBgNVBAMMJll1YmljbyBVMkYgRUUgU2VyaWFsIDIzOTI1NzM0ODExMTE3OTAxMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEvd9nk9t3lMNQMXHtLE1FStlzZnUaSLql2fm1ajoggXlrTt8rzXuSehSTEPvEaEdv/FeSqX22L6Aoa8ajIAIOY6M7MDkwIgYJKwYBBAGCxAoCBBUxLjMuNi4xLjQuMS40MTQ4Mi4xLjUwEwYLKwYBBAGC5RwCAQEEBAMCBSAwDQYJKoZIhvcNAQELBQADggEBAKrADVEJfuwVpIazebzEg0D4Z9OXLs5qZ/ukcONgxkRZ8K04QtP/CB5x6olTlxsj+SXArQDCRzEYUgbws6kZKfuRt2a1P+EzUiqDWLjRILSr+3/o7yR7ZP/GpiFKwdm+czb94POoGD+TS1IYdfXj94mAr5cKWx4EKjh210uovu/pLdLjc8xkQciUrXzZpPR9rT2k/q9HkZhHU+NaCJzky+PTyDbq0KKnzqVhWtfkSBCGw3ezZkTS+5lrvOKbIa24lfeTgu7FST5OwTPCFn8HcfWZMXMSD/KNU+iBqJdAwTLPPDRoLLvPTl29weCAIh+HUpmBQd0UltcPOrA/LFvAf61oYXV0aERhdGFYwnSm6pITyZwvdLIkkrMgz0AmKpTBqVCgOX8pJQtghB7wQQAAAAAAAAAAAAAAAAAAAAAAAAAAAECKU1ppjl9gmhHWyDkgHsUvZmhr6oF3/lD3llzLE2SaOSgOGIsIuAQqgp8JQSUu3r/oOaP8RS44dlQjrH+ALfYtpAECAyUhWCAxnqAfESXOYjKUc2WACuXZ3ch0JHxV0VFrrTyjyjIHXCJYIFnx8H87L4bApR4M+hPcV+fHehEOeW+KCyd0H+WGY8s6')  # noqa
+
 
 @pytest.fixture
 def disable_webauthn_origin_verification(app_, mocker):
@@ -56,3 +59,30 @@ def test_from_registration_encodes_as_unicode(disable_webauthn_origin_verificati
 
     assert type(serialized_credential['credential_data']) == str
     assert type(serialized_credential['registration_response']) == str
+
+
+def test_from_registration_handles_library_errors(app_):
+    # enable origin verification for non-HTTPS test
+    webauthn_server.init_app(app_)
+
+    registration_response = {
+       'clientDataJSON': CLIENT_DATA_JSON,
+       'attestationObject': ATTESTATION_OBJECT,
+    }
+
+    with pytest.raises(RegistrationError) as exc_info:
+        WebAuthnCredential.from_registration(SESSION_STATE, registration_response)
+
+    assert 'Invalid origin' in str(exc_info.value)
+
+
+def test_from_registration_handles_unsupported_keys(disable_webauthn_origin_verification):
+    registration_response = {
+       'clientDataJSON': CLIENT_DATA_JSON,
+       'attestationObject': UNSUPPORTED_ATTESTATION_OBJECT,
+    }
+
+    with pytest.raises(RegistrationError) as exc_info:
+        WebAuthnCredential.from_registration(SESSION_STATE, registration_response)
+
+    assert 'Encryption algorithm not supported' in str(exc_info.value)

tests/javascripts/registerSecurityKey.test.js

@@ -98,9 +98,10 @@ describe('Register security key', () => {
   })
 
   test.each([
-    ['network'],
-    ['server'],
-  ])('alerts if sending WebAuthn credentials fails (%s error)', (errorType, done) => {
+    ['network error'],
+    ['internal server error'],
+    ['bad request'],
+  ])('alerts if sending WebAuthn credentials fails (%s)', (errorType, done) => {
     jest.spyOn(window.navigator.credentials, 'create').mockImplementation(() => {
       // fake PublicKeyCredential response from WebAuthn API
       return Promise.resolve({ response: {} })
@@ -117,10 +118,15 @@ describe('Register security key', () => {
 
       // subsequent POST of credential data to server
       } else {
-        if (errorType == 'network') {
-          return Promise.reject('error')
-        } else {
-          return Promise.resolve({ ok: false, statusText: 'error' })
+        switch (errorType) {
+          case 'network error':
+            return Promise.reject('error')
+          case 'bad request':
+            message = Promise.resolve(window.CBOR.encode('error'))
+            return Promise.resolve({ ok: false, arrayBuffer: () => message })
+          case 'internal server error':
+            message = Promise.reject('encoding error')
+            return Promise.resolve({ ok: false, arrayBuffer: () => message, statusText: 'error' })
         }
       }
     })