KJUR.asn1.cms.CMSUtil.verifySignedData error

coachaac commented 2 years ago

hello, i run into an error when using KJUR.asn1.cms.CMSUtil.verifySignedData. Data has been signed by 2 signers.

here is the error :

TypeError: Cannot read property 'length' of undefined
    at x (/Users/me/dev/a-test/node_modules/jsrsasign/lib/jsrsasign.js:228:19181)
    at e (/Users/me/dev/a-test/node_modules/jsrsasign/lib/jsrsasign.js:228:19532)
    at o (/Users/me/dev/a-test/node_modules/jsrsasign/lib/jsrsasign.js:228:18857)

the crash is on the last line of this code (aIdx is undefined) :

    var _findCert = function(hCMS, result, si, idx) {
    var certsIdx = result.parse.certsIdx;
    var aCert;

    if (result.certs === undefined) {
        aCert = [];
        result.certkeys = [];
        var aIdx = _getChildIdx(hCMS, certsIdx);
        for (var i = 0; i < aIdx.length; i++) {
        var hCert = _getTLV(hCMS, aIdx[i]);
        var x = new X509();
        x.readCertHex(hCert);
        aCert[i] = x;
        result.certkeys[i] = x.getPublicKey();
        }
        result.certs = aCert;
    } else {
        aCert = result.certs;
    }

    result.cccc = aCert.length;
    result.cccci = aIdx.length;

after some debugging , i saw that aIdx is undefined because result.certs is not undefined. i'm not sure if my fix is correct so i do not send you a pull request

but i think this will fix the bug :

 var _findCert = function(hCMS, result, si, idx) {
    var certsIdx = result.parse.certsIdx;
    var aCert;
         var aIdx;   <--- variable scope here

    if (result.certs === undefined) {
        aCert = [];
        result.certkeys = [];
         aIdx = _getChildIdx(hCMS, certsIdx);
        for (var i = 0; i < aIdx.length; i++) {
        var hCert = _getTLV(hCMS, aIdx[i]);
        var x = new X509();
        x.readCertHex(hCert);
        aCert[i] = x;
        result.certkeys[i] = x.getPublicKey();
        }
        result.certs = aCert;
    } else {
        aCert = result.certs;
            aIdx = _getChildIdx(hCMS, certsIdx); <--- aIdx generation
    }

    result.cccc = aCert.length;
    result.cccci = aIdx.length;

Yann.

kjur commented 2 years ago

Could you share the signed data and certificates which raise the error to investigate it?

coachaac commented 2 years ago

sure .

First certificate :

second certificate :

Signed Data :

-----BEGIN CMS----- MIIF9gYJKoZIhvcNAQcCoIIF5zCCBeMCAQExDzANBglghkgBZQMEAgEFADAdBgkq hkiG9w0BBwGgEAQOVEhJUyBJUyBBIFRFU1SgggL+MIIBejCCASGgAwIBAgIJBDEY G/yQGfWgMAoGCCqGSM49BAMCMBAxDjAMBgNVBAMMBXVzZXIxMB4XDTIxMTAwNjAw MDAwMFoXDTI1MTIzMTIzNTk1OVowJjEkMCIGA1UEAwwbQ29hY2ggQUFDIERpZ2l0 YWwgU2lnbmF0dXJlMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEcGdPwx0YLoZZ PKj8X9Wa3WXd99XG+a0fzVba+ya86acXVdK9cK6FQPfKnXHJkWbaJDoDptXCPbeh 0Q91DaYut6NOMEwwCQYDVR0TBAIwADAOBgNVHQ8BAf8EBAMCB4AwLwYDVR0fBCgw JjAkoCKgIIYeaHR0cHM6Ly93d3cuY29hY2hhYWMuY29tL2EuY3JsMAoGCCqGSM49 BAMCA0cAMEQCIEeRJlFU7Gk1XIOHqd5Z3FneQoBYiwRJQqn9qgmrKb6wAiBJd3UV 4cxiYlzktk1jaXsTbZ6itIri8f9RDnwmgBHzmzCCAXwwggEhoAMCAQICCQLqb/b/ pM8QUDAKBggqhkjOPQQDAjAQMQ4wDAYDVQQDDAV1c2VyMjAeFw0yMTEwMDYwMDAw MDBaFw0yNTEyMzEyMzU5NTlaMCYxJDAiBgNVBAMMG0NvYWNoIEFBQyBEaWdpdGFs IFNpZ25hdHVyZTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABFE6zU6GGEfQNZZe YCUsN2eVViqd8efymimmCm3ShbQqNySP8k31CDGzlace4bXK0OUZpziAkART5vs5 ymsk4HCjTjBMMAkGA1UdEwQCMAAwDgYDVR0PAQH/BAQDAgeAMC8GA1UdHwQoMCYw JKAioCCGHmh0dHBzOi8vd3d3LmNvYWNoYWFjLmNvbS9hLmNybDAKBggqhkjOPQQD AgNJADBGAiEAimiEkb1OU/N+QN788RLTcKJ+mN/zIpGEJzS+nkjbZuQCIQC7rQE2 BBC1pveC+ICi2yyU8iyRNkYFSjtWJ+M/P2mB0zGCAqowgdYCAQEwHTAQMQ4wDAYD VQQDDAV1c2VyMgIJAupv9v+kzxBQMA0GCWCGSAFlAwQCAQUAoEswGAYJKoZIhvcN AQkDMQsGCSqGSIb3DQEHATAvBgkqhkiG9w0BCQQxIgQg9rUaA96/aAvcwhX0Ke7U me9ZM+dZYU2iXtRFE+kYVh0wDQYJKoZIhvcNAQELBQAERzBFAiEAp+3ykAq/Th6C gBhhag5iC870pBgllSj2EtU2s8+0wbwCIBH+33fWWg+dfMqABXnI/eQ94yx9BOIL wiIXVF7psM9fMIIBzQIBATAdMBAxDjAMBgNVBAMMBXVzZXIxAgkEMRgb/JAZ9aAw DQYJYIZIAWUDBAIBBQCgggE/MBgGCSqGSIb3DQEJAzELBgkqhkiG9w0BBwEwHAYJ KoZIhvcNAQkFMQ8XDTIxMTAwODA1NDgwOFowHAYJKoZIhvcNAQkFMQ8XDTIxMTAw ODA1NDgwOFowLwYJKoZIhvcNAQkEMSIEIPa1GgPev2gL3MIV9Cnu1JnvWTPnWWFN ol7URRPpGFYdMFoGCyqGSIb3DQEJEAIvMUswSTBHMEUEILz6HDA/kX/ryiwjAhHu Vyf1iMiVgtykY27NMvkVxKu0MCEwFKQSMBAxDjAMBgNVBAMMBXVzZXIxAgkEMRgb /JAZ9aAwWgYLKoZIhvcNAQkQAi8xSzBJMEcwRQQg450E70ZJK1Gc7CC0sUlfnRj+ Oh2TjAAVLhwaoOkYPVowITAUpBIwEDEOMAwGA1UEAwwFdXNlcjICCQLqb/b/pM8Q UDANBgkqhkiG9w0BAQsFAARIMEYCIQD0lEKAAboVCDq2Kuwn45/0QRqi/XHr1GcJ QnOC04acsAIhAOgAljF5a1Dl3AU04qYWQgZdW+0MpA26WWZC+2/OlX+6 -----END CMS-----

And the bug fix i propose is not working because it crash just after (2 lignes after : x.getIssuerHex(); )

kjur commented 2 years ago

Thank you. I'll investigate them and let you know the result later.

kjur commented 2 years ago

It seems wrong CMS signed data. The first signerInfo has no SigningCertificateV2 attribute. This may be no problem. However the second signerInfo has two SigningCertificate attributes they are for user1 and user2. This raise conflict. It seems the reason why verifySignedData failed.

coachaac commented 2 years ago

You were right about an error in my CMS Signed data . all certificate were added on the second signerInfo. After correction i still have a problem, i'm working on ... BUT it seems that there is still a bug in the code :

    var _findCert = function(hCMS, result, si, idx) {
    var certsIdx = result.parse.certsIdx;
    var aCert;

    if (result.certs === undefined) {
        aCert = [];
        result.certkeys = [];
        var aIdx = _getChildIdx(hCMS, certsIdx);
        for (var i = 0; i < aIdx.length; i++) {
        var hCert = _getTLV(hCMS, aIdx[i]);
        var x = new X509();
        x.readCertHex(hCert);
        aCert[i] = x;
        result.certkeys[i] = x.getPublicKey();
        }
        result.certs = aCert;
    } else {
        aCert = result.certs;
    }

    result.cccc = aCert.length;
    result.cccci = aIdx.length;

    for (var i = 0; i < aCert.length; i++) {
        var issuer2 = x.getIssuerHex();         <<<<<<<<<  x is  not cycling with the for (i ), must be replace by aCert[i]
        var serial2 = x.getSerialNumberHex();
        if (si.signerid_issuer1 === issuer2 &&
        si.signerid_serial1 === serial2) {
        si.certkey_idx = i;
        }
    }
    };

in the last for section, in the case of multiple certificate , and if certificate are not the same, si.certKey_idx will not be set. replacing x by aCert[i] solve the problem.

Yann (still testing )

coachaac commented 2 years ago

i also think that my first code change for aIdx is a bug correction ( even if my test case where wrong at this time ) .

Yann.

kjur commented 2 years ago

If you send me proper signature to investigate issue, I can fix the bug.

coachaac commented 2 years ago

here is my testing datas :

Signed Data : -----BEGIN CMS----- MIIF9gYJKoZIhvcNAQcCoIIF5zCCBeMCAQExDzANBglghkgBZQMEAgEFADAdBgkq hkiG9w0BBwGgEAQOVEhJUyBJUyBBIFRFU1SgggL+MIIBezCCASGgAwIBAgIJApW0 F9tUSBLQMAoGCCqGSM49BAMCMBAxDjAMBgNVBAMMBXVzZXIxMB4XDTIxMTAwNjAw MDAwMFoXDTI1MTIzMTIzNTk1OVowJjEkMCIGA1UEAwwbQ29hY2ggQUFDIERpZ2l0 YWwgU2lnbmF0dXJlMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE4Fl6y9klstVV 2Jj9QzFYJzTMLEEqfOKCNM2Vst2Qfr/lZYhY2iiF2gRMZF56aEbwyHx/ybPEnay2 cpvPbw3p+6NOMEwwCQYDVR0TBAIwADAOBgNVHQ8BAf8EBAMCB4AwLwYDVR0fBCgw JjAkoCKgIIYeaHR0cHM6Ly93d3cuY29hY2hhYWMuY29tL2EuY3JsMAoGCCqGSM49 BAMCA0gAMEUCIQDmdreGaxxLnt96vqmoCNEo3Gcbjpe4NTryoZwq2dGC/QIgKxIT j+FCC+EPVwU5FfAL/l0sfampm3HN4wsIXrY81PUwggF7MIIBIaADAgECAgkCpmjw i0XFVxAwCgYIKoZIzj0EAwIwEDEOMAwGA1UEAwwFdXNlcjIwHhcNMjExMDA2MDAw MDAwWhcNMjUxMjMxMjM1OTU5WjAmMSQwIgYDVQQDDBtDb2FjaCBBQUMgRGlnaXRh bCBTaWduYXR1cmUwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAS82IQQlDrQdNUR +JT9RX8eVoZ0OOmXEE6ELoiP75WOku1obvQOkBgFbmwk1bN3OuGPn7F1yKF877i0 hrhlyKxQo04wTDAJBgNVHRMEAjAAMA4GA1UdDwEB/wQEAwIHgDAvBgNVHR8EKDAm MCSgIqAghh5odHRwczovL3d3dy5jb2FjaGFhYy5jb20vYS5jcmwwCgYIKoZIzj0E AwIDSAAwRQIgM4ydwP6lVlmGzleTJE8iF8n1t07+MN/xoQuWI5yh7F4CIQDvsnz/ 5ZeF6r0lfpb39v17vUGNqvv/yptXFmZEVj9emzGCAqowggFRAgEBMB0wEDEOMAwG A1UEAwwFdXNlcjECCQKVtBfbVEgS0DANBglghkgBZQMEAgEFAKCBxTAYBgkqhkiG 9w0BCQMxCwYJKoZIhvcNAQcBMBwGCSqGSIb3DQEJBTEPFw0yMTEwMTExMjE0NDFa MC8GCSqGSIb3DQEJBDEiBCD2tRoD3r9oC9zCFfQp7tSZ71kz51lhTaJe1EUT6RhW HTBaBgsqhkiG9w0BCRACLzFLMEkwRzBFBCAFueBkvayKcto/KTk3ICPDEuQmJla2 paspxsd69iyL2jAhMBSkEjAQMQ4wDAYDVQQDDAV1c2VyMQIJApW0F9tUSBLQMA0G CSqGSIb3DQEBCwUABEcwRQIgVDAlx7u0l7CKsYoUekoupQWgLLL+orTuWkjMj3cK /pwCIQDkTZckvkf8xh3rEm960mry1/Bd6vr02oAG+kf2Ac+tkjCCAVECAQEwHTAQ MQ4wDAYDVQQDDAV1c2VyMgIJAqZo8ItFxVcQMA0GCWCGSAFlAwQCAQUAoIHFMBgG CSqGSIb3DQEJAzELBgkqhkiG9w0BBwEwHAYJKoZIhvcNAQkFMQ8XDTIxMTAxMTEy MTQ0MVowLwYJKoZIhvcNAQkEMSIEIPa1GgPev2gL3MIV9Cnu1JnvWTPnWWFNol7U RRPpGFYdMFoGCyqGSIb3DQEJEAIvMUswSTBHMEUEIGwdpakNPaYPs3kjZiQlMNh/ Pb3Ubs5Temb4x5Ah8nbdMCEwFKQSMBAxDjAMBgNVBAMMBXVzZXIyAgkCpmjwi0XF VxAwDQYJKoZIhvcNAQELBQAERzBFAiEA8btpuWVztGCcWpUsUl7DShYChrvjLeIk cE80s4QZfCkCICpivbVMXTIs/vgCHM38zWH/mcYswFvjS9QSO39jqneH -----END CMS-----

first certificate : -----BEGIN CERTIFICATE----- MIIBezCCASGgAwIBAgIJApW0F9tUSBLQMAoGCCqGSM49BAMCMBAxDjAMBgNVBAMM BXVzZXIxMB4XDTIxMTAwNjAwMDAwMFoXDTI1MTIzMTIzNTk1OVowJjEkMCIGA1UE AwwbQ29hY2ggQUFDIERpZ2l0YWwgU2lnbmF0dXJlMFkwEwYHKoZIzj0CAQYIKoZI zj0DAQcDQgAE4Fl6y9klstVV2Jj9QzFYJzTMLEEqfOKCNM2Vst2Qfr/lZYhY2iiF 2gRMZF56aEbwyHx/ybPEnay2cpvPbw3p+6NOMEwwCQYDVR0TBAIwADAOBgNVHQ8B Af8EBAMCB4AwLwYDVR0fBCgwJjAkoCKgIIYeaHR0cHM6Ly93d3cuY29hY2hhYWMu Y29tL2EuY3JsMAoGCCqGSM49BAMCA0gAMEUCIQDmdreGaxxLnt96vqmoCNEo3Gcb jpe4NTryoZwq2dGC/QIgKxITj+FCC+EPVwU5FfAL/l0sfampm3HN4wsIXrY81PU= -----END CERTIFICATE-----

second certificate : -----BEGIN CERTIFICATE----- MIIBezCCASGgAwIBAgIJAqZo8ItFxVcQMAoGCCqGSM49BAMCMBAxDjAMBgNVBAMM BXVzZXIyMB4XDTIxMTAwNjAwMDAwMFoXDTI1MTIzMTIzNTk1OVowJjEkMCIGA1UE AwwbQ29hY2ggQUFDIERpZ2l0YWwgU2lnbmF0dXJlMFkwEwYHKoZIzj0CAQYIKoZI zj0DAQcDQgAEvNiEEJQ60HTVEfiU/UV/HlaGdDjplxBOhC6Ij++VjpLtaG70DpAY BW5sJNWzdzrhj5+xdcihfO+4tIa4ZcisUKNOMEwwCQYDVR0TBAIwADAOBgNVHQ8B Af8EBAMCB4AwLwYDVR0fBCgwJjAkoCKgIIYeaHR0cHM6Ly93d3cuY29hY2hhYWMu Y29tL2EuY3JsMAoGCCqGSM49BAMCA0gAMEUCIDOMncD+pVZZhs5XkyRPIhfJ9bdO /jDf8aELliOcoexeAiEA77J8/+WXheq9JX6W9/b9e71Bjar7/8qbVxZmRFY/Xps= -----END CERTIFICATE-----

i send you a pull request with my bugfix

Yann.

kjur / jsrsasign

KJUR.asn1.cms.CMSUtil.verifySignedData error #509