Openssl 1.1.0 support for 7.0 #2438

bukka · 2017-03-26T17:39:13Z

This is a backport and some 7.0 specific ports for OpenSSL 1.1 support.

@weltling Are you ok with that for 7.0 branch? The reason for that is that some distros already applies their own patches just to support OpenSSL 1.1 so this will prevent further diversion. In addition it unifies the code base for openssl ext so it is easier to write fixes for some parts (mainly current pkey bugs) that diverged between 7.0 and 7.1 (I mean I don't have to write it twice if I want to fix it in 7.0...)

bukka · 2017-03-26T17:42:16Z

Btw. I see few fails on my 7.0 build with OpenSSL 1.1 but I see them also with 7.1 so I will take a look on them together later.

weltling · 2017-03-27T12:13:06Z

ext/openssl/openssl.c

 /* {{{ php_openssl_pkey_init_dsa */
-zend_bool php_openssl_pkey_init_dsa(DSA *dsa)
+zend_bool php_openssl_pkey_init_dsa(DSA *dsa, zval *data)


I'd suggest not to change signatures in this and similar cases. Reason - the visibility with at least gcc makes such functions visible, so there might be possible breakage. With later versions like master, such functions should be marked as static, so any arbitrary change won't for sure affect the outer world. Probably same would concern the newly introduced ones like php_openssl_pkey_init_and_assign_rsa().

Thanks.

Do we really need to be so strict? That function is not in any header and even marked as API one (meaning not exported on win). Someone would have to copy the signature which seems very unlikely to me considering that I think I added that function in 7.0 as part of one bugfix. I agree that they should be marked as static. It was my omission but I still don't think it needs to be considered as part of the ABI. Anyway if you think there might be some breakage, I will keep the old one around and rename this one but it will be inconsistent with 7.1...

Yeah, i see that, just it was a possible issue that i saw. I've mentioned gcc in particular, as some might willingly misuse the default visibility :) Didn't check it were not in the header, in that case we should be fine to have the signature change as in the patch. I see no other potential issues, so it's now only about to test and to get some feedback.

Thanks.

weltling · 2017-03-27T12:25:16Z

@bukka, currently looks good to me. The patch doesn't breach any structures, and it's even better. Probably even if it would, it wouldn't matter much, as the possible dependencies have to depend on OpenSSL 1.1 as well. It were to double check, whether there is yet some other place in the core to support the newer OpenSSL version. Probably ext/ftp depends on it directly, others like ext/pgsql seem only to depend on what the corresponding lib provides. Also not sure about Windows side. Probably it can be ignored there, as our default build and all the dependencies are based on OpenSSL 1.0.2 in all the stable branches. It'd only be useful if someone really rebuilds all the stuff depending on 1.1, which is possible but unlikely. Since last week OpenSSL 1.1 dependent deps was placed for AppVeyor and snaps, so it'll be only supported officially starting with PHP 7.2 on Widows side.

I'll have time to test by the end of the week. Feedback from yet anyone interested would be great. @remicollet, poke.

Thanks.

weltling · 2017-03-29T18:04:13Z

@bukka i came earlier to it, so could already test the main part. The current test with OpenSSL 1.0.1 shows no regressions. Yet to test the dependent exts - ftp and possibly some other.

With 1.1 i've got these failing

OpenSSL private key functions [ext/openssl/tests/001.phpt]
openssl_error_string() tests [ext/openssl/tests/openssl_error_string_basic.phpt]
void openssl_free_key ( resource $key_identifier ); [ext/openssl/tests/openssl_free_key.phpt]
sni_server [ext/openssl/tests/sni_server.phpt]

We've already discussed the SNI one on ML, probably. The others might be worth investigating anyway, as 7.1 supports this OpenSSL version as well. Whereby the most important thing is, that there are no issues with OpenSSL 1.0. As for me, the status is good so far. I'll come back later after i made some further tests.

Thanks.

This reverts commit 015a80e.

bukka · 2017-04-02T17:57:11Z

@weltling I have just done further testing with all supported versions and fixed all issues (was failing only with 0.9.8). The tests that you mention are exactly the ones that I plan to look at after this is merged. It will be empty merge for 7.1 so don't want to do it in here. This is just to make it the same as it is in 7.1. I think that it's good to go. Are you ok if I merge it tomorrow or do you plan to do more testing?

weltling · 2017-04-02T23:07:13Z

@bukka i couldn't come to additional tests after my last message, unfortunately. Hope i can do more the coming week. It should be ok to merge into dev next week, yes, please squash before merging. And still it'd be great if @remicollet could check, too.

Thanks.

remicollet · 2017-04-04T05:56:25Z

ext/openssl/openssl.c

+	if (!cipher_ctx) {
+		php_error_docref(NULL, E_WARNING, "Failed to create cipher context");
+		RETURN_FALSE;
+	}


memleak on outbuf ?
Shouldn't this test move above, before allocation ?

Yep, outbuf leaked, easy to fix before merge.

Should be fixed ;)

