1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-17 00:33:44 +00:00

[EC-598] feat: add separate nonDiscoverableId to keys

This commit is contained in:
Andreas Coroiu
2023-03-28 15:04:16 +02:00
parent 6d90489ace
commit c882c37f82
6 changed files with 159 additions and 55 deletions

View File

@@ -1,6 +1,7 @@
import { BaseResponse } from "../../../models/response/base.response"; import { BaseResponse } from "../../../models/response/base.response";
export class Fido2KeyApi extends BaseResponse { export class Fido2KeyApi extends BaseResponse {
nonDiscoverableId: string;
keyType: "public-key"; keyType: "public-key";
keyAlgorithm: "ECDSA"; keyAlgorithm: "ECDSA";
keyCurve: "P-256"; keyCurve: "P-256";
@@ -20,6 +21,7 @@ export class Fido2KeyApi extends BaseResponse {
return; return;
} }
this.nonDiscoverableId = this.getResponseProperty("NonDiscoverableId");
this.keyType = this.getResponseProperty("KeyType"); this.keyType = this.getResponseProperty("KeyType");
this.keyAlgorithm = this.getResponseProperty("KeyType"); this.keyAlgorithm = this.getResponseProperty("KeyType");
this.keyCurve = this.getResponseProperty("KeyCurve"); this.keyCurve = this.getResponseProperty("KeyCurve");

View File

@@ -1,6 +1,7 @@
import { Fido2KeyApi } from "../api/fido2-key.api"; import { Fido2KeyApi } from "../api/fido2-key.api";
export class Fido2KeyData { export class Fido2KeyData {
nonDiscoverableId: string;
keyType: "public-key"; keyType: "public-key";
keyAlgorithm: "ECDSA"; keyAlgorithm: "ECDSA";
keyCurve: "P-256"; keyCurve: "P-256";
@@ -19,6 +20,7 @@ export class Fido2KeyData {
return; return;
} }
this.nonDiscoverableId = data.nonDiscoverableId;
this.keyType = data.keyType; this.keyType = data.keyType;
this.keyAlgorithm = data.keyAlgorithm; this.keyAlgorithm = data.keyAlgorithm;
this.keyCurve = data.keyCurve; this.keyCurve = data.keyCurve;

View File

@@ -7,6 +7,7 @@ import { Fido2KeyData } from "../data/fido2-key.data";
import { Fido2KeyView } from "../view/fido2-key.view"; import { Fido2KeyView } from "../view/fido2-key.view";
export class Fido2Key extends Domain { export class Fido2Key extends Domain {
nonDiscoverableId: EncString | null = null;
keyType: EncString; keyType: EncString;
keyAlgorithm: EncString; keyAlgorithm: EncString;
keyCurve: EncString; keyCurve: EncString;
@@ -30,6 +31,7 @@ export class Fido2Key extends Domain {
this, this,
obj, obj,
{ {
nonDiscoverableId: null,
keyType: null, keyType: null,
keyAlgorithm: null, keyAlgorithm: null,
keyCurve: null, keyCurve: null,
@@ -49,6 +51,7 @@ export class Fido2Key extends Domain {
return this.decryptObj( return this.decryptObj(
new Fido2KeyView(), new Fido2KeyView(),
{ {
nonDiscoverableId: null,
keyType: null, keyType: null,
keyAlgorithm: null, keyAlgorithm: null,
keyCurve: null, keyCurve: null,
@@ -68,6 +71,7 @@ export class Fido2Key extends Domain {
toFido2KeyData(): Fido2KeyData { toFido2KeyData(): Fido2KeyData {
const i = new Fido2KeyData(); const i = new Fido2KeyData();
this.buildDataModel(this, i, { this.buildDataModel(this, i, {
nonDiscoverableId: null,
keyType: null, keyType: null,
keyAlgorithm: null, keyAlgorithm: null,
keyCurve: null, keyCurve: null,
@@ -87,6 +91,7 @@ export class Fido2Key extends Domain {
return null; return null;
} }
const nonDiscoverableId = EncString.fromJSON(obj.nonDiscoverableId);
const keyType = EncString.fromJSON(obj.keyType); const keyType = EncString.fromJSON(obj.keyType);
const keyAlgorithm = EncString.fromJSON(obj.keyAlgorithm); const keyAlgorithm = EncString.fromJSON(obj.keyAlgorithm);
const keyCurve = EncString.fromJSON(obj.keyCurve); const keyCurve = EncString.fromJSON(obj.keyCurve);
@@ -99,6 +104,7 @@ export class Fido2Key extends Domain {
const origin = EncString.fromJSON(obj.origin); const origin = EncString.fromJSON(obj.origin);
return Object.assign(new Fido2Key(), obj, { return Object.assign(new Fido2Key(), obj, {
nonDiscoverableId,
keyType, keyType,
keyAlgorithm, keyAlgorithm,
keyCurve, keyCurve,

View File

@@ -3,6 +3,7 @@ import { Jsonify } from "type-fest";
import { ItemView } from "../../../vault/models/view/item.view"; import { ItemView } from "../../../vault/models/view/item.view";
export class Fido2KeyView extends ItemView { export class Fido2KeyView extends ItemView {
nonDiscoverableId: string;
keyType: "public-key"; keyType: "public-key";
keyAlgorithm: "ECDSA"; keyAlgorithm: "ECDSA";
keyCurve: "P-256"; keyCurve: "P-256";

View File

@@ -92,7 +92,67 @@ describe("FidoAuthenticatorService", () => {
describe.skip("when extensions parameter is present", () => undefined); describe.skip("when extensions parameter is present", () => undefined);
describe("when vault contains excluded credential", () => { describe("when vault contains excluded non-discoverable credential", () => {
let excludedCipherView: CipherView;
let params: Fido2AuthenticatorMakeCredentialsParams;
beforeEach(async () => {
const excludedCipher = createCipher({ type: CipherType.Login });
excludedCipherView = await excludedCipher.decrypt();
excludedCipherView.fido2Key.nonDiscoverableId = Utils.newGuid();
params = await createParams({
excludeCredentialDescriptorList: [
{
id: Fido2Utils.stringToBuffer(excludedCipherView.fido2Key.nonDiscoverableId),
type: "public-key",
},
],
});
cipherService.get.mockImplementation(async (id) =>
id === excludedCipher.id ? excludedCipher : undefined
);
cipherService.getAllDecrypted.mockResolvedValue([excludedCipherView]);
});
/**
* Spec: collect an authorization gesture confirming user consent for creating a new credential.
* Deviation: Consent is not asked and the user is simply informed of the situation.
**/
it("should inform user", async () => {
userInterface.informExcludedCredential.mockResolvedValue();
try {
await authenticator.makeCredential(params);
// eslint-disable-next-line no-empty
} catch {}
expect(userInterface.informExcludedCredential).toHaveBeenCalled();
});
/** Spec: return an error code equivalent to "NotAllowedError" and terminate the operation. */
it("should throw error", async () => {
userInterface.informExcludedCredential.mockResolvedValue();
const result = async () => await authenticator.makeCredential(params);
await expect(result).rejects.toThrowError(Fido2AutenticatorErrorCode.NotAllowed);
});
it("should not inform user of duplication when input data does not pass checks", async () => {
userInterface.informExcludedCredential.mockResolvedValue();
const invalidParams = await createInvalidParams();
for (const p of Object.values(invalidParams)) {
try {
await authenticator.makeCredential(p);
// eslint-disable-next-line no-empty
} catch {}
}
expect(userInterface.informExcludedCredential).not.toHaveBeenCalled();
});
});
describe("when vault contains excluded discoverable credential", () => {
let excludedCipherView: CipherView; let excludedCipherView: CipherView;
let params: Fido2AuthenticatorMakeCredentialsParams; let params: Fido2AuthenticatorMakeCredentialsParams;
@@ -153,6 +213,7 @@ describe("FidoAuthenticatorService", () => {
beforeEach(async () => { beforeEach(async () => {
params = await createParams({ requireResidentKey: true }); params = await createParams({ requireResidentKey: true });
cipherService.getAllDecrypted.mockResolvedValue([]);
}); });
/** /**
@@ -193,6 +254,7 @@ describe("FidoAuthenticatorService", () => {
name: params.rpEntity.name, name: params.rpEntity.name,
fido2Key: expect.objectContaining({ fido2Key: expect.objectContaining({
nonDiscoverableId: null,
keyType: "public-key", keyType: "public-key",
keyAlgorithm: "ECDSA", keyAlgorithm: "ECDSA",
keyCurve: "P-256", keyCurve: "P-256",
@@ -274,6 +336,7 @@ describe("FidoAuthenticatorService", () => {
name: existingCipherView.name, name: existingCipherView.name,
fido2Key: expect.objectContaining({ fido2Key: expect.objectContaining({
nonDiscoverableId: expect.anything(),
keyType: "public-key", keyType: "public-key",
keyAlgorithm: "ECDSA", keyAlgorithm: "ECDSA",
keyCurve: "P-256", keyCurve: "P-256",
@@ -311,56 +374,82 @@ describe("FidoAuthenticatorService", () => {
}); });
}); });
describe("attestation of new credential", () => { for (const requireResidentKey of [true, false]) {
const cipherId = "75280e7e-a72e-4d6c-bf1e-d37238352f9b"; describe(`attestation of new ${
const cipherIdBytes = new Uint8Array([ requireResidentKey ? "discoverable" : "non-discoverable"
0x75, 0x28, 0x0e, 0x7e, 0xa7, 0x2e, 0x4d, 0x6c, 0xbf, 0x1e, 0xd3, 0x72, 0x38, 0x35, 0x2f, } credential`, () => {
0x9b, const cipherId = "75280e7e-a72e-4d6c-bf1e-d37238352f9b";
]); const cipherIdBytes = new Uint8Array([
let params: Fido2AuthenticatorMakeCredentialsParams; 0x75, 0x28, 0x0e, 0x7e, 0xa7, 0x2e, 0x4d, 0x6c, 0xbf, 0x1e, 0xd3, 0x72, 0x38, 0x35, 0x2f,
0x9b,
]);
const nonDiscoverableId = "52217b91-73f1-4fea-b3f2-54a7959fd5aa";
const nonDiscoverableIdBytes = new Uint8Array([
0x52, 0x21, 0x7b, 0x91, 0x73, 0xf1, 0x4f, 0xea, 0xb3, 0xf2, 0x54, 0xa7, 0x95, 0x9f, 0xd5,
0xaa,
]);
let params: Fido2AuthenticatorMakeCredentialsParams;
beforeEach(async () => { beforeEach(async () => {
params = await createParams({ requireResidentKey: true }); const cipher = createCipher({ id: cipherId, type: CipherType.Login });
userInterface.confirmNewCredential.mockResolvedValue(true); params = await createParams({ requireResidentKey });
cipherService.encrypt.mockResolvedValue({} as unknown as Cipher); userInterface.confirmNewNonDiscoverableCredential.mockResolvedValue(cipherId);
cipherService.createWithServer.mockImplementation(async (cipher) => { userInterface.confirmNewCredential.mockResolvedValue(true);
cipher.id = cipherId; cipherService.get.mockImplementation(async (cipherId) =>
return cipher; cipherId === cipher.id ? cipher : undefined
);
cipherService.getAllDecrypted.mockResolvedValue([await cipher.decrypt()]);
cipherService.encrypt.mockImplementation(async (cipher) => {
cipher.fido2Key.nonDiscoverableId = nonDiscoverableId; // Replace id for testability
return {} as any;
});
cipherService.createWithServer.mockImplementation(async (cipher) => {
cipher.id = cipherId;
return cipher;
});
cipherService.updateWithServer.mockImplementation(async (cipher) => {
cipher.id = cipherId;
return cipher;
});
});
it("should return attestation object", async () => {
const result = await authenticator.makeCredential(params);
const attestationObject = CBOR.decode(result.buffer);
const encAuthData: Uint8Array = attestationObject.authData;
const rpIdHash = encAuthData.slice(0, 32);
const flags = encAuthData.slice(32, 33);
const counter = encAuthData.slice(33, 37);
const aaguid = encAuthData.slice(37, 53);
const credentialIdLength = encAuthData.slice(53, 55);
const credentialId = encAuthData.slice(55, 71);
// Public key format is not tested here since it will be tested
// by the assertion tests.
// const publicKey = encAuthData.slice(87);
expect(attestationObject.fmt).toBe("none");
expect(attestationObject.attStmt).toEqual({});
expect(rpIdHash).toEqual(
new Uint8Array([
0x22, 0x6b, 0xb3, 0x92, 0x02, 0xff, 0xf9, 0x22, 0xdc, 0x74, 0x05, 0xcd, 0x28, 0xa8,
0x34, 0x5a, 0xc4, 0xf2, 0x64, 0x51, 0xd7, 0x3d, 0x0b, 0x40, 0xef, 0xf3, 0x1d, 0xc1,
0xd0, 0x5c, 0x3d, 0xc3,
])
);
expect(flags).toEqual(new Uint8Array([0b00000001])); // UP = true
expect(counter).toEqual(new Uint8Array([0, 0, 0, 0])); // 0 because of new counter
expect(aaguid).toEqual(AAGUID);
expect(credentialIdLength).toEqual(new Uint8Array([0, 16])); // 16 bytes because we're using GUIDs
if (requireResidentKey) {
expect(credentialId).toEqual(cipherIdBytes);
} else {
expect(credentialId).toEqual(nonDiscoverableIdBytes);
}
}); });
}); });
}
it("should throw error if user denies creation request", async () => {
const result = await authenticator.makeCredential(params);
const attestationObject = CBOR.decode(result.buffer);
const encAuthData: Uint8Array = attestationObject.authData;
const rpIdHash = encAuthData.slice(0, 32);
const flags = encAuthData.slice(32, 33);
const counter = encAuthData.slice(33, 37);
const aaguid = encAuthData.slice(37, 53);
const credentialIdLength = encAuthData.slice(53, 55);
const credentialId = encAuthData.slice(55, 71);
// Public key format is not tested here since it will be tested
// by the assertion tests.
// const publicKey = encAuthData.slice(87);
expect(attestationObject.fmt).toBe("none");
expect(attestationObject.attStmt).toEqual({});
expect(rpIdHash).toEqual(
new Uint8Array([
0x22, 0x6b, 0xb3, 0x92, 0x02, 0xff, 0xf9, 0x22, 0xdc, 0x74, 0x05, 0xcd, 0x28, 0xa8,
0x34, 0x5a, 0xc4, 0xf2, 0x64, 0x51, 0xd7, 0x3d, 0x0b, 0x40, 0xef, 0xf3, 0x1d, 0xc1,
0xd0, 0x5c, 0x3d, 0xc3,
])
);
expect(flags).toEqual(new Uint8Array([0b00000001])); // UP = true
expect(counter).toEqual(new Uint8Array([0, 0, 0, 0])); // 0 because of new counter
expect(aaguid).toEqual(AAGUID);
expect(credentialIdLength).toEqual(new Uint8Array([0, 16])); // 16 bytes because we're using GUIDs
expect(credentialId).toEqual(cipherIdBytes);
});
});
async function createParams( async function createParams(
params: Partial<Fido2AuthenticatorMakeCredentialsParams> = {} params: Partial<Fido2AuthenticatorMakeCredentialsParams> = {}
@@ -469,6 +558,7 @@ function createCipher(data: Partial<Cipher> = {}): Cipher {
const cipher = new Cipher(); const cipher = new Cipher();
cipher.id = data.id ?? Utils.newGuid(); cipher.id = data.id ?? Utils.newGuid();
cipher.type = data.type ?? CipherType.Fido2Key; cipher.type = data.type ?? CipherType.Fido2Key;
cipher.login = data.type ?? data.type === CipherType.Login ? new Login() : null;
cipher.fido2Key = data.fido2Key ?? new Fido2Key(); cipher.fido2Key = data.fido2Key ?? new Fido2Key();
return cipher; return cipher;
} }

View File

@@ -123,7 +123,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
attStmt: {}, attStmt: {},
authData: await generateAuthData({ authData: await generateAuthData({
rpId: params.rpEntity.id, rpId: params.rpEntity.id,
credentialId: cipher.id, credentialId: params.requireResidentKey ? cipher.id : cipher.fido2Key.nonDiscoverableId,
counter: cipher.fido2Key.counter, counter: cipher.fido2Key.counter,
userPresence: true, userPresence: true,
userVerification: false, userVerification: false,
@@ -153,13 +153,15 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
} }
private async vaultContainsId(ids: string[]): Promise<boolean> { private async vaultContainsId(ids: string[]): Promise<boolean> {
for (const id of ids) { const ciphers = await this.cipherService.getAllDecrypted();
if ((await this.cipherService.get(id)) != undefined) {
return true;
}
}
return false; return ciphers.some(
(cipher) =>
(cipher.type === CipherType.Fido2Key && ids.includes(cipher.id)) ||
(cipher.type === CipherType.Login &&
cipher.fido2Key != undefined &&
ids.includes(cipher.fido2Key.nonDiscoverableId))
);
} }
private async createKeyPair() { private async createKeyPair() {
@@ -180,6 +182,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
const pcks8Key = await crypto.subtle.exportKey("pkcs8", keyValue); const pcks8Key = await crypto.subtle.exportKey("pkcs8", keyValue);
const fido2Key = new Fido2KeyView(); const fido2Key = new Fido2KeyView();
fido2Key.nonDiscoverableId = params.requireResidentKey ? null : Utils.newGuid();
fido2Key.keyType = "public-key"; fido2Key.keyType = "public-key";
fido2Key.keyAlgorithm = "ECDSA"; fido2Key.keyAlgorithm = "ECDSA";
fido2Key.keyCurve = "P-256"; fido2Key.keyCurve = "P-256";