1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-14 15:23:33 +00:00

[EC-598] fix: invalid signature due to double hashing

This commit is contained in:
Andreas Coroiu
2023-03-31 17:45:59 +02:00
parent 7039ad1870
commit bdadf68274
3 changed files with 45 additions and 26 deletions

View File

@@ -34,19 +34,6 @@ describe("FidoAuthenticatorService", () => {
cipherService = mock<CipherService>(); cipherService = mock<CipherService>();
userInterface = mock<Fido2UserInterfaceService>(); userInterface = mock<Fido2UserInterfaceService>();
authenticator = new Fido2AuthenticatorService(cipherService, userInterface); authenticator = new Fido2AuthenticatorService(cipherService, userInterface);
// crypto.subtle.importKey doesn't work properly in jest, so we need to mock it with new keys.
const privateKey = (
await crypto.subtle.generateKey(
{
name: "ECDSA",
namedCurve: "P-256",
},
true,
["sign"]
)
).privateKey;
crypto.subtle.importKey = jest.fn().mockResolvedValue(privateKey);
}); });
describe("makeCredential", () => { describe("makeCredential", () => {
@@ -648,16 +635,24 @@ describe("FidoAuthenticatorService", () => {
describe(`assertion of ${ describe(`assertion of ${
residentKey ? "discoverable" : "non-discoverable" residentKey ? "discoverable" : "non-discoverable"
} credential`, () => { } credential`, () => {
let keyPair: CryptoKeyPair;
let credentialIds: string[]; let credentialIds: string[];
let selectedCredentialId: string; let selectedCredentialId: string;
let ciphers: CipherView[]; let ciphers: CipherView[];
let params: Fido2AuthenticatorGetAssertionParams; let params: Fido2AuthenticatorGetAssertionParams;
beforeEach(async () => { beforeEach(async () => {
keyPair = await createKeyPair();
credentialIds = [Utils.newGuid(), Utils.newGuid()]; credentialIds = [Utils.newGuid(), Utils.newGuid()];
const keyValue = Fido2Utils.bufferToString(
await crypto.subtle.exportKey("pkcs8", keyPair.privateKey)
);
if (residentKey) { if (residentKey) {
ciphers = credentialIds.map((id) => ciphers = credentialIds.map((id) =>
createCipherView({ type: CipherType.Fido2Key }, { rpId: RpId, counter: 9000 }) createCipherView(
{ type: CipherType.Fido2Key },
{ rpId: RpId, counter: 9000, keyValue }
)
); );
selectedCredentialId = ciphers[0].id; selectedCredentialId = ciphers[0].id;
params = await createParams({ params = await createParams({
@@ -723,8 +718,20 @@ describe("FidoAuthenticatorService", () => {
); );
expect(flags).toEqual(new Uint8Array([0b00000001])); // UP = true expect(flags).toEqual(new Uint8Array([0b00000001])); // UP = true
expect(counter).toEqual(new Uint8Array([0, 0, 0x23, 0x29])); // 9001 in hex expect(counter).toEqual(new Uint8Array([0, 0, 0x23, 0x29])); // 9001 in hex
// Signatures are non-deterministic, and webcrypto can't verify DER signature format
// expect(result.signature).toMatchSnapshot(); // Verify signature
// TODO: Cannot verify signature because it has been converted into DER format
// const sigBase = new Uint8Array([
// ...result.authenticatorData,
// ...Fido2Utils.bufferSourceToUint8Array(params.hash),
// ]);
// const isValidSignature = await crypto.subtle.verify(
// { name: "ECDSA", hash: { name: "SHA-256" } },
// keyPair.publicKey,
// result.signature,
// sigBase
// );
// expect(isValidSignature).toBe(true);
}); });
/** Spec: If any error occurred while generating the assertion signature, return an error code equivalent to "UnknownError" and terminate the operation. */ /** Spec: If any error occurred while generating the assertion signature, return an error code equivalent to "UnknownError" and terminate the operation. */
@@ -804,3 +811,14 @@ async function createClientDataHash() {
function randomBytes(length: number) { function randomBytes(length: number) {
return new Uint8Array(Array.from({ length }, (_, k) => k % 255)); return new Uint8Array(Array.from({ length }, (_, k) => k % 255));
} }
async function createKeyPair() {
return await crypto.subtle.generateKey(
{
name: "ECDSA",
namedCurve: "P-256",
},
true,
["sign", "verify"]
);
}

View File

@@ -123,7 +123,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
const credentialId = params.requireResidentKey ? cipher.id : cipher.fido2Key.nonDiscoverableId; const credentialId = params.requireResidentKey ? cipher.id : cipher.fido2Key.nonDiscoverableId;
const authData = await generateAuthData({ const authData = await generateAuthData({
rpId: params.rpEntity.id, rpId: params.rpEntity.id,
credentialId, credentialId: Utils.guidToRawFormat(credentialId),
counter: cipher.fido2Key.counter, counter: cipher.fido2Key.counter,
userPresence: true, userPresence: true,
userVerification: false, userVerification: false,
@@ -201,7 +201,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
const authenticatorData = await generateAuthData({ const authenticatorData = await generateAuthData({
rpId: selectedCipher.fido2Key.rpId, rpId: selectedCipher.fido2Key.rpId,
credentialId: selectedCredentialId, credentialId: Utils.guidToRawFormat(selectedCredentialId),
counter: selectedCipher.fido2Key.counter, counter: selectedCipher.fido2Key.counter,
userPresence: true, userPresence: true,
userVerification: false, userVerification: false,
@@ -209,7 +209,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
const signature = await generateSignature({ const signature = await generateSignature({
authData: authenticatorData, authData: authenticatorData,
clientData: params.hash, clientDataHash: params.hash,
privateKey: await getPrivateKeyFromCipher(selectedCipher), privateKey: await getPrivateKeyFromCipher(selectedCipher),
}); });
@@ -340,7 +340,7 @@ async function getPrivateKeyFromCipher(cipher: CipherView): Promise<CryptoKey> {
interface AuthDataParams { interface AuthDataParams {
rpId: string; rpId: string;
credentialId: string; credentialId: BufferSource;
userPresence: boolean; userPresence: boolean;
userVerification: boolean; userVerification: boolean;
counter: number; counter: number;
@@ -380,7 +380,7 @@ async function generateAuthData(params: AuthDataParams) {
attestedCredentialData.push(...AAGUID); attestedCredentialData.push(...AAGUID);
// credentialIdLength (2 bytes) and credential Id // credentialIdLength (2 bytes) and credential Id
const rawId = Utils.guidToRawFormat(params.credentialId); const rawId = Fido2Utils.bufferSourceToUint8Array(params.credentialId);
const credentialIdLength = [(rawId.length - (rawId.length & 0xff)) / 256, rawId.length & 0xff]; const credentialIdLength = [(rawId.length - (rawId.length & 0xff)) / 256, rawId.length & 0xff];
attestedCredentialData.push(...credentialIdLength); attestedCredentialData.push(...credentialIdLength);
attestedCredentialData.push(...rawId); attestedCredentialData.push(...rawId);
@@ -408,14 +408,15 @@ async function generateAuthData(params: AuthDataParams) {
interface SignatureParams { interface SignatureParams {
authData: Uint8Array; authData: Uint8Array;
clientData: BufferSource; clientDataHash: BufferSource;
privateKey: CryptoKey; privateKey: CryptoKey;
} }
async function generateSignature(params: SignatureParams) { async function generateSignature(params: SignatureParams) {
const clientData = Fido2Utils.bufferSourceToUint8Array(params.clientData); const sigBase = new Uint8Array([
const clientDataHash = await crypto.subtle.digest({ name: "SHA-256" }, clientData); ...params.authData,
const sigBase = new Uint8Array([...params.authData, ...new Uint8Array(clientDataHash)]); ...Fido2Utils.bufferSourceToUint8Array(params.clientDataHash),
]);
const p1336_signature = new Uint8Array( const p1336_signature = new Uint8Array(
await crypto.subtle.sign( await crypto.subtle.sign(
{ {

View File

@@ -159,7 +159,7 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
} }
const collectedClientData = { const collectedClientData = {
type: "webauthn.create", type: "webauthn.get",
challenge: params.challenge, challenge: params.challenge,
origin: params.origin, origin: params.origin,
crossOrigin: !params.sameOriginWithAncestors, crossOrigin: !params.sameOriginWithAncestors,