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

fix: check Destination url against the request URL or the ACS url #577

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: check Destination url against the request URL or the ACS url #577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 14 additions & 11 deletions service_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ func (sp *ServiceProvider) handleArtifactRequest(ctx context.Context, artifactID
retErr.PrivateErr = fmt.Errorf("Error during artifact resolution: %s", err)
return nil, retErr
}
assertion, err := sp.ParseXMLArtifactResponse(responseBody, possibleRequestIDs, artifactResolveRequest.ID)
assertion, err := sp.ParseXMLArtifactResponse(responseBody, possibleRequestIDs, artifactResolveRequest.ID, *req.URL)
if err != nil {
return nil, err
}
Expand All @@ -670,7 +670,7 @@ func (sp *ServiceProvider) parseResponseHTTP(req *http.Request, possibleRequestI
return nil, retErr
}

assertion, err := sp.ParseXMLResponse(rawResponseBuf, possibleRequestIDs)
assertion, err := sp.ParseXMLResponse(rawResponseBuf, possibleRequestIDs, *req.URL)
if err != nil {
return nil, err
}
Expand All @@ -687,7 +687,7 @@ func (sp *ServiceProvider) parseResponseHTTP(req *http.Request, possibleRequestI
// properties are useful in describing which part of the parsing process
// failed. However, to discourage inadvertent disclosure the diagnostic
// information, the Error() method returns a static string.
func (sp *ServiceProvider) ParseXMLArtifactResponse(soapResponseXML []byte, possibleRequestIDs []string, artifactRequestID string) (*Assertion, error) {
func (sp *ServiceProvider) ParseXMLArtifactResponse(soapResponseXML []byte, possibleRequestIDs []string, artifactRequestID string, currentURL url.URL) (*Assertion, error) {
now := TimeNow()
retErr := &InvalidResponseError{
Response: string(soapResponseXML),
Expand Down Expand Up @@ -727,10 +727,10 @@ func (sp *ServiceProvider) ParseXMLArtifactResponse(soapResponseXML []byte, poss
return nil, retErr
}

return sp.parseArtifactResponse(artifactResponseEl, possibleRequestIDs, artifactRequestID, now)
return sp.parseArtifactResponse(artifactResponseEl, possibleRequestIDs, artifactRequestID, now, currentURL)
}

func (sp *ServiceProvider) parseArtifactResponse(artifactResponseEl *etree.Element, possibleRequestIDs []string, artifactRequestID string, now time.Time) (*Assertion, error) {
func (sp *ServiceProvider) parseArtifactResponse(artifactResponseEl *etree.Element, possibleRequestIDs []string, artifactRequestID string, now time.Time, currentURL url.URL) (*Assertion, error) {
retErr := &InvalidResponseError{
Now: now,
Response: elementToString(artifactResponseEl),
Expand Down Expand Up @@ -778,7 +778,7 @@ func (sp *ServiceProvider) parseArtifactResponse(artifactResponseEl *etree.Eleme
return nil, retErr
}

assertion, err := sp.parseResponse(responseEl, possibleRequestIDs, now, signatureRequirement)
assertion, err := sp.parseResponse(responseEl, possibleRequestIDs, now, signatureRequirement, currentURL)
if err != nil {
retErr.PrivateErr = err
return nil, retErr
Expand All @@ -798,7 +798,7 @@ func (sp *ServiceProvider) parseArtifactResponse(artifactResponseEl *etree.Eleme
// properties are useful in describing which part of the parsing process
// failed. However, to discourage inadvertent disclosure the diagnostic
// information, the Error() method returns a static string.
func (sp *ServiceProvider) ParseXMLResponse(decodedResponseXML []byte, possibleRequestIDs []string) (*Assertion, error) {
func (sp *ServiceProvider) ParseXMLResponse(decodedResponseXML []byte, possibleRequestIDs []string, currentURL url.URL) (*Assertion, error) {
now := TimeNow()
var err error
retErr := &InvalidResponseError{
Expand All @@ -822,7 +822,7 @@ func (sp *ServiceProvider) ParseXMLResponse(decodedResponseXML []byte, possibleR
return nil, retErr
}

assertion, err := sp.parseResponse(doc.Root(), possibleRequestIDs, now, signatureRequired)
assertion, err := sp.parseResponse(doc.Root(), possibleRequestIDs, now, signatureRequired, currentURL)
if err != nil {
retErr.PrivateErr = err
return nil, retErr
Expand All @@ -844,7 +844,7 @@ const (
// This function handles decrypting the message, verifying the digital
// signature on the assertion, and verifying that the specified conditions
// and properties are met.
func (sp *ServiceProvider) parseResponse(responseEl *etree.Element, possibleRequestIDs []string, now time.Time, signatureRequirement signatureRequirement) (*Assertion, error) {
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 {
Expand All @@ -867,8 +867,11 @@ func (sp *ServiceProvider) parseResponse(responseEl *etree.Element, possibleRequ

// If the response is *not* signed, the Destination may be omitted.
if responseHasSignature || response.Destination != "" {
if response.Destination != sp.AcsURL.String() {
return nil, fmt.Errorf("`Destination` does not match AcsURL (expected %q, actual %q)", sp.AcsURL.String(), 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())
}
}

Expand Down
Loading
Loading