From 8bf74ed007c52fd0be266da3aac544f454719f4e Mon Sep 17 00:00:00 2001 From: Zachary Huff Date: Fri, 21 Jan 2022 07:06:24 -0500 Subject: [PATCH 1/5] Fix panic on invalid client data --- protocol/authenticator.go | 18 ++++++++++++++---- protocol/authenticator_test.go | 11 +++++++---- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/protocol/authenticator.go b/protocol/authenticator.go index dbdbcf5..c889c80 100644 --- a/protocol/authenticator.go +++ b/protocol/authenticator.go @@ -7,7 +7,10 @@ import ( "github.com/duo-labs/webauthn/protocol/webauthncbor" ) -var minAuthDataLength = 37 +var ( + minAuthDataLength = 37 + minAttestedAuthLength = 55 +) // Authenticators respond to Relying Party requests by returning an object derived from the // AuthenticatorResponse interface. See ยง5.2. Authenticator Responses @@ -158,8 +161,11 @@ func (a *AuthenticatorData) Unmarshal(rawAuthData []byte) error { remaining := len(rawAuthData) - minAuthDataLength if a.Flags.HasAttestedCredentialData() { - if len(rawAuthData) > minAuthDataLength { - a.unmarshalAttestedData(rawAuthData) + if len(rawAuthData) > minAttestedAuthLength { + validError := a.unmarshalAttestedData(rawAuthData) + if validError != nil { + return validError + } attDataLen := len(a.AttData.AAGUID) + 2 + len(a.AttData.CredentialID) + len(a.AttData.CredentialPublicKey) remaining = remaining - attDataLen } else { @@ -188,11 +194,15 @@ func (a *AuthenticatorData) Unmarshal(rawAuthData []byte) error { } // If Attestation Data is present, unmarshall that into the appropriate public key structure -func (a *AuthenticatorData) unmarshalAttestedData(rawAuthData []byte) { +func (a *AuthenticatorData) unmarshalAttestedData(rawAuthData []byte) error { a.AttData.AAGUID = rawAuthData[37:53] idLength := binary.BigEndian.Uint16(rawAuthData[53:55]) + if len(rawAuthData) < int(55+idLength) { + return ErrBadRequest.WithDetails("Authenticator attestation data length too short") + } a.AttData.CredentialID = rawAuthData[55 : 55+idLength] a.AttData.CredentialPublicKey = unmarshalCredentialPublicKey(rawAuthData[55+idLength:]) + return nil } // Unmarshall the credential's Public Key into CBOR encoding diff --git a/protocol/authenticator_test.go b/protocol/authenticator_test.go index bc42b04..941ef52 100644 --- a/protocol/authenticator_test.go +++ b/protocol/authenticator_test.go @@ -184,9 +184,10 @@ func TestAuthenticatorData_unmarshalAttestedData(t *testing.T) { rawAuthData []byte } tests := []struct { - name string - fields fields - args args + name string + fields fields + args args + wantErr bool }{ // TODO: Add test cases. } @@ -199,7 +200,9 @@ func TestAuthenticatorData_unmarshalAttestedData(t *testing.T) { AttData: tt.fields.AttData, ExtData: tt.fields.ExtData, } - a.unmarshalAttestedData(tt.args.rawAuthData) + if err := a.unmarshalAttestedData(tt.args.rawAuthData); (err != nil) != tt.wantErr { + t.Errorf("AuthenticatorData.unmarshalAttestedData() error = %v, wantErr %v", err, tt.wantErr) + } }) } } From c466d6ebd75eb718d152968396dba1bdcfe7a6b7 Mon Sep 17 00:00:00 2001 From: William Storey Date: Tue, 29 Mar 2022 21:46:51 +0000 Subject: [PATCH 2/5] Check if ID is too long --- protocol/authenticator.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/protocol/authenticator.go b/protocol/authenticator.go index 2ca4612..6ac1790 100644 --- a/protocol/authenticator.go +++ b/protocol/authenticator.go @@ -216,6 +216,9 @@ func (a *AuthenticatorData) unmarshalAttestedData(rawAuthData []byte) error { if len(rawAuthData) < int(55+idLength) { return ErrBadRequest.WithDetails("Authenticator attestation data length too short") } + if idLength > 1023 { + return ErrBadRequest.WithDetails("Authenticator attestation data credential id length too long") + } a.AttData.CredentialID = rawAuthData[55 : 55+idLength] a.AttData.CredentialPublicKey = unmarshalCredentialPublicKey(rawAuthData[55+idLength:]) return nil From c3adc0ca19acf2534cae330efaab3bd9dc25ed92 Mon Sep 17 00:00:00 2001 From: William Storey Date: Tue, 29 Mar 2022 21:47:55 +0000 Subject: [PATCH 3/5] Change minimum variables to constants --- protocol/authenticator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocol/authenticator.go b/protocol/authenticator.go index 6ac1790..b928832 100644 --- a/protocol/authenticator.go +++ b/protocol/authenticator.go @@ -8,7 +8,7 @@ import ( "github.com/duo-labs/webauthn/protocol/webauthncbor" ) -var ( +const ( minAuthDataLength = 37 minAttestedAuthLength = 55 ) From 4bc16d79e17c91e499ee9eca83e15018c27c80c9 Mon Sep 17 00:00:00 2001 From: William Storey Date: Tue, 29 Mar 2022 21:49:22 +0000 Subject: [PATCH 4/5] Move value to constant --- protocol/authenticator.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/protocol/authenticator.go b/protocol/authenticator.go index b928832..d366053 100644 --- a/protocol/authenticator.go +++ b/protocol/authenticator.go @@ -11,6 +11,9 @@ import ( const ( minAuthDataLength = 37 minAttestedAuthLength = 55 + + // https://w3c.github.io/webauthn/#attested-credential-data + maxCredentialIDLength = 1023 ) // Authenticators respond to Relying Party requests by returning an object derived from the @@ -216,7 +219,7 @@ func (a *AuthenticatorData) unmarshalAttestedData(rawAuthData []byte) error { if len(rawAuthData) < int(55+idLength) { return ErrBadRequest.WithDetails("Authenticator attestation data length too short") } - if idLength > 1023 { + if idLength > maxCredentialIDLength { return ErrBadRequest.WithDetails("Authenticator attestation data credential id length too long") } a.AttData.CredentialID = rawAuthData[55 : 55+idLength] From a8c3693eeaca29f3b900a675c9302a7f438d5ed4 Mon Sep 17 00:00:00 2001 From: William Storey Date: Tue, 29 Mar 2022 22:15:02 +0000 Subject: [PATCH 5/5] Add tests --- protocol/authenticator_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/protocol/authenticator_test.go b/protocol/authenticator_test.go index 032e37b..7d43ab6 100644 --- a/protocol/authenticator_test.go +++ b/protocol/authenticator_test.go @@ -2,6 +2,7 @@ package protocol import ( "encoding/base64" + "encoding/binary" "reflect" "testing" ) @@ -133,6 +134,14 @@ func TestAuthenticatorData_Unmarshal(t *testing.T) { noneAuthData, _ := base64.StdEncoding.DecodeString("pkLSG3xtVeHOI8U5mCjSx0m/am7y/gPMnhDN9O1TCItBAAAAAAAAAAAAAAAAAAAAAAAAAAAAQMAxl6G32ykWaLrv/ouCs5HoGsvONqBtOb7ZmyMs8K8PccnwyyqPzWn/yZuyQmQBguvjYSvH6gDBlFG65quUDCSlAQIDJiABIVggyJGP+ra/u/eVjqN4OeYXUShRWxrEeC6Sb5/bZmJ9q8MiWCCHIkRdg5oRb1RHoFVYUpogcjlObCKFsV1ls1T+uUc6rA==") attAuthData, _ := base64.StdEncoding.DecodeString("lWkIjx7O4yMpVANdvRDXyuORMFonUbVZu4/Xy7IpvdRBAAAAAAAAAAAAAAAAAAAAAAAAAAAAQIniszxcGnhupdPFOHJIm6dscrWCC2h8xHicBMu91THD0kdOdB0QQtkaEn+6KfsfT1o3NmmFT8YfXrG734WfVSmlAQIDJiABIVggyoHHeiUw5aSbt8/GsL9zaqZGRzV26A4y3CnCGUhVXu4iWCBMnc8za5xgPzIygngAv9W+vZTMGJwwZcM4sjiqkcb/1g==") + attAuthDataIDExceedsBounds := make([]byte, len(attAuthData)) + copy(attAuthDataIDExceedsBounds, attAuthData) + binary.BigEndian.PutUint16(attAuthDataIDExceedsBounds[53:], 256) + + attAuthDataIDIsTooLarge := make([]byte, len(attAuthData)+2048) + copy(attAuthDataIDIsTooLarge, attAuthData) + binary.BigEndian.PutUint16(attAuthDataIDIsTooLarge[53:], maxCredentialIDLength+1) + tests := []struct { name string fields fields @@ -155,6 +164,22 @@ func TestAuthenticatorData_Unmarshal(t *testing.T) { }, false, }, + { + "Attestation data says the ID length is too large - extending beyond the buffer we're given", + fields{}, + args{ + attAuthDataIDExceedsBounds, + }, + true, + }, + { + "Attestation data says the ID length is too large - violates spec", + fields{}, + args{ + attAuthDataIDIsTooLarge, + }, + true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {