Skip to content

Commit

Permalink
Passkeys improvements (keepassxreboot#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 keepassxreboot#10287.
  • Loading branch information
varjolintu authored Mar 6, 2024
1 parent dff2f18 commit ac2b445
Show file tree
Hide file tree
Showing 33 changed files with 1,248 additions and 269 deletions.
48 changes: 44 additions & 4 deletions share/translations/keepassxc_en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -976,6 +976,10 @@ Do you want to overwrite the Passkey in %1 - %2?</source>
<source>KeePassXC - New key association request</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Passkey</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>BrowserSettingsWidget</name>
Expand Down Expand Up @@ -1222,6 +1226,14 @@ Do you want to overwrite the Passkey in %1 - %2?</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 @@ -8419,10 +8431,6 @@ Kernel: %3 %4</source>
<source>Invalid URL provided</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Resident Keys are not supported</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Passkeys</source>
<translation type="unfinished"></translation>
Expand Down Expand Up @@ -8483,6 +8491,38 @@ Kernel: %3 %4</source>
<source>Failed to decrypt key data.</source>
<translation type="unfinished"></translation>
</message>
<message>
<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>
</context>
<context>
<name>QtIOCompressor</name>
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 @@ -541,7 +542,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 @@ -574,8 +575,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
18 changes: 16 additions & 2 deletions src/browser/BrowserMessageBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,22 @@ QString BrowserMessageBuilder::getErrorMessage(const int errorCode) const
return QObject::tr("Empty public key");
case ERROR_PASSKEYS_INVALID_URL_PROVIDED:
return QObject::tr("Invalid URL provided");
case ERROR_PASSKEYS_RESIDENT_KEYS_NOT_SUPPORTED:
return QObject::tr("Resident Keys are not supported");
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
11 changes: 9 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 @@ -55,7 +55,14 @@ namespace
ERROR_PASSKEYS_INVALID_USER_VERIFICATION = 23,
ERROR_PASSKEYS_EMPTY_PUBLIC_KEY = 24,
ERROR_PASSKEYS_INVALID_URL_PROVIDED = 25,
ERROR_PASSKEYS_RESIDENT_KEYS_NOT_SUPPORTED = 26,
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 ac2b445

Please sign in to comment.