Throw a DataError rather than OperationError in some cases when importing invalid EC key.

Throw a DataError rather than OperationError in some cases when importing invalid EC key.

Committed: https://crrev.com/09b74b34183c7fbe5de5f5ec89bfb60482b25074
Cr-Commit-Position: refs/heads/master@{#313417}

[eroman, 2015-01-06 23:41:30]

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

[davidben, 2015-01-09 20:01:32]

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

[davidben, 2015-01-09 20:01:33]

Sorry, I somehow missed this.

+agl: Do we want to instead tweak how the various checks happen? It seems
awkward to expect code to know which functions do and don't implicitly
EC_KEY_check_key and which checks are and aren't skipped given how far along the
EC_KEY is in initialization.

[agl, 2015-01-09 20:06:05]

If BoringSSL is being a pain and we're working around it in Chromium code, then
we should change BoringSSL where possible.

[davidben, 2015-01-12 21:51:05]

On 2015/01/09 20:06:05, agl wrote:
> If BoringSSL is being a pain and we're working around it in Chromium code,
then
> we should change BoringSSL where possible.

So I guess, with the first half of the CL, d2i_ECPrivateKey should arrange for
any checks to happen as part of the import. Then we can get rid of the
EC_KEY_check_key call in VerifyEcKeyAfterSpkiOrPkcs8Import. That seems
unambiguously better.


With the second half, I'm not sure what the tidiest API would be if you're
assembling the public and private halves one at a time. We could say that, if
you're doing that, it's your responsibility to EC_KEY_check_key. But that's a
little weird when you can avoid the check if you be sure to set the private key
first and public key second (as the second half of this CL does). I could
imagine saying that setting either half should perform what checks are possible
given the half-built state of the key, but I worry that would get little messy
with the number of cases. Though if we're sure we'll never grow beyond the two
halves (and not, say, through an independently set group into the mix), so not
excessively so? Then it would look something like:

set_private_key:
  - Perform private-key-only checks (less than order, etc.)
  - If public key not NULL, check they correspond.

set_public_key_affine_coordinates:
  - Perform public-key-only checks (on curve, etc.)
  - If private key not NULL, check they correspond.

check_key:
  - Do all the checks again for fun. Or maybe no-op? EC_KEY is opaque... though,
nothing stops you from calling set_group after everything else has been
initialized. That they API lets you do that is mildly disconcerting...

Note that this changes the semantics of set_private_key. Before, you could
set_group and set_private_key in any order. You could also set_private_key
garbage. set_private_key couldn't fail unless malloc did. Now it'll actually do
checks.

[eroman, 2015-01-27 00:45:30]

PTAL: I decided to just duplicate EC_KEY_check_key() in all cases.


I played around with this a bit and didn't find a solution on the BoringSSL side
that I liked.

I experimented making d2i_ECPrivateKey() consistently error if given an invalid
EC key (for all combinations of public/private key being set).

This led to some implementation ugliness, as I either relied on
EC_POINT_oct2point() indirectly calling EC_KEY_check_key() to avoid duplicating
it, or I call EC_KEY_check_key() in cases where it isn't needed.

Overall this pattern is not idomatic. RSA key import for instance expects
RSA_KEY_check() to be called, so expecting callers to know that this is not
needed for EC keys is also not ideal.

I can re-visit this if the minor performance difference is something we end up
caring about. But in the meantime I don't think the additional complexity would
be right tradeoff, so happy to stick with the status quo.

Cheers.

[davidben, 2015-01-27 23:20:26]

lgtm

(I guess I'll make a note in my spreadsheet to toy with the check-on-import
thing later then. I do think it makes sense for the import function within
BoringSSL to only return valid keys, though that RSA doesn't is interesting. I
can see how it'd be awkward with how set_private_key works though.)

[eroman, 2015-01-27 23:43:30]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2015-01-27 23:44:46]

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

[commit-bot: I haz the power, 2015-01-28 00:43:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-28 00:45:28]

Patchset 3 (id:??) landed as
https://crrev.com/09b74b34183c7fbe5de5f5ec89bfb60482b25074
Cr-Commit-Position: refs/heads/master@{#313417}