remicollet · 2017-04-04T06:00:52Z

Test build (7.0.18RC1 + this patch)

Fedora 25 (OpenSSL 1.0.2k) = > Test suite passes.

Fedora 25 (Openssl 1.1.0e) => 4 failed test

OpenSSL private key functions [ext/openssl/tests/001.phpt]
openssl_error_string() tests [ext/openssl/tests/openssl_error_string_basic.phpt]
void openssl_free_key ( resource $key_identifier ); [ext/openssl/tests/openssl_free_key.phpt]
sni_server [ext/openssl/tests/sni_server.phpt]

I think 3 are fixed in 7.1, so only need some cherry-pick, sni_server.phpt also fails in 7.1

weltling · 2017-04-04T10:40:01Z

So looks like it's good to go :) Thanks for the check, @remicollet.

bukka · 2017-04-04T20:14:22Z

I just double checked and I see I missed the private key related tests in your info. The reason why I missed them is that I have got a bit different setting for OpenSSL. Basically I have a custom build without using system openssl.cnf and it seems that those tests are dependent on it which is not right of course. For example if I use this one:

https://github.com/php/php-src/blob/3c750d91ee9eeba00563a7a2d94a7470406b1b7e/ext/openssl/tests/openssl.cnf

I don't see the fails that you both do for 001 and openssl_free_key so I think that the tests are broken actually and they need a look. I think it would be much better to use the same cnf for all tests (e.g. using OPENSSL_CONF env).

@remicollet I'm actually interested about your openssl.cnf that you used as it could be a bug in this patch if it's fine for 7.1 and failing for 7.0?

remicollet · 2017-04-05T05:03:20Z

Here is the Fedora 26 default openssl.cnf file

#
# OpenSSL example configuration file.
# This is mostly being used for generation of certificate requests.
#

# This definition stops the following lines choking if HOME isn't
# defined.
HOME			= .
RANDFILE		= $ENV::HOME/.rnd

# Extra OBJECT IDENTIFIER info:
#oid_file		= $ENV::HOME/.oid
oid_section		= new_oids

# To use this configuration file with the "-extfile" option of the
# "openssl x509" utility, name here the section containing the
# X.509v3 extensions to use:
# extensions		= 
# (Alternatively, use a configuration file that has only
# X.509v3 extensions in its main [= default] section.)

[ new_oids ]

# We can add new OIDs in here for use by 'ca', 'req' and 'ts'.
# Add a simple OID like this:
# testoid1=1.2.3.4
# Or use config file substitution like this:
# testoid2=${testoid1}.5.6

# Policies used by the TSA examples.
tsa_policy1 = 1.2.3.4.1
tsa_policy2 = 1.2.3.4.5.6
tsa_policy3 = 1.2.3.4.5.7

####################################################################
[ ca ]
default_ca	= CA_default		# The default ca section

####################################################################
[ CA_default ]

dir		= /etc/pki/CA		# Where everything is kept
certs		= $dir/certs		# Where the issued certs are kept
crl_dir		= $dir/crl		# Where the issued crl are kept
database	= $dir/index.txt	# database index file.
#unique_subject	= no			# Set to 'no' to allow creation of
					# several certs with same subject.
new_certs_dir	= $dir/newcerts		# default place for new certs.

certificate	= $dir/cacert.pem 	# The CA certificate
serial		= $dir/serial 		# The current serial number
crlnumber	= $dir/crlnumber	# the current crl number
					# must be commented out to leave a V1 CRL
