Decrypt assertion broken

[isanttila]

Since a tightened checking was introduced in xmldom a week ago, decryptAssertion in libsaml has been broken. Here is the change that affects how replaceChild behaves: https://github.com/xmldom/xmldom/commit/3bc6ccffc89cda391a4e003d36002dfbca2f5403

Because of this change, xml.replaceChild(assertionNode, encryptedAssertions[0]) fails with the error 'Not found: child not in parent' and an ERR_EXCEPTION_OF_ASSERTION_DECRYPTION is thrown.

This happens at least when the SAML response XML contains a header in the beginning (e.g. <?xml version="1.0" encoding="UTF-8"?>). When this is the case, entireXML contains the header as the first element, and Response as the second element, and EncryptedAssertion is a child of Response. Therefore, EncryptedAssertion is not a direct child of entireXML, and replaceChild fails due to the tightened checking.

[ambigus9]

Any Idea how to fix this?

[jsgsdev]

I also have the same problem, have you found a possible solution?

[ambigus9]

@isanttila and @jsgsdev Tempory solved using an old version of samplify 2.7.7.

[jsgsdev]

@ambigus9 ohhhhhh yeah bro, fix the problem thank you!

[KuSh]

@isanttila the commit doesn't seem to be the only culprit as using an overrides with @xmldom/xmldom 0.8.5, which doesn't contain the backported commit, doesnt fix the problem :

❯ npm ls --all
samlify@1.0.0 /Users/nlecam/Developpement/tests/samlify
└─┬ samlify@2.8.7
  ├─┬ @authenio/xml-encryption@2.0.2
  │ ├── @xmldom/xmldom@0.8.5 overridden
  │ ├── escape-html@1.0.3
  │ └── xpath@0.0.32 deduped
  ├── @xmldom/xmldom@0.8.5 overridden
  ├── camelcase@6.3.0
  ├── node-forge@1.3.1
  ├─┬ node-rsa@1.1.1
  │ └─┬ asn1@0.2.6
  │   └── safer-buffer@2.1.2
  ├── pako@1.0.11
  ├── uuid@8.3.2
  ├─┬ xml-crypto@3.0.1
  │ ├── @xmldom/xmldom@0.8.5 overridden
  │ └── xpath@0.0.32 deduped
  ├── xml@1.0.1
  └── xpath@0.0.32

❯ npm test
Error: error:1E08010C:DECODER routines::unsupported
    at Object.privateDecrypt (node:internal/crypto/cipher:79:12)
    at decryptKeyInfoWithScheme (/Users/nlecam/Developpement/tests/samlify/node_modules/@authenio/xml-encryption/lib/xmlenc.js:258:26)
    at decryptKeyInfo (/Users/nlecam/Developpement/tests/samlify/node_modules/@authenio/xml-encryption/lib/xmlenc.js:246:14)
    at Object.decrypt (/Users/nlecam/Developpement/tests/samlify/node_modules/@authenio/xml-encryption/lib/xmlenc.js:187:24)
    at /Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/libsaml.js:568:31
    at new Promise (<anonymous>)
    at Object.decryptAssertion (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/libsaml.js:553:20)
    at /Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:222:60
    at step (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:33:23)
    at Object.next (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:14:53) {
  library: 'DECODER routines',
  reason: 'unsupported',
  code: 'ERR_OSSL_UNSUPPORTED'
}
Error: ERR_EXCEPTION_OF_ASSERTION_DECRYPTION
    at /Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/libsaml.js:573:39
    at Object.decrypt (/Users/nlecam/Developpement/tests/samlify/node_modules/@authenio/xml-encryption/lib/xmlenc.js:214:12)
    at /Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/libsaml.js:568:31
    at new Promise (<anonymous>)
    at Object.decryptAssertion (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/libsaml.js:553:20)
    at /Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:222:60
    at step (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:33:23)
    at Object.next (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:14:53)
    at fulfilled (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:5:58)

