Don't implement RsaMethodSignRaw on Windows.

Don't implement RsaMethodSignRaw on Windows.

As of https://boringssl-review.googlesource.com/#/c/2420/, BoringSSL
now calls the sign hook when signing a pre-TLS-1.2 handshake hash.
There's no need for hooking it at the sign_raw layer on Windows.

BUG=none

Committed: https://crrev.com/1d0461f9542801b0cf0cba6fb63740cb2f71fcac
Cr-Commit-Position: refs/heads/master@{#306696}

[davidben, 2014-12-02 23:04:51]

davidben@chromium.org changed reviewers:
+ rsleevi@chromium.org

[davidben, 2014-12-02 23:04:51]

This to resolve the comment you had on the original version. I split it out
because it works fine without it and this isn't a re-land of a prior CL.

[Ryan Sleevi, 2014-12-03 14:10:39]

From looking at https://boringssl-review.googlesource.com/#/c/2420/ , I didn't
see where you ensured there wouldn't be a DigestInfo encoding.

In particular,  you didn't seem to update the EVP_PKEY_sign bits re: DigestInfo
encoding (c.f.
https://boringssl-review.googlesource.com/#/c/2420/3/ssl/s3_both.c line 526 of
the original)

https://codereview.chromium.org/762203003/diff/1/net/ssl/openssl_platform_key...
File net/ssl/openssl_platform_key_win.cc (right):

https://codereview.chromium.org/762203003/diff/1/net/ssl/openssl_platform_key...
net/ssl/openssl_platform_key_win.cc:234: in_len, out, RSA_size(rsa),
&signature_len, BCRYPT_PAD_PKCS1);
Is this entirely safe? Does RSA_size handle any space needed for leading zeros
when encoding signatures? Just wanting to confirm that there's not a need to
allocate RSA_size(...)+1 for any BN encoding issues.

[davidben, 2014-12-03 16:01:38]

So, NID_md5_sha1 actually already took care of it.
https://boringssl.googlesource.com/boringssl/+/master/crypto/rsa/rsa.c#363

Turns out I broke this in https://boringssl-review.googlesource.com/#/c/1633/
and https://boringssl-review.googlesource.com/#/c/1122/ when I made them go
through EVP in all codepaths rather than call RSA_sign and RSA_verify manually.
Giving EVP a NULL signature MD does give us the right signature, but then it
goes straight to RSA_sign_raw rather than call RSA_sign. All EVP needs to plumb
it right is for there to be an EVP_MD with type NID_md5_sha1. See:

https://boringssl.googlesource.com/boringssl/+/master/crypto/evp/p_rsa.c#169

https://codereview.chromium.org/762203003/diff/1/net/ssl/openssl_platform_key...
File net/ssl/openssl_platform_key_win.cc (right):

https://codereview.chromium.org/762203003/diff/1/net/ssl/openssl_platform_key...
net/ssl/openssl_platform_key_win.cc:234: in_len, out, RSA_size(rsa),
&signature_len, BCRYPT_PAD_PKCS1);
On 2014/12/03 14:10:39, Ryan Sleevi wrote:
> Is this entirely safe? Does RSA_size handle any space needed for leading zeros
> when encoding signatures? Just wanting to confirm that there's not a need to
> allocate RSA_size(...)+1 for any BN encoding issues.

RSA_size is the number of bytes needed to represent the modulus, so that
includes leading zeros and all. Changing RSA_METHOD to still pass in a max_out
parameter to the size hook might not be a bad plan so we don't have to have the
bounds implicit...

The reason sign doesn't have a max_out while sign_raw does is because sign_raw
mirrors RSA_sign_raw which is new API and has a max_out parameter. sigh mirrors
RSA_sign which is existing API and doesn't. The caller is responsible for
ensuring there are at least RSA_size(rsa) bytes of space, so max_out is
implicitly always RSA_size(rsa).

https://boringssl.googlesource.com/boringssl/+/master/include/openssl/rsa.h#180

[Ryan Sleevi, 2014-12-03 16:13:52]

Thanks for explaining. LGTM.

[davidben, 2014-12-03 21:32:50]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-12-03 21:34:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/762203003/1

[commit-bot: I haz the power, 2014-12-03 22:44:32]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-12-03 22:45:21]

Patchset 1 (id:??) landed as
https://crrev.com/1d0461f9542801b0cf0cba6fb63740cb2f71fcac
Cr-Commit-Position: refs/heads/master@{#306696}