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

Inconsistent base64 encoding/decoding via JSON #97

Closed
ricecake opened this issue Jan 26, 2023 · 6 comments · Fixed by #98
Closed

Inconsistent base64 encoding/decoding via JSON #97

ricecake opened this issue Jan 26, 2023 · 6 comments · Fixed by #98
Labels
status/needs-triage Issues that need to be triaged. type/potential-bug Potential Bugs

Comments

@ricecake
Copy link

Version

0.3.1

Description

The protocol.CredentialCreation object returned by webauthn.BeginRegistration uses the WebauthnID method from the webauthn.User interface to get the Id of the user.

If this object is serialized via the encoding/json module, the field gets serialized as base64 via base64.StdEncoding.

Later, after registration, if the credential is sent back as part of the login process, it will be decoded as the UserHandle portion of a AuthenticatorAssertionResponse, which has the type defined as URLEncodedBase64, which defines a custom json marshaller which builds on base64.RawURLEncoding.

Since these two base64 encoding aren't compatible, parsing the assertion fails and login can't proceed, specifically generating the error "Parse error for Assertion".

Reproduction

This issue can be reproduced by registering a new credential, and returning the result of BeginRegistration directly encoded as json. Finish the registration without changing or parsing the Id on the client side.

Attempt to use the credential without changing the UserHandle field after deserialzing via the standard json encoder.

Expectations

The expected behavior is that the WebauthnID as returned from the interface method would be able to make a roundtrip in this fashion without needed modification on the server or client side.

Documentation

Updating the locations that reference []byte to use URLEncodedBase64 should do it, but there might be backwards compatibility concerns?

WebAuthnID() []byte

ID []byte `json:"id"`

UserHandle URLEncodedBase64 `json:"userHandle,omitempty"`

return nil, ErrBadRequest.WithDetails("Parse error for Assertion")

@ricecake ricecake added status/needs-triage Issues that need to be triaged. type/potential-bug Potential Bugs labels Jan 26, 2023
@james-d-elliott
Copy link
Member

james-d-elliott commented Jan 26, 2023

Can you try with the following edit to confirm I understand the problem accurately:

go mod edit -replace=github.com/go-webauthn/webauthn=github.com/go-webauthn/[email protected]

@ricecake
Copy link
Author

That did seem to resolve the issue.

@ricecake
Copy link
Author

Looking through the change in #95 , I think that for it to properly solve this issue in all cases, it would also need to replace '+' and '/' with '-' and '_' respectively.
That way it'll fully convert the standard base64 encoding used by encoding/json into the unpadded url safe encoding being used here.

@james-d-elliott
Copy link
Member

james-d-elliott commented Jan 26, 2023

The intention in #95 is to avoid compatibility issues specifically with padding, as it's reasonable to accommodate that I think. I'm willing to take a harder stance on Standard Base64 encoding as it's not permitted at all within the webauthn spec unless I'm mistaken?

Also I think your fix is still appropriate since we should only handle encoding with the web based (without padding) encoding per the spec.

@james-d-elliott
Copy link
Member

Can you check #97 also solves your issue? I suspect it will. In which case I'll merge that before the others and release it as a patch.

james-d-elliott added a commit that referenced this issue Jan 28, 2023
This fixes an issue where the protocol.UserEntity field ID was encoded with Base64 URL Encoding with padding instead of without padding.

Fixes #97
@ricecake
Copy link
Author

Just to circle back and confirm: That did resolve the issue. Thanks a bunch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/needs-triage Issues that need to be triaged. type/potential-bug Potential Bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants