Skip to content
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.

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

test(protocol): add coverage to 256bit ecdsa curve #72

Merged

Conversation

ArnaudBrousseau
Copy link
Contributor

[this repost of https://github.com/duo-labs/webauthn/pull/158, after seeing https://github.com/duo-labs/webauthn/issues/155]

For a few hours I was convinced that this library (webauthncose package specifically) wasn't doing its job properly because my code was accepting bad signatures as valid. So I set out to write a test to capture this. Turns out: this library is doing its job, and my code was at fault!

Shameful details
func checkWebauthnSignature(...) error {
    valid, err := webauthncose.VerifySignature(key, message, sigBytes)
    if !valid || err != nil {
        return errors.Wrap(err, "error verifying signature")
    }
}

The bug in the above snippet: errors.Wrap(nil, "something") is nil. Which means when valid is false, nil is returned 🤦

Instead of discarding this test I thought it'd be a good addition to the test suite.

@ArnaudBrousseau ArnaudBrousseau requested a review from a team as a code owner November 17, 2022 05:13
@james-d-elliott james-d-elliott changed the title Extra coverage in webauthncose (P-256 signature test vector) test(protocol): add coverage to 256bit ecdsa curve Nov 17, 2022
Copy link
Member

@james-d-elliott james-d-elliott left a comment

Choose a reason for hiding this comment

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

LGTM

@james-d-elliott james-d-elliott merged commit 568f8c6 into go-webauthn:master Nov 17, 2022
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.

None yet

2 participants