Implement PKCS#7 signature verification. #706

roysjosh · 2019-08-22T14:02:42Z

No description provided.

roysjosh · 2019-08-22T14:15:00Z

There are a few improvements that could be made but the code works. For example, if there is a timestamp in the signature it isn't verified to be within the signing certificate's valid range. It also might make sense to add some sort of callback for each signature that is verified so the caller can display "(in)valid signature by $DN" etc.

Please let me know what you think. This PR is in support of accordproject/template-archive#262.

roysjosh · 2019-08-22T14:42:38Z

Also, verification of whether or not the certificate is trusted is currently left to the caller. It might make sense to add a caStore parameter to the verify method and handle that in forge.

mattcollier

Thanks for the PR! Quick pass for style.

lib/pkcs7.js

roysjosh · 2019-08-22T15:44:38Z

I have added CA chain verification and a per-signer status callback. I can squash the commits if needed.

joosep-wm · 2019-09-03T12:55:08Z

@roysjosh
We have been working on PKCS7 verification on our own fork. So it's a nice surprise to see another implementation of it. We tried your solution with other PKCS7 signatures, namely one produced with openssl. The verification does not pass for some reason. Do you have an idea why not?

Here's an example test case

var ASSERT = require('assert');
var PKCS7 = require('../../lib/pkcs7');
var PKI = require('../../lib/pki');