crl		= $dir/crl.pem 		# The current CRL
private_key	= $dir/private/cakey.pem# The private key
RANDFILE	= $dir/private/.rand	# private random number file

x509_extensions	= usr_cert		# The extensions to add to the cert

# Comment out the following two lines for the "traditional"
# (and highly broken) format.
name_opt 	= ca_default		# Subject Name options
cert_opt 	= ca_default		# Certificate field options

# Extension copying option: use with caution.
# copy_extensions = copy

# Extensions to add to a CRL. Note: Netscape communicator chokes on V2 CRLs
# so this is commented out by default to leave a V1 CRL.
# crlnumber must also be commented out to leave a V1 CRL.
# crl_extensions	= crl_ext

default_days	= 365			# how long to certify for
default_crl_days= 30			# how long before next CRL
default_md	= sha256		# use SHA-256 by default
preserve	= no			# keep passed DN ordering

# A few difference way of specifying how similar the request should look
# For type CA, the listed attributes must be the same, and the optional
# and supplied fields are just that :-)
policy		= policy_match

# For the CA policy
[ policy_match ]
countryName		= match
stateOrProvinceName	= match
organizationName	= match
organizationalUnitName	= optional
commonName		= supplied
emailAddress		= optional

# For the 'anything' policy
# At this point in time, you must list all acceptable 'object'
# types.
[ policy_anything ]
countryName		= optional
stateOrProvinceName	= optional
localityName		= optional
organizationName	= optional
organizationalUnitName	= optional
commonName		= supplied
emailAddress		= optional

####################################################################
[ req ]
default_bits		= 2048
default_md		= sha256
default_keyfile 	= privkey.pem
distinguished_name	= req_distinguished_name
attributes		= req_attributes
x509_extensions	= v3_ca	# The extensions to add to the self signed cert

# Passwords for private keys if not present they will be prompted for
# input_password = secret
# output_password = secret

# This sets a mask for permitted string types. There are several options. 
# default: PrintableString, T61String, BMPString.
# pkix	 : PrintableString, BMPString (PKIX recommendation before 2004)
# utf8only: only UTF8Strings (PKIX recommendation after 2004).
# nombstr : PrintableString, T61String (no BMPStrings or UTF8Strings).
# MASK:XXXX a literal mask value.
# WARNING: ancient versions of Netscape crash on BMPStrings or UTF8Strings.
string_mask = utf8only

# req_extensions = v3_req # The extensions to add to a certificate request

[ req_distinguished_name ]
countryName			= Country Name (2 letter code)
countryName_default		= XX
countryName_min			= 2
countryName_max			= 2

stateOrProvinceName		= State or Province Name (full name)
#stateOrProvinceName_default	= Default Province

localityName			= Locality Name (eg, city)
localityName_default		= Default City

0.organizationName		= Organization Name (eg, company)
0.organizationName_default	= Default Company Ltd

# we can do this but it is not needed normally :-)
#1.organizationName		= Second Organization Name (eg, company)
#1.organizationName_default	= World Wide Web Pty Ltd

organizationalUnitName		= Organizational Unit Name (eg, section)
#organizationalUnitName_default	=

