fixes #78: make sure the correct Signature elements are verified

[miszobi]

• checks that the found Signature elements match their expected schema locations, and that there's only one of each
• make sure the expected Signatures are always verified
• a few other small hardenings

[coveralls]

Coverage decreased (-0.1%) to 96.495% when pulling 40eacc6 on miszobi:issue/78-avoid-response-signature-spoofing into cc296ae on onelogin:v2.0.0.

[miszobi]

This signature was found by processSignedElements, and the validation considered the Response signed, but subsequently wasn't validated by validateSign, because that one was looking for the Signature in the expected place. validateSign still passed, because the Assertion signature was okay, and it only required one of the signatures to be valid.

[pitbulk]

Let me the weekend to review the PR and maybe find an alternative solution, and let's push that and more tests to prevent it on monday.

[miszobi]

The only strictly required part is the one in processSignedElements.

The others are general security hardening fixes: using absolute xpaths where applicable, making sure the nodes we expect a single occurrence of actually appear only once - all these are good practices, as recommended by OWASP (also here)

[pitbulk]

I'm agree with the improvement except the change of the validateSign method and the checkSignature method.

Can we do the wantResponseSigned & wantAssertionSigned check on the processSignedElements instead doing that at the validateSign method?

And leave the validateSign:

    public static Boolean validateSign(Document doc, X509Certificate cert, String fingerprint, String alg) {
        NodeList signNodesToValidate;
        try {
            signNodesToValidate = query(doc, Util.RESPONSE_SIGNATURE_XPATH);
            if (signNodesToValidate.getLength() == 0) {
                signNodesToValidate = query(doc, Util.ASSERTION_SIGNATURE_XPATH);
            }

            if (signNodesToValidate.getLength() == 1) {
                return validateSignNode(signNodesToValidate.item(0), cert, fingerprint, alg);
            }
        } catch (XPathExpressionException e) {
                log.warn("Failed to find signature nodes", e);   
        }
        return false;
    }

[miszobi]

I can drop the wantResponseSigned & wantAssertionSigned parameters from validateSign- the signatures being present, singular, and in the right spot is in fact done by processSignedElements.

However the proposed implementation of validateSign doesn't check the Assertion signature if the Response signature is found.
As per SAML core I believe both should be checked if present:

lines 637-640:

The SAML assertion MAY be signed, which provides both authentication of the issuer and integrity protection.
If such a signature is used, then the ds:Signature element MUST be present, and a relying party MUST verify that the signature is valid

lines 1562-1565:

The SAML request MAY be signed, which provides both authentication of the requester and message integrity.
If such a signature is used, then the ds:Signature element MUST be present, and the SAML responder MUST verify that the signature is valid

So I believe we should go with something like:

    public static Boolean validateSign(Document doc, X509Certificate cert, String fingerprint, String alg) {
        try {
            final NodeList responseSignature = query(doc, RESPONSE_SIGNATURE_XPATH);
            final NodeList assertionSignature = query(doc, ASSERTION_SIGNATURE_XPATH);

            if (responseSignature.getLength() == 0 && assertionSignature.getLength() == 0) {
                return false;
            }

            boolean validResponseSignature = responseSignature.getLength() == 1 ? validateSignNode(responseSignature.item(0), cert, fingerprint, alg) : true;
            boolean validAssertionSignature = assertionSignature.getLength() == 1 ? validateSignNode(assertionSignature.item(0), cert, fingerprint, alg) : true;

            return validResponseSignature && validAssertionSignature;
        } catch (XPathExpressionException e) {
            log.warn("Failed to find signature nodes", e);
        }
        return false;
    }

[pitbulk]

If you validate the integrity of the container (Response), the Assertion (signed or not) included should be considered correct too.

[miszobi]

True, but the spec does specifically say each MUST be checked if present. I see it as an additional defense-in-depth check, where even if the attacker could for example wrap the Response signature, the Assertion signature will also be checked. Any reasons for not doing it this way?

[pitbulk]

Only the over cost. I understand that the spec says that, but maybe because the doc contains a non-related definition of each element

[pitbulk]

Also take in mind that when the encryption appears on the assertion, the signature validation is not that easy:

• Signature Response validation should use the original SAMLResponse.
• Signature Assertion validation should use the SAMLResponse with the EncryptedAssertion decrypted.

[miszobi]

Right. So maybe instead of calling validateSign from SamlResponse.isValid, and assuming that both signatures come from the same document, let's have two calls, each pointing to the right document. The added benefit would be that isValid knows which signatures it expects, and can make the right calls. Does that sound reasonable?

As for performance, using a simple benchmark of running the assertions in testValidateSignInvalidInputs 1000 times, the change does seem introduce a ~20% performance cost:

check one signature:
1000x testValidateSign: 8s161ms

check both signature:
1000x testValidateSign: 9s833ms

However note that each of those tests runs validation 12 times, so that's 12000 validations, changing the per-validation cost from ~68ms to ~82ms. Seeing as during authentication the caller is usually dealing with several http requests and redirects, I don't think this is really notable. Also as mentioned above we'd be following the spec on this, and adding some extra verifications to the authentication process.

[pitbulk]

Ok, sound reasonable. validateSign can validate a specific signature, based on the doc and the path.

[coveralls]

Coverage decreased (-0.03%) to 96.572% when pulling bb18cf2 on miszobi:issue/78-avoid-response-signature-spoofing into cc296ae on onelogin:v2.0.0.

[miszobi]

Updated as discussed.