@ahacker1-securesaml, @p- can you help me review this PR? Thanks

If we are refactoring, I just suggest we remove the Response/SignedDocument class, and make it into a function instead. This OOP style is extremely hard to audit.

@pitbulk Yes I will help 👍 But I think I can't start before next week.

Hi, yes I agree I am intending to make the code more functional. This PR was intended firstly to get all tests green on Nokogiri while changing relatively little.

I am thinking something like separating Response class into:

• MessageParser base class (shared between Response, LogoutResponse, and SloLogoutRequest
• Response (Parser)
• Assertion -- immutable object representing the Assertion portion of the XML, after it has been parsed -- still thinking this over...
• ResponseValidator -- functional module which handles the validation aspects.

I need to think a bit more about exactly where the SignedDocument logic belongs. The biggest problem with it is not that it is OOP per-se, but that it co-mingles two responsibilities, namely "locating the signed element" and "validating the signature". It might be possible to split these responsibilities into two passes, but this feels much less secure since it re-introduces the possibility of a "parser differential" as in the last CVE.

I may not have time to look at this further for a few weeks however.

ok, ResponseValidator -> change to Assertion Validator, we don't care about the Response, since IdPs don't even sign it. Also don't mix this with the signature verification steps.
To clarify (edited): the AssertionValidator verifies the business logic associated with a SAML Assertion.

Response (Parser) -> Since the individual assertions are signed, this should return a list of Signed Assertions, and not nesscarily the whole (unsigned) SAML Authentication response.

And maybe we should do something like SignedAssertion class?

Would remove the SignedDocument class. Let's replace this with something called SignedXML, which cryptographically verifies some bytes of XML at instantianation.

Response parser can then accept SignedXML only.

Hence, whatever is used for the Response class is always signed.

@ahacker1-securesaml I agree with all those points, that sounds along the right lines. I will need more time to get my head around it, the code is really spaghetti today 🍝 and the real trick is getting all the tests to pass (fortunately the test suite is quite robust!!)

The main challenge is to:

First, verify the structure, number of signatures, number of elements, etc

• validate_structure
• validate_version
• validate_num_assertion
• validate_signed_elements
• validate_signature

And once the signature is validated and we know we can trust the Assertion (because it had a Signature, or its Response has a Signature), use the right XML to keep validating rest of elements and make that the methods that retrieve info (attributes, nameId, etc use the right XML as well).

• validate_id
• validate_success_status
• validate_no_duplicated_attributes
• validate_in_response_to
• validate_one_conditions
• validate_conditions
• validate_one_authnstatement
• validate_audience
• validate_destination
• validate_issuer
• validate_session_expiration
• validate_subject_confirmation
• validate_name_id

The main problem is that most of the tests use payloads with invalid signatures and in the current implementation, this is not a problem due the validation order.

As making signature validation optional is something that we better don't even add in the code, I will try to start updating the
invalid payloads, when possible to contain a valid Signature. In cases where this is not possible, we will need to adapt the test cases.

@pitbulk making the signatures validatable in the tests would be a huge help to give us flexibility in refactoring.

First step should be verifying signature IMO. After verifying signature, we can get some signed bytes of XML. Then Assertion re-parses the signed bytes of XML, and verifies the assertion data.

That gives us much more flexibility, if we want to test some assertion on some unsigned data, we simply mock the payload as signed.

@ahacker1-securesaml, If we identify that something is wrong in the XML (xsd validation, number of elements that we want to accept, etc) we save us of checking signatures on XMLs that we gonna consider already invalids.
We can revalidate the signed bytes of XML as well.

I can see the performance benefits there. However I feel strongly that we would be mixing the SAML Assertion processing logic and the Signature logic.

Ideally we want to keep them separate as much as possible. And separating logic would help with security

@ahacker1-securesaml, would you be able to add some context to the following?

ResponseValidator -> change to Assertion Validator, we don't care about the Response, since IdPs don't even sign it.

Signing the Response alone is something that identity providers can do in practice (Entra ID is an example). The Assertion should inherit the signature.

Three ways SAML can be signed:

Assertions alone: default for most Identity Providers
Assertion & Responses: default for Okta, and maybe some other IdPs
Responses alone: have not seen any IdP do this. Would require specialized configuration.

In the responses alone case, I have seem some SAML libraries only verify signatures on SAML Assertions. So it would break their code.

For the Assertions & Responses case: an attacker can trivially remove the signature on the response, and the SAML payload would still be valid.

For assertions alone case: an attacker can just change whatever they want on the response i.e. statuscode ...

So IMO, there's no point in verifying the properties of a SAML responses i.e. checking status code .... It would mean mixing unsigned data (Response) and the signed data (Assertion). That makes it really hard to analyze in terms of security, and has directly lead to vulnerabilities in libraries such as https://github.com/node-saml/node-saml

My idea is to only verify whatever is in the (signed) Assertion, be it the AudienceRestriction. These are actually important for security

AND

we separate the signature verification logic into it's own module. I don't believe it should be part of verifying the assertion logic.

Agree with the principles described, but not supporting a configuration that signs the Response alone would be a breaking change, given that it's currently supported in the wild:

• Entra
• Key Cloak

We still support signed responses, we separate business logic from signature verification steps

extracted_signature = extract_signature(untrusted_doc)
signed_bytes = get_signed_bytes(extracted_signature)
# if signed_bytes happens to be a Response, then we get the Assertion from ./saml:Assertion 
assertion = ParseAssertion(signed_bytes) 
AssertionValidator.validate(assertion) # business logic
return assertion

we just don't verify the business logic of a Response that the SAML spec supposedly requires. For example, verifying the StatusCode, or even the Version.

It would definitely help us to get a library of all the vendor IDP SAML variations in the wild... maybe some other SAML lib already has one?

As it stands the current RubySaml test suite is pretty good, it should protect us from a pretty wide range of possible regressions.

In terms of security, I agree that if the Response is not signed, and the StatusCode is not "protected" anyone can modify it, but in terms of business, if I receive a StatusCode != Success, I don't need to do anything else, I will simply reject this SAMLResponse and don't grant access to the app, the same applies to malformed XMLs received or XMLs wellformed, but that we want to avoid and consider invalids (multiple Assertions, more than 2 Signatures).

@johnnyshields This PR works fine with omniauth-saml. Anything specific I can help test?

Just a heads up here--@pitbulk I'm not going to have the bandwidth for several months to take this PR any further--have to focus on running my company. So the last mile will have to be carried by someone else.

Understood, thanks for your hard work!

@pitbulk I think we should merge this branch into v2.x--or even master--regardless. I realize there are more refactors needed, but perhaps we can track those in a follow-up ticket?

@johnnyshields, I was able to reorder the validations and adjust tests and payloads:
#764

@pitbulk I will release this branch to production tomorrow. I think after it runs stablely for a week we should look at releasing it as v2.x.

It's not "perfect" but it's a heck of a lot better than the current v1.x main release.

@johnnyshields @pitbulk anything I can do to help with v2.x? Happy to lend a hand here or elsewhere.