Add security key support #496

Sneezry · 2020-06-08T15:43:10Z

No description provided.

mymindstorm

Please ignore all the other review comments and the TODOs below for now.

This just encrypts the user's password directly with the credential id. The credential id isn't secure (I.e. we can't guarantee that it's random data). We can sign and verify data, but webauthn does not provide any way to securely encrypt data (it looks as if only client-server flows were considered). It seems to be possible using native code, but that would probably be too complicated from a user's perspective. I found lots of good discussion on encryption using fido keys below. It looks like our best bet would be to propose changes to the WebAuthn API...

https://groups.google.com/a/fidoalliance.org/d/topic/fido-dev/kMe6Kb4GCpU/discussion
https://bugs.chromium.org/p/chromium/issues/detail?id=1023225#c7
https://bugs.chromium.org/p/chromium/issues/detail?id=1023225#c12
https://fidoalliance.org/specs/fido-v2.0-ps-20190130/fido-client-to-authenticator-protocol-v2.0-ps-20190130.html#sctn-hmac-secret-extension

Old review comments

WebAuthn supports linux
check if webauthn is supported on the browser before showing the button https://caniuse.com/#search=webauthn / MDN
~~Can we store the securityTokenEncryptedKey in key so that it is stored in backups? How will backups work with this in general~~

mymindstorm · 2020-06-08T15:47:06Z

_locales/en/messages.json