(function() {
  var _pem = {
    signature: "-----BEGIN PKCS7-----\n" +
      "MIIIKAYJKoZIhvcNAQcCoIIIGTCCCBUCAQExDTALBglghkgBZQMEAgEwGAYJKoZI\n" +
      "hvcNAQcBoAsECW15IGRhdGENCqCCBXEwggVtMIIEVaADAgECAhIDzPiv475hpMq1\n" +
      "1ghTQFdshHwwDQYJKoZIhvcNAQELBQAwSjELMAkGA1UEBhMCVVMxFjAUBgNVBAoT\n" +
      "DUxldCdzIEVuY3J5cHQxIzAhBgNVBAMTGkxldCdzIEVuY3J5cHQgQXV0aG9yaXR5\n" +
      "IFgzMB4XDTE5MDgyODA4MjgyMloXDTE5MTEyNjA4MjgyMlowHDEaMBgGA1UEAwwR\n" +
      "Ki56Lmd1YXJkdGltZS5jb20wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIB\n" +
      "AQDPeNmcy7HYxfpmlLY488FC8IRQL0BHTu/IZRWIXBw+JN6hdahAUcoKd9mNDmii\n" +
      "R33IuB3mazQ2V5KyPHcHR6LqkGDgW9ZKmj5jf74Cv6DVvw4FnLlAy+4cErK4udmZ\n" +
      "pAT8L8YaLA+m5a1bAHNf4hG4sheE4jf+IkJ6VtZXhEpsew+FFjCNEmt8lYtRQita\n" +
      "5wWmEIii3qiOv/vQPmtW5mlVCnM1M8nY5LUDVCBtBmpP24uBif/kSUiIEQ6W0Puy\n" +
      "AbZbCc1N3el/yKY1RKgZcnu99zwDAM5gyBfDSn5E2jBbbkRDtT0LKtmRLGBtLgoo\n" +
      "Ho7tMZ+hKdPeEBPXj+nn/PzPAgMBAAGjggJ5MIICdTAOBgNVHQ8BAf8EBAMCBaAw\n" +
      "HQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsGAQUFBwMCMAwGA1UdEwEB/wQCMAAwHQYD\n" +
      "VR0OBBYEFI7gRMD4mehG1v3a0N3SRxHHf3yUMB8GA1UdIwQYMBaAFKhKamMEfd26\n" +
      "5tE5t6ZFZe/zqOyhMG8GCCsGAQUFBwEBBGMwYTAuBggrBgEFBQcwAYYiaHR0cDov\n" +
      "L29jc3AuaW50LXgzLmxldHNlbmNyeXB0Lm9yZzAvBggrBgEFBQcwAoYjaHR0cDov\n" +
      "L2NlcnQuaW50LXgzLmxldHNlbmNyeXB0Lm9yZy8wLQYDVR0RBCYwJIIRKi56Lmd1\n" +
      "YXJkdGltZS5jb22CD3ouZ3VhcmR0aW1lLmNvbTBMBgNVHSAERTBDMAgGBmeBDAEC\n" +
      "ATA3BgsrBgEEAYLfEwEBATAoMCYGCCsGAQUFBwIBFhpodHRwOi8vY3BzLmxldHNl\n" +
      "bmNyeXB0Lm9yZzCCAQYGCisGAQQB1nkCBAIEgfcEgfQA8gB3AOJpS64m6OlACeiG\n" +
      "G7Y7g9Q+5/50iPukjyiTAZ3d8dv+AAABbNeN/FIAAAQDAEgwRgIhALXcp2zae1wa\n" +
      "lmxb9iLBf+AKhM83ROfrNVMg7X7yuc92AiEA6QnZFrPcs0r1Yg99waISQGoirwl5\n" +
      "iVZJsZnIx6N3LUQAdwApPFGWVMg5ZbqqUPxYB9S3b79Yeily3KTDDPTlRUf0eAAA\n" +
      "AWzXjfxGAAAEAwBIMEYCIQCrUYNqkXq5/JzUGsHDT7nHrpbr54Lw2eb57jwbxf2i\n" +
      "9gIhAL0zZ0kHuzV0GeobVU8oZd2KTvWbBoKF6GdPOKg0lCfmMA0GCSqGSIb3DQEB\n" +
      "CwUAA4IBAQBXcYIkIwildnIfkabesGSSnTMiJpiknqlRrAfRdQCCzmTgeJOHuWcn\n" +
      "zFbIkK1p/Y7v1mo0q7avI4OTrevvwNhBqbzgJU5XHTITDqPydf6Yi3cExgJJEhVP\n" +
      "1IGcoHF/B1qLTSj5sUNBQpLHh5XOpqftCtQyEnjZrDPLG/X8Fb+8LaqWwsZL3GNf\n" +
      "RswWXqm7RCpm/UVhaA/0eYkzqt1YJfdP/r+ZV0kJr9RWTheAoG7m2wF8sKRG0Mmv\n" +
      "ezy7GcNiXvzHZW3zY55ZrLHv6C4T/LWeDyfdv6Mj1SBT6TquVe1YrqnrGUy9UvRL\n" +
      "H2g8wPkBe0R3Z2L4LIHtJISJvSq5etHjMYICcDCCAmwCAQEwYDBKMQswCQYDVQQG\n" +
      "EwJVUzEWMBQGA1UEChMNTGV0J3MgRW5jcnlwdDEjMCEGA1UEAxMaTGV0J3MgRW5j\n" +
      "cnlwdCBBdXRob3JpdHkgWDMCEgPM+K/jvmGkyrXWCFNAV2yEfDALBglghkgBZQME\n" +
      "AgGggeQwGAYJKoZIhvcNAQkDMQsGCSqGSIb3DQEHATAcBgkqhkiG9w0BCQUxDxcN\n" +
      "MTkwOTAzMTAzODIzWjAvBgkqhkiG9w0BCQQxIgQgJJLU6KuD8gawc+WYypnHBE9M\n" +
      "lcb5lG/lENcFJZv1GYcweQYJKoZIhvcNAQkPMWwwajALBglghkgBZQMEASowCwYJ\n" +
      "YIZIAWUDBAEWMAsGCWCGSAFlAwQBAjAKBggqhkiG9w0DBzAOBggqhkiG9w0DAgIC\n" +
      "AIAwDQYIKoZIhvcNAwICAUAwBwYFKw4DAgcwDQYIKoZIhvcNAwICASgwDQYJKoZI\n" +
      "hvcNAQEBBQAEggEAH+drkNfXK2uR6TmE02NriCHkf5PLN0kZGm/Mg2mhDWSt5b1f\n" +
      "RULwJZC5SYSNlpXnCnE0uyrZVrgcOz0W10ZisztASvzr0MRdQvx1nrlOMi79x2kw\n" +
      "RYlI1VD5NkhIIAxU1Tmn+xBM5GjqUOuEEphByknam+NEYC513vBxcSm1RR0zSOcB\n" +
      "q5yBv7JuAw7RHcqS9L5npJQ/KkrIwxUTFTr0kbl6739U2VbUlb/mPOGXNqZdWTsn\n" +
      "GZJDMT1xHDCP4vtE/0HVXjdSJirmI2loVGgarKc3QaJ36rOUcvPyCswk5CnIwcX9\n" +
      "rm3VxOUwDh4LkEorBaya68TOeycBgXcPxqPeOg==\n" +
      "-----END PKCS7-----",
    cert:   "-----BEGIN CERTIFICATE-----\n" +
      "MIIEkjCCA3qgAwIBAgIQCgFBQgAAAVOFc2oLheynCDANBgkqhkiG9w0BAQsFADA/\n" +
      "MSQwIgYDVQQKExtEaWdpdGFsIFNpZ25hdHVyZSBUcnVzdCBDby4xFzAVBgNVBAMT\n" +
      "DkRTVCBSb290IENBIFgzMB4XDTE2MDMxNzE2NDA0NloXDTIxMDMxNzE2NDA0Nlow\n" +
      "SjELMAkGA1UEBhMCVVMxFjAUBgNVBAoTDUxldCdzIEVuY3J5cHQxIzAhBgNVBAMT\n" +
      "GkxldCdzIEVuY3J5cHQgQXV0aG9yaXR5IFgzMIIBIjANBgkqhkiG9w0BAQEFAAOC\n" +
      "AQ8AMIIBCgKCAQEAnNMM8FrlLke3cl03g7NoYzDq1zUmGSXhvb418XCSL7e4S0EF\n" +
      "q6meNQhY7LEqxGiHC6PjdeTm86dicbp5gWAf15Gan/PQeGdxyGkOlZHP/uaZ6WA8\n" +
      "SMx+yk13EiSdRxta67nsHjcAHJyse6cF6s5K671B5TaYucv9bTyWaN8jKkKQDIZ0\n" +
      "Z8h/pZq4UmEUEz9l6YKHy9v6Dlb2honzhT+Xhq+w3Brvaw2VFn3EK6BlspkENnWA\n" +
      "a6xK8xuQSXgvopZPKiAlKQTGdMDQMc2PMTiVFrqoM7hD8bEfwzB/onkxEz0tNvjj\n" +
      "/PIzark5McWvxI0NHWQWM6r6hCm21AvA2H3DkwIDAQABo4IBfTCCAXkwEgYDVR0T\n" +
      "AQH/BAgwBgEB/wIBADAOBgNVHQ8BAf8EBAMCAYYwfwYIKwYBBQUHAQEEczBxMDIG\n" +
      "CCsGAQUFBzABhiZodHRwOi8vaXNyZy50cnVzdGlkLm9jc3AuaWRlbnRydXN0LmNv\n" +
      "bTA7BggrBgEFBQcwAoYvaHR0cDovL2FwcHMuaWRlbnRydXN0LmNvbS9yb290cy9k\n" +
      "c3Ryb290Y2F4My5wN2MwHwYDVR0jBBgwFoAUxKexpHsscfrb4UuQdf/EFWCFiRAw\n" +
      "VAYDVR0gBE0wSzAIBgZngQwBAgEwPwYLKwYBBAGC3xMBAQEwMDAuBggrBgEFBQcC\n" +
      "ARYiaHR0cDovL2Nwcy5yb290LXgxLmxldHNlbmNyeXB0Lm9yZzA8BgNVHR8ENTAz\n" +
      "MDGgL6AthitodHRwOi8vY3JsLmlkZW50cnVzdC5jb20vRFNUUk9PVENBWDNDUkwu\n" +
      "Y3JsMB0GA1UdDgQWBBSoSmpjBH3duubRObemRWXv86jsoTANBgkqhkiG9w0BAQsF\n" +
      "AAOCAQEA3TPXEfNjWDjdGBX7CVW+dla5cEilaUcne8IkCJLxWh9KEik3JHRRHGJo\n" +
      "uM2VcGfl96S8TihRzZvoroed6ti6WqEBmtzw3Wodatg+VyOeph4EYpr/1wXKtx8/\n" +
      "wApIvJSwtmVi4MFU5aMqrSDE6ea73Mj2tcMyo5jMd6jmeWUHK8so/joWUoHOUgwu\n" +
      "X4Po1QYz+3dszkDqMp4fklxBwXRsW10KXzPMTZ+sOPAveyxindmjkW8lGy+QsRlG\n" +
      "PfZ+G6Z6h7mjem0Y+iWlkYcV4PIWL1iwBi8saCbGS5jN2p8M+X+Q7UNKEkROb3N6\n" +
      "KOqkqm57TH2H3eDJAkSnh6/DNFu0Qg==\n" +
      "-----END CERTIFICATE-----"
  };

  describe('pkcs7', function() {
    it('should verify PKCS#7 signature', function() {
      var p7 = PKCS7.messageFromPem(_pem.signature);
      var verified = p7.verify(PKI.createCaStore([_pem.cert]));
      ASSERT.equal(verified, true);
    });
  });
})();

