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

Decrypted Assertion modified namespace results as assertion Signature not valid. #578

Open
agrisvv opened this issue Mar 16, 2024 · 5 comments

Comments

@agrisvv
Copy link

agrisvv commented Mar 16, 2024

Document
Incoming saml Response are signed, Assertion also are signed and encrypted.
PHP 8.2.16

Problem
Assertion signature not valid after it are modified with this line:

 $decrypted->setAttributeNS('http://www.w3.org/2000/xmlns/', $ns, Constants::NS_SAML);

Document parts examples - response and decrypted assertion:

<samlp:Response ID="_07e4c1c6-xxxxxxxxxxxxxxxxxxxxxxxx"
                Version="2.0"
                IssueInstant="2024-03-15T09:25:10.000Z"
                Destination="https://xxxxxxxxxxxxxxxxxxxxxx"
                Consent="urn:oasis:names:tc:SAML:2.0:consent:unspecified"
                InResponseTo="ONELOGIN_719fa4a0xxxxxxxxxxxxxxxxxxxxxx"
                xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
                >
<saml:Assertion ID="_45179449-a9XXXXXXXXXXXXXXXXXX" IssueInstant="2024-03-15T14:22:42.000Z"
    Version="2.0" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">

Main elements:

<Signature xmlns="http://www.w3.org/2000/09/xmldsig#">
<EncryptedAssertion xmlns="urn:oasis:names:tc:SAML:2.0:assertion">

Auth fails at:

if ($hasSignedAssertion && !Utils::validateSign($documentToCheckAssertion, $cert, $fingerprint, $fingerprintalg, Utils::ASSERTION_SIGNATURE_XPATH, $multiCerts)) {
   throw new ValidationError(
     "Signature validation failed. SAML Response rejected",
     ValidationError::INVALID_SIGNATURE
     );
}

Opinion:
So i think what it is wrong to modify signed assertion. Maybe responses are also are not 100% perfect, but o can't modify them.
Maybe php bug in function ->hasAttributeNS dose not detect existing xmlns..

&& !$container->hasAttributeNS('http://www.w3.org/2000/xmlns/', 'xmlns:saml')

In result next function write one more $qualifiedName 'xmlns'

Solutions:

  1. Remove assertion namespace modification.
  2. Add option to check in plain text for value.
  3. Try to find why hasAttributeNS not work.
@maijs
Copy link

maijs commented Apr 9, 2024

This issue has been around for quite a while (see SAML-Toolkits/wordpress-saml#136), and it's certainly a bug.

@pitbulk
Copy link
Contributor

pitbulk commented May 13, 2024

@maijs , have you tested that instead:

{
    if (strpos($encryptedAssertion->tagName, 'saml2:') !== false) {
        $ns = 'xmlns:saml2';
    } else if (strpos($encryptedAssertion->tagName, 'saml:') !== false) {
        $ns = 'xmlns:saml';
    } else {
        $ns = 'xmlns';
    }
    $decrypted->setAttributeNS('http://www.w3.org/2000/xmlns/', $ns, Constants::NS_SAML);
}

having:

{
    if (strpos($encryptedAssertion->tagName, 'saml2:') !== false) {
        $decrypted->setAttributeNS('http://www.w3.org/2000/xmlns/', 'xmlns:saml2', Constants::NS_SAML);
    } else if (strpos($encryptedAssertion->tagName, 'saml:') !== false) {
        $decrypted->setAttributeNS('http://www.w3.org/2000/xmlns/', 'xmlns:saml', Constants::NS_SAML);
    }
}

solves your issue?

Can you generate self-signed cert and private key, use them on the Idp to generate a fresh new Signed Encrypted Assertion and share untouched encodebased64 SAMLResponse with the self-signed cert and private key to allow me explore?

@pitbulk
Copy link
Contributor

pitbulk commented May 30, 2024

@agrisvv ,@maijs were you able to check my previous comment?

@maijs
Copy link

maijs commented May 31, 2024

@pitbulk Yes, the code you provided does solve the issue, because it doesn't alter the object, at least in my case where none of the two conditions return true.

@pitbulk
Copy link
Contributor

pitbulk commented May 31, 2024

@maijs thanks for confirming.

The IdPs that I usually use for testing php-saml (Okta, OneLogin, Azure, simpleSAMLphp) generate encrypted assertions
that does not have this problem.

Would you mind to generate and share with me a base64 encoded SAMLResponse with encrypted assertion payload and provide to me self-signed private key and public cert used for this specific purpose (so its ok to make them public), or you even could reuse the sp.key and sp.crt as the one to be used at the IdP, so I could properly apply the fix and add test coverage so no regressions gonna be introduced in the future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants