diff --git a/service_provider.go b/service_provider.go index c97886d0..59157fc7 100644 --- a/service_provider.go +++ b/service_provider.go @@ -986,12 +986,8 @@ const ( // and properties are met. func (sp *ServiceProvider) parseResponse(responseEl *etree.Element, possibleRequestIDs []string, now time.Time, signatureRequirement signatureRequirement, currentURL url.URL) (*Assertion, error) { var responseSignatureErr error - var responseHasSignature bool if signatureRequirement == signatureRequired { responseSignatureErr = sp.validateSignature(responseEl) - if responseSignatureErr != errSignatureElementNotPresent { - responseHasSignature = true - } // Note: we're deferring taking action on the signature validation until after we've // processed the request attributes, because certain test cases seem to require this mis-feature. @@ -1005,14 +1001,18 @@ func (sp *ServiceProvider) parseResponse(responseEl *etree.Element, possibleRequ return nil, fmt.Errorf("cannot unmarshal response: %v", err) } - // If the response is *not* signed, the Destination may be omitted. - if responseHasSignature || response.Destination != "" { - // Per section 3.4.5.2 of the SAML spec, Destination must match the location at which the response was received, i.e. currentURL. - // Historically, we checked against the SP's ACS URL instead of currentURL, which is usually the same but may differ in query params. - // To mitigate the risk of switching to comparing against currentURL, we still allow it if the ACS URL matches, even if the current URL doesn't. - if response.Destination != currentURL.String() && response.Destination != sp.AcsURL.String() { - return nil, fmt.Errorf("`Destination` does not match requested URL or AcsURL (destination %q, requested %q, acs %q)", response.Destination, currentURL.String(), sp.AcsURL.String()) - } + // Per section 3.4.5.2 of the SAML spec, Destination MUST be present + // when a signature is present, and SHOULD be present otherwise. We + // require it always: for unsigned responses the Destination is the + // only non-cryptographic binding to this SP, and omitting it would + // let an attacker replay a response across service providers. + if response.Destination == "" { + return nil, fmt.Errorf("`Destination` is required but missing") + } + // Historically, we checked against the SP's ACS URL instead of currentURL, which is usually the same but may differ in query params. + // To mitigate the risk of switching to comparing against currentURL, we still allow it if the ACS URL matches, even if the current URL doesn't. + if response.Destination != currentURL.String() && response.Destination != sp.AcsURL.String() { + return nil, fmt.Errorf("`Destination` does not match requested URL or AcsURL (destination %q, requested %q, acs %q)", response.Destination, currentURL.String(), sp.AcsURL.String()) } if err := sp.validateRequestID(response, possibleRequestIDs); err != nil { diff --git a/service_provider_test.go b/service_provider_test.go index 35103d6b..189bb3a5 100644 --- a/service_provider_test.go +++ b/service_provider_test.go @@ -946,7 +946,7 @@ func (test *ServiceProviderTest) replaceDestination(newDestination string) { []byte(`Destination="https://15661444.ngrok.io/saml2/acs"`), []byte(newStr), 1) } -func TestSPCanProcessResponseWithoutDestination(t *testing.T) { +func TestSPRejectsResponseWithoutDestination(t *testing.T) { test := NewServiceProviderTest(t) s := ServiceProvider{ Key: test.Key, @@ -962,7 +962,41 @@ func TestSPCanProcessResponseWithoutDestination(t *testing.T) { test.replaceDestination("") req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString(test.SamlResponse)) _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) + assert.Check(t, is.Error(err.(*InvalidResponseError).PrivateErr, + "`Destination` is required but missing")) +} + +func TestSPRejectsUnsignedResponseWithoutDestination(t *testing.T) { + // Regression test for issue #12: On main, unsigned responses without + // Destination were silently accepted. The fix requires Destination always, + // because for unsigned responses it is the only binding to this SP. + test := NewServiceProviderTest(t) + s := ServiceProvider{ + Key: test.Key, + Certificate: test.Certificate, + MetadataURL: mustParseURL("https://15661444.ngrok.io/saml2/metadata"), + AcsURL: mustParseURL("https://15661444.ngrok.io/saml2/acs"), + IDPMetadata: &EntityDescriptor{}, + } + err := xml.Unmarshal(test.IDPMetadata, &s.IDPMetadata) assert.Check(t, err) + + // Build a minimal unsigned SAML response with no Destination attribute. + // This simulates an IdP sending a response without any audience binding. + unsignedResp := `` + + `` + + `https://idp.testshib.org/idp/shibboleth` + + `` + + `` + + req := http.Request{PostForm: url.Values{}, URL: &s.AcsURL} + req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString([]byte(unsignedResp))) + _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) + assert.Check(t, is.Error(err.(*InvalidResponseError).PrivateErr, + "`Destination` is required but missing")) } func (test *ServiceProviderTest) responseDom(t *testing.T) (doc *etree.Document) { @@ -1079,7 +1113,7 @@ func TestServiceProviderMissingDestinationWithSignaturePresent(t *testing.T) { req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString(bytes)) _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) assert.Check(t, is.Error(err.(*InvalidResponseError).PrivateErr, - "`Destination` does not match requested URL or AcsURL (destination \"\", requested \"https://15661444.ngrok.io/saml2/acs\", acs \"https://15661444.ngrok.io/saml2/acs\")")) + "`Destination` is required but missing")) } func TestSPMismatchedDestinationsWithSignaturePresent(t *testing.T) { @@ -1142,7 +1176,7 @@ func TestSPMissingDestinationWithSignaturePresent(t *testing.T) { req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString(bytes)) _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) assert.Check(t, is.Error(err.(*InvalidResponseError).PrivateErr, - "`Destination` does not match requested URL or AcsURL (destination \"\", requested \"https://15661444.ngrok.io/saml2/acs\", acs \"https://15661444.ngrok.io/saml2/acs\")")) + "`Destination` is required but missing")) } func TestSPInvalidAssertions(t *testing.T) {