roysjosh · 2019-09-03T16:20:16Z

@roysjosh
We have been working on PKCS7 verification on our own fork. So it's a nice surprise to see another implementation of it. We tried your solution with other PKCS7 signatures, namely one produced with openssl. The verification does not pass for some reason. Do you have an idea why not?

Ah, this is because the code I added to parse the attributes back into forge objects in _attributeFromAsn1 doesn't handle S/MIME capabilities or error out to let you know of that current limitation. I'll do one of those two now. Thanks for the example!

mattcollier · 2019-09-09T16:06:02Z

lib/pkcs7.js

@@ -537,6 +548,134 @@ p7.createSignedData = function() {
    // add signer info
    msg.signerInfos = _signersToAsn1(msg.signers);
  }
+
+  function verifySignerInfos(mds, caStore, options) {


I see you're providing for options.callback. This seems to me to be an unconventional way of implementing a callback which would not be compatible with util.promisify implementations which expect the last param to be a callback function.

That said, can you say more about the use case for callbacks here? Given that all the code here is synchronous, does the callback add value?

If the callback is going to remain part of this API, it appears to me that it is not being called in a number of situations. For example, if the conditional on L556 fails we end up at L677 retrurn rval; and the callback is never called. Also, for the most part this function is written to throw errors at various times when it may be appropriate to call the callback with callback(err); instead.

Do you think it would be simpler to eliminate the callback functionality?

I added a callback so that as a part of the verification process you would get information on good signatures to display to the user. I would like to keep it for that reason. I'll clean it up and make it promisifyable and we'll see how it looks.

Pushed, let me know what you think. Double check the code flow? L556-573 is a contained if/else block.

Hello, are you saying you implemented the callback in order to return different values from a synchronous call vs a callback call? It does appear that is what is happening? I think we need the sync/callback versions to return the same data structure, is there something preventing consistency here? And if the synchronous version can return the same data structure as the callback, do we still need the callback? https://github.com/digitalbazaar/forge/pull/706/files#diff-d4c741d422d58aea2d7ffefe1687abd2R678

Also, without a process.nextTick or something to defer execution, I believe this code effectively runs synchronously with or without the callback.

The purpose of the callback isn't to get a result so much as status updates on each signature processed. The verify function could return an array of results, yes. At that point I would just eliminate the callback. Is that what you would prefer?

I understand now that my confusion has been caused by referring to this passed in function as a callback. I think of a callback as a function that is called when the called function completes/errors. This is reflected in the util.promisify API such that the Promise is resolved when the callback is invoked, which is not what you are attempting to do here.

If this were a long running asynchronous process, a solution involving event emitters might be appropriate.

Now that I understand what you're trying to do and options.onSignatureVerificationComplete handler might be appropriate.

Do you have some use case that requires there to be status updates on each signature? If not, then I think removing the status callback function would be best.

If I modify the return to contain the same info as the status callback/emitter then no, I don't need it. I'll do that now.

I ended up going with the per-signature event emitter vs a return of an array of results. Let me know what you think.

There should be a common pattern in the whole lib for events and more detailed results. That's a bigger project. This does do the common return true/false and that will work for now.

lib/pkcs7.js

roysjosh · 2019-12-06T19:02:13Z

Ping?

roysjosh · 2020-03-03T14:39:09Z

Is there any interest in merging this?

davidlehn · 2020-03-10T19:27:00Z

@roysjosh Yes. We've been busy for a long time and haven't been keeping up with forge PRs. Apologies for that, we really do appreciate the contribution! Could you add a line or two example to the README? Our README needs improvement, but good to at least have something to note a feature exists.

davidlehn · 2020-03-10T19:41:22Z

lib/pkcs7.js

+    bytes.getByte();
+    // read and discard length bytes
+    asn1.getBerValueLength(bytes);
+    bytes = bytes.getBytes();


Does any deeper validation need to happen here? Or if the stream is bogus will the digests just not match so that's fine?

davidlehn · 2020-03-10T20:07:18Z

lib/util.js

+  } else if(l.attributes.length === r.attributes.length) {
+    // all attributes are the same so issuer matches subject
+    rval = true;
+    var iattr, sattr;


Should switch from this "issuer" "subject" naming to match the "l" "r" naming.

Now dn1attr and dn2attr

davidlehn · 2020-03-10T20:08:14Z

lib/util.js

@@ -2998,3 +2998,26 @@ util.estimateCores = function(options, callback) {
    }, 0);
  }
 };
