Contrib.social

SAML nested assertion

Open on Drupal.org →
Created on 1 December 2023, 7 months ago

Problem/Motivation

When the IdP returns its response, the assertion is nested, hence there are two and not one assertion, and the authentication is rejected with the error: 'Error(s) encountered during processing of authentication response. Type(s): invalid_response; reason given for last error: SAML Response must contain 1 assertion.'

I have applied a temporary fix by modifying the public function validateNumAssertions() located in vendor/onelogin/php-saml/src/Saml2/Response.php (line 855) in this way:

public function validateNumAssertions()
    {
        $encryptedAssertionNodes = $this->document->getElementsByTagName('EncryptedAssertion');
        $assertionNodes = $this->document->getElementsByTagName('Assertion');

        //$valid = $assertionNodes->length + $encryptedAssertionNodes->length == 1;
        //
        //if ($this->encrypted) {
        //    $assertionNodes = $this->decryptedDocument->getElementsByTagName('Assertion');
        //    $valid = $valid && $assertionNodes->length == 1;
        //}

        // Validation logic: allow more than one assertion.
        $valid = ($assertionNodes->length + $encryptedAssertionNodes->length) > 0; // More assertions

        if ($this->encrypted) {
            $assertionNodes = $this->decryptedDocument->getElementsByTagName('Assertion');
            $valid = $valid && ($assertionNodes->length > 0); // Check into decripted document
        }

        return $valid;
    }

I am not sure how this modification impacts security.

💬 Support request
Status

Active

Version

3.9

Component

Miscellaneous

Created by

🇮🇹Italy braintec Perugia, Umbria

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @braintec
  • Comment 7 months ago →
    🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU

    You must hack the SAML PHP Toolkit library.

    This kind of error seems like it's not the module's problem but the libary's problem. (Or an IdP problem. this problem has not been reported before, and I don't know of nested SAML assertion -- though I'm not a SAML protocol expert.)

    However... inspecting the code, it seems like the error is thrown in a non-critical part of the code, after the response was already processed.

    Please comment out this line in \OneLogin\Saml2\Auth::processResponse()

    $this->_lastAssertionId = $response->getAssertionId();

    ...and respond if it fixes your problem.
    If it does,

    • that is going to be an extra reason for me getting rid of the Auth class which I was already planning. So this issue can stay open as a reminder
    • I may have questions around the structure of your SAML assertion, when I do. (But as long as this stays a volunteer project, it might be years away. The planned re-work is fairly big.)
  • Comment 7 months ago →
    🇮🇹Italy braintec Perugia, Umbria
  • Comment 7 months ago →
    🇮🇹Italy braintec Perugia, Umbria

    I had just updated the issue with the modification I made...

  • Comment 7 months ago →
    🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU

    OK, our comments intersected.

    Re. security:

    This is not a nice thing to do in principle. (IMHO it also shows there may be something weird with your IdP's responses because the code isn't there for nothing. And that is up to you to decide.)

    However, assuming that the path to the incoming responses from your IdP is secure, in practice this doesn't matter right now because

    • validateNumAssertions() seems to be called only by Auth::processResponse() - the line I quoted above
    • The Auth class is a weird one (just a 'helper' class but IMHO the logic isn't structured nicely)
    • Nothing in the process / the samlauth code checks $this->_lastAssertionId, ever.

    ...so nothing is currently affected, except the ability of 'custom code' to get to the "last assertion id" value.
    It seems to be just an extra check outside of validating the response right now. Why? Dunno.

    (That is -unless I'm reading the code wrong and just changing that single line I quoted, is not enough.)

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024