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

feat: top origin verification #217

Merged
merged 3 commits into from
Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions protocol/assertion.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,14 @@ func (car CredentialAssertionResponse) Parse() (par *ParsedCredentialAssertionDa
// documentation.
//
// Specification: §7.2 Verifying an Authentication Assertion (https://www.w3.org/TR/webauthn/#sctn-verifying-assertion)
func (p *ParsedCredentialAssertionData) Verify(storedChallenge string, relyingPartyID string, relyingPartyOrigins []string, appID string, verifyUser bool, credentialBytes []byte) error {
func (p *ParsedCredentialAssertionData) Verify(storedChallenge string, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, appID string, verifyUser bool, credentialBytes []byte) error {
// Steps 4 through 6 in verifying the assertion data (https://www.w3.org/TR/webauthn/#verifying-assertion) are
// "assertive" steps, i.e "Let JSONtext be the result of running UTF-8 decode on the value of cData."
// We handle these steps in part as we verify but also beforehand

// Handle steps 7 through 10 of assertion by verifying stored data against the Collected Client Data
// returned by the authenticator
validError := p.Response.CollectedClientData.Verify(storedChallenge, AssertCeremony, relyingPartyOrigins)
validError := p.Response.CollectedClientData.Verify(storedChallenge, AssertCeremony, rpOrigins, rpTopOrigins, rpTopOriginsVerify)
if validError != nil {
return validError
}
Expand Down
2 changes: 1 addition & 1 deletion protocol/assertion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func TestParsedCredentialAssertionData_Verify(t *testing.T) {
Raw: tt.fields.Raw,
}

if err := p.Verify(tt.args.storedChallenge.String(), tt.args.relyingPartyID, tt.args.relyingPartyOrigin, "", tt.args.verifyUser, tt.args.credentialBytes); (err != nil) != tt.wantErr {
if err := p.Verify(tt.args.storedChallenge.String(), tt.args.relyingPartyID, tt.args.relyingPartyOrigin, nil, TopOriginIgnoreVerificationMode, "", tt.args.verifyUser, tt.args.credentialBytes); (err != nil) != tt.wantErr {
Copy link

Choose a reason for hiding this comment

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

The addition of nil and TopOriginIgnoreVerificationMode as parameters in the Verify method call within TestParsedCredentialAssertionData_Verify aligns with the new verification logic introduced for topOrigin. However, it's crucial to ensure that corresponding test cases are added or updated to specifically test the behavior of topOrigin verification under different scenarios, including the TopOriginIgnoreVerificationMode. This will help in validating the correctness and robustness of the new feature.

t.Errorf("ParsedCredentialAssertionData.Verify() error = %v, wantErr %v", err, tt.wantErr)
}
})
Expand Down
2 changes: 1 addition & 1 deletion protocol/attestation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestAttestationVerify(t *testing.T) {
pcc.Response = *parsedAttestationResponse

// Test Base Verification
err = pcc.Verify(options.Response.Challenge.String(), false, options.Response.RelyingParty.ID, []string{options.Response.RelyingParty.Name})
err = pcc.Verify(options.Response.Challenge.String(), false, options.Response.RelyingParty.ID, []string{options.Response.RelyingParty.Name}, nil, TopOriginIgnoreVerificationMode)
if err != nil {
t.Fatalf("Not valid: %+v (%s)", err, err.(*Error).DevInfo)
}
Expand Down
85 changes: 82 additions & 3 deletions protocol/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ type CollectedClientData struct {
Type CeremonyType `json:"type"`
Challenge string `json:"challenge"`
Origin string `json:"origin"`
TopOrigin string `json:"topOrigin"`
CrossOrigin bool `json:"crossOrigin,omitempty"`
TokenBinding *TokenBinding `json:"tokenBinding,omitempty"`

// Chromium (Chrome) returns a hint sometimes about how to handle clientDataJSON in a safe manner.
Expand Down Expand Up @@ -77,7 +79,10 @@ func FullyQualifiedOrigin(rawOrigin string) (fqOrigin string, err error) {
// new credential and steps 7 through 10 of verifying an authentication assertion
// See https://www.w3.org/TR/webauthn/#registering-a-new-credential
// and https://www.w3.org/TR/webauthn/#verifying-assertion
func (c *CollectedClientData) Verify(storedChallenge string, ceremony CeremonyType, rpOrigins []string) error {
//
// Note: the rpTopOriginsVerify parameter does not accept the TopOriginVerificationMode value of
// TopOriginDefaultVerificationMode as it's expected this value is updated by the config validation process.
func (c *CollectedClientData) Verify(storedChallenge string, ceremony CeremonyType, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode) (err error) {
// Registration Step 3. Verify that the value of C.type is webauthn.create.

// Assertion Step 7. Verify that the value of C.type is the string webauthn.get.
Expand All @@ -101,8 +106,9 @@ func (c *CollectedClientData) Verify(storedChallenge string, ceremony CeremonyTy

// Registration Step 5 & Assertion Step 9. Verify that the value of C.origin matches
// the Relying Party's origin.
fqOrigin, err := FullyQualifiedOrigin(c.Origin)
if err != nil {
var fqOrigin string

if fqOrigin, err = FullyQualifiedOrigin(c.Origin); err != nil {
return ErrParsingData.WithDetails("Error decoding clientData origin as URL")
}

Expand All @@ -121,6 +127,54 @@ func (c *CollectedClientData) Verify(storedChallenge string, ceremony CeremonyTy
WithInfo(fmt.Sprintf("Expected Values: %s, Received: %s", rpOrigins, fqOrigin))
}

if rpTopOriginsVerify != TopOriginIgnoreVerificationMode {
switch len(c.TopOrigin) {
case 0:
break
default:
if !c.CrossOrigin {
return ErrVerification.
WithDetails("Error validating topOrigin").
WithInfo("The topOrigin can't have values unless crossOrigin is true.")
}

var (
fqTopOrigin string
possibleTopOrigins []string
)

if fqTopOrigin, err = FullyQualifiedOrigin(c.TopOrigin); err != nil {
return ErrParsingData.WithDetails("Error decoding clientData topOrigin as URL")
}

switch rpTopOriginsVerify {
case TopOriginExplicitVerificationMode:
possibleTopOrigins = rpTopOrigins
case TopOriginAutoVerificationMode:
possibleTopOrigins = append(rpTopOrigins, rpOrigins...)
case TopOriginImplicitVerificationMode:
possibleTopOrigins = rpOrigins
default:
return ErrNotImplemented.WithDetails("Error handling unknown Top Origin verification mode")
}

found = false

for _, origin := range possibleTopOrigins {
if strings.EqualFold(fqTopOrigin, origin) {
found = true
break
}
}

if !found {
return ErrVerification.
WithDetails("Error validating top origin").
WithInfo(fmt.Sprintf("Expected Values: %s, Received: %s", possibleTopOrigins, fqTopOrigin))
}
}
}

// Registration Step 6 and Assertion Step 10. Verify that the value of C.tokenBinding.status
// matches the state of Token Binding for the TLS connection over which the assertion was
// obtained. If Token Binding was used on that TLS connection, also verify that C.tokenBinding.id
Comment on lines 127 to 180
Copy link

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [82-177]

The modifications in the Verify method of CollectedClientData to include rpTopOrigins and rpTopOriginsVerify parameters, and the updated logic to handle TopOrigin verification based on different verification modes, are well-structured and align with the objectives of enhancing origin verification. However, it's important to ensure comprehensive testing of this logic under various scenarios to guarantee its correctness and robustness. Additionally, consider adding more detailed error messages to aid in debugging and troubleshooting.

Expand All @@ -140,3 +194,28 @@ func (c *CollectedClientData) Verify(storedChallenge string, ceremony CeremonyTy

return nil
}

type TopOriginVerificationMode int

const (
// TopOriginDefaultVerificationMode represents the default verification mode for the Top Origin. At this time this
// mode is the same as TopOriginIgnoreVerificationMode until such a time as the specification becomes stable. This
// value is intended as a fallback value and implementers should very intentionally pick another option if they want
// stability.
TopOriginDefaultVerificationMode TopOriginVerificationMode = iota

// TopOriginIgnoreVerificationMode ignores verification entirely.
TopOriginIgnoreVerificationMode

// TopOriginAutoVerificationMode represents the automatic verification mode for the Top Origin. In this mode the
// If the Top Origins parameter has values it checks against this, otherwise it checks against the Origins parameter.
TopOriginAutoVerificationMode

// TopOriginImplicitVerificationMode represents the implicit verification mode for the Top Origin. In this mode the
// Top Origin is verified against the allowed Origins values.
TopOriginImplicitVerificationMode

// TopOriginExplicitVerificationMode represents the explicit verification mode for the Top Origin. In this mode the
// Top Origin is verified against the allowed Top Origins values.
TopOriginExplicitVerificationMode
)
8 changes: 4 additions & 4 deletions protocol/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func TestVerifyCollectedClientData(t *testing.T) {

var storedChallenge = newChallenge

if err = ccd.Verify(storedChallenge.String(), ccd.Type, []string{ccd.Origin}); err != nil {
if err = ccd.Verify(storedChallenge.String(), ccd.Type, []string{ccd.Origin}, nil, TopOriginIgnoreVerificationMode); err != nil {
t.Fatalf("error verifying challenge: expected %#v got %#v", ccd.Challenge, storedChallenge)
}
}
Expand All @@ -44,7 +44,7 @@ func TestVerifyCollectedClientDataIncorrectChallenge(t *testing.T) {
t.Fatalf("error creating challenge: %s", err)
}

if err = ccd.Verify(bogusChallenge.String(), ccd.Type, []string{ccd.Origin}); err == nil {
if err = ccd.Verify(bogusChallenge.String(), ccd.Type, []string{ccd.Origin}, nil, TopOriginIgnoreVerificationMode); err == nil {
t.Fatalf("error expected but not received. expected %#v got %#v", ccd.Challenge, bogusChallenge)
}
}
Expand All @@ -59,7 +59,7 @@ func TestVerifyCollectedClientDataUnexpectedOrigin(t *testing.T) {
storedChallenge := newChallenge
expectedOrigins := []string{"http://different.com"}

if err = ccd.Verify(storedChallenge.String(), ccd.Type, expectedOrigins); err == nil {
if err = ccd.Verify(storedChallenge.String(), ccd.Type, expectedOrigins, nil, TopOriginIgnoreVerificationMode); err == nil {
t.Fatalf("error expected but not received. expected %#v got %#v", expectedOrigins, ccd.Origin)
}
}
Expand All @@ -76,7 +76,7 @@ func TestVerifyCollectedClientDataWithMultipleExpectedOrigins(t *testing.T) {

expectedOrigins := []string{"https://exmaple.com", "9C:B4:AE:EF:05:53:6E:73:0E:C4:B8:02:E7:67:F6:7D:A4:E7:BC:26:D7:42:B5:27:FF:01:7D:68:2A:EB:FA:1D", ccd.Origin}

if err = ccd.Verify(storedChallenge.String(), ccd.Type, expectedOrigins); err != nil {
if err = ccd.Verify(storedChallenge.String(), ccd.Type, expectedOrigins, nil, TopOriginIgnoreVerificationMode); err != nil {
t.Fatalf("error verifying challenge: expected %#v got %#v", expectedOrigins, ccd.Origin)
}
}
Expand Down
4 changes: 2 additions & 2 deletions protocol/credential.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ func (ccr CredentialCreationResponse) Parse() (pcc *ParsedCredentialCreationData
// Verify the Client and Attestation data.
//
// Specification: §7.1. Registering a New Credential (https://www.w3.org/TR/webauthn/#sctn-registering-a-new-credential)
func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, relyingPartyID string, relyingPartyOrigins []string) error {
func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode) error {
// Handles steps 3 through 6 - Verifying the Client Data against the Relying Party's stored data
verifyError := pcc.Response.CollectedClientData.Verify(storedChallenge, CreateCeremony, relyingPartyOrigins)
verifyError := pcc.Response.CollectedClientData.Verify(storedChallenge, CreateCeremony, rpOrigins, rpTopOrigins, rpTopOriginsVerify)
if verifyError != nil {
return verifyError
}
Expand Down
2 changes: 1 addition & 1 deletion protocol/credential_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ func TestParsedCredentialCreationData_Verify(t *testing.T) {
Response: tt.fields.Response,
Raw: tt.fields.Raw,
}
if err := pcc.Verify(tt.args.storedChallenge.String(), tt.args.verifyUser, tt.args.relyingPartyID, tt.args.relyingPartyOrigin); (err != nil) != tt.wantErr {
if err := pcc.Verify(tt.args.storedChallenge.String(), tt.args.verifyUser, tt.args.relyingPartyID, tt.args.relyingPartyOrigin, nil, TopOriginIgnoreVerificationMode); (err != nil) != tt.wantErr {
t.Errorf("ParsedCredentialCreationData.Verify() error = %+v, wantErr %v", err, tt.wantErr)
}
})
Expand Down
3 changes: 2 additions & 1 deletion webauthn/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -285,14 +285,15 @@ func (webauthn *WebAuthn) validateLogin(user User, session SessionData, parsedRe

rpID := webauthn.Config.RPID
rpOrigins := webauthn.Config.RPOrigins
rpTopOrigins := webauthn.Config.RPTopOrigins

appID, err := parsedResponse.GetAppID(session.Extensions, loginCredential.AttestationType)
if err != nil {
return nil, err
}

// Handle steps 4 through 16.
validError := parsedResponse.Verify(session.Challenge, rpID, rpOrigins, appID, shouldVerifyUser, loginCredential.PublicKey)
validError := parsedResponse.Verify(session.Challenge, rpID, rpOrigins, rpTopOrigins, webauthn.Config.RPTopOriginVerificationMode, appID, shouldVerifyUser, loginCredential.PublicKey)
if validError != nil {
return nil, validError
}
Expand Down
2 changes: 1 addition & 1 deletion webauthn/registration.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ func (webauthn *WebAuthn) CreateCredential(user User, session SessionData, parse

shouldVerifyUser := session.UserVerification == protocol.VerificationRequired

invalidErr := parsedResponse.Verify(session.Challenge, shouldVerifyUser, webauthn.Config.RPID, webauthn.Config.RPOrigins)
invalidErr := parsedResponse.Verify(session.Challenge, shouldVerifyUser, webauthn.Config.RPID, webauthn.Config.RPOrigins, webauthn.Config.RPTopOrigins, webauthn.Config.RPTopOriginVerificationMode)
if invalidErr != nil {
return nil, invalidErr
}
Expand Down
18 changes: 18 additions & 0 deletions webauthn/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ type Config struct {
// qualified origins.
RPOrigins []string

// RPTopOrigins configures the list of Relying Party Server Top Origins that are permitted. These should be fully
// qualified origins.
RPTopOrigins []string

// RPTopOriginVerificationMode determines the verification mode for the Top Origin value. By default the
// TopOriginIgnoreVerificationMode is used however this is going to change at such a time as WebAuthn Level 3
// becomes recommended, implementers should explicitly set this value if they want stability.
RPTopOriginVerificationMode protocol.TopOriginVerificationMode
Comment on lines +39 to +46
Copy link

Choose a reason for hiding this comment

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

The addition of RPTopOrigins and RPTopOriginVerificationMode fields to the Config struct is a significant enhancement for configuring top origin verification. Ensure that the documentation for these fields is clear and provides guidance on how to use them effectively. Additionally, consider validating the contents of RPTopOrigins to ensure they are well-formed URLs.

+ // Ensure RPTopOrigins contains well-formed URLs.
+ for _, origin := range config.RPTopOrigins {
+     if _, err := url.ParseRequestURI(origin); err != nil {
+         return fmt.Errorf("RPTopOrigins contains an invalid URL: %s", origin)
+     }
+ }

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
// RPTopOrigins configures the list of Relying Party Server Top Origins that are permitted. These should be fully
// qualified origins.
RPTopOrigins []string
// RPTopOriginVerificationMode determines the verification mode for the Top Origin value. By default the
// TopOriginIgnoreVerificationMode is used however this is going to change at such a time as WebAuthn Level 3
// becomes recommended, implementers should explicitly set this value if they want stability.
RPTopOriginVerificationMode protocol.TopOriginVerificationMode
// RPTopOrigins configures the list of Relying Party Server Top Origins that are permitted. These should be fully
// qualified origins.
RPTopOrigins []string
// Ensure RPTopOrigins contains well-formed URLs.
for _, origin := range config.RPTopOrigins {
if _, err := url.ParseRequestURI(origin); err != nil {
return fmt.Errorf("RPTopOrigins contains an invalid URL: %s", origin)
}
}
// RPTopOriginVerificationMode determines the verification mode for the Top Origin value. By default the
// TopOriginIgnoreVerificationMode is used however this is going to change at such a time as WebAuthn Level 3
// becomes recommended, implementers should explicitly set this value if they want stability.
RPTopOriginVerificationMode protocol.TopOriginVerificationMode


// AttestationPreference sets the default attestation conveyance preferences.
AttestationPreference protocol.ConveyancePreference

Expand Down Expand Up @@ -153,6 +162,15 @@ func (config *Config) validate() error {
return fmt.Errorf("must provide at least one value to the 'RPOrigins' field")
}

switch config.RPTopOriginVerificationMode {
case protocol.TopOriginDefaultVerificationMode:
config.RPTopOriginVerificationMode = protocol.TopOriginIgnoreVerificationMode
case protocol.TopOriginImplicitVerificationMode:
if len(config.RPTopOrigins) == 0 {
return fmt.Errorf("must provide at least one value to the 'RPTopOrigins' field when 'RPTopOriginVerificationMode' field is set to protocol.TopOriginImplicitVerificationMode")
}
}

if config.AuthenticatorSelection.RequireResidentKey == nil {
config.AuthenticatorSelection.RequireResidentKey = protocol.ResidentKeyNotRequired()
}
Expand Down
Loading