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

[pbs-devel] [PATCH proxmox-backup v2 0/4] 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'

changes from v1:
* show webauthn errors on login
* explicitly handle register errors, and try to give a meaningful message
  for errors that indicate a duplicate authenticator

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

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

 src/config/tfa.rs         | 19 ++++++++++++++++---
 www/LoginView.js          | 19 ++++++++++++++-----
 www/window/AddWebauthn.js | 34 ++++++++++++++++++++++++++--------
 www/window/Settings.js    | 30 +-----------------------------
 4 files changed, 57 insertions(+), 45 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 1/4] 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 that[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 loggin in harder, since the key will by default want to verify
the user

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 v2 2/4] 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 skips the
userverification 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 v2 3/4] 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/solokey+chromium)
while is seems this is currently a bug in chromium, in a future spec
update the underlying behaviour should be better defined, making this
an authenticator bug

also explicitly catch registering errors and show appropriate error messages

0: https://bugs.chromium.org/p/chromium/issues/detail?id=1087642

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/config/tfa.rs         | 15 +++++++++++++--
 www/window/AddWebauthn.js | 27 ++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 3 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..f6d1df61 100644
--- a/www/window/AddWebauthn.js
+++ b/www/window/AddWebauthn.js
@@ -82,13 +82,38 @@ 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'),
 		    buttons: [],
 		});
 
-		let token_response = await navigator.credentials.create(challenge_obj);
+		let token_response;
+		try {
+		    token_response = await navigator.credentials.create(challenge_obj);
+		} catch (error) {
+		    let errmsg = `<i class="fa fa-warning warning"></i>
+			${error.name}: ${error.message}`;
+		    if (error.name === 'InvalidStateError') {
+			// probably a duplicate token
+			throw `${gettext('There was an error during authenticator registration.')}
+			    <br>
+			    ${gettext('This probably means that this authenticator is already registered.')}
+			    <br><br>
+			    ${errmsg}`;
+		    } else {
+			throw `${gettext('There was an error during token registration.')}
+			    <br><br>
+			    ${errmsg}`;
+		    }
+		}
 
 		// We cannot pass ArrayBuffers to the API, so extract & convert the data.
 		let response = {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 4/4] ui: LoginView: show webauthn errors in window

[Dominik Csapak]

instead of silently discarding the error, else the user might be
confused because nothing happened

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/LoginView.js | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/www/LoginView.js b/www/LoginView.js
index 1c7a977c..6dd09646 100644
--- a/www/LoginView.js
+++ b/www/LoginView.js
@@ -383,6 +383,7 @@ Ext.define('PBS.login.TfaWindow', {
 	    let view = me.getView();
 
 	    me.lookup('webAuthnWaiting').setVisible(true);
+	    me.lookup('webAuthnError').setVisible(false);
 
 	    let challenge = view.challenge.webauthn;
 
@@ -409,6 +410,10 @@ Ext.define('PBS.login.TfaWindow', {
 		// checking for error.code === DOMException.ABORT_ERR only works in firefox -.-
 		this.getViewModel().set('canConfirm', true);
 		// FIXME: better handling, show some message, ...?
+		me.lookup('webAuthnError').setData({
+		    error: Ext.htmlEncode(error.toString()),
+		});
+		me.lookup('webAuthnError').setVisible(true);
 		return;
 	    } finally {
 		let waitingMessage = me.lookup('webAuthnWaiting');
@@ -505,6 +510,15 @@ Ext.define('PBS.login.TfaWindow', {
 			reference: 'webAuthnWaiting',
 			hidden: true,
 		    },
+		    {
+			xtype: 'box',
+			data: {
+			    error: '',
+			},
+			tpl: '<i class="fa fa-warning warning"></i> {error}',
+			reference: 'webAuthnError',
+			hidden: true,
+		    },
 		],
 	    },
 	    {
-- 
2.20.1

[pbs-devel] applied-series: [PATCH proxmox-backup v2 0/4] improving webauthn handling

[Thomas Lamprecht]

On 25.02.21 10:01, Dominik Csapak wrote:
> 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'
> 
> changes from v1:
> * show webauthn errors on login
> * explicitly handle register errors, and try to give a meaningful message
>   for errors that indicate a duplicate authenticator
> 
> 0: https://github.com/kanidm/webauthn-rs/pull/49
> 
> Dominik Csapak (4):
>   config/tfa: set UserVerificationPolicy to Discouraged
>   Revert "ui: window/Settings / WebAuthn: add browser setting for
>     userVerificationo"
>   config/tfa: webauthn: disallow registering a token twice
>   ui: LoginView: show webauthn errors in window
> 
>  src/config/tfa.rs         | 19 ++++++++++++++++---
>  www/LoginView.js          | 19 ++++++++++++++-----
>  www/window/AddWebauthn.js | 34 ++++++++++++++++++++++++++--------
>  www/window/Settings.js    | 30 +-----------------------------
>  4 files changed, 57 insertions(+), 45 deletions(-)
> 



applied series, with a followup for the exception message as talked off-list, thanks!