commonName			= Common Name (eg, your name or your server\'s hostname)
commonName_max			= 64

emailAddress			= Email Address
emailAddress_max		= 64

# SET-ex3			= SET extension number 3

[ req_attributes ]
challengePassword		= A challenge password
challengePassword_min		= 4
challengePassword_max		= 20

unstructuredName		= An optional company name

[ usr_cert ]

# These extensions are added when 'ca' signs a request.

# This goes against PKIX guidelines but some CAs do it and some software
# requires this to avoid interpreting an end user certificate as a CA.

basicConstraints=CA:FALSE

# Here are some examples of the usage of nsCertType. If it is omitted
# the certificate can be used for anything *except* object signing.

# This is OK for an SSL server.
# nsCertType			= server

# For an object signing certificate this would be used.
# nsCertType = objsign

# For normal client use this is typical
# nsCertType = client, email

# and for everything including object signing:
# nsCertType = client, email, objsign

# This is typical in keyUsage for a client certificate.
# keyUsage = nonRepudiation, digitalSignature, keyEncipherment

# This will be displayed in Netscape's comment listbox.
nsComment			= "OpenSSL Generated Certificate"

# PKIX recommendations harmless if included in all certificates.
subjectKeyIdentifier=hash
authorityKeyIdentifier=keyid,issuer

# This stuff is for subjectAltName and issuerAltname.
# Import the email address.
# subjectAltName=email:copy
# An alternative to produce certificates that aren't
# deprecated according to PKIX.
# subjectAltName=email:move

# Copy subject details
# issuerAltName=issuer:copy

#nsCaRevocationUrl		= http://www.domain.dom/ca-crl.pem
#nsBaseUrl
#nsRevocationUrl
#nsRenewalUrl
#nsCaPolicyUrl
#nsSslServerName

# This is required for TSA certificates.
# extendedKeyUsage = critical,timeStamping

[ v3_req ]

# Extensions to add to a certificate request

basicConstraints = CA:FALSE
keyUsage = nonRepudiation, digitalSignature, keyEncipherment

[ v3_ca ]


# Extensions for a typical CA


# PKIX recommendation.

subjectKeyIdentifier=hash

authorityKeyIdentifier=keyid:always,issuer

basicConstraints = critical,CA:true

# Key usage: this is typical for a CA certificate. However since it will
# prevent it being used as an test self-signed certificate it is best
# left out by default.
# keyUsage = cRLSign, keyCertSign

# Some might want this also
# nsCertType = sslCA, emailCA

# Include email address in subject alt name: another PKIX recommendation
# subjectAltName=email:copy
# Copy issuer details
# issuerAltName=issuer:copy

# DER hex encoding of an extension: beware experts only!
# obj=DER:02:03
# Where 'obj' is a standard or added object
# You can even override a supported extension:
# basicConstraints= critical, DER:30:03:01:01:FF

[ crl_ext ]

# CRL extensions.
# Only issuerAltName and authorityKeyIdentifier make any sense in a CRL.

# issuerAltName=issuer:copy
authorityKeyIdentifier=keyid:always

[ proxy_cert_ext ]
# These extensions should be added when creating a proxy certificate

# This goes against PKIX guidelines but some CAs do it and some software
# requires this to avoid interpreting an end user certificate as a CA.

basicConstraints=CA:FALSE

# Here are some examples of the usage of nsCertType. If it is omitted
# the certificate can be used for anything *except* object signing.

# This is OK for an SSL server.
# nsCertType			= server

# For an object signing certificate this would be used.
# nsCertType = objsign

# For normal client use this is typical
# nsCertType = client, email

# and for everything including object signing:
# nsCertType = client, email, objsign

# This is typical in keyUsage for a client certificate.
# keyUsage = nonRepudiation, digitalSignature, keyEncipherment

# This will be displayed in Netscape's comment listbox.
nsComment			= "OpenSSL Generated Certificate"

# PKIX recommendations harmless if included in all certificates.
subjectKeyIdentifier=hash
authorityKeyIdentifier=keyid,issuer

# This stuff is for subjectAltName and issuerAltname.
# Import the email address.
# subjectAltName=email:copy
# An alternative to produce certificates that aren't
# deprecated according to PKIX.
# subjectAltName=email:move

# Copy subject details
# issuerAltName=issuer:copy

#nsCaRevocationUrl		= http://www.domain.dom/ca-crl.pem
#nsBaseUrl
#nsRevocationUrl
#nsRenewalUrl
#nsCaPolicyUrl
#nsSslServerName

# This really needs to be in place for it to be a proxy certificate.
proxyCertInfo=critical,language:id-ppl-anyLanguage,pathlen:3,policy:foo

####################################################################
[ tsa ]

default_tsa = tsa_config1	# the default TSA section

[ tsa_config1 ]

# These are used by the TSA reply generation only.
dir		= ./demoCA		# TSA root directory
serial		= $dir/tsaserial	# The current serial number (mandatory)
crypto_device	= builtin		# OpenSSL engine to use for signing
signer_cert	= $dir/tsacert.pem 	# The TSA signing certificate
					# (optional)
certs		= $dir/cacert.pem	# Certificate chain to include in reply
					# (optional)
signer_key	= $dir/private/tsakey.pem # The TSA private key (optional)
signer_digest  = sha256			# Signing digest to use. (Optional)
default_policy	= tsa_policy1		# Policy if request did not specify it
					# (optional)
other_policies	= tsa_policy2, tsa_policy3	# acceptable policies (optional)
digests     = sha1, sha256, sha384, sha512  # Acceptable message digests (mandatory)
accuracy	= secs:1, millisecs:500, microsecs:100	# (optional)
clock_precision_digits  = 0	# number of digits after dot. (optional)
ordering		= yes	# Is ordering defined for timestamps?
				# (optional, default: no)
tsa_name		= yes	# Must the TSA name be included in the reply?
				# (optional, default: no)
ess_cert_id_chain	= no	# Must the ESS cert id chain be included?
				# (optional, default: no)

remicollet · 2017-04-05T05:06:05Z

I think you need to cherry-pick, from 7.1

It makes the test work with OpenSSL 1.1

…faults

bukka · 2017-04-05T19:48:12Z

@remicollet Thanks. I cherry picked all suggested commits so I think it's good to go if you or @weltling don't see any other issues?

weltling · 2017-04-05T21:46:40Z

@bukka no issues from my side. There'll be yet a couple of weeks and the next RC for the close checks.

Thanks.

remicollet · 2017-04-06T07:22:05Z

Ok for me (perhaps should squash all not cherry-picked commit to have a clean history).

weltling · 2017-04-06T09:50:39Z

Yeah, as a single commit, please :)

