This PR adds support for CMS. #5251

elear · 2020-03-10T13:27:03Z

Hi everyone,

This is my first PR for this project. It contains support for CMS, which is RFC 5652. The code borrows heavily from earlier work, because mostly speaking CMS is an incremental upgrade of PKCS#7. The notable changes are these:

PKCS7 struct to CMS_ContentInfo
Some internalizing of tests (e.g, they're shoved down to OpenSSL)
change in the field size for flags

CMS is not present in all versions of OpenSSL. Notably Apple is doing something funky these days with LibreSSL. No idea how they are compiling PHP, but if a dependency is required, it is not yet in this PR.

elear · 2020-03-11T20:13:02Z

I am thinking of adding some additional capability to express DER/PEM/CMS output in openssl_cms_sign. This provides a bit more flexibility beyond email of the CMS routines.

elear · 2020-03-13T06:51:09Z

Please note a additional commits that provide for use and testing of different encodings.

bukka · 2020-03-15T20:33:47Z

Just had a quick look and essentially it is pkcs7 moved to CMS which I think is probably a good way how to get it finally in. It might be worth to merge some common parts together. It would be also good to have a way to get CMS_ContentInfo to user so further operation can be performed on it but I think that's way out of scope for this.

I need to do a bit more thorough review but could you please clean it up a little bit. Mainly those parts:

When it's exactly the same code in pkcs7 and CMS and can be separated from, it would be good to move it to the function so there is some code re-use.
Variables are still named for pkcs7 so might be good to use names for CMS.
CS doesn't look right (e.g. switches and some conditions as well as enum defs and pointers) - we use just tabs and spaces between =. Although it's not that consistent in openssl.c, it would be good if new code tries to follow it.

elear · 2020-03-15T21:07:18Z

@bukka Thank you very much for your review. I'll take a look at code reuse. I think there should be a bit, but some logic has changed. For instance CMS_ContentInfo really is an opaque struct. If you like, what I can do is expose the various cms_get0 functions that have same semantic value as poking around the PKCS7 struct. Wilco on the rest.

elear · 2020-03-16T13:50:48Z

Hi,

I've looked at factoring out code to create static functions. At the end of the day, it's not worth it for several reasons:

The type being operated upon is changed throughout- (PKCS7->CMS_ContentInfo)
Some of the logic has indeed changed. PKCS7 is not an opaque struct whereas CMS_ContentInfo is.
A few of the calling interfaces have changed. This includes the width of number of flag fields.
I am introducing substantial new functionality with CMS. I am not doing this with PKCS#7 because I would rather first see that the code is found to be generally stable with CMS and then backport the capability to PKCS#7.

In the last commit I did my best to bring the coding style in line with what you would expect. I welcome further edits on top if you spot any issues.

elear · 2020-03-30T13:14:08Z

@bukka any chance to move this one along?

ext/openssl/openssl.c

ext/openssl/tests/openssl_cms_decrypt_basic.phpt

ext/openssl/tests/openssl_cms_sign_basic.phpt

bukka · 2020-04-05T19:19:47Z

Added some comments but looks mostly good.

elear · 2020-04-06T17:43:59Z

Not yet safe to merge. Look for another commit shortly.

elear · 2020-04-07T05:46:58Z

@bukka Please have one more look. Thanks!

ext/openssl/openssl.c

cmb69 · 2020-05-31T15:04:09Z

Let me XFAIL the test for Windows for now.

But it does not fail, does it?

elear · 2020-05-31T15:05:30Z

It does on Appveyor for some reason - sometimes. Serious head scratching going on here.

elear · 2020-05-31T15:39:02Z

@cmb69 ok, I see. I hadn't realized that an XFAIL would warn on a pass. Suggestions?

cmb69 · 2020-05-31T15:47:54Z

Well, is it possible that the issue is due to text mode translation (LF <-> CRLF)?

elear · 2020-05-31T15:52:48Z

I don't think so. When it blows up, what I am seeing is garbage out of CMS_encrypt: an invalid der file is being created, and no error is produced.

elear · 2020-05-31T15:56:49Z

Also- I think I did try setting CMS_BINARY and the same issue occurred. Again, it only seems to happen periodically with Appveyor.

cmb69 · 2020-05-31T21:35:55Z

When it blows up, what I am seeing is garbage out of CMS_encrypt: an invalid der file is being created, and no error is produced.

Is it really CMS_encrypt producing garbage, or is there a chance that another test run in parallel interferes (maybe due to an issue with tmpname on Windows, or whatever)?

Anyhow, I would suggest to dynamically mark the test as "xfail" on Windows (you can just replace the "skip" argument of the print statement with "xfail"; ideally add some reason for why it is marked as xfail), so that it is run (and as such produces a diff), and also because the test ought to pass on Windows. The warning, in case the test succeedes, isn't a real issue.

bukka · 2020-05-31T22:12:19Z

Hmm that's strange. Might be worth to add some debugging output (e.g. openssl errors) or maybe hack it a bit so more info is visible in AppVeyor.

If skip or xfail is added, it should be clear what the reason is for that so we are sure that it's not a real issue.

elear · 2020-06-01T07:55:01Z

ok, i'm going to trim down the test to make sure there's no extraneous gunk

elear · 2020-06-01T09:23:29Z

Just to follow up, the test is currently passing, so it is possible there was something going on with tempnam() on Windows. I propose to just do a bunch of git push -fs during the week to see if the problem recurs.

elear · 2020-06-01T18:01:28Z

FYI I pounded the CI servers all day today and didn't run into any CMS-related failures.

elear · 2020-06-02T09:12:12Z

@bukka @cmb69 Ok, I'm comfortable with this code getting merged.

cmb69 · 2020-06-02T11:44:41Z

Fine, thanks for checking the AppVeyor issue!

I'm not sure what to do with the RFC. I don't think this PR needs an RFC.

elear · 2020-06-02T12:02:15Z

@cmb69 I am happy to retire the RFC of course if the maintainers would just like to merge. I would like to retain it in the archives, because it will help with documentation.

cmb69 · 2020-06-02T12:05:08Z

Yes, the RFC is definitely very useful for the documentation, and UPGRADING could just get a link to the RFC instead of lengthy explanation.

Ocramius · 2020-06-04T13:39:07Z

ext/openssl/openssl.stub.php

+ */
+function openssl_cms_decrypt(string $infilename, string $outfilename, $recipcert, $recipkey, int $encoding = OPENSSL_ENCODING_SMIME): bool {}
+
+function openssl_cms_read(string $infilename, &$certs): bool {}


Any better type possible for $certs?

For references, the type is only checked on input, but this is an out parameter. So no, no better type possible for now.

That and this is directly analogous to the pkcs7 call.

is an update to PKCS7. The functions are analogous BUT NOT IDENTICAL to openssl_pkcs7*. In particular, support for different encodings (PEM, DER, SMIME) is now available.

elear · 2020-06-07T08:27:36Z

@bukka good to go?

bukka · 2020-06-07T20:15:59Z

@elear Yes just merged as 367c55f (sorry for small grammar mistake that I did in the slightly changed git commit message: should be it adds - hopefully no one reads commit messages :) ).

