Use C source for openssl RC4 calculation instead of ASM

Use C source for openssl RC4 calculation instead of ASM

Due to a problem with rc4-x86_64.S,
We use the C rc4 source instead of the ASM source.
This hurts performance, but it's not a problem because
no production code uses openssl on x86-64.

BUG=240674
R=agl@chromium.org, digit@chromium.org, rsleevi@chromium.org, wtc@chromium.org
TBR=darin

Committed: 229995

[digit1, 2013-10-10 07:55:03]

https://codereview.chromium.org/26651005/diff/3001/openssl.gyp
File openssl.gyp (right):

https://codereview.chromium.org/26651005/diff/3001/openssl.gyp#newcode72
openssl.gyp:72: 'sources+': [ 'openssl/crypto/rc4/rc4-enc.S' ],
This doesn't build, I think you meant 'rc4-enc.c' here, not 'rc4-enc.S' :-)

[digit1, 2013-10-10 07:56:02]

https://codereview.chromium.org/26651005/diff/3001/openssl.gyp
File openssl.gyp (right):

https://codereview.chromium.org/26651005/diff/3001/openssl.gyp#newcode72
openssl.gyp:72: 'sources+': [ 'openssl/crypto/rc4/rc4-enc.S' ],
Actually, it should be "rc4_enc.c"

[digit1, 2013-10-10 08:02:51]

https://codereview.chromium.org/26651005/diff/3001/openssl.gyp
File openssl.gyp (right):

https://codereview.chromium.org/26651005/diff/3001/openssl.gyp#newcode72
openssl.gyp:72: 'sources+': [ 'openssl/crypto/rc4/rc4-enc.S' ],
While we're at it, Intel is contributing many patches to port Android to x86_64
at the moment, can I ask you to put this change in a 'conditions' block like 'os
!= "android"' to ensure the assembly version would still be used here?

(I'm assuming that, given that the toolchain will be different, Goma may not
have issues compiling this later).

[yhirano, 2013-10-10 11:49:39]

https://codereview.chromium.org/26651005/diff/3001/openssl.gyp
File openssl.gyp (right):

https://codereview.chromium.org/26651005/diff/3001/openssl.gyp#newcode72
openssl.gyp:72: 'sources+': [ 'openssl/crypto/rc4/rc4-enc.S' ],
On 2013/10/10 08:02:52, digit1 wrote:
> While we're at it, Intel is contributing many patches to port Android to
x86_64
> at the moment, can I ask you to put this change in a 'conditions' block like
'os
> != "android"' to ensure the assembly version would still be used here?
> 
> (I'm assuming that, given that the toolchain will be different, Goma may not
> have issues compiling this later).

Thanks, done.

[digit1, 2013-10-10 12:27:37]

https://codereview.chromium.org/26651005/diff/13001/openssl.gyp
File openssl.gyp (right):

