Skip to content

Commit

Permalink
Passkeys improvements (#10318)
Browse files Browse the repository at this point in the history
Refactors the Passkey implementation to include more checks and a structure that is more aligned with the official specification.
Notable changes:
- _BrowserService_ no longer does the checks by itself. A new class _BrowserPasskeysClient_ constructs the relevant objects, acting as a client. _BrowserService_ only acts as a bridge between the client and _BrowserPasskeys_ (authenticator) and calls the relevant popups for user interaction.
- A new helper class _PasskeyUtils_ includes the actual checks and parses the objects.
- _BrowserPasskeys_ is pretty much intact, but some functions have been moved to PasskeyUtils.
- Fixes Ed25519 encoding in _BrowserCBOR_.
- Adds new error messages.
- User confirmation for Passkey retrieval is also asked even if `discouraged` is used. This goes against the specification, but currently there's no other way to verify the user.
- `cross-platform` is also accepted for compatibility. This could be removed if there's a potential issue with it.
- Extension data is now handled correctly during Authentication.
- Allowed and excluded credentials are now handled correctly.
- `KPEX_PASSKEY_GENERATED_USER_ID` is renamed to `KPEX_PASSKEY_CREDENTIAL_ID`
- Adds a new option "Allow localhost with Passkeys" to Browser Integration -> Advanced tab. By default it's not allowed to access HTTP sites, but `http://localhost` can be allowed for debugging and testing purposes for local servers.
- Add tag `Passkey` to a Passkey entry, or an entry with an imported Passkey.

Fixes #10287.
  • Loading branch information
droidmonkey committed Mar 7, 2024
1 parent 067deb9 commit 717b465
Show file tree
Hide file tree
Showing 33 changed files with 1,298 additions and 264 deletions.
86 changes: 79 additions & 7 deletions share/translations/keepassxc_en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -972,6 +972,10 @@ Do you want to overwrite the Passkey in %1 - %2?</source>
<source>Converting attributes to custom data…</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Passkey</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Abort</source>
<translation type="unfinished"></translation>
Expand Down Expand Up @@ -1252,6 +1256,14 @@ Would you like to migrate your existing settings now?</source>
<source>&lt;b&gt;Error:&lt;/b&gt; The installed proxy executable is missing from the expected location: %1&lt;br/&gt;Please set a custom proxy location in the advanced settings or reinstall the application.</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Allows using insecure http://localhost with Passkeys for testing purposes.</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Allow using localhost with Passkeys</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>CloneDialog</name>
Expand Down Expand Up @@ -8208,11 +8220,7 @@ Kernel: %3 %4</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Cannot remove password: The database does not have a password.</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Cannot remove file key: The database does not have a file key.</source>
<source>Passkeys</source>
<translation type="unfinished"></translation>
</message>
<message>
Expand All @@ -8229,11 +8237,75 @@ This options is deprecated, use --set-key-file instead.</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Passkeys</source>
<source>allow screenshots and app recording (Windows/macOS)</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>allow screenshots and app recording (Windows/macOS)</source>
<source>Origin is empty or not allowed</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Effective domain is not a valid domain</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Origin and RP ID do not match</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>No supported algorithms were provided</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Wait for timer to expire</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Unknown Passkeys error</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Challenge is shorter than required minimum length</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>user.id does not match the required length</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Access to all entries is denied</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Attestation not supported</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Credential is excluded</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Passkeys request canceled</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Invalid user verification</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Empty public key</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Invalid URL provided</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Cannot remove password: The database does not have a password.</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Cannot remove file key: The database does not have a file key.</source>
<translation type="unfinished"></translation>
</message>
</context>
Expand Down
9 changes: 5 additions & 4 deletions src/browser/BrowserAction.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2023 KeePassXC Team <[email protected]>
* Copyright (C) 2024 KeePassXC Team <[email protected]>
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
Expand All @@ -19,6 +19,7 @@
#include "BrowserMessageBuilder.h"
#ifdef WITH_XC_BROWSER_PASSKEYS
#include "BrowserPasskeys.h"
#include "PasskeyUtils.h"
#endif
#include "BrowserSettings.h"
#include "core/Global.h"
Expand Down Expand Up @@ -507,7 +508,7 @@ QJsonObject BrowserAction::handlePasskeysGet(const QJsonObject& json, const QStr
}

const auto origin = browserRequest.getString("origin");
if (!origin.startsWith("https://")) {
if (!passkeyUtils()->isOriginAllowedWithLocalhost(browserSettings()->allowLocalhostWithPasskeys(), origin)) {
return getErrorReply(action, ERROR_PASSKEYS_INVALID_URL_PROVIDED);
}

Expand Down Expand Up @@ -540,8 +541,8 @@ QJsonObject BrowserAction::handlePasskeysRegister(const QJsonObject& json, const
}

const auto origin = browserRequest.getString("origin");
if (!origin.startsWith("https://")) {
return getErrorReply(action, ERROR_KEEPASS_ACTION_CANCELLED_OR_DENIED);
if (!passkeyUtils()->isOriginAllowedWithLocalhost(browserSettings()->allowLocalhostWithPasskeys(), origin)) {
return getErrorReply(action, ERROR_PASSKEYS_INVALID_URL_PROVIDED);
}

const auto keyList = getConnectionKeys(browserRequest);
Expand Down
5 changes: 5 additions & 0 deletions src/browser/BrowserAction.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ struct BrowserRequest
return decrypted.value(param).toArray();
}

inline bool getBool(const QString& param) const
{
return decrypted.value(param).toBool();
}

inline QJsonObject getObject(const QString& param) const
{
return decrypted.value(param).toObject();
Expand Down
42 changes: 28 additions & 14 deletions src/browser/BrowserCbor.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2023 KeePassXC Team <[email protected]>
* Copyright (C) 2024 KeePassXC Team <[email protected]>
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
Expand Down Expand Up @@ -48,6 +48,16 @@ QByteArray BrowserCbor::cborEncodeAttestation(const QByteArray& authData) const
// https://w3c.github.io/webauthn/#authdata-attestedcredentialdata-credentialpublickey
QByteArray BrowserCbor::cborEncodePublicKey(int alg, const QByteArray& first, const QByteArray& second) const
{
const auto keyType = getCoseKeyType(alg);
if (keyType == 0) {
return {};
}

const auto curveParameter = getCurveParameter(alg);
if ((alg == WebAuthnAlgorithms::ES256 || alg == WebAuthnAlgorithms::EDDSA) && curveParameter == 0) {
return {};
}

QByteArray result;
QCborStreamWriter writer(&result);

Expand All @@ -56,15 +66,15 @@ QByteArray BrowserCbor::cborEncodePublicKey(int alg, const QByteArray& first, co

// Key type
writer.append(1);
writer.append(getCoseKeyType(alg));
writer.append(keyType);

// Signature algorithm
writer.append(3);
writer.append(alg);

// Curve parameter
writer.append(-1);
writer.append(getCurveParameter(alg));
writer.append(curveParameter);

// Key x-coordinate
writer.append(-2);
Expand All @@ -80,7 +90,7 @@ QByteArray BrowserCbor::cborEncodePublicKey(int alg, const QByteArray& first, co

// Key type
writer.append(1);
writer.append(getCoseKeyType(alg));
writer.append(keyType);

// Signature algorithm
writer.append(3);
Expand All @@ -96,21 +106,24 @@ QByteArray BrowserCbor::cborEncodePublicKey(int alg, const QByteArray& first, co

writer.endMap();
} else if (alg == WebAuthnAlgorithms::EDDSA) {
// https://www.rfc-editor.org/rfc/rfc8152#section-13.2
writer.startMap(3);
writer.startMap(4);

// Key type
writer.append(1);
writer.append(keyType);

// Algorithm
writer.append(3);
writer.append(alg);

// Curve parameter
writer.append(-1);
writer.append(getCurveParameter(alg));
writer.append(curveParameter);

// Public key
writer.append(-2);
writer.append(first);

// Private key
writer.append(-4);
writer.append(second);

writer.endMap();
}

Expand Down Expand Up @@ -230,7 +243,7 @@ unsigned int BrowserCbor::getCurveParameter(int alg) const
case WebAuthnAlgorithms::EDDSA:
return WebAuthnCurveKey::ED25519;
default:
return WebAuthnCurveKey::P256;
return WebAuthnCurveKey::INVALID_CURVE_KEY;
}
}

Expand All @@ -240,14 +253,15 @@ unsigned int BrowserCbor::getCoseKeyType(int alg) const
{
switch (alg) {
case WebAuthnAlgorithms::ES256:
return WebAuthnCoseKeyType::EC2;
case WebAuthnAlgorithms::ES384:
case WebAuthnAlgorithms::ES512:
return WebAuthnCoseKeyType::EC2;
return WebAuthnCoseKeyType::INVALID_COSE_KEY_TYPE;
case WebAuthnAlgorithms::EDDSA:
return WebAuthnCoseKeyType::OKP;
case WebAuthnAlgorithms::RS256:
return WebAuthnCoseKeyType::RSA;
default:
return WebAuthnCoseKeyType::EC2;
return WebAuthnCoseKeyType::INVALID_COSE_KEY_TYPE;
}
}
4 changes: 3 additions & 1 deletion src/browser/BrowserCbor.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2023 KeePassXC Team <[email protected]>
* Copyright (C) 2024 KeePassXC Team <[email protected]>
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
Expand Down Expand Up @@ -35,6 +35,7 @@ enum WebAuthnAlgorithms : int
// https://www.rfc-editor.org/rfc/rfc9053#section-7.1
enum WebAuthnCurveKey : int
{
INVALID_CURVE_KEY = 0,
P256 = 1, // EC2, NIST P-256, also known as secp256r1
P384 = 2, // EC2, NIST P-384, also known as secp384r1
P521 = 3, // EC2, NIST P-521, also known as secp521r1
Expand All @@ -48,6 +49,7 @@ enum WebAuthnCurveKey : int
// For RSA: https://www.rfc-editor.org/rfc/rfc8230#section-4
enum WebAuthnCoseKeyType : int
{
INVALID_COSE_KEY_TYPE = 0,
OKP = 1, // Octet Keypair
EC2 = 2, // Elliptic Curve
RSA = 3 // RSA
Expand Down
30 changes: 30 additions & 0 deletions src/browser/BrowserMessageBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,36 @@ QString BrowserMessageBuilder::getErrorMessage(const int errorCode) const
return QObject::tr("Cannot create new group");
case ERROR_KEEPASS_NO_VALID_UUID_PROVIDED:
return QObject::tr("No valid UUID provided");
case ERROR_KEEPASS_ACCESS_TO_ALL_ENTRIES_DENIED:
return QObject::tr("Access to all entries is denied");
case ERROR_PASSKEYS_ATTESTATION_NOT_SUPPORTED:
return QObject::tr("Attestation not supported");
case ERROR_PASSKEYS_CREDENTIAL_IS_EXCLUDED:
return QObject::tr("Credential is excluded");
case ERROR_PASSKEYS_REQUEST_CANCELED:
return QObject::tr("Passkeys request canceled");
case ERROR_PASSKEYS_INVALID_USER_VERIFICATION:
return QObject::tr("Invalid user verification");
case ERROR_PASSKEYS_EMPTY_PUBLIC_KEY:
return QObject::tr("Empty public key");
case ERROR_PASSKEYS_INVALID_URL_PROVIDED:
return QObject::tr("Invalid URL provided");
case ERROR_PASSKEYS_ORIGIN_NOT_ALLOWED:
return QObject::tr("Origin is empty or not allowed");
case ERROR_PASSKEYS_DOMAIN_IS_NOT_VALID:
return QObject::tr("Effective domain is not a valid domain");
case ERROR_PASSKEYS_DOMAIN_RPID_MISMATCH:
return QObject::tr("Origin and RP ID do not match");
case ERROR_PASSKEYS_NO_SUPPORTED_ALGORITHMS:
return QObject::tr("No supported algorithms were provided");
case ERROR_PASSKEYS_WAIT_FOR_LIFETIMER:
return QObject::tr("Wait for timer to expire");
case ERROR_PASSKEYS_UNKNOWN_ERROR:
return QObject::tr("Unknown Passkeys error");
case ERROR_PASSKEYS_INVALID_CHALLENGE:
return QObject::tr("Challenge is shorter than required minimum length");
case ERROR_PASSKEYS_INVALID_USER_ID:
return QObject::tr("user.id does not match the required length");
default:
return QObject::tr("Unknown error");
}
Expand Down
12 changes: 10 additions & 2 deletions src/browser/BrowserMessageBuilder.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2023 KeePassXC Team <[email protected]>
* Copyright (C) 2024 KeePassXC Team <[email protected]>
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
Expand Down Expand Up @@ -54,7 +54,15 @@ namespace
ERROR_PASSKEYS_REQUEST_CANCELED = 22,
ERROR_PASSKEYS_INVALID_USER_VERIFICATION = 23,
ERROR_PASSKEYS_EMPTY_PUBLIC_KEY = 24,
ERROR_PASSKEYS_INVALID_URL_PROVIDED = 25
ERROR_PASSKEYS_INVALID_URL_PROVIDED = 25,
ERROR_PASSKEYS_ORIGIN_NOT_ALLOWED = 26,
ERROR_PASSKEYS_DOMAIN_IS_NOT_VALID = 27,
ERROR_PASSKEYS_DOMAIN_RPID_MISMATCH = 28,
ERROR_PASSKEYS_NO_SUPPORTED_ALGORITHMS = 29,
ERROR_PASSKEYS_WAIT_FOR_LIFETIMER = 30,
ERROR_PASSKEYS_UNKNOWN_ERROR = 31,
ERROR_PASSKEYS_INVALID_CHALLENGE = 32,
ERROR_PASSKEYS_INVALID_USER_ID = 33,
};
}

Expand Down
Loading

0 comments on commit 717b465

Please sign in to comment.