Thanks.

bukka · 2017-04-10T16:35:57Z

This was merged in cdc3325

krakjoe added the Feature label Mar 27, 2017

weltling reviewed Mar 27, 2017

View reviewed changes

nikic changed the title ~~Openssl 1.1.0 support for 7 0~~ Openssl 1.1.0 support for 7.0 Mar 27, 2017

bukka added 7 commits April 2, 2017 17:57

Add OpenSSL 1.1 compat mainly for PKEY stuff

f4aa819

Add OpenSSL compat for other openssl.c bits

54e0ede

Revert "report about OpenSSL 1.1 not supported during configure"

91a7666

This reverts commit 015a80e.

Port the rest of openssl ext to OpenSSL 1.1.0

fbb5c99

Port phar to OpenSSL 1.1

fea8cdf

Add few more test and code fixes for OpenSSL 1.1

ca03d65

Fix compilation with 0.9.8

da42399

bukka force-pushed the openssl_1_1_for_7_0 branch from 7ddf3b1 to da42399 Compare April 2, 2017 16:57

Fix tests for 0.9.8

d13375c

remicollet reviewed Apr 4, 2017

View reviewed changes

Fix possibly memlean in openssl_encrypt

6d62f0f

bukka and others added 3 commits April 5, 2017 20:44

Remove expected openssl errors that are not needed anymore

9e353c9

Do not expect error 0407006A in openssl_private_(en|de)crypt

170c87f

It makes the test work with OpenSSL 1.1

rely on provided openssl.cnf to avoid possible failure with system de…

9f681d5

…faults

bukka closed this Apr 10, 2017

Openssl 1.1.0 support for 7.0 #2438

Openssl 1.1.0 support for 7.0 #2438

Uh oh!

Conversation

bukka commented Mar 26, 2017

Uh oh!

bukka commented Mar 26, 2017

Uh oh!

weltling Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

bukka Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

weltling Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

weltling commented Mar 27, 2017

Uh oh!

weltling commented Mar 29, 2017

Uh oh!

bukka commented Apr 2, 2017

Uh oh!

weltling commented Apr 2, 2017

Uh oh!

remicollet Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

weltling Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

bukka Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

remicollet commented Apr 4, 2017

Uh oh!

weltling commented Apr 4, 2017

Uh oh!

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

Uh oh!

Uh oh!

remicollet commented Apr 5, 2017

Uh oh!

remicollet commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bukka commented Apr 5, 2017

Uh oh!

weltling commented Apr 5, 2017

Uh oh!

remicollet commented Apr 6, 2017

Uh oh!

weltling commented Apr 6, 2017

Uh oh!

bukka commented Apr 10, 2017

Uh oh!

Uh oh!

bukka commented Apr 4, 2017 •

edited

Loading

remicollet commented Apr 5, 2017 •

edited

Loading