https://codereview.chromium.org/26651005/diff/13001/openssl.gyp#newcode68
openssl.gyp:68: 'conditions': [['OS != "android"', {
nit: Ah, can I ask you to avoid the double brackets here, and use identation
instead? It's easier to follow / avoids ambiguities (your change is technically
correct, but it looked like a bug to me on first look :-))

https://codereview.chromium.org/26651005/diff/13001/openssl.gyp#newcode76
openssl.gyp:76: ['include', 'openssl/crypto/rc4/rc4_skey\\.c' ],
Just for clarification, why this change exactly? I could rebuild without this
source file.

[yhirano, 2013-10-11 01:42:25]

https://codereview.chromium.org/26651005/diff/13001/openssl.gyp
File openssl.gyp (right):

https://codereview.chromium.org/26651005/diff/13001/openssl.gyp#newcode68
openssl.gyp:68: 'conditions': [['OS != "android"', {
On 2013/10/10 12:27:38, digit1 wrote:
> nit: Ah, can I ask you to avoid the double brackets here, and use identation
> instead? It's easier to follow / avoids ambiguities (your change is
technically
> correct, but it looked like a bug to me on first look :-))

Done.

https://codereview.chromium.org/26651005/diff/13001/openssl.gyp#newcode76
openssl.gyp:76: ['include', 'openssl/crypto/rc4/rc4_skey\\.c' ],
On 2013/10/10 12:27:38, digit1 wrote:
> Just for clarification, why this change exactly? I could rebuild without this
> source file.
I found that rc4-x86_64.S defined private_RC4_set_key and RC4_options as well as
RC4.
So I added rc4_skey.c which defined them.

[digit1, 2013-10-15 19:16:39]

Thanks, sorry for the delay, lgtm.

I also added agl and wtc as reviewers, please wait for their input.

[agl, 2013-10-15 19:19:02]

Why the OS != "android"? Is it because Android doesn't build with Goma? Might it
in the future?

Could you link to a bug or description of what the problem is with that .S file
and goma?

[Ryan Sleevi, 2013-10-15 19:30:07]

lgtm

[wtc, 2013-10-15 22:39:14]

Patch set 5 LGTM.

https://chromiumcodereview.appspot.com/26651005/diff/18001/openssl.gyp
File openssl.gyp (right):

https://chromiumcodereview.appspot.com/26651005/diff/18001/openssl.gyp#newcode67
openssl.gyp:67: 'sources!': [ '<@(openssl_x86_64_source_excludes)' ],

Should we change the definitions of openssl_x86_64_sources and
openssl_x86_64_source_excludes instead?

It will require changing openssl/openssl.config and re-running
import_from_android.sh, but it seems like a better way to do this. This assumes
the 'OS != "android"' condition is not necessary.

This may be nontrivial to figure out because we may need to change
openssl/openssl.config with a Chromium-specific patch file. I remember I had a
hard time figuring out how to do this, even though digit documented the
procedure in README.chromium clearly.

[yhirano, 2013-10-16 05:58:39]

On 2013/10/15 19:19:02, agl wrote:
> Why the OS != "android"? Is it because Android doesn't build with Goma? Might
it
> in the future?
>
https://codereview.chromium.org/26651005/diff/3001/openssl.gyp#newcode72
It is a suggestion by David.
I think android on x86_64 has the same problem but nobody builds chrome with
such configuration.

> 
> Could you link to a bug or description of what the problem is with that .S
file
> and goma?
I pasted the link and the contents to http://crbug.com/240674 .

[Yoshisato Yanagisawa, 2013-10-16 06:45:19]

Sorry for jumping in,
I am a goma developer, and maintaining goma client SSL code.

Sorry for making Yutaka misunderstood, this is not a goma bug.  I believe this
is a chromium OpenSSL bug.  In other words, Yutaka would see the same issue even
if he builds code without goma.

Goma is using OpenSSL in
https://chromium.googlesource.com/chromium/deps/openssl.git.  I saw SEGV in RC4
code when I updated the commit goma client is depending on to the commit
39898464beeee474c1134...  From that time, I have disabled assembly version RC4. 
At that time, I have remained the explanation why I have disabled the use of
RC4:
# According to valgrind, invalid memory reads/writes are recognized in
# private_RC4_set_key crypto/rc4/asm/rc4-x86_64.S.
# I saw 4 invalid writes and 2 invalid reads, and all shows error in the same
# function.  I picked up one back trace of errors:
# at private_RC4_set_key
#   (third_party/openssl/openssl/crypto/rc4/asm/rc4-x86_64.S:542)
# by rc4_init_key (third_party/openssl/openssl/crypto/evp/e_rc4.c:126)
# by EVP_CipherInit_ex (third_party/openssl/openssl/crypto/evp/evp_enc.c:253)
# by tls1_change_cipher_state (third_party/openssl/openssl/ssl/t1_enc.c:552)
# by ssl3_connect (third_party/openssl/openssl/ssl/s3_clnt.c:507)
# by SSL_connect (third_party/openssl/openssl/ssl/ssl_lib.c:952)
# by devtools_goma::OpenSSLEngine::Connect() (client/openssl_engine.cc:635)
# by devtools_goma::OpenSSLEngine::SetDataFromTransport(StringPiece const&)
#   (client/openssl_engine.cc:584)
# by devtools_goma::TLSDescriptor::TransportLayerReadable()
#   (client/tls_descriptor.cc:170)
# ...snip...
#
# The commit log for rc4-x86_64.S is:
# https://android-review.googlesource.com/#/c/47913/
#
http://git.chromium.org/gitweb/?p=chromium/deps/openssl.git;a=commit;h=e4dea7...

FYI, after I knew the RC4 vulnerability (http://www.isg.rhul.ac.uk/tls/), goma
client stopped using RC4.

[Yoshisato Yanagisawa, 2013-10-16 06:51:16]

I also suggest to change the description.

Now:
> Due to a goma problem with compiling rc4-x86_64.S, 

I suggest:
> Due to a problem with rc4-x86_64.S,

[yhirano, 2013-10-16 06:53:22]

Thank you very much, Yanagisawa-san, and I'm sorry for confusing reviewers.
I fixed the CL and the description.

[agl, 2013-10-16 13:32:58]

The valgrind trace suggests to me that we might be building the files with the
wrong flags: some code might be assuming byte-per-entry and other code
word-per-entry. (OpenSSL supports the RC4 state as both a uint8[256] and
uint32[256].)

But disabling is fine.

LGTM.

[yhirano, 2013-10-17 05:31:42]

https://chromiumcodereview.appspot.com/26651005/diff/18001/openssl.gyp
File openssl.gyp (right):

https://chromiumcodereview.appspot.com/26651005/diff/18001/openssl.gyp#newcode67
openssl.gyp:67: 'sources!': [ '<@(openssl_x86_64_source_excludes)' ],
On 2013/10/15 22:39:15, wtc wrote:
> 
> Should we change the definitions of openssl_x86_64_sources and
> openssl_x86_64_source_excludes instead?
> 
> It will require changing openssl/openssl.config and re-running
> import_from_android.sh, but it seems like a better way to do this. This
assumes
> the 'OS != "android"' condition is not necessary.
> 
> This may be nontrivial to figure out because we may need to change
> openssl/openssl.config with a Chromium-specific patch file. I remember I had a
> hard time figuring out how to do this, even though digit documented the
> procedure in README.chromium clearly.
I filed a bug https://code.google.com/p/chromium/issues/detail?id=308375

[yhirano, 2013-10-17 11:04:35]

Use C source for openssl RC4 calculation instead of ASM 

Due to a problem with rc4-x86_64.S, 
We use the C rc4 source instead of the ASM source. 
This hurts performance, but it's not a problem because 
no production code uses openssl on x86-64. 

BUG=240674

[yhirano, 2013-10-17 11:12:04]

David, can you tell me how to commit this CL?
Isn't just checking the box enough?

[digit1, 2013-10-17 12:06:55]

Hi yhirona, this needs to be accepted by one of the third_party/owners, I've
added two of them here.

darin@chromium.org: Please review changes in 
ben@chromium.org: Please review changes in

[yhirano, 2013-10-17 12:18:25]

Thank you very much!

[yhirano, 2013-10-18 06:00:23]

I found a comment in third_party/OWNERS,

# Owner approval for 3rd party is only required for
# adding new libraries. For changes to existing code
# simply TBR= one of people below.

Then, just adding TBR is suffice?

[Ryan Sleevi, 2013-10-21 18:06:40]

On 2013/10/18 06:00:23, yhirano wrote:
> I found a comment in third_party/OWNERS,
> 
> # Owner approval for 3rd party is only required for
> # adding new libraries. For changes to existing code
> # simply TBR= one of people below.
> 
> Then, just adding TBR is suffice?

Yes

[yhirano, 2013-10-22 01:10:40]

Committed patchset #8 manually as r229995 (presubmit successful).