1341290 - Upgrade OpenSSL for use with WebPush.

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, defect)

Comment 1

[James Hugman [:jhugman] [@jhugman]]

Using ecec, which depends on OpenSSL version 1.1.0+.

Comment 2

[James Hugman [:jhugman] [@jhugman]]

Attachment: Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/2457

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8840867 [details] [review]
Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/2457

I think Steph's review for this is sufficient.

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I added review notes mostly about checking return values of OpenSSL functions.


One thing that I am a bit worried about is the fact that we don't seem to initialize the OpenSSL library. The OpenSSL wiki says:

"If you fail to initialize the library, then you will experience unexplained errors like SSL_CTX_new returning NULL, error messages like SSL_CTX_new:library has no ciphers and alert handshake failure with no shared ciphers."

and suggests:

"If you are using OpenSSL 1.0.2 or below, then you would use SSL_library_init. If you are using OpenSSL 1.1.0 or above, then you would use OPENSSL_init_ssl. "

See https://wiki.openssl.org/index.php/Library_Initialization

We only use the libcrypto part of OpenSSL, so I am not sure if this is problematic or not. It is unclear if we need to do anything at all.

Comment 5

[James Hugman [:jhugman] [@jhugman]]

Pre-upgrade, we were initializing OPENSSL_init_ssl only as part of sqlcipher initialization. This remains the same following the upgade.

However, as per https://wiki.openssl.org/index.php/Library_Initialization#libcrypto_Initialization 

> libcrypto should be initialized with calls to OpenSSL_add_all_algorithms and ERR_load_crypto_strings.
We should probably be doing this, independently of sqlcipher initialization.

Comment 6

[Stephan Leroux [:sleroux]]

Comment on attachment 8840867 [details] [review]
Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/2457

Looked over the PR and looks OK by me but :st3fan is the expert here so deferring to him :)

Comment 7

[James Hugman [:jhugman] [@jhugman]]

Ready for r? from :st3fan.

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8840867 [details] [review]
Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/2457

LGTM If the unit tests are all green, land it on master please.

Comment 9

[James Hugman [:jhugman] [@jhugman]]

Merged.