Using samlify 2.7.7 fixes the problem :

❯ npm ls --all
samlify@1.0.0 /Users/nlecam/Developpement/tests/samlify
└─┬ samlify@2.7.7
  ├─┬ @authenio/xml-encryption@1.3.0
  │ ├── @xmldom/xmldom@0.7.9
  │ ├── escape-html@1.0.3
  │ ├── node-forge@0.10.0 deduped
  │ └── xpath@0.0.32
  ├── @types/xmldom@0.1.31
  ├── camelcase@5.3.1
  ├── node-forge@0.10.0
  ├─┬ node-rsa@1.1.1
  │ └─┬ asn1@0.2.6
  │   └── safer-buffer@2.1.2
  ├── pako@1.0.11
  ├── uuid@3.4.0
  ├─┬ xml-crypto@2.1.5
  │ ├── @xmldom/xmldom@0.7.9 deduped
  │ └── xpath@0.0.32
  ├── xml@1.0.1
  ├── xmldom@0.6.0
  └── xpath@0.0.27

❯ npm test

<nameID> {
  Role: [
    'offline_access',
    'view-profile',
    'default-roles-master',
    'manage-account-links',
    'uma_authorization',
    'manage-account'
  ]
}

I'll check to see if I find a more recent xmldom version which works

[isanttila]

@KuSh Ok, good to know. For us, Samlify 2.8.6 with xmldom downgraded to 0.8.3 is working, but this may depend on the exact details of the SAML Response in each case.

[KuSh]

Using samlify 2.8.6 with xmldom 0.8.3 gives me Error: error:1E08010C:DECODER routines::unsupported which I don't have with 2.7.7

[tngan]

Hi everyone, check if the private key file you are using, see if there are

-----BEGIN RSA PRIVATE KEY----- and -----END RSA PRIVATE KEY-----

Edit: This is for the error Error: error:1E08010C:DECODER routines::unsupported

[tngan]

It is better to get fixed soon, coz the upgraded version includes security patches.

[tngan]

Since a tightened checking was introduced in xmldom a week ago, decryptAssertion in libsaml has been broken. Here is the change that affects how replaceChild behaves: xmldom/xmldom@3bc6ccf

Because of this change, xml.replaceChild(assertionNode, encryptedAssertions[0]) fails with the error 'Not found: child not in parent' and an ERR_EXCEPTION_OF_ASSERTION_DECRYPTION is thrown.

This happens at least when the SAML response XML contains a header in the beginning (e.g. <?xml version="1.0" encoding="UTF-8"?>). When this is the case, entireXML contains the header as the first element, and Response as the second element, and EncryptedAssertion is a child of Response. Therefore, EncryptedAssertion is not a direct child of entireXML, and replaceChild fails due to the tightened checking.

@isanttila Do you have a sample code snippet or saml document without sensitive information? Thanks for reporting.

[KuSh]

Hi everyone, check if the private key file you are using, see if there are

-----BEGIN RSA PRIVATE KEY----- and -----END RSA PRIVATE KEY-----

Edit: This is for the error Error: error:1E08010C:DECODER routines::unsupported

It has but without new lines, I'll reformat it to see if it fixes anything, but the same code works without changes just by downgrading samlify 2.7.7

[KuSh]

Ok. With correct formatting of keys and certs test code works with samlify 2.8.7 and xmldom 0.8.3 but fails with xmldom >=0.8.4

[isanttila]

@tngan Do you have a sample code snippet or saml document without sensitive information?

This is what I can get easily. It's a sample response, before and after decryption. haka-response.zip

[sellonen]

I also suffer from this bug. In addition, switching to samlify 2.7.7 does not work for me, because the IDP I'm using does not accept the request in that case.

It's a shame because inside a debugger I can already see all the decrypted data just fine, the code really trips at the finish line.

Still, thank you a lot for a very well documented library! The best I've seen for saml2 @tngan

[sellonen]