+
+util.compareDN = function(l, r) {


This is currently just used internally, but some doc strings would be good. "l" may not be the best choice for a param. Looks a lot like "I" in many fonts. I'm guessing this is left vs right. Could just as easily be a b or x0 x1, etc.

Documentation added. Parameters renamed to dn1 and dn2.

dselman · 2020-07-27T08:30:59Z

There are a few improvements that could be made but the code works. For example, if there is a timestamp in the signature it isn't verified to be within the signing certificate's valid range. It also might make sense to add some sort of callback for each signature that is verified so the caller can display "(in)valid signature by $DN" etc.

Please let me know what you think. This PR is in support of accordproject/cicero#262.

Any update on this please? We are waiting to merge a downstream change that requires this.

hamano · 2021-11-26T07:48:33Z

Hi, I just added verification with RSASSA-PSS.
osstech-jp@370d3d3

also ECDSA verification is works with #925
many thanks.

dhensby · 2022-06-22T12:45:22Z

@davidlehn - it'd be really great to have this merged in as pkcs7 is an area I'm working in and having signature verification implemented would be really helpful 🙏

dhensby · 2022-06-22T16:06:14Z

Something that would be useful would be a way to provide the signing certificate instead of the caStore and having the chain validated.

Much like the -noverify flag for openssl

Currently the user must have the full chain available, but they may only have the signing certificate, and if this is the case (and it's trusted) there's no need to verify the whole chain.

mattcollier requested changes Aug 22, 2019

View reviewed changes

lib/pkcs7.js Outdated Show resolved Hide resolved

lib/pkcs7.js Outdated Show resolved Hide resolved

roysjosh force-pushed the ImplementPkcs7Verify branch from f5115f9 to 6c28624 Compare August 22, 2019 14:46

roysjosh force-pushed the ImplementPkcs7Verify branch 2 times, most recently from e0c242b to 2df6389 Compare August 23, 2019 13:12

roysjosh requested a review from mattcollier September 9, 2019 13:50

mattcollier reviewed Sep 9, 2019

View reviewed changes

mattcollier requested review from davidlehn and dlongley September 9, 2019 16:07

roysks added 4 commits October 11, 2019 10:29

Implement PKCS#7 signature verification.

cfff1d7

Add certificate chain verification.

c1f5084

Add callback for each PKCS#7 signature verification.

fd656a1

Support arbitrary attributes (e.g. S/MIME caps)

43ff714

roysjosh force-pushed the ImplementPkcs7Verify branch from 9720a1a to 43ff714 Compare October 11, 2019 14:33

pkcs7: allow for null validityCheckDate for consistency with x509

25c7b12

roysjosh requested a review from mattcollier November 1, 2019 18:03

roysjosh mentioned this pull request Dec 11, 2019

Signed Cicero Template Archives accordproject/template-archive#262

Open

jeromesimeon mentioned this pull request Dec 12, 2019

feat(archive): add signing support (#262) accordproject/template-archive#496

Closed

roysjosh mentioned this pull request Dec 12, 2019

Add extKeyUsage to supported extensions in pki.verifyCertificateChain #500

Open

davidlehn added the feature label Mar 10, 2020

davidlehn reviewed Mar 10, 2020

View reviewed changes

roysks added 2 commits March 11, 2020 15:15

Add example of PKCS#7 verification to README

b4385e2

Add util.compareDN documentation and update variable names

fce10da

x-077 mentioned this pull request Jun 25, 2021

When will the version be updated? How often is the update? #882

Closed

davidlehn added this to the v1.x milestone Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PKCS#7 signature verification. #706

Implement PKCS#7 signature verification. #706

roysjosh commented Aug 22, 2019

roysjosh commented Aug 22, 2019

roysjosh commented Aug 22, 2019

mattcollier left a comment

roysjosh commented Aug 22, 2019

joosep-wm commented Sep 3, 2019 •

edited

roysjosh commented Sep 3, 2019

mattcollier Sep 9, 2019

roysjosh Sep 11, 2019

roysjosh Sep 12, 2019

mattcollier Sep 12, 2019

roysjosh Sep 16, 2019

mattcollier Sep 16, 2019

roysjosh Sep 17, 2019

roysjosh Oct 11, 2019

davidlehn Mar 10, 2020

roysjosh commented Dec 6, 2019

roysjosh commented Mar 3, 2020

davidlehn commented Mar 10, 2020

davidlehn Mar 10, 2020

davidlehn Mar 10, 2020

roysjosh Mar 11, 2020

davidlehn Mar 10, 2020

roysjosh Mar 11, 2020

dselman commented Jul 27, 2020 •

edited

hamano commented Nov 26, 2021

dhensby commented Jun 22, 2022

dhensby commented Jun 22, 2022

Implement PKCS#7 signature verification. #706

Are you sure you want to change the base?

Implement PKCS#7 signature verification. #706

Conversation

roysjosh commented Aug 22, 2019

roysjosh commented Aug 22, 2019

roysjosh commented Aug 22, 2019

mattcollier left a comment

Choose a reason for hiding this comment

roysjosh commented Aug 22, 2019

joosep-wm commented Sep 3, 2019 • edited

roysjosh commented Sep 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roysjosh commented Dec 6, 2019

roysjosh commented Mar 3, 2020

davidlehn commented Mar 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dselman commented Jul 27, 2020 • edited

hamano commented Nov 26, 2021

dhensby commented Jun 22, 2022

dhensby commented Jun 22, 2022

joosep-wm commented Sep 3, 2019 •

edited

dselman commented Jul 27, 2020 •

edited