@@ -353,5 +353,20 @@
    },
    "algorithm": {
        "message": "Algorithm"
+    },
+    "operationFailed": {
+        "message": "Operation failed."


Crowdin is going to flood my email again :(

use updateFailure

mymindstorm · 2020-06-08T15:48:15Z

_locales/en/messages.json

+        "message": "Operation failed."
+    },
+    "noPassphraseWarning": {
+        "message": "You must set a passphrase before enable security key."


Suggested change

"message": "You must set a passphrase before enable security key."

"message": "You must set a password before adding a security key."

mymindstorm · 2020-06-08T15:48:31Z

_locales/en/messages.json

+        "message": "You must set a passphrase before enable security key."
+    },
+    "securityKeyEnabled": {
+        "message": "Security key enabled."


Suggested change

"message": "Security key enabled."

"message": "Security key added."

mymindstorm · 2020-06-08T19:57:49Z

src/components/Popup/EnterPasswordPage.vue

@@ -29,12 +40,31 @@ export default Vue.extend({
  computed: {
    wrongPassword() {
      return this.$store.state.accounts.wrongPassword;
+    },
+    securityKeyEnabled() {
+      return !!localStorage.securityTokenEncryptedKey;


!!localStorage.securityTokenEncryptedKey always has bool value

mymindstorm · 2020-06-08T20:18:14Z

_locales/en/messages.json

+        "message": "Security key enabled."
+    },
+    "alreadyEnabledSecurityKeyWarning": {
+        "message": "You have already enabled security key, do you want to continue?"


Suggested change

"message": "You have already enabled security key, do you want to continue?"

"message": "Your old security key will not work, are you sure you want to continue?"

mymindstorm · 2020-06-08T20:45:42Z

src/store/Accounts.ts

+              user: {
+                name: "Authenticator",
+                displayName: "Authenticator",
+                id: new Uint8Array(16)


Is it really ok for us to just have blank ids?

I thought it will be some random chars without setting a value just like C language. I will update to use a random string.

mymindstorm · 2020-06-08T20:46:08Z

src/store/Accounts.ts

+                displayName: "Authenticator",
+                id: new Uint8Array(16)
+              },
+              challenge: new Uint8Array(16),


this should have some random data. we should verify that the key is working and response is signed properly

We don't care about the credential, we are only intereted in credential id

mymindstorm · 2020-06-08T20:48:48Z

src/store/Accounts.ts

+                { type: "public-key", alg: -7 },
+                { type: "public-key", alg: -35 },
+                { type: "public-key", alg: -36 },
+                { type: "public-key", alg: -257 },


This is deprecated

https://www.iana.org/assignments/cose/cose.xhtml#algorithms

It's the only supported algorithm for Windows Hello. https://docs.microsoft.com/en-us/microsoft-edge/dev-guide/windows-integration/web-authentication

mymindstorm · 2020-06-08T20:49:07Z

src/store/Accounts.ts

+                { type: "public-key", alg: -35 },
+                { type: "public-key", alg: -36 },
+                { type: "public-key", alg: -257 },
+                { type: "public-key", alg: -258 },


Not recommended

https://www.iana.org/assignments/cose/cose.xhtml#algorithms

mymindstorm · 2020-06-08T20:49:18Z

src/store/Accounts.ts

+                { type: "public-key", alg: -36 },
+                { type: "public-key", alg: -257 },
+                { type: "public-key", alg: -258 },
+                { type: "public-key", alg: -259 },


Not recommended

https://www.iana.org/assignments/cose/cose.xhtml#algorithms

mymindstorm · 2020-06-08T21:49:14Z

There is already a draft: w3c/webauthn#1424

Sneezry · 2020-06-09T14:04:08Z

This just encrypts the user's password directly with the credential id.

Correct. Credential id is generated with the hardware, and it's value can be gotten only when the user touch the figure scanner or type the PIN. Though WebAuthn doesn't provide a way to encrypt data, but we also never expose the credential id, so I don't think there is a risk.

It seems to be possible using native code

No, we shouldn't touch native code. It's beyond our scope. And we can't maintain native code for all platforms.

mymindstorm · 2020-06-09T19:59:43Z

Correct. Credential id is generated with the hardware, and it's value can be gotten only when the user touch the figure scanner or type the PIN.

Unfortunately, this is not the case. There is nothing preventing a vendor from making the id a hash of publicly known information or a serial that is incremented every time a new credential is generated. In either case an attacker would be able to get plaintext user password with current implementation.

The below email thread is a good example of a security key vendor about to do that. It's worth noting that the Yubico dev says that it's completely up to the vendor how this is implemented.

https://groups.google.com/a/fidoalliance.org/d/topic/fido-dev/5mwvgna3RKc/discussion

Below is a link to the spec, the "key handle" is the rawId. Same as above, there is nothing explicitly preventing an easily guessable id.

https://fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-raw-message-formats-v1.1-id-20160915.html#registration-response-message-success

Though WebAuthn doesn't provide a way to encrypt data, but we also never expose the credential id, so I don't think there is a risk.

As I understand it, normally you are supposed to store the credential id somewhere and use it when calling credentials.get. Without storing the id, there is no guarantee that we will get the same credential if the user ever regenerates their key.

I recently found that some browsers already have an hmacCreateSecret extension we could use to do this in a secure way. It seems that it was made specifically for Windows Hello support. What do you think about using that?

https://groups.google.com/a/fidoalliance.org/d/topic/fido-dev/SX6AoLAoOx8/discussion

Sneezry · 2020-06-10T13:23:18Z

Unfortunately, this is not the case...

There isn't a standard for how to generate credential id hash, so the attacker also has no idea how to generate the credential id with RP id. Also, user id is a random number that only use once when we register the device. We don't store user id, either. The way we use webauthn here is very different from how it is designed. We don't store anything but the encrypted secret with the credential id. The only thing can be known without customer's permission is RP id.

As I understand it, normally you are supposed to store the credential id somewhere...

The way we use webauthn here is very different from how it is designed, we don't store the credential id. The credential id is stored in the hardware security chip. Instead of using allowCredentials, which requires the credential id, we just list all credentials match our RP id, and ask customer to choose the correct one - even if the customer has the correct PIN, if the chosen credential is not the one to encrypt the password, decyption will fail (for example, customer use Windows Hello to encrypt data, however, chose another hardward security token to decrypt, will meet an error).

I understand webauthn is not designed for encryption. However, if the credential id cannot be read without customer's permission, it can be a good key to protect the data.

mymindstorm · 2020-06-13T22:28:04Z

I have thought about this for the past few days, and I am still not comfortable with this. I do not think that using a standard in a non-intended way is a smart move when dealing with sensitive user data. We do not have any guarantees nor can we make any assumptions about the entropy of the key handle.

If you still want to move forward with this and keep the same encryption scheme, we should send an email to the fido-dev mailing list and get an opinion from the implementors and developers of the spec.

Sneezry · 2020-06-14T11:28:51Z

I do not think that using a standard in a non-intended way is a smart move when dealing with sensitive user data.

I totally agree with you.

If you still want to move forward with this and keep the same encryption scheme...

We have no need to move forward in such a hurry. This is not an urgent or required feature. I just publish it for review and discussion. Let's make everything reliable and ready before ship it.

Sneezry · 2020-07-01T15:28:13Z

I know that HMAC secret description is misleading, but you can not use it for "encryption". The extension is generating HMAC key, scoped to the credential. During GetAssertion you can provide HMAC secret extensions with one or two salts, and you will be given HMAC for each of the salts. Platform can leverage that to do offline authentication by rolling salts.

And yes, browser are not supporting it.

Pleaser read HMAC secret extension definition properly https://fidoalliance.org/specs/fido-v2.0-id-20180227/fido-client-to-authenticator-protocol-v2.0-id-20180227.html#sctn-hmac-secret-extension.

https://groups.google.com/a/fidoalliance.org/d/msg/fido-dev/kMe6Kb4GCpU/EZjN19qhBQAJ

It seems HMAC extension is not what we want

mymindstorm · 2020-07-02T17:54:52Z

I missed that, I was mistaken. After re-reading that thread, it is clear that we should not be using WebAuthn for encryption at all.

Sneezry · 2020-07-03T13:20:10Z

Correct. We have to wait for new web standard for data encryption scenario. I'm closing this PR, and keep an eye on the change in W3C org.

Sneezry added 3 commits June 2, 2020 00:34

add security key

cfad8a4

remove secrity key when remove passphrase

50bbfac

disable security key when update passphrase

27a2aca

Sneezry requested a review from mymindstorm June 8, 2020 15:43

mymindstorm requested changes Jun 8, 2020

View reviewed changes

Sneezry closed this Jul 3, 2020

Sneezry mentioned this pull request Jan 26, 2021

Unlock Authenticator with U2F #586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add security key support #496

Add security key support #496

Sneezry commented Jun 8, 2020

mymindstorm left a comment •

edited

Loading

mymindstorm Jun 8, 2020

mymindstorm Jun 8, 2020

mymindstorm Jun 8, 2020

mymindstorm Jun 8, 2020

Sneezry Jun 10, 2020

mymindstorm Jun 8, 2020

mymindstorm Jun 8, 2020

Sneezry Jun 10, 2020 •

edited

Loading

mymindstorm Jun 8, 2020 •

edited

Loading

Sneezry Jun 10, 2020

mymindstorm Jun 8, 2020

Sneezry Jun 10, 2020

mymindstorm Jun 8, 2020

mymindstorm Jun 8, 2020

mymindstorm commented Jun 8, 2020

Sneezry commented Jun 9, 2020 •

edited

Loading

mymindstorm commented Jun 9, 2020 •

edited

Loading

Sneezry commented Jun 10, 2020 •

edited

Loading

mymindstorm commented Jun 13, 2020

Sneezry commented Jun 14, 2020

Sneezry commented Jul 1, 2020

mymindstorm commented Jul 2, 2020

Sneezry commented Jul 3, 2020

	"message": "You must set a passphrase before enable security key."
	"message": "You must set a password before adding a security key."

	"message": "Security key enabled."
	"message": "Security key added."

	"message": "You have already enabled security key, do you want to continue?"
	"message": "Your old security key will not work, are you sure you want to continue?"

Add security key support #496

Add security key support #496

Conversation

Sneezry commented Jun 8, 2020

mymindstorm left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sneezry Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

mymindstorm Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mymindstorm commented Jun 8, 2020

Sneezry commented Jun 9, 2020 • edited Loading

mymindstorm commented Jun 9, 2020 • edited Loading

Sneezry commented Jun 10, 2020 • edited Loading

mymindstorm commented Jun 13, 2020

Sneezry commented Jun 14, 2020

Sneezry commented Jul 1, 2020

mymindstorm commented Jul 2, 2020

Sneezry commented Jul 3, 2020

mymindstorm left a comment •

edited

Loading

Sneezry Jun 10, 2020 •

edited

Loading

mymindstorm Jun 8, 2020 •

edited

Loading

Sneezry commented Jun 9, 2020 •

edited

Loading

mymindstorm commented Jun 9, 2020 •

edited

Loading

Sneezry commented Jun 10, 2020 •

edited

Loading