OpenSSL output P7B structure in open_pkcs7_verify and add a openssl_pkcs7_read function #2563

jelly · 2017-06-07T12:48:15Z

This PR adds support for retrieving the P7B structure which may exists in a signed email, this structure usually contains intermediate certificates which are needed for OCSP validation.
Therefore openssl_pkcs7_verify has an extra parameter added to save this structure to a file.

Since PHP has no support to read PKCS#7 structure, add a new function openss_pkcs7_read, which is based on openssl_pkcs12_read.
There has been a request for reading a PKCS#7 structure for a while.

Background information, with openssl's command line utlity this can be achieved as following:

openssl smime -verify -pk7out  -in signed_email.eml  > foo.pkcs7
openssl pkcs7 -print_certs -in foo.pkcs7

There is more information available in my mail to php-internals http://news.php.net/php.internals/99293

bukka · 2017-06-07T19:17:42Z

ext/openssl/openssl.c

+	int i;
+
+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "sz/", &p7b, &p7b_len,
+				&zout) == FAILURE)


Please use braces after if (it's PHP core CS)

bukka · 2017-06-07T19:19:32Z

ext/openssl/openssl.c

+
+	if (0 >= BIO_write(bio_in, p7b, (int)p7b_len)) {
+		php_openssl_store_errors();
+		goto clean_exit;


wouldn't that mean that you try to free p7 that wasn't allocated yet. I don't think we should be doing that even it's NULL and some platform accept that (not sure how portable it is)...

Hmm C89 denotes:

Quoting the C standard, 7.20.3.2/2 from [ISO-IEC 9899](http://www.open-std.org/JTC1/SC22/wg14/www/docs/n1124.pdf) void free(void *ptr); If ptr is a null pointer, no action occurs.

But I'm fine with changing it, since the rest of the code follows the same style guide.

Yep but some old compilers were failing on that. I don't actually thing we support any such system but it's still better to follow this rule...

bukka · 2017-06-07T19:20:58Z

ext/openssl/openssl.c

+
+	PHP_OPENSSL_CHECK_SIZE_T_TO_INT(p7b_len, p7b);
+
+	bio_in = BIO_new(BIO_s_mem());


you should probably check if it's not NULL..

it would be actually caught below as BIO_write will check that

Ok, then I won't add a check.

I say add the check anyway. We can't guarantee that OpenSSL won't suddenly start assuming bio_in is valid. Compared to the file access/crypto cost, one int branch won't be the perf killer.

Yeah I agree with @sgolemon. It's better to add checks than relay on library.

bukka · 2017-06-07T19:21:37Z

ext/openssl/openssl.c

+	p7 = PEM_read_bio_PKCS7(bio_in, NULL, NULL, NULL);
+	if (p7 == NULL) {
+		php_openssl_store_errors();
+		goto clean_exit;


again, it will free NULL...

Adjusted, by checking the value in the exit label

bukka · 2017-06-07T19:28:34Z

ext/openssl/tests/openssl_pkcs7_read.phpt

@@ -0,0 +1,28 @@
+--TEST--


might be good to rename to openssl_pkcs7_read_basic.phpt to keep it consistent

Makes sense will do.

bukka · 2017-06-07T19:30:52Z

ext/openssl/tests/openssl_pkcs7_verify_basic.phpt

@@ -36,6 +42,11 @@ if (file_exists($contentfile)) {
 	echo "true\n";
 	unlink($contentfile);
 }
+
+if (file_exists($pkcsfile)) {


Pls move it to the --CLEAN-- section (see openssl_x509_export_to_file_basic.phpt for example...)

Ok, maybe the others should be moved there too. But I would rather see that in a separate PR.

bukka · 2017-06-07T19:31:43Z

ext/openssl/tests/openssl_pkcs7_verify_basic.phpt

@@ -14,6 +14,11 @@ if ($contentfile === false) {
 	die("failed to get a temporary filename!");
 }

+$pkcsfile = tempnam(sys_get_temp_dir(), "ssl");


it's better to not use temp for test files to keep it consistent (see openssl_x509_export_to_file_basic.phpt for example of the usual naming )

bukka · 2017-06-07T19:32:32Z

ext/openssl/openssl.c

+	if (certs != NULL) {
+		for (i = 0; i < sk_X509_num(certs); i++) {
+			X509* aCA = sk_X509_pop(certs);
+			if (!aCA) break;


braces should be here and a new line...

bukka · 2017-06-07T19:32:44Z

ext/openssl/openssl.c

+		X509_CRL *crl;
+		for (i = 0; i < sk_X509_CRL_num(crls); i++) {
+			X509_CRL* crl = sk_X509_CRL_pop(crls);
+			if (!crl) break;


braces here as well ;)

bukka · 2017-06-07T19:33:57Z

ext/openssl/openssl.c

+				add_index_zval(zout, i, &zcert);
+			}
+
+			BIO_free(bio_out);


if BIO_new fails, then it would be NULL...

Moved the free into the BIO_write if statement as it can't be NULL then.

bukka · 2017-06-11T17:36:26Z

@jelly It looks ok from the quick look now. Please squash it and will test it next week. If no one objects till then and all is fine, I will try to merge as well.

sgolemon · 2017-06-11T23:32:12Z

ext/openssl/openssl.c

+	}
+
+	zval_dtor(zout);
+	array_init(zout);


It seems wierd to return by ref through an arg when you're essentially not using the return value at all.

Why not write your structure to return_value instead?

I've based the function on openssl_pkcs12_read() which does the same, and I think it's nice to keep the functions in php-openssl consistent. One exception is openssl_x509_read which returns a resource or false which makes my argument a bit weaker.

This is actually how it works for most openssl ext function (this one is actually based on openssl_pkcs12_read). I'm not saying that I like it but I think that the consistency is much more important in here so I would prefer to leave it as it is.

m'kay. Local consistency is preferable, just struck me as odd at first glance. Freely ignore this review comment. :D

Ok, I will drop the commits. @sgolemon thanks for the review btw 👍

@bukka reverted those changes, if everything looks fine I will squash the commits.

@jelly Looks good. Please squash the commits and I will test and possibly merge.

sgolemon · 2017-06-11T23:34:08Z

ext/openssl/openssl.c

+	PKCS7 * p7 = NULL;
+	int i;
+
+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "sz/", &p7b, &p7b_len,


Minor nit; New code should prefer "S" (zend_string*) for string args. It makes it easier to share refs and pass data around to other APIs.

Could you please elaborate how it makes things easier in this particular case?

sgolemon · 2017-06-11T23:35:42Z

ext/openssl/openssl.c

+
+	PHP_OPENSSL_CHECK_SIZE_T_TO_INT(p7b_len, p7b);
+
+	bio_in = BIO_new(BIO_s_mem());


I say add the check anyway. We can't guarantee that OpenSSL won't suddenly start assuming bio_in is valid. Compared to the file access/crypto cost, one int branch won't be the perf killer.

sgolemon · 2017-06-11T23:39:20Z

ext/openssl/openssl.c

+
+	if (certs != NULL) {
+		for (i = 0; i < sk_X509_num(certs); i++) {
+			X509* aCA = sk_X509_pop(certs);


I don't know the API here, but pop would imply that it's removing the cert from the certs vector. If that's the case, then on the next loop sk_X509_num will be one less while i continues to increase and you'll end up with only the first half of your entries.

Have you tried this with more than one cert?

@sgolemon Good catch! pop decreases the num so the num should be first saved to the variable and the variable used in the loop. Alternatively sk_X509_value could be used IIRC.

sgolemon · 2017-06-11T23:41:25Z

ext/openssl/openssl.c

+			}
+
+			bio_out = BIO_new(BIO_s_mem());
+			if (PEM_write_bio_X509(bio_out, aCA)) {


As with above, please code defensively.
if (bio_out && PEM_writre_bio_X509(...)) {

sgolemon · 2017-06-11T23:44:10Z

ext/openssl/tests/openssl_pkcs7_read_basic.phpt

+var_dump(openssl_pkcs7_read("", $result));
+var_dump(openssl_pkcs7_read($certfile, $result));
+var_dump(openssl_pkcs7_read($infile, $result));
+var_dump(empty($result));


Maybe dump the actual certificate contents? If it contains unpredictable components, you can use an EXPECTF

Hmm it doesn't contain unpredictable components and should match openssl pkcs7 -in cert.p7b -print_certs (but without the subject= and issuer=)

sgolemon · 2017-06-11T23:45:21Z

ext/openssl/tests/openssl_pkcs7_verify_basic.phpt

@@ -26,6 +28,7 @@ var_dump(openssl_pkcs7_verify($eml, 0));
 var_dump(openssl_pkcs7_verify($eml, 0, $empty));
 var_dump(openssl_pkcs7_verify($eml, PKCS7_NOVERIFY, $outfile));
 var_dump(openssl_pkcs7_verify($eml, PKCS7_NOVERIFY, $outfile, $cainfo, $outfile, $contentfile));
+var_dump(openssl_pkcs7_verify($eml, PKCS7_NOVERIFY, $outfile, $cainfo, $outfile, $contentfile, $pkcsfile));


As above, let's verify something about the written contents. Whether that's a readfile or even just a filesize

Added a check to verify the contents of $pkcsfile matches the expected pk7b output.

sgolemon · 2017-06-11T23:46:50Z

ext/openssl/openssl.c

+	ZEND_ARG_INFO(0, infilename)
+	ZEND_ARG_INFO(1, certs)
+ZEND_END_ARG_INFO()
+


Don't forget to add the new param to arginfo_openssl_pkcs7_verify

sgolemon · 2017-06-11T23:49:08Z

ext/openssl/openssl.c

 				&flags, &signersfilename, &signersfilename_len, &cainfo,
-				&extracerts, &extracerts_len, &datafilename, &datafilename_len) == FAILURE) {
+				&extracerts, &extracerts_len, &datafilename, &datafilename_len, &p7bfilename, &p7bfilename_len) == FAILURE) {


Minor nit; As with below, prefer P for pathnames as zend_string rather than separate pieces. No need to change the existing args in the same commit, but let's do new stuff right.

I don't see much point in here as we need just an actual string to pass to the library. I see the point if it's in internal API but if it's just passed to the library, then I don't think we should be changing that. It will just bring addition of extra macro wrapping around zend_string values when calling the library func.

sgolemon · 2017-06-11T23:50:47Z

ext/openssl/openssl.c

+
+			if (p7bout) {
+				PEM_write_bio_PKCS7(p7bout, p7);
+				BIO_free(p7bout);


Looks like p7out can leak if the PKCS7_verify call fails.

That seems right, thanks! I'll move it to the exit label.

bukka · 2017-06-12T15:57:24Z

ext/openssl/openssl.c

+		goto clean_exit;
+	}
+
+	if (0 >= BIO_write(bio_in, ZSTR_VAL(p7b), (int)ZSTR_LEN(p7b))) {


This is exactly what I don't like. What's the point of using zend_string if the result is just extra macros. Please can you change it back to s unless there is some specific reason why we should use zend_string.

Ok, I'll drop the two commits.

bukka · 2017-06-12T16:00:45Z

ext/openssl/openssl.c

+			goto clean_exit;
+		}
+
+		p7bout = BIO_new_file(ZSTR_VAL(p7bfilename), "w");


I'd really prefer not to use ZSTR_VAL and rather use p

krakjoe · 2017-06-13T08:08:57Z

@bukka I think we will wait for your review of this ... please do merge if okay ...

jelly · 2017-06-15T08:14:52Z

@bukka squashed the commits

bukka · 2017-06-18T18:34:05Z

@jelly It actually segfaults for me with OpenSSL 1.0.2 when running ext/openssl/tests/openssl_pkcs7_read_basic.phpt. I did a quick debug and it fails when freeing p7. It might be because you are freeing ca using X509_free which is actually part of p7. Are you sure that it should be freed? I haven't had time to look into it properly but will take a look if you are not able to recreate it.

bukka · 2017-06-18T18:36:31Z

Well it even fails on travis so you should be able to recreate it:

https://travis-ci.org/php/php-src/jobs/243162346#L1331

jelly · 2017-06-19T08:31:51Z

Hmm it works for me with OpenSSL 1.1, but indeed on Debian Jessie fails with OpenSSL 1.0 so I will take a look at it.

Add an optional argument to openssl_pkcs7_verify to save the P7B structure which can contain extra CA intermediate certificates send along with an S/MIME signed email. Introduce a new function called openssl_pkcs7_read, which can read a PKCS#7 structure passed as a string and returns by reference an array with PEM certificates formatted as a string.

bukka · 2017-06-22T19:41:29Z

Thanks for the fix. I have tested on all supported version and all is good. So it's merged now!

php-pulls · 2017-06-22T19:42:34Z

Comment on behalf of bukka at php.net:

Merged via 787a18a

bukka reviewed Jun 7, 2017

View reviewed changes

sgolemon requested changes Jun 11, 2017

View reviewed changes

bukka reviewed Jun 12, 2017

View reviewed changes

krakjoe added Waiting on Review Feature labels Jun 13, 2017

jelly force-pushed the openssl_pkcs7_read branch 2 times, most recently from 92742b2 to 4041dee Compare June 15, 2017 08:14

jelly force-pushed the openssl_pkcs7_read branch 2 times, most recently from b23135d to 3aa1ee6 Compare June 19, 2017 11:26

jelly force-pushed the openssl_pkcs7_read branch from 3aa1ee6 to 3b7dc1f Compare June 22, 2017 09:20

php-pulls closed this Jun 22, 2017

jelly deleted the openssl_pkcs7_read branch July 4, 2017 08:27


		PHP_OPENSSL_CHECK_SIZE_T_TO_INT(p7b_len, p7b);

		bio_in = BIO_new(BIO_s_mem());

OpenSSL output P7B structure in open_pkcs7_verify and add a openssl_pkcs7_read function #2563

OpenSSL output P7B structure in open_pkcs7_verify and add a openssl_pkcs7_read function #2563

Uh oh!

Conversation

jelly commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelly Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka commented Jun 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelly Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelly Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka Jun 7, 2017 •

edited

Loading

jelly Jun 8, 2017 •

edited

Loading

jelly Jun 13, 2017 •

edited

Loading

jelly Jun 13, 2017 •

edited

Loading