Remove fallback to P256 in WebAuthn signer#6337
Conversation
|
The latest updates on your security scan. Learn more about OpenZeppelin Platform.
|
|
WalkthroughThe changes modify SignerWebAuthn._rawSignatureValidation to remove the fallback delegation to the parent's raw P256 validation. When WebAuthn.tryDecodeAuth(signature) fails, the function now returns false directly instead of delegating to super._rawSignatureValidation. The corresponding docstring is updated to reflect this behavior change, where non-WebAuthn assertions now yield false. A changelog entry documents this behavioral modification. Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CHANGELOG.md (1)
50-50:⚠️ Potential issue | 🟡 MinorInconsistent documentation: line 50 still references the removed P256 fallback.
This entry under "Cryptography > Signers" still says
SignerWebAuthnhas "a P256 fallback," which contradicts the breaking change at line 10 that explicitly removes this fallback. Consider updating this description to reflect the current behavior.📝 Suggested fix
-- `SignerWebAuthn`: Add an abstract signer that verifies WebAuthn signatures, with a P256 fallback. ([`#5809`](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5809)) +- `SignerWebAuthn`: Add an abstract signer that verifies WebAuthn signatures. ([`#5809`](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5809))contracts/utils/cryptography/signers/SignerWebAuthn.sol (1)
10-14:⚠️ Potential issue | 🟡 MinorContract docstring is inconsistent with new behavior.
The docstring states "It allows for both WebAuthn and raw P256 signature validation, providing compatibility with both signature types," but the new implementation only supports WebAuthn signatures and returns
falsefor anything else. This documentation should be updated to reflect the current behavior.📝 Suggested fix
/** * `@dev` Implementation of {SignerP256} that supports WebAuthn authentication assertions. * - * This contract enables signature validation using WebAuthn authentication assertions, - * leveraging the P256 public key stored in the contract. It allows for both WebAuthn - * and raw P256 signature validation, providing compatibility with both signature types. + * This contract enables signature validation using WebAuthn authentication assertions, + * leveraging the P256 public key stored in the contract. Signatures that are not valid + * WebAuthn authentication assertions will be rejected. To also support raw P256 + * signatures, override {_rawSignatureValidation} and add a fallback to + * {SignerP256-_rawSignatureValidation}. *
🧹 Nitpick comments (1)
CHANGELOG.md (1)
10-10: Missing PR reference in changelog entry.Other changelog entries include PR references (e.g.,
([#5809](...))) for traceability. This entry should follow the same pattern.📝 Suggested fix
-- `SignerWebAuthn`: The `_rawSignatureValidation` function now returns `false` when the signature is not a valid WebAuthn authentication assertion. P256 fallback is removed. Developers can add it back by overriding the function. +- `SignerWebAuthn`: The `_rawSignatureValidation` function now returns `false` when the signature is not a valid WebAuthn authentication assertion. P256 fallback is removed. Developers can add it back by overriding the function. ([`#6337`](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/6337))
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Fixes #????
PR Checklist
npx changeset add)