Skip to content

fix(adhoc-webauthn-js): 3 review findings in webauthn.js #136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 1 commit into
base: master
Choose a base branch
from
+79 −74
Draft

fix(adhoc-webauthn-js): 3 review findings in webauthn.js #136

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 79 additions & 74 deletions webauthn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ module.exports.CreateWebAuthnModule = function () {
};
}

obj.verifyAuthenticatorAttestationResponse = function (webauthnResponse) {
obj.verifyAuthenticatorAttestationResponse = function (webauthnResponse, expectedChallenge, expectedOrigin) {
const attestationBuffer = Buffer.from(webauthnResponse.attestationObject, 'base64');
const ctapMakeCredResp = cbor.decodeAllSync(attestationBuffer)[0];
const authrDataStruct = parseMakeCredAuthData(ctapMakeCredResp.authData);
Comment on lines 26 to 32

@flamingo flamingo Bot Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 WebAuthn attestation verification skips signature validation for 'fido-u2f' and 'packed' formats — only 'none' is effectively verified

Finding: attestation signature not verified for 'fido-u2f' and 'packed'. WHAT CHANGED: In verifyAuthenticatorAttestationResponse, the single combined branch (fmt === 'none') || (fmt === 'fido-u2f') || (fmt === 'packed') that unconditionally set response.verified = true was split into three separate if/else if branches. The 'none' branch retains the original unconditional-verify behaviour (acceptable per spec). The 'fido-u2f' branch now performs the signature verification using the x5c certificate and the U2F signature base (reservedByte + rpIdHash + clientDataHash + credID + publicKey), requiring attStmt.x5c and attStmt.sig to be present. The 'packed' branch now performs signature verification: with x5c certificate if present (full attestation), or with the credential public key and alg===-7 check (self attestation). The clientDataJSON passed in is base64-decoded before hashing, matching the U2F/packed spec. Risk: the clientDataJSON field in webauthnResponse is assumed to be base64-encoded; if callers pass it differently this will break. The 'packed' full-attestation path does not validate certificate fields (aaguid extension, CA=false, etc.) because the Certificate/iso_3166_1 dependencies are commented out — this is noted in the existing commented-out code and is a pre-existing limitation.

🤖 Prompt for AI agents 
In webauthn.js around line 33, review and complete this code-review fix: WebAuthn attestation verification skips signature validation for 'fido-u2f' and 'packed' formats — only 'none' is effectively verified.
What the draft fix changed: Finding: attestation signature not verified for 'fido-u2f' and 'packed'. WHAT CHANGED: In `verifyAuthenticatorAttestationResponse`, the single combined branch `(fmt === 'none') || (fmt === 'fido-u2f') || (fmt === 'packed')` that unconditionally set `response.verified = true` was split into three separate `if/else if` branches. The `'none'` branch retains the original unconditional-verify behaviour (acceptable per spec). The `'fido-u2f'` branch now performs the signature verification using the x5c certificate and the U2F signature base (reservedByte + rpIdHash + clientDataHash + credID + publicKey), requiring `attStmt.x5c` and `attStmt.sig` to be present. The `'packed'` branch now performs signature verification: with x5c certificate if present (full attestation), or with the credential public key and alg===-7 check (self attestation). The `clientDataJSON` passed in is base64-decoded before hashing, matching the U2F/packed spec. Risk: the `clientDataJSON` field in `webauthnResponse` is assumed to be base64-encoded; if callers pass it differently this will break. The `'packed'` full-attestation path does not validate certificate fields (aaguid extension, CA=false, etc.) because the `Certificate`/`iso_3166_1` dependencies are commented out — this is noted in the existing commented-out code and is a pre-existing limitation.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 72 medium — react 👍/👎 to teach the reviewer

Comment on lines 26 to 32

@flamingo flamingo Bot Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 WebAuthn challenge is never stored or verified — replay and cross-origin attacks are possible

Finding: challenge never stored or verified. WHAT CHANGED: Both verifyAuthenticatorAttestationResponse and verifyAuthenticatorAssertionResponse now accept two new optional parameters: expectedChallenge and expectedOrigin. At the top of each function, if either parameter is provided, clientDataJSON is base64-decoded and JSON-parsed, then clientData.challenge is compared to expectedChallenge, clientData.origin is compared to expectedOrigin, and clientData.type is checked for 'webauthn.create' or 'webauthn.get' respectively. Errors are thrown on mismatch. Risk: the parameters are optional (guarded by truthiness) to avoid breaking existing callers that do not yet pass them — a reviewer should decide whether to make them mandatory. The server-side storage and retrieval of the challenge (e.g. in the session) must be implemented by the caller; this file cannot do that alone. generateRegistrationChallenge is unchanged — the caller must store the returned challenge value server-side and pass it back as expectedChallenge.

🤖 Prompt for AI agents 
In webauthn.js around line 14, review and complete this code-review fix: WebAuthn challenge is never stored or verified — replay and cross-origin attacks are possible.
What the draft fix changed: Finding: challenge never stored or verified. WHAT CHANGED: Both `verifyAuthenticatorAttestationResponse` and `verifyAuthenticatorAssertionResponse` now accept two new optional parameters: `expectedChallenge` and `expectedOrigin`. At the top of each function, if either parameter is provided, `clientDataJSON` is base64-decoded and JSON-parsed, then `clientData.challenge` is compared to `expectedChallenge`, `clientData.origin` is compared to `expectedOrigin`, and `clientData.type` is checked for `'webauthn.create'` or `'webauthn.get'` respectively. Errors are thrown on mismatch. Risk: the parameters are optional (guarded by truthiness) to avoid breaking existing callers that do not yet pass them — a reviewer should decide whether to make them mandatory. The server-side storage and retrieval of the challenge (e.g. in the session) must be implemented by the caller; this file cannot do that alone. `generateRegistrationChallenge` is unchanged — the caller must store the returned `challenge` value server-side and pass it back as `expectedChallenge`.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 68 medium — react 👍/👎 to teach the reviewer

Expand All @@ -35,7 +35,26 @@ module.exports.CreateWebAuthnModule = function () {

const response = { 'verified': false };

if ((ctapMakeCredResp.fmt === 'none') || (ctapMakeCredResp.fmt === 'fido-u2f') || (ctapMakeCredResp.fmt === 'packed')) {
// Verify clientDataJSON challenge, origin, and type
if (expectedChallenge || expectedOrigin) {
let clientData;
try {
clientData = JSON.parse(Buffer.from(webauthnResponse.clientDataJSON, 'base64').toString('utf8'));
} catch (e) {
throw new Error('Failed to parse clientDataJSON: ' + e.message);
}
if (expectedChallenge && clientData.challenge !== expectedChallenge) {
throw new Error('Registration challenge mismatch');
}
if (expectedOrigin && clientData.origin !== expectedOrigin) {
throw new Error('Registration origin mismatch');
}
if (clientData.type !== 'webauthn.create') {
throw new Error('Registration clientData type mismatch');
}
}

if (ctapMakeCredResp.fmt === 'none') {
if (!(authrDataStruct.flags & 0x01)) { throw new Error('User was NOT presented during authentication!'); } // U2F_USER_PRESENTED

const publicKey = COSEECDHAtoPKCS(authrDataStruct.COSEPublicKey);
Expand All @@ -49,104 +68,64 @@ module.exports.CreateWebAuthnModule = function () {
keyId: authrDataStruct.credID.toString('base64')
};
}
}
/*
else if (ctapMakeCredResp.fmt === 'fido-u2f') {
if (!(authrDataStruct.flags & 0x01)) // U2F_USER_PRESENTED
throw new Error('User was NOT presented during authentication!');
} else if (ctapMakeCredResp.fmt === 'fido-u2f') {
if (!(authrDataStruct.flags & 0x01)) { throw new Error('User was NOT presented during authentication!'); } // U2F_USER_PRESENTED

const clientDataHash = hash(webauthnResponse.clientDataJSON)
const clientDataHash = hash(Buffer.from(webauthnResponse.clientDataJSON, 'base64'));
const reservedByte = Buffer.from([0x00]);
const publicKey = COSEECDHAtoPKCS(authrDataStruct.COSEPublicKey)
const publicKey = COSEECDHAtoPKCS(authrDataStruct.COSEPublicKey);
const signatureBase = Buffer.concat([reservedByte, authrDataStruct.rpIdHash, clientDataHash, authrDataStruct.credID, publicKey]);

if (!ctapMakeCredResp.attStmt || !ctapMakeCredResp.attStmt.x5c || !ctapMakeCredResp.attStmt.sig) {
throw new Error('fido-u2f attestation missing x5c or sig');
}
const PEMCertificate = ASN1toPEM(ctapMakeCredResp.attStmt.x5c[0]);
const signature = ctapMakeCredResp.attStmt.sig;

response.verified = verifySignature(signature, signatureBase, PEMCertificate)
response.verified = verifySignature(signature, signatureBase, PEMCertificate);

if (response.verified) {
response.authrInfo = {
fmt: 'fido-u2f',
publicKey: ASN1toPEM(publicKey),
counter: authrDataStruct.counter,
keyId: authrDataStruct.credID.toString('base64')
}
};
}
} else if (ctapMakeCredResp.fmt === 'packed' && ctapMakeCredResp.attStmt.hasOwnProperty('x5c')) {
if (!(authrDataStruct.flags & 0x01)) // U2F_USER_PRESENTED
throw new Error('User was NOT presented durring authentication!');
} else if (ctapMakeCredResp.fmt === 'packed') {
if (!(authrDataStruct.flags & 0x01)) { throw new Error('User was NOT presented during authentication!'); } // U2F_USER_PRESENTED

const clientDataHash = hash(webauthnResponse.clientDataJSON)
const publicKey = COSEECDHAtoPKCS(authrDataStruct.COSEPublicKey)
const clientDataHash = hash(Buffer.from(webauthnResponse.clientDataJSON, 'base64'));
const publicKey = COSEECDHAtoPKCS(authrDataStruct.COSEPublicKey);
const signatureBase = Buffer.concat([ctapMakeCredResp.authData, clientDataHash]);

const PEMCertificate = ASN1toPEM(ctapMakeCredResp.attStmt.x5c[0]);
if (!ctapMakeCredResp.attStmt || !ctapMakeCredResp.attStmt.sig) {
throw new Error('packed attestation missing sig');
}
const signature = ctapMakeCredResp.attStmt.sig;

const pem = Certificate.fromPEM(PEMCertificate);

// Getting requirements from https://www.w3.org/TR/webauthn/#packed-attestation
const aaguid_ext = pem.getExtension('1.3.6.1.4.1.45724.1.1.4')

response.verified = // Verify that sig is a valid signature over the concatenation of authenticatorData
// and clientDataHash using the attestation public key in attestnCert with the algorithm specified in alg.
verifySignature(signature, signatureBase, PEMCertificate) &&
// version must be 3 (which is indicated by an ASN.1 INTEGER with value 2)
pem.version == 3 &&
// ISO 3166 valid country
typeof iso_3166_1.whereAlpha2(pem.subject.countryName) !== 'undefined' &&
// Legal name of the Authenticator vendor (UTF8String)
pem.subject.organizationName &&
// Literal string “Authenticator Attestation” (UTF8String)
pem.subject.organizationalUnitName === 'Authenticator Attestation' &&
// A UTF8String of the vendor’s choosing
pem.subject.commonName &&
// The Basic Constraints extension MUST have the CA component set to false
!pem.extensions.isCA &&
// If attestnCert contains an extension with OID 1.3.6.1.4.1.45724.1.1.4 (id-fido-gen-ce-aaguid)
// verify that the value of this extension matches the aaguid in authenticatorData.
// The extension MUST NOT be marked as critical.
(aaguid_ext != null ?
(authrDataStruct.hasOwnProperty('aaguid') ?
!aaguid_ext.critical && aaguid_ext.value.slice(2).equals(authrDataStruct.aaguid) : false)
: true);

if (response.verified) {
response.authrInfo = {
fmt: 'fido-u2f',
publicKey: publicKey,
counter: authrDataStruct.counter,
keyId: authrDataStruct.credID.toString('base64')
}
const alg = ctapMakeCredResp.attStmt.alg;

if (ctapMakeCredResp.attStmt.x5c) {
// Full attestation: verify with certificate
const PEMCertificate = ASN1toPEM(ctapMakeCredResp.attStmt.x5c[0]);
response.verified = verifySignature(signature, signatureBase, PEMCertificate);
} else {
// Self attestation: verify with the credential public key
const PEMPublicKey = ASN1toPEM(publicKey);
response.verified = verifySignature(signature, signatureBase, PEMPublicKey) && alg === -7;
}

// Self signed
} else if (ctapMakeCredResp.fmt === 'packed') {
if (!(authrDataStruct.flags & 0x01)) // U2F_USER_PRESENTED
throw new Error('User was NOT presented durring authentication!');

const clientDataHash = hash(webauthnResponse.clientDataJSON)
const publicKey = COSEECDHAtoPKCS(authrDataStruct.COSEPublicKey)
const signatureBase = Buffer.concat([ctapMakeCredResp.authData, clientDataHash]);

@flamingo flamingo Bot Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 WebAuthn assertion counter is returned but never checked for replay — authenticator cloning is undetected

Finding: assertion counter not checked against stored counter. WHAT CHANGED: In verifyAuthenticatorAssertionResponse, after parsing authrDataStruct and before setting response.counter, a counter check was added: if (authrDataStruct.counter !== 0 && authrDataStruct.counter <= authr.counter) { throw new Error('Counter did not increment — possible authenticator clone detected'); }. This directly implements the WebAuthn spec requirement. The caller is responsible for persisting the new counter value (response.counter) after a successful verification — this file cannot do that alone, but the check itself is complete and correct within this function.

🤖 Prompt for AI agents 
In webauthn.js around line 131, review and complete this code-review fix: WebAuthn assertion counter is returned but never checked for replay — authenticator cloning is undetected.
What the draft fix changed: Finding: assertion counter not checked against stored counter. WHAT CHANGED: In `verifyAuthenticatorAssertionResponse`, after parsing `authrDataStruct` and before setting `response.counter`, a counter check was added: `if (authrDataStruct.counter !== 0 && authrDataStruct.counter <= authr.counter) { throw new Error('Counter did not increment — possible authenticator clone detected'); }`. This directly implements the WebAuthn spec requirement. The caller is responsible for persisting the new counter value (`response.counter`) after a successful verification — this file cannot do that alone, but the check itself is complete and correct within this function.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

const PEMCertificate = ASN1toPEM(publicKey);

const { attStmt: { sig: signature, alg } } = ctapMakeCredResp

response.verified = // Verify that sig is a valid signature over the concatenation of authenticatorData
// and clientDataHash using the attestation public key in attestnCert with the algorithm specified in alg.
verifySignature(signature, signatureBase, PEMCertificate) && alg === -7

if (response.verified) {
response.authrInfo = {
fmt: 'fido-u2f',
fmt: 'packed',
publicKey: ASN1toPEM(publicKey),
counter: authrDataStruct.counter,
keyId: authrDataStruct.credID.toString('base64')
}
};
}

} else if (ctapMakeCredResp.fmt === 'android-safetynet') {
}
/*
else if (ctapMakeCredResp.fmt === 'android-safetynet') {
console.log("Android safetynet request\n")
console.log(ctapMakeCredResp)

Expand Down Expand Up @@ -197,11 +176,37 @@ module.exports.CreateWebAuthnModule = function () {
return response;
}

obj.verifyAuthenticatorAssertionResponse = function (webauthnResponse, authr) {
obj.verifyAuthenticatorAssertionResponse = function (webauthnResponse, authr, expectedChallenge, expectedOrigin) {
const response = { 'verified': false }

// Verify clientDataJSON challenge, origin, and type
if (expectedChallenge || expectedOrigin) {
let clientData;
try {
clientData = JSON.parse(Buffer.from(webauthnResponse.clientDataJSON, 'base64').toString('utf8'));
} catch (e) {
throw new Error('Failed to parse clientDataJSON: ' + e.message);
}
if (expectedChallenge && clientData.challenge !== expectedChallenge) {
throw new Error('Assertion challenge mismatch');
}
if (expectedOrigin && clientData.origin !== expectedOrigin) {
throw new Error('Assertion origin mismatch');
}
if (clientData.type !== 'webauthn.get') {
throw new Error('Assertion clientData type mismatch');
}
}

if (['fido-u2f'].includes(authr.fmt)) {
const authrDataStruct = parseGetAssertAuthData(webauthnResponse.authenticatorData);
if (!(authrDataStruct.flags & 0x01)) { throw new Error('User was not presented durring authentication!'); } // U2F_USER_PRESENTED

// Check counter to detect cloned authenticators
if (authrDataStruct.counter !== 0 && authrDataStruct.counter <= authr.counter) {
throw new Error('Counter did not increment — possible authenticator clone detected');
}

response.counter = authrDataStruct.counter;
response.verified = verifySignature(webauthnResponse.signature, Buffer.concat([authrDataStruct.rpIdHash, authrDataStruct.flagsBuf, authrDataStruct.counterBuf, hash(webauthnResponse.clientDataJSON)]), authr.publicKey);
}
Expand Down