Break the signature validation (>0.8.2)

[tngan]

It works in release 0.8.2 but not 0.8.3. Follow the steps to replicate this issue.

This is the test script, test.js

var SignedXml = require('xml-crypto').SignedXml;
var dom = require('xmldom').DOMParser;
var rfc1951 = require('deflate-js');
var forge = require('node-forge');
var pki = forge.pki;
var fs = require('fs');
var select = require('xml-crypto').xpath
var dom = require('xmldom').DOMParser
var FileKeyInfo = require('xml-crypto').FileKeyInfo  

function  getKeyInfo(x509Certificate) {
  this.getKeyInfo = function(key) {
    return '<X509Data><X509Certificate>' + x509Certificate + '</X509Certificate></X509Data>';
  };
  this.getKey = function(keyInfo) {
      return getPublicKeyPemFromCertificate(x509Certificate).toString();
  };
}

function getPublicKeyPemFromCertificate(x509Certificate){
    var certDerBytes = forge.util.decode64(x509Certificate);
    var obj = forge.asn1.fromDer(certDerBytes);
    var cert = forge.pki.certificateFromAsn1(obj);
    return pki.publicKeyToPem(cert.publicKey);
}

var xml = fs.readFileSync("signed.xml").toString()
var doc = new dom().parseFromString(xml)    

var signature = select(doc, "/*/*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']")[0]
var sig = new SignedXml()
 //sig.keyInfoProvider = new FileKeyInfo("client_public.pem")
 sig.keyInfoProvider = new getKeyInfo('MIIDlzCCAn+gAwIBAgIJAO1ymQc33+bWMA0GCSqGSIb3DQEBCwUAMGIxCzAJBgNVBAYTAkhLMRMwEQYDVQQIDApTb21lLVN0YXRlMRowGAYDVQQKDBFJZGVudGl0eSBQcm92aWRlcjEUMBIGA1UECwwLRGV2ZWxvcG1lbnQxDDAKBgNVBAMMA0lEUDAeFw0xNTA3MDUxODAyMjdaFw0xODA3MDQxODAyMjdaMGIxCzAJBgNVBAYTAkhLMRMwEQYDVQQIDApTb21lLVN0YXRlMRowGAYDVQQKDBFJZGVudGl0eSBQcm92aWRlcjEUMBIGA1UECwwLRGV2ZWxvcG1lbnQxDDAKBgNVBAMMA0lEUDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAODZsWhCe+yG0PalQPTUoD7yko5MTWMCRxJ8hSm2k7mG3Eg/Y2v0EBdCmTw7iDCevRqUmbmFnq7MROyV4eriJzh0KabAdZf7/k6koghst3ZUtWOwzshyxkBtWDwGmBpQGTGsKxJ8M1js3aSqNRXBT4OBWM9w2Glt1+8ty30RhYv3pSF+/HHLH7Ac+vLSIAlokaFW34RWTcJ/8rADuRWlXih4GfnIu0W/ncm5nTSaJiRAvr3dGDRO/khiXoJdbbOj7dHPULxVGbH9IbPK76TCwLbF7ikIMsPovVbTrpyL6vsbVUKeEl/5GKppTwp9DLAOeoSYpCYkkDkYKu9TRQjF02MCAwEAAaNQME4wHQYDVR0OBBYEFP2ut2AQdy6D1dwdwK740IHmbh38MB8GA1UdIwQYMBaAFP2ut2AQdy6D1dwdwK740IHmbh38MAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQELBQADggEBANMZUoPNmHzgja2PYkbvBYMHmpvUkVoiuvQ9cJPlqGTB2CRfG68BNNs/Clz8P7cIrAdkhCUwi1rSBhDuslGFNrSaIpv6B10FpBuKwef3G7YrPWFNEN6khY7aHNWSTHqKgs1DrGef2B9hvkrnHWbQVSVXrBFKe1wTCqcgGcOpYoSK7L8C6iX6uIA/uZYnVQ4NgBrizJ0azkjdegz3hwO/gt4malEURy8D85/AAVt6PAzhpb9VJUGxSXr/EfntVUEz3L2gUFWWk1CnZFyz0rIOEt/zPmeAY8BLyd/Tjxm4Y+gwNazKq5y9AJS+m858b/nM4QdCnUE4yyoWAJDUHiAmvFA=</X509Certificate></X509Data>');
 sig.loadSignature(signature)
 var res = sig.checkSignature(xml)
if (!res) console.log(sig.validationErrors)
else console.log('success');

Put the signed xml into same folder, called signed.xml

<samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="_8e8dc5f69a98cc4c1ff3427e5ce34606fd672f91e6" Version="2.0" IssueInstant="2014-07-17T01:01:48Z" Destination="http://sp.example.com/demo1/index.php?acs" InResponseTo="_11e97f890395cx395c4e7510273e3b046x85">
  <saml:Issuer>https://idp.example.com/metadata</saml:Issuer>
  <samlp:Status>
    <samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/>
  </samlp:Status>
  <saml:Assertion xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xs="http://www.w3.org/2001/XMLSchema" ID="_d71a3a8e9fcc45c9e9d248ef7049393fc8f04e5f75" Version="2.0" IssueInstant="2014-07-17T01:01:48Z">
    <saml:Issuer>https://idp.example.com/metadata</saml:Issuer>
    <saml:Subject>
      <saml:NameID SPNameQualifier="https://sp.example.com/metadata" Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient">_ce3d2948b4cf20146dee0a0b3dd6f69b6cf86f62d7</saml:NameID>
      <saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
        <saml:SubjectConfirmationData NotOnOrAfter="2024-01-18T06:21:48Z" Recipient="http://sp.example.com/demo1/index.php?acs" InResponseTo="_4fee3b046395c4e751011e97f8900b5273d56685"/>
      </saml:SubjectConfirmation>
    </saml:Subject>
    <saml:Conditions NotBefore="2014-07-17T01:01:18Z" NotOnOrAfter="2024-01-18T06:21:48Z">
      <saml:AudienceRestriction>
        <saml:Audience>https://sp.example.com/metadata</saml:Audience>
      </saml:AudienceRestriction>
    </saml:Conditions>
    <saml:AuthnStatement AuthnInstant="2014-07-17T01:01:48Z" SessionNotOnOrAfter="2024-07-17T09:01:48Z" SessionIndex="_be9967abd904ddcae3c0eb4189adbe3f71e327cf93">
      <saml:AuthnContext>
        <saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:Password</saml:AuthnContextClassRef>
      </saml:AuthnContext>
    </saml:AuthnStatement>
    <saml:AttributeStatement>
      <saml:Attribute Name="uid" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
        <saml:AttributeValue xsi:type="xs:string">test</saml:AttributeValue>
      </saml:Attribute>
      <saml:Attribute Name="mail" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
        <saml:AttributeValue xsi:type="xs:string">[email protected]</saml:AttributeValue>
      </saml:Attribute>
      <saml:Attribute Name="eduPersonAffiliation" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
        <saml:AttributeValue xsi:type="xs:string">users</saml:AttributeValue>
        <saml:AttributeValue xsi:type="xs:string">examplerole1</saml:AttributeValue>
      </saml:Attribute>
    </saml:AttributeStatement>
  </saml:Assertion>
<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/2000/09/xmldsig#rsa-sha1"/><Reference URI="#_d71a3a8e9fcc45c9e9d248ef7049393fc8f04e5f75"><Transforms><Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/></Transforms><DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/><DigestValue>upUaQyNo8KYfaugAsM9I6e/HYNQ=</DigestValue></Reference></SignedInfo><SignatureValue>T3eM5SxwxP8wH+w1jxH/YezcsC03xU0N19uEggWbL//adCbdvPEWqnYDRlRi6HqdfLTAxqt0LqXzy4Sx2Sj/G3dKA1fTu8qZueqg1oTdKIHEO7BKQlWj+vhsxP4RJwOaooZ195Ez0DsqBjIYM8O+hrmGCpKoJNz7ZaASDhnBBqg4UyilA/VXwP2Wq/LwXdQQ+SfgbtmqxfrcqRQtN87aL3YdPFoS4oR6Q8d97g+YiSbsSvTrxfU5vDEo1tRKxvAQXDDxRBral9kELhqcd3Aumcr9zPF00KxkYIAKtyWd5RsWgOcfbiEQpCit0vh74Y1HplLhzeLhPI/RrLB5gSI4DA==</SignatureValue></Signature></samlp:Response>

Run node test.js with version 0.8.2 and 0.8.3.

[yaronn]

that version included this fix of special characters encoding:
#99
I don't see any of those characters in the sample xml though, maybe with the exception of line breaks.
check the first changed file in the link, if changing one of the methods back will work we can test further.

[tngan]

@yaronn

Here is an update for this issue. I have discovered that there are special characters in the given xml (it might not be shown in the above code quotation), which are and line breaks \n. Before I removed them all, in 0.8.2, the calculated digest is upUaQyNo8KYfaugAsM9I6e/HYNQ= which is expected. After I remove line breaks and with 0.8.2, the calculated digest is sZOR3aMpVBn1CoSmP674OQfCcyg=.

Then I upgrade the package into the latest version 0.8.5, if I didn't remove them, the calculated digest is /+BwSjipFXUNtDRhRKdMSYHZSPg=, but if I removed them all, the calculated digest is same as the one with 0.8.2.

I can make further tests later on if needed.

[cjbarth]

@tngan , that's excellent work! Would you mind creating a PR with at least a test replicating this issue, and perhaps a fix for it. We don't want this to break for future users, especially in a semver-patch release!