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.

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

Jump to bottom

Webauthn Credential ID length too short #20457

Closed
spotlightishere opened this issue Jul 22, 2022 · 6 comments · Fixed by #20530
Closed

Webauthn Credential ID length too short #20457

spotlightishere opened this issue Jul 22, 2022 · 6 comments · Fixed by #20530
Labels
type/bug

Comments

@spotlightishere
Copy link

spotlightishere commented Jul 22, 2022 •
edited
Loading

Description

Hi all 👋 I recently received a new FIDO2 security key, a SoloKey v2.

Upon registering it to my Gitea instance, a server error was encountered:

WebauthnRegisterPost() [E] [62db09ae] CreateCredential: pq: value too long for type character varying(410)

While debugging this, I noticed that a 270 byte credential ID was generated for my instance. It seems that the current maximum credential ID length is 255 bytes:

gitea/models/migrations/v210.go

Line 28 in 886b1de

CredentialID string `xorm:"INDEX VARCHAR(410)"` // CredentalID in U2F is at most 255bytes / 5 * 8 = 408 - add a few extra characters for safety

After some tentative searching through the linked WebAuthn specification, I noticed the following:

Credential ID

A probabilistically-unique byte sequence identifying a public key credential source and its authentication assertions. At most 1023 bytes long.

I think this issue could be resolved by changing the maximum credential ID length to 1023 bytes raw, or 1640 bytes base32-encoded (per head -c 1023 /dev/random | base32 --wrap=0 | wc -c).

Gitea Version

1.17.0-rc2

Can you reproduce the bug on the Gitea demo site?

No - its credential ID was exactly 255 bytes, which probably shouldn't be relied on

Log Gist

https://gist.github.com/spotlightishere/8bcfd9591ca7ec421873091c77644e91

Screenshots

No response

Git Version

No response

Operating System

Debian 11

How are you running Gitea?

Downloaded from https://dl.gitea.io/, running underneath systemd

Database

PostgreSQL
@Gusted
Copy link
Contributor

Gusted commented Jul 22, 2022 •
edited
Loading

CC @zeripath migration v4? Didn't expect another issue to pop up after you fiddling with this for so long.

For context: #18770 this isn't the first time issues with this length has popped up.

@zeripath
Copy link
Contributor

ffs.

They changed the spec.

THIS WAS THE SPEC https://www.w3.org/TR/2021/REC-webauthn-2-20210408/#credential-id

@Gusted
Copy link
Contributor

Gusted commented Jul 22, 2022

w3c/webauthn#1664 changed the spec

@zeripath
Copy link
Contributor

anyway we'll need yet another migration and we'll need to backport the other migration changes.

I don't know what we can do for 1.16 - a doctor command perhaps.

@Gusted
Copy link
Contributor

Gusted commented Jul 22, 2022

1.17 should be around the corner. There aren't any other commit pending for a 1.16.10 release. "Just upgrade" advice?

@zeripath
Copy link
Contributor

As an aside:

This is literally unbelievable they changed the spec 2 days ago and we're already getting behaviour that demands the new spec?!

It's just unreasonable

@Erisa Erisa mentioned this issue Jul 25, 2022
Merged
zeripath added a commit to zeripath/gitea that referenced this issue Jul 28, 2022
@zeripath
WebAuthn CredentialID field needs to be increased in size
835cf4c 
WebAuthn have updated their specification to set the maximum size of the
CredentialID to 1023 bytes. This is somewhat larger than our current
size and therefore we need to migrate.

Fix go-gitea#20457

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this issue Jul 28, 2022
Merged
6543 pushed a commit that referenced this issue Jul 30, 2022
@zeripath
WebAuthn CredentialID field needs to be increased in size (#20530)
e819da0 
WebAuthn have updated their specification to set the maximum size of the
CredentialID to 1023 bytes. This is somewhat larger than our current
size and therefore we need to migrate.

The PR changes the struct to add CredentialIDBytes and migrates the CredentialID string 
to the bytes field before another migration drops the old CredentialID field. Another migration
renames this field back.

Fix #20457

Signed-off-by: Andrew Thornton <art27@cantab.net>
6543 pushed a commit to 6543-forks/gitea that referenced this issue Jul 30, 2022
@zeripath @6543
WebAuthn CredentialID field needs to be increased in size (go-gitea#2…
c5a1289 
…0530)

WebAuthn have updated their specification to set the maximum size of the
CredentialID to 1023 bytes. This is somewhat larger than our current
size and therefore we need to migrate.

The PR changes the struct to add CredentialIDBytes and migrates the CredentialID string
to the bytes field before another migration drops the old CredentialID field. Another migration
renames this field back.

Fix go-gitea#20457

Signed-off-by: Andrew Thornton <art27@cantab.net>
6543 added a commit that referenced this issue Jul 30, 2022
@6543 @zeripath
WebAuthn CredentialID field needs to be increased in size (#20530) (#…
09f2e1e 
…20555)

WebAuthn have updated their specification to set the maximum size of the
CredentialID to 1023 bytes. This is somewhat larger than our current
size and therefore we need to migrate.

The PR changes the struct to add CredentialIDBytes and migrates the CredentialID string
to the bytes field before another migration drops the old CredentialID field. Another migration
renames this field back.

Fix #20457

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: zeripath <art27@cantab.net>
vsysoev pushed a commit to IntegraSDL/gitea that referenced this issue Aug 10, 2022
@zeripath
WebAuthn CredentialID field needs to be increased in size (go-gitea#2…
d6f15bf 
…0530)

WebAuthn have updated their specification to set the maximum size of the
CredentialID to 1023 bytes. This is somewhat larger than our current
size and therefore we need to migrate.

The PR changes the struct to add CredentialIDBytes and migrates the CredentialID string 
to the bytes field before another migration drops the old CredentialID field. Another migration
renames this field back.

Fix go-gitea#20457

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
type/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WebAuthn CredentialID field needs to be increased in size zeripath/gitea
3 participants
@zeripath @spotlightishere @Gusted