[pbs-devel] [PATCH proxmox-backup 0/3] improving webauthn handling

[pbs-devel] [PATCH proxmox-backup 0/3] improving webauthn handling

[Dominik Csapak]

it seems my gui patch for setting the userverification was a bit
hasty, since the rust crate has some options for that

this series reverts the gui part, and sets the backend
to 'discourage' userVerification, since 'Preferred' is not more secure
and makes logging in harder (on some devices)

in the future (when [0] is solved), we could expose a server
setting (either per instance or per user) that sets either always
'Discouraged' or 'Required'

0: https://github.com/kanidm/webauthn-rs/pull/49

Dominik Csapak (3):
  config/tfa: set UserVerificationPolicy to Discouraged
  Revert "ui: window/Settings / WebAuthn: add browser setting for
    userVerificationo"
  config/tfa: webauthn: disallow registering a token twice

 src/config/tfa.rs         | 19 ++++++++++++++++---
 www/LoginView.js          |  5 -----
 www/window/AddWebauthn.js | 14 +++++++-------
 www/window/Settings.js    | 30 +-----------------------------
 4 files changed, 24 insertions(+), 44 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/3] config/tfa: set UserVerificationPolicy to Discouraged

[Dominik Csapak]

the current default is 'Preferred', which is not really useful, as the
(web) client can simply change this to discouraged, since the
webauthn_rs crate does not verify the 'user_verified' bit of the
response in that case

setting this to 'Required' is not really useful either at the moment,
since a user can have a mix of different authenticators that may or
may not support user verification

there is ongoing discussion in the crate how to handle user verification[0]

we could probably expose this setting(discouraged/required) to the user/admin
and save it to the credential and allow only registering credentials
of the same type or filter them out on login (i.e. if there is an
authenticator that can handle userVerification, require it)

in any case, the current default is not helpful for security, but
makes logging in harder, since the key will by default want to verify
the user (if it can)

0: https://github.com/kanidm/webauthn-rs/pull/49

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/config/tfa.rs | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/config/tfa.rs b/src/config/tfa.rs
index 5afb5827..29e0fb48 100644
--- a/src/config/tfa.rs
+++ b/src/config/tfa.rs
@@ -13,7 +13,7 @@ use openssl::pkey::PKey;
 use openssl::sign::Signer;
 use serde::{de::Deserializer, Deserialize, Serialize};
 use serde_json::Value;
-use webauthn_rs::Webauthn;
+use webauthn_rs::{proto::UserVerificationPolicy, Webauthn};
 
 use webauthn_rs::proto::Credential as WebauthnCredential;
 
@@ -804,7 +804,8 @@ impl TfaUserData {
         description: String,
     ) -> Result<String, Error> {
         let userid_str = userid.to_string();
-        let (challenge, state) = webauthn.generate_challenge_register(&userid_str, None)?;
+        let (challenge, state) = webauthn
+            .generate_challenge_register(&userid_str, Some(UserVerificationPolicy::Discouraged))?;
         let challenge_string = challenge.public_key.challenge.to_string();
         let challenge = serde_json::to_string(&challenge)?;
 
@@ -923,7 +924,8 @@ impl TfaUserData {
             return Ok(None);
         }
 
-        let (challenge, state) = webauthn.generate_challenge_authenticate(creds, None)?;
+        let (challenge, state) = webauthn
+            .generate_challenge_authenticate(creds, Some(UserVerificationPolicy::Discouraged))?;
         let challenge_string = challenge.public_key.challenge.to_string();
         let mut data = TfaUserChallengeData::open(userid)?;
         data.inner
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/3] Revert "ui: window/Settings / WebAuthn: add browser setting for userVerificationo"

[Dominik Csapak]

even if the options *could* be set in the frontend, the backend
actually has to do validation of those settings, thus we should not
make that a browser setting

additionally, having the value 'preferred' does not actually make sense,
since it does not add any security (the backend currently skips the
user verification check then)

This reverts commit aca4c2b5a9de23f7cafab92da0f88123f4ca5d8c.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/LoginView.js          |  5 -----
 www/window/AddWebauthn.js |  7 -------
 www/window/Settings.js    | 30 +-----------------------------
 3 files changed, 1 insertion(+), 41 deletions(-)

diff --git a/www/LoginView.js b/www/LoginView.js
index a3ffec77..1c7a977c 100644
--- a/www/LoginView.js
+++ b/www/LoginView.js
@@ -390,11 +390,6 @@ Ext.define('PBS.login.TfaWindow', {
 		// Byte array fixup, keep challenge string:
 		challenge.string = challenge.publicKey.challenge;
 		challenge.publicKey.challenge = PBS.Utils.base64url_to_bytes(challenge.string);
-		let userVerification = Ext.state.Manager.getProvider().get('webauthn-user-verification');
-		if (userVerification !== undefined) {
-		    challenge.publicKey.userVerification = userVerification;
-		}
-
 		for (const cred of challenge.publicKey.allowCredentials) {
 		    cred.id = PBS.Utils.base64url_to_bytes(cred.id);
 		}
diff --git a/www/window/AddWebauthn.js b/www/window/AddWebauthn.js
index d2434f2c..16731a63 100644
--- a/www/window/AddWebauthn.js
+++ b/www/window/AddWebauthn.js
@@ -79,13 +79,6 @@ Ext.define('PBS.window.AddWebauthn', {
 		// string to pass in the response:
 		let challenge_str = challenge_obj.publicKey.challenge;
 		challenge_obj.publicKey.challenge = PBS.Utils.base64url_to_bytes(challenge_str);
-		let userVerification = Ext.state.Manager.getProvider().get('webauthn-user-verification');
-		if (userVerification !== undefined) {
-		    challenge_obj.publicKey.authenticatorSelection = {
-			userVerification,
-		    };
-		}
-
 		challenge_obj.publicKey.user.id =
 		    PBS.Utils.base64url_to_bytes(challenge_obj.publicKey.user.id);
 
diff --git a/www/window/Settings.js b/www/window/Settings.js
index 7059605c..ee8464be 100644
--- a/www/window/Settings.js
+++ b/www/window/Settings.js
@@ -30,9 +30,6 @@ Ext.define('PBS.window.Settings', {
 	    let username = sp.get('login-username') || Proxmox.Utils.noneText;
 	    me.lookupReference('savedUserName').setValue(Ext.String.htmlEncode(username));
 
-	    let userverification= sp.get('webauthn-user-verification') || '__default__';
-	    me.lookupReference('webauthnUserVerification').setValue(userverification);
-
 	    let settings = ['fontSize', 'fontFamily', 'letterSpacing', 'lineHeight'];
 	    settings.forEach(function(setting) {
 		let val = localStorage.getItem('pve-xterm-' + setting);
@@ -94,7 +91,7 @@ Ext.define('PBS.window.Settings', {
 	    },
 	    'button[name=reset]': {
 		click: function() {
-		    let blacklist = ['login-username', 'webauthn-user-verification'];
+		    let blacklist = ['login-username'];
 		    let sp = Ext.state.Manager.getProvider();
 		    for (const state of Object.values(sp.state)) {
 			if (blacklist.indexOf(state) !== -1) {
@@ -117,14 +114,6 @@ Ext.define('PBS.window.Settings', {
 		    sp.clear('login-username');
 		},
 	    },
-	    'field[reference=webauthnUserVerification]': {
-		change: function(e, v) {
-		    if (v === '__default__') {
-			v = undefined;
-		    }
-		    Ext.state.Manager.getProvider().set('webauthn-user-verification', v);
-		},
-	    },
 	},
     },
 
@@ -185,23 +174,6 @@ Ext.define('PBS.window.Settings', {
 		    },
 		],
 	    },
-	    {
-		xtype: 'box',
-		autoEl: { tag: 'hr' },
-	    },
-	    {
-		xtype: 'proxmoxKVComboBox',
-		fieldLabel: gettext('WebAuthn User Verification') + ':',
-		labelWidth: 150,
-		stateId: 'webauthn-user-verification',
-		reference: 'webauthnUserVerification',
-		value: '__default__',
-		comboItems: [
-		    ['__default__', Proxmox.Utils.defaultText],
-		    ['discouraged', gettext('Discouraged')],
-		    ['preferred', gettext('Preferred')],
-		],
-	    },
 	],
     },
     {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/3] config/tfa: webauthn: disallow registering a token twice

[Dominik Csapak]

by adding the existing credential id to the 'excludeCredentials' list

this prevents the browser from registering a token twice, which
lets authentication fail on some browser/token combinations
(e.g. onlykey+chromium)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/config/tfa.rs         | 15 +++++++++++++--
 www/window/AddWebauthn.js |  7 +++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/config/tfa.rs b/src/config/tfa.rs
index 29e0fb48..7c656d20 100644
--- a/src/config/tfa.rs
+++ b/src/config/tfa.rs
@@ -803,9 +803,20 @@ impl TfaUserData {
         userid: &Userid,
         description: String,
     ) -> Result<String, Error> {
+        let cred_ids: Vec<_> = self
+            .enabled_webauthn_entries()
+            .map(|cred| cred.cred_id.clone())
+            .collect();
+
         let userid_str = userid.to_string();
-        let (challenge, state) = webauthn
-            .generate_challenge_register(&userid_str, Some(UserVerificationPolicy::Discouraged))?;
+        let (challenge, state) = webauthn.generate_challenge_register_options(
+            userid_str.as_bytes().to_vec(),
+            userid_str.clone(),
+            userid_str.clone(),
+            Some(cred_ids),
+            Some(UserVerificationPolicy::Discouraged),
+        )?;
+
         let challenge_string = challenge.public_key.challenge.to_string();
         let challenge = serde_json::to_string(&challenge)?;
 
diff --git a/www/window/AddWebauthn.js b/www/window/AddWebauthn.js
index 16731a63..a3888206 100644
--- a/www/window/AddWebauthn.js
+++ b/www/window/AddWebauthn.js
@@ -82,6 +82,13 @@ Ext.define('PBS.window.AddWebauthn', {
 		challenge_obj.publicKey.user.id =
 		    PBS.Utils.base64url_to_bytes(challenge_obj.publicKey.user.id);
 
+		// convert existing authenticators structure
+		challenge_obj.publicKey.excludeCredentials =
+		    (challenge_obj.publicKey.excludeCredentials || []).map((cred) => ({
+			id: PBS.Utils.base64url_to_bytes(cred.id),
+			type: cred.type,
+		    }));
+
 		let msg = Ext.Msg.show({
 		    title: `Webauthn: ${gettext('Setup')}`,
 		    message: gettext('Please press the button on your Webauthn Device'),
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup 3/3] config/tfa: webauthn: disallow registering a token twice

[Thomas Lamprecht]

On 22.02.21 10:43, Dominik Csapak wrote:
> by adding the existing credential id to the 'excludeCredentials' list

But the webauthn does not cares about this, meaning its intended to
work.

> this prevents the browser from registering a token twice, which
> lets authentication fail on some browser/token combinations
> (e.g. onlykey+chromium)

isn't that a FW bug there and should be fixed there?

Would like to avoid such special handling for buggy FW/HW/.. especially
if the workaround is as simple as "just don't register it twice"
(outside of testing I never came to the idea of registering a token
more than once in those accounts I use a fido/u2f token)

> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/config/tfa.rs         | 15 +++++++++++++--
>  www/window/AddWebauthn.js |  7 +++++++
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/src/config/tfa.rs b/src/config/tfa.rs
> index 29e0fb48..7c656d20 100644
> --- a/src/config/tfa.rs
> +++ b/src/config/tfa.rs
> @@ -803,9 +803,20 @@ impl TfaUserData {
>          userid: &Userid,
>          description: String,
>      ) -> Result<String, Error> {
> +        let cred_ids: Vec<_> = self
> +            .enabled_webauthn_entries()
> +            .map(|cred| cred.cred_id.clone())
> +            .collect();
> +
>          let userid_str = userid.to_string();
> -        let (challenge, state) = webauthn
> -            .generate_challenge_register(&userid_str, Some(UserVerificationPolicy::Discouraged))?;
> +        let (challenge, state) = webauthn.generate_challenge_register_options(
> +            userid_str.as_bytes().to_vec(),
> +            userid_str.clone(),
> +            userid_str.clone(),
> +            Some(cred_ids),
> +            Some(UserVerificationPolicy::Discouraged),
> +        )?;
> +
>          let challenge_string = challenge.public_key.challenge.to_string();
>          let challenge = serde_json::to_string(&challenge)?;
>  
> diff --git a/www/window/AddWebauthn.js b/www/window/AddWebauthn.js
> index 16731a63..a3888206 100644
> --- a/www/window/AddWebauthn.js
> +++ b/www/window/AddWebauthn.js
> @@ -82,6 +82,13 @@ Ext.define('PBS.window.AddWebauthn', {
>  		challenge_obj.publicKey.user.id =
>  		    PBS.Utils.base64url_to_bytes(challenge_obj.publicKey.user.id);
>  
> +		// convert existing authenticators structure
> +		challenge_obj.publicKey.excludeCredentials =
> +		    (challenge_obj.publicKey.excludeCredentials || []).map((cred) => ({
> +			id: PBS.Utils.base64url_to_bytes(cred.id),
> +			type: cred.type,
> +		    }));
> +
>  		let msg = Ext.Msg.show({
>  		    title: `Webauthn: ${gettext('Setup')}`,
>  		    message: gettext('Please press the button on your Webauthn Device'),
> 

Re: [pbs-devel] [PATCH proxmox-backup 3/3] config/tfa: webauthn: disallow registering a token twice

[Dominik Csapak]

On 2/22/21 15:08, Thomas Lamprecht wrote:
> On 22.02.21 10:43, Dominik Csapak wrote:
>> by adding the existing credential id to the 'excludeCredentials' list
> 
> But the webauthn does not cares about this, meaning its intended to
> work.

yes it should work, but this option is exactly made for this purpose.

excludeCredentials, of type sequence<PublicKeyCredentialDescriptor>, 
defaulting to []

This member is intended for use by Relying Parties that wish to limit 
the creation of multiple credentials for the same account on a single 
authenticator. The client is requested to return an error if the new 
credential would be created on an authenticator that also contains one 
of the credentials enumerated in this parameter.

the spec recommends it even for having multiple authenticators.

Relying Parties SHOULD allow and encourage users to register multiple 
credentials to the same account.
Relying Parties SHOULD make use of the excludeCredentials and user.id 
options to ensure that these different credentials are bound to 
different authenticators.

> 
>> this prevents the browser from registering a token twice, which
>> lets authentication fail on some browser/token combinations
>> (e.g. onlykey+chromium)
> 
> isn't that a FW bug there and should be fixed there?

it seems that it is actually more a chromium/chrome bug, since
it works with firefox

there seems to be something weird with how chrome sends the
challenges [0][1] though i am not sure i completely understand
the merge request

a discussion on the onlykey forums suggests that the same problem
occurs on solokey and on a yubikey 5 too [0]

the tests we ran before were all with yubikey 4 (afaik)
which do not support fido2 only u2f, which is handled differently
in chromium

> 
> Would like to avoid such special handling for buggy FW/HW/.. especially
> if the workaround is as simple as "just don't register it twice"
> (outside of testing I never came to the idea of registering a token
> more than once in those accounts I use a fido/u2f token)

well the use case of having the same authenticator registered twice
is not really clear to me, and the browser gives a nice
error message that the key is already registered

this avoid a situation where e.g. a user wants to register multiple
tokens (for redundancy/backup) and accidentally uses the
same token twice without noticing

in case he loses the first, there would be no way to login again without
interfering from an admin or using a recovery key, etc)

0: 
https://onlykey.discourse.group/t/multiple-webauthn-registrations-fail/238/10
1: https://github.com/duo-labs/webauthn.io/issues/15
2: https://chromium-review.googlesource.com/c/chromium/src/+/1629587

> 
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   src/config/tfa.rs         | 15 +++++++++++++--
>>   www/window/AddWebauthn.js |  7 +++++++
>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/config/tfa.rs b/src/config/tfa.rs
>> index 29e0fb48..7c656d20 100644
>> --- a/src/config/tfa.rs
>> +++ b/src/config/tfa.rs
>> @@ -803,9 +803,20 @@ impl TfaUserData {
>>           userid: &Userid,
>>           description: String,
>>       ) -> Result<String, Error> {
>> +        let cred_ids: Vec<_> = self
>> +            .enabled_webauthn_entries()
>> +            .map(|cred| cred.cred_id.clone())
>> +            .collect();
>> +
>>           let userid_str = userid.to_string();
>> -        let (challenge, state) = webauthn
>> -            .generate_challenge_register(&userid_str, Some(UserVerificationPolicy::Discouraged))?;
>> +        let (challenge, state) = webauthn.generate_challenge_register_options(
>> +            userid_str.as_bytes().to_vec(),
>> +            userid_str.clone(),
>> +            userid_str.clone(),
>> +            Some(cred_ids),
>> +            Some(UserVerificationPolicy::Discouraged),
>> +        )?;
>> +
>>           let challenge_string = challenge.public_key.challenge.to_string();
>>           let challenge = serde_json::to_string(&challenge)?;
>>   
>> diff --git a/www/window/AddWebauthn.js b/www/window/AddWebauthn.js
>> index 16731a63..a3888206 100644
>> --- a/www/window/AddWebauthn.js
>> +++ b/www/window/AddWebauthn.js
>> @@ -82,6 +82,13 @@ Ext.define('PBS.window.AddWebauthn', {
>>   		challenge_obj.publicKey.user.id =
>>   		    PBS.Utils.base64url_to_bytes(challenge_obj.publicKey.user.id);
>>   
>> +		// convert existing authenticators structure
>> +		challenge_obj.publicKey.excludeCredentials =
>> +		    (challenge_obj.publicKey.excludeCredentials || []).map((cred) => ({
>> +			id: PBS.Utils.base64url_to_bytes(cred.id),
>> +			type: cred.type,
>> +		    }));
>> +
>>   		let msg = Ext.Msg.show({
>>   		    title: `Webauthn: ${gettext('Setup')}`,
>>   		    message: gettext('Please press the button on your Webauthn Device'),
>>
> 

Re: [pbs-devel] [PATCH proxmox-backup 3/3] config/tfa: webauthn: disallow registering a token twice

[Thomas Lamprecht]

On 22.02.21 15:47, Dominik Csapak wrote:
> On 2/22/21 15:08, Thomas Lamprecht wrote:
>> On 22.02.21 10:43, Dominik Csapak wrote:
>>> by adding the existing credential id to the 'excludeCredentials' list
>>
>> But the webauthn does not cares about this, meaning its intended to
>> work.
> 
> yes it should work, but this option is exactly made for this purpose.
> 
> excludeCredentials, of type sequence<PublicKeyCredentialDescriptor>, defaulting to []
> 
> This member is intended for use by Relying Parties that wish to limit the creation of multiple credentials for the same account on a single authenticator. The client is requested to return an error if the new credential would be created on an authenticator that also contains one of the credentials enumerated in this parameter.
> 
> the spec recommends it even for having multiple authenticators.
> 
> Relying Parties SHOULD allow and encourage users to register multiple credentials to the same account.
> Relying Parties SHOULD make use of the excludeCredentials and user.id options to ensure that these different credentials are bound to different authenticators.

hmm, OK

> 
>>
>>> this prevents the browser from registering a token twice, which
>>> lets authentication fail on some browser/token combinations
>>> (e.g. onlykey+chromium)
>>
>> isn't that a FW bug there and should be fixed there?
> 
> it seems that it is actually more a chromium/chrome bug, since
> it works with firefox
> 
> there seems to be something weird with how chrome sends the
> challenges [0][1] though i am not sure i completely understand
> the merge request
> 
> a discussion on the onlykey forums suggests that the same problem
> occurs on solokey and on a yubikey 5 too [0]
> 
> the tests we ran before were all with yubikey 4 (afaik)
> which do not support fido2 only u2f, which is handled differently
> in chromium

for the record, some yubikey 5 got ordered, so we can check that too.

> 
>>
>> Would like to avoid such special handling for buggy FW/HW/.. especially
>> if the workaround is as simple as "just don't register it twice"
>> (outside of testing I never came to the idea of registering a token
>> more than once in those accounts I use a fido/u2f token)
> 
> well the use case of having the same authenticator registered twice
> is not really clear to me, and the browser gives a nice
> error message that the key is already registered

all browser we support? I.e., Firefox and Safari too?

If that's the case the change seems sensible, lets see what Wolfgang
thinks..

> 
> this avoid a situation where e.g. a user wants to register multiple
> tokens (for redundancy/backup) and accidentally uses the
> same token twice without noticing
> 
> in case he loses the first, there would be no way to login again without
> interfering from an admin or using a recovery key, etc)
> 
> 0: https://onlykey.discourse.group/t/multiple-webauthn-registrations-fail/238/10
> 1: https://github.com/duo-labs/webauthn.io/issues/15
> 2: https://chromium-review.googlesource.com/c/chromium/src/+/1629587
> 
>>
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>>   src/config/tfa.rs         | 15 +++++++++++++--
>>>   www/window/AddWebauthn.js |  7 +++++++
>>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/config/tfa.rs b/src/config/tfa.rs
>>> index 29e0fb48..7c656d20 100644
>>> --- a/src/config/tfa.rs
>>> +++ b/src/config/tfa.rs
>>> @@ -803,9 +803,20 @@ impl TfaUserData {
>>>           userid: &Userid,
>>>           description: String,
>>>       ) -> Result<String, Error> {
>>> +        let cred_ids: Vec<_> = self
>>> +            .enabled_webauthn_entries()
>>> +            .map(|cred| cred.cred_id.clone())
>>> +            .collect();
>>> +
>>>           let userid_str = userid.to_string();
>>> -        let (challenge, state) = webauthn
>>> -            .generate_challenge_register(&userid_str, Some(UserVerificationPolicy::Discouraged))?;
>>> +        let (challenge, state) = webauthn.generate_challenge_register_options(
>>> +            userid_str.as_bytes().to_vec(),
>>> +            userid_str.clone(),
>>> +            userid_str.clone(),
>>> +            Some(cred_ids),
>>> +            Some(UserVerificationPolicy::Discouraged),
>>> +        )?;
>>> +
>>>           let challenge_string = challenge.public_key.challenge.to_string();
>>>           let challenge = serde_json::to_string(&challenge)?;
>>>   diff --git a/www/window/AddWebauthn.js b/www/window/AddWebauthn.js
>>> index 16731a63..a3888206 100644
>>> --- a/www/window/AddWebauthn.js
>>> +++ b/www/window/AddWebauthn.js
>>> @@ -82,6 +82,13 @@ Ext.define('PBS.window.AddWebauthn', {
>>>           challenge_obj.publicKey.user.id =
>>>               PBS.Utils.base64url_to_bytes(challenge_obj.publicKey.user.id);
>>>   +        // convert existing authenticators structure
>>> +        challenge_obj.publicKey.excludeCredentials =
>>> +            (challenge_obj.publicKey.excludeCredentials || []).map((cred) => ({
>>> +            id: PBS.Utils.base64url_to_bytes(cred.id),
>>> +            type: cred.type,
>>> +            }));
>>> +
>>>           let msg = Ext.Msg.show({
>>>               title: `Webauthn: ${gettext('Setup')}`,
>>>               message: gettext('Please press the button on your Webauthn Device'),
>>>
>>
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>