I debugged the issue today and I'm able to overcome the problem by changing the function decryptAssertion() in the file build/src/libsaml.js so that

1. I remove the xml.replaceChild()
2. change it to use the parentNode directly
3. instead of replacing a child I separately append and remove the children:

const parent = encryptedAssertions[0].parentNode;

parent.appendChild(assertionNode);
parent.removeChild(encryptedAssertions[0]);
// xml.replaceChild(assertionNode, encryptedAssertions[0]);

If I don't use the parentNode, then I get the error as described in the original post. If I use the parentNode but try to do it with a single line with replaceChild() then I get the error Error: Hierarchy request error: Unexpected node type 9 for parent node type 1.

EDIT: link to changes I made in my fork that I will use https://github.com/sellonen/samlify/blob/95a9edf02dd8205580f3b2357b989a447e575521/src/libsaml.ts#L680

[ygrandgirard]

@sellonen encryptAssertion() has a similar issue and fails with both Not found: child not in parent and ERR_EXCEPTION_OF_ASSERTION_ENCRYPTION errors. However, it can apparently be fixed pretty much the same way:

const parent = assertions[0].parentNode;
if (parent) {
    parent.appendChild(encryptAssertionNode);
    parent.removeChild(assertions[0]);
}
// doc.replaceChild(encryptAssertionNode, assertions[0]);

Could you please update your fork (libsaml.ts:637) to include this change as well?

[sellonen]

@yanngrandgirard Thanks, I just did, even though it worked in my case even before the change. For anyone stumbling here before the issue is fixed in the original repository, you may temporarily put this line in your package.json dependencies and then use it like any other package. (But the versioning is total crap, I don't recommend this in the long term)

"samlify": "https://github.com/sellonen/samlify.git"

In the case of decryptAssertion(), I think the problem is indeed the presence of a header in the beginning of the response (e.g. <?xml version="1.0" encoding="UTF-8"?> ). Are you able to analyze what causes the problem in the encryption case?

[ygrandgirard]

@sellonen Thanks, it will greatly help until this gets fixed in the original repo!

However, I think what I found could be of interest. From what I have learned so far, the two lines below create a Document containing the samlp:Response as a child (either the first one, or the second one if there is a header line):

// encryptAssertion(), libsaml.ts:607
const doc = new dom().parseFromString(xml);

// decryptAssertion(), libsaml.ts:661
const xml = new dom().parseFromString(entireXML);

So, I would say the <?xml version="1.0" encoding="UTF-8"?> line does not matter much here. In fact, in my case the generated response never contained any such line. The real issue apparently comes from the fact that doc and xml do not contain the replaced nodes in their children, at all.

As for this message: Error: Hierarchy request error: Unexpected node type 9 for parent node type 1, it is pretty much the same idea:

// encryptAssertion(), libsaml.ts:636
const encryptAssertionNode = new dom().parseFromString(...);

// decryptAssertion(), libsaml.ts:679
const assertionNode = new dom().parseFromString(res);

The two lines above also create Document objects containing the saml:EncryptedAssertion (or saml:Assertion) as their child.

---

Putting it all together, another way to fix this bug could be:

// encryptAssertion(), libsaml.ts:637
// doc.replaceChild(encryptAssertionNode, assertions[0]);
doc.lastChild.replaceChild(encryptAssertionNode.firstChild, assertions[0]);

// decryptAssertion(), libsaml.ts:680
// xml.replaceChild(assertionNode, encryptedAssertions[0]);
xml.lastChild.replaceChild(assertionNode.firstChild, encryptedAssertions[0]);

Although I am not sure firstChild and lastChild are the best choices here. I unfortunately do not know enough about Samlify or SAML, and I am sure @tngan will find a better solution if there is one.

Disclaimer: my use case does not call decryptAssertion(), so what I just said about it is based on what I read here and in the source code. I have not tested it!

[mastermatt]

I opened https://github.com/tngan/samlify/pull/511 just now to fix this.