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


 in SAML response not sanitized before validation #599

Open
ichi234 opened this issue May 30, 2024 · 3 comments
Open


 in SAML response not sanitized before validation #599

ichi234 opened this issue May 30, 2024 · 3 comments

Comments

@ichi234
Copy link

ichi234 commented May 30, 2024

Summary

When validating a SAML response, if a newline in the response is 
 instead of 
, it is not sanitized and the validation fails.

Environment in which the event occurred

  • Browser: Microsoft Edge on Windows 10
  • Execution environment: Next.js application on WSL

Related Library Versions

"@boxyhq/saml20": "1.5.1"
"@boxyhq/saml-jackson": "1.26.1"
"next-auth": "4.24.7"

Detailed Situation

In the case in issue, the X509Certificate portion of the xml argument passed to the validateSignature.hasValidSignature method represented line breaks as 
.
This representation is not sanitized before use and therefore fails validation.

Actual response data (excerpts)

<KeyInfo><X509Data><X509Certificate>MIID2TCCAsGgAwIBAgIBAzANBgkqhkiG9w0BAQsFADBvMQswCQYDVQQGEwJKUDERMA8GA1UECAwI&#13;
SE9ra2FpZG8xFDASBgNVBAcMC1NBUFBPUk8tU0hJMSowKAYDVQQKDCFIb2trYWlkbyBFbGVjdHJp&#13;
YyBQb3dlciBDby4sIEluYy4xCzAJBgNVBAMMAmNhMCAXDTIxMDMwMzAwMzgyMVoYDzIwNTEwMjI0&#13;
MDAzODIxWjByMQswCQYDVQQGEwJKUDERMA8GA1UECAwISE9ra2FpZG8xKjAoBgNVBAoMIUhva2th&#13;
...
vs1WiMb2KXjA5d/xGzkvmxl9Ng6Ama6p3sg38AaLcB72WCNRo5PSYL3aRta5GyfaIAeohJk1Lne+&#13;
neTrBOIBI/dbYuEe9oHPr/IW9fffE4X0iGqBVMy4UzMxIM/DNEmcp7ixVm/u6ybCDbGVWBezSVFy&#13;
SBDO6kbSaR5BAQlvMoF9+3Wixyv7Q3JomUDe7nnp2JrSvh2T1+8tFQWkEa9/1JkJT3O6XrBeY8Me&#13;
n4P9U4e8TThs3VH1lBRV8T1NYUs=</X509Certificate></X509Data></KeyInfo>

Proposed Solution

By manually modifying the contents under node_modules for testing, the following adjustment was found to resolve the issue:

xml = xml.replace(/&#x(d|D);/gi, '');
  • Suggested modification:
xml = xml.replace(/(&#x(d|D);|&#13;)/gi, '');

This modification ensures that &#13; is appropriately sanitized, allowing the validation to proceed correctly.

@rileyw
Copy link

rileyw commented Jun 6, 2024

@ichi234 Would you check that this test case matches the SAML response that you have observed to be problematic? https://github.com/rileyw/saml20/blob/issue/599/test/assets/saml20.validResponseSignedMessage-unsanitized.xml#L7-L19

@ichi234
Copy link
Author

ichi234 commented Jun 7, 2024

@rileyw Yes, it matches.
The content of the X509Certificate element is broken every 76 characters, and the CRLF at the line break is converted as follows.

  • CR -> &#13;
  • LF -> as is

To put a little more detail into it, the value portion of a newline is formatted without line indentation as follows.

<?xml version="1.0" encoding="UTF-8" standalone="no"?><samlp:Response xmlns="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:xenc="http://www.w3.org/2001/04/xmlenc#" ID="iCznFHxFh9cF3HIwkqxL7kxRI4UbiVU9pCaZGX6R9" IssueInstant="2024-06-07T00:26:59Z" Version="2.0">
  <Issuer>https://example.com/</Issuer>
  <Signature xmlns="http://www.w3.org/2000/09/xmldsig#"><SignedInfo><CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/><SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/><Reference URI="#iCznFHxFh9cF3HIwkqxL7kxRI4UbiVU9pCaZGX6R9"><Transforms><Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/><Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/></Transforms><DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/><DigestValue>RkCumiTsjLOTY3zpqUs7I4p7IZVPtnRNokk8UhIlRzM=</DigestValue></Reference></SignedInfo><SignatureValue>WJoBk4I5H8QwfP66IxFbPnUK5Dinv8tRfyeRodIS0xcCdTK+tR984zSRe27ihtM3wEih0vjT3Dd3&#13;
D4Q5DldbtQXCW7urS69XG7+CgskO8WCKyoZJXr3/sS4tS5FGHtE9Ef9H0j07L9ag2Z/SmSgGXhOT&#13;
...
ylzVGe6f3zs659YWI0XLRWpBuhktYO3NlZSmHmGo+r+IRZY7/lG30rS+VTqWXQNnRxqwlO8iv5sm&#13;
go46r6rpaAY27fr/E8xb00jbQGKELU/HTLnuVg==</SignatureValue><KeyInfo><X509Data><X509Certificate>MIID2TCCAsGgAwIBAgIBAzANBgkqhkiG9w0BAQsFADBvMQswCQYDVQQGEwJKUDERMA8GA1UECAwI&#13;
SE9ra2FpZG8xFDASBgNVBAcMC1NBUFBPUk8tU0hJMSowKAYDVQQKDCFIb2trYWlkbyBFbGVjdHJp&#13;
YyBQb3dlciBDby4sIEluYy4xCzAJBgNVBAMMAmNhMCAXDTIxMDMwMzAwMzgyMVoYDzIwNTEwMjI0&#13;
...
neTrBOIBI/dbYuEe9oHPr/IW9fffE4X0iGqBVMy4UzMxIM/DNEmcp7ixVm/u6ybCDbGVWBezSVFy&#13;
SBDO6kbSaR5BAQlvMoF9+3Wixyv7Q3JomUDe7nnp2JrSvh2T1+8tFQWkEa9/1JkJT3O6XrBeY8Me&#13;
n4P9U4e8TThs3VH1lBRV8T1NYUs=</X509Certificate></X509Data></KeyInfo></Signature><samlp:Status>
    <samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/>
  </samlp:Status>
  <Assertion ID=...

@VuBui83
Copy link

VuBui83 commented Nov 24, 2024

Facing same issue, I found another solution, loadSignature can take in a node or string, remove the toString solved it for me

signed.loadSignature(signature.toString());

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