Thanks for this very nice addition to the OpenSSL ext!

bukka · 2020-06-07T20:17:15Z

Oh wrong commit linked. The 367c55f is for NEWS and UPGRADING changes that I did.

This is the commit with the actual change: 8583b8a

elear · 2020-06-08T06:16:52Z

Thank you all!

nikic · 2020-06-08T08:15:36Z

I've applied a small memory leak fix in 57e17e5.

> - Added openssl_cms_encrypt() encrypts the message in the file with the > certificates and outputs the result to the supplied file. > - Added openssl_cms_decrypt() that decrypts the S/MIME message in the file > and outputs the results to the supplied file. > - Added openssl_cms_read() that exports the CMS file to an array of PEM > certificates. > - Added openssl_cms_sign() that signs the MIME message in the file with > a cert and key and output the result to the supplied file. > - Added openssl_cms_verify() that verifies that the data block is intact, > the signer is who they say they are, and returns the certs of the signers. Refs: * https://wiki.php.net/rfc/add-cms-support * https://github.com/php/php-src/blob/c0172aa2bdb9ea223c8491bdb300995b93a857a0/UPGRADING#L727-L736 * php/php-src#5251 * php/php-src@8583b8a Includes unit tests.

> Added Cryptographic Message Syntax (CMS) (RFC 5652) support composed of > functions for encryption, decryption, signing, verifying and reading. The > API is similar to the API for PKCS #7 functions with an addition of new > encoding constants: OPENSSL_ENCODING_DER, OPENSSL_ENCODING_SMIME and > OPENSSL_ENCODING_PEM. And from the RFC: > The following analogs to PKCS#7 are also added: > OPENSSL_CMS_DETACHED > OPENSSL_CMS_TEXT > OPENSSL_CMS_NOINTERN > OPENSSL_CMS_NOVERIFY > OPENSSL_CMS_NOCERTS > OPENSSL_CMS_NOATTR > OPENSSL_CMS_BINARY > OPENSSL_CMS_NOSIGS Refs: * https://wiki.php.net/rfc/add-cms-support * https://github.com/php/php-src/blob/c0172aa2bdb9ea223c8491bdb300995b93a857a0/UPGRADING#L603-L608 * php/php-src#5251 * php/php-src@8583b8a Includes unit test.

nikic · 2020-08-14T12:40:05Z

For both openssl_pkcs7_decrypt and openssl_cms_decrypt, what is the reason for $recipkey to be optional? The current logic tries to take the private key from the recipcert if recipkey is not given, but how can the certificate contain a private key?

It doesn't look like there's test coverage for such usage.

elear · 2020-08-14T20:04:33Z

Hi @nikic I think the magic is in php_openssl_pkey_from_zval() which does indeed try to go hunting for a private key in a cert. I haven't chased this down for the cms routine, but a test case would not be out of order.

carusogabriel added the RFC label Mar 10, 2020

nikic requested a review from bukka March 11, 2020 20:16

bukka reviewed Apr 5, 2020

View reviewed changes