Skip to content

Fixes passkeys validation errors#2916

Open
a2kolbasov wants to merge 2 commits intokeepassxreboot:developfrom
a2kolbasov:passkeys/validation-errors
Open

Fixes passkeys validation errors#2916
a2kolbasov wants to merge 2 commits intokeepassxreboot:developfrom
a2kolbasov:passkeys/validation-errors

Conversation

@a2kolbasov
Copy link
Copy Markdown
Contributor

  1. length - the length in elements, byteLength - the length in bytes. Also length does not exist in ArrayBuffer. For ArrayBuffer validation is undefined < 16. But BigUint64Array requires 128 bytes to pass validation.

    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray
    https://www.w3.org/TR/webauthn-2/#sctn-cryptographic-challenges

  2. rp.id in PublicKeyCredentialCreationOptions, but rpId in PublicKeyCredentialRequestOptions

    https://www.w3.org/TR/webauthn-2/#idl-index

  3. strict ROR validation

    • Prevent tracking and non-domain requests
    • Checking response status code

    https://www.w3.org/TR/webauthn-3/#sctn-validating-relation-origin


Fixes #2915

Screenshots or videos

Testing strategy

  1. Manually via debugger
  2. Register and authenticate on https://webauthn.io/

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

1. `length` - the length in elements, `byteLength` - the length in bytes. Also `length` does not exist in `ArrayBuffer`.
  - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray
  - https://www.w3.org/TR/webauthn-2/#sctn-cryptographic-challenges

2. `rp.id` in PublicKeyCredentialCreationOptions, but `rpId` in PublicKeyCredentialRequestOptions
  - https://www.w3.org/TR/webauthn-2/#idl-index
- Prevent tracking and non-domain requests
- Checking response status code

https://www.w3.org/TR/webauthn-3/#sctn-validating-relation-origin
@varjolintu
Copy link
Copy Markdown
Member

Validation of Related Origins is made in the KeePassXC side.

}

if (pkOptions.challenge.length < 16) {
if (!Number.isSafeInteger(pkOptions.challenge?.byteLength) || pkOptions.challenge.byteLength < 16) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

pkOptions.challenge.byteLength would be enough here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I want to make sure challenge exists and is the correct type. That it's not a base64string or something like that. So that there's no more undefined < 16 check.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does your silence mean that I should refuse this additional check anyway?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My silence means I haven't answered you yet. if (pkOptions.challenge.byteLength < 16) is enough here.

Comment thread keepassxc-browser/background/keepass.js
@a2kolbasov
Copy link
Copy Markdown
Contributor Author

Validation of Related Origins is made in the KeePassXC side.

I'm not checking ROR. I'm checking the data for a GET request the same way you check the JSON<string[]> compliance of the response. (#2915 (comment))

Comment thread keepassxc-browser/background/keepass.js
let hostname;
try {
hostname = new URL(`https://${rpId}`).hostname;
} catch { }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should return already in here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So there should be code duplication?

try {
  if (new URL(`https://${rpId}`).hostname !== rpId) {
    logError(`getRelatedOrigins error: "${rpId}" is wrong rpId`);
    return [];
  }
} catch {
  logError(`getRelatedOrigins error: "${rpId}" is wrong rpId`);
  return [];
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Of course we want to handle the exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

There is no check on rpId to ensure it's a hostname

3 participants