Switch //net to the new SPKI and PKCS#8 APIs.

net/base/keygen_handler_openssl.cc

Index: net/base/keygen_handler_openssl.cc
diff --git a/net/base/keygen_handler_openssl.cc b/net/base/keygen_handler_openssl.cc
index bc34012c1e8dd6ffe2e03f6d57278a130becbaf8..5e979e08375d7f47837cb86a3b590aa4465d5e50 100644
--- a/net/base/keygen_handler_openssl.cc
+++ b/net/base/keygen_handler_openssl.cc
@@ -4,11 +4,18 @@
 
 #include "net/base/keygen_handler.h"
 
+#include <openssl/bytestring.h>
+#include <openssl/digest.h>
+#include <openssl/evp.h>
 #include <openssl/mem.h>
-#include <openssl/ssl.h>
+#include <stdint.h>
 
+#include "base/base64.h"
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/strings/string_piece.h"
+#include "crypto/auto_cbb.h"
 #include "crypto/openssl_util.h"
 #include "crypto/rsa_private_key.h"
 #include "crypto/scoped_openssl_types.h"
@@ -24,21 +31,81 @@ std::string KeygenHandler::GenKeyAndSignChallenge() {
   if (stores_key_)
     OpenSSLPrivateKeyStore::StoreKeyPair(url_, pkey);
 
-  crypto::ScopedOpenSSL<NETSCAPE_SPKI, NETSCAPE_SPKI_free> spki(
-      NETSCAPE_SPKI_new());
-  ASN1_STRING_set(spki.get()->spkac->challenge,
-                  challenge_.data(), challenge_.size());
-  NETSCAPE_SPKI_set_pubkey(spki.get(), pkey);
-  // Using MD5 as this is what is required in HTML5, even though the SPKI
-  // structure does allow the use of a SHA-1 signature.
-  NETSCAPE_SPKI_sign(spki.get(), pkey, EVP_md5());
-  char* spkistr = NETSCAPE_SPKI_b64_encode(spki.get());
+  // Serialize the following structure, from
+  // https://developer.mozilla.org/en-US/docs/Web/HTML/Element/keygen.
+  //
+  //   PublicKeyAndChallenge ::= SEQUENCE {
+  //       spki SubjectPublicKeyInfo,
+  //       challenge IA5STRING
+  //   }
+  //
+  //   SignedPublicKeyAndChallenge ::= SEQUENCE {
+  //       publicKeyAndChallenge PublicKeyAndChallenge,
+  //       signatureAlgorithm AlgorithmIdentifier,
+  //       signature BIT STRING
+  //   }
+  //
+  // The signature is over the PublicKeyAndChallenge.
 
-  std::string result(spkistr);
-  OPENSSL_free(spkistr);
+  crypto::OpenSSLErrStackTracer tracer(FROM_HERE);
 
+  // Serialize up to the PublicKeyAndChallenge.
+  crypto::AutoCBB cbb;
+  CBB spkac, public_key_and_challenge, challenge;
+  if (!CBB_init(cbb.get(), 0) ||
+      !CBB_add_asn1(cbb.get(), &spkac, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1(&spkac, &public_key_and_challenge, CBS_ASN1_SEQUENCE) ||
+      !EVP_marshal_public_key(&public_key_and_challenge, pkey) ||
+      !CBB_add_asn1(&public_key_and_challenge, &challenge,
+                    CBS_ASN1_IA5STRING) ||
+      !CBB_add_bytes(&challenge,
+                     reinterpret_cast<const uint8_t*>(challenge_.data()),
+                     challenge_.size()) ||

[Ryan Sleevi, 2016/03/03 18:32:06]

Is this fine if challenge_ is empty? I suspect any issue is pre-existing, but
wasn't sure if the ASN1_STRING_set handled things differently than CBB_add_bytes

[davidben, 2016/03/03 19:58:15]

If challenge is empty, we'll emit an empty IA5STRING which I believe should be
correct. Empty strings are valid strings and the field isn't OPTIONAL or
anything. I have no clue what the old crazy code did. :-)

[Ryan Sleevi, 2016/03/03 20:27:41]

Specifically, by using .data() rather than .c_str(), we can't guarantee that if
.size() == 0 that *data() will return anything meaningful (that is, it's
undefined behaviour). I wanted to ensure that this function, if .size() == 0,
did not try to access .data()

[davidben, 2016/03/03 21:59:36]

If you pass CBB_add_bytes a length of zero, it's not going to do anything dumb
to it. If it does, that's a bug. We're not going to have ridiculous APIs like
that in BoringSSL, particularly not our own code. :-P

(But C++11 makes .c_str() and .data() identical, so this is moot.)

+      !CBB_flush(&spkac)) {
+    return std::string();
+  }
+
+  // Hash what's been written so far.
+  crypto::ScopedEVP_MD_CTX ctx(EVP_MD_CTX_create());
+  if (!EVP_DigestSignInit(ctx.get(), nullptr, EVP_md5(), nullptr, pkey) ||
+      !EVP_DigestSignUpdate(ctx.get(), CBB_data(&spkac), CBB_len(&spkac))) {
+    return std::string();
+  }
+
+  // The DER encoding of the AlgorithmIdentifier for MD5 with RSA.
+  static const uint8_t kMd5WithRsaEncryption[] = {
+      0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86,
+      0xf7, 0x0d, 0x01, 0x01, 0x04, 0x05, 0x00,
+  };

[Ryan Sleevi, 2016/03/03 18:32:06]

Surely there's a less-gross way than hardcoding - can't we rely on BoringSSL to
keep this AlgorithmIdentifier? Or is this part of what you're trying to excise?

[davidben, 2016/03/03 19:58:15]

There's OBJ_nid2cbb that would work. I didn't use it because I'm hoping to
excise that later too. OBJ_nid2cbb involves a HUGE table of every OID OpenSSL
knows about, which is ridiculous. That table ends up showing up in all of
Cronet's largest symbols. It's difficult to know what we can prune that table
because it's shared by the X.509 code and anything else that happens to ask
OpenSSL to parse or even pretty-print an OID.

I haven't decoupled BoringSSL core's internals from that table yet (have a
branch not yet reviewed), but I'm envisioning that, like the net/cert code, each
module which needs to know about particular OIDs would just embed them and
memcmp as appropriate. For the most part, OIDs of different "types" are
disjoint. No key type is also a hash type, no X.509 extension is also a
signature algorithm, etc.

The idea is that you'll only ship OID definitions you care about.

What do you think about instead only burning in the encoded form of the OID, but
keeping the rest structured? (So CBB calls for the NULLs, etc.) I did this just
because it's always constant and it seemed easy.

I'm also happy to write this assuming OBJ_nid2cbb is legal, since that hasn't
actually landed yet and maybe we'll, I dunno, have constants in BoringSSL for
the OID definitions or something. (Though, see note in comment below about who
should actually be handling signature algorithms.)

[Ryan Sleevi, 2016/03/03 20:27:41]

I do not think that would be a good state - it would duplicate that string
through multiple TUs. There's no guarantee that the linker will be able to
de-dup these strings.

That means everything dealing with MD5 will have to carry the full weight of MD5
with them everywhere.

I totally understand there are tradeoffs, but this feels like going too far in
the other way, in that it multiplies the size across TUs. Should we have a
common header with this value (extern'd)? If so, should that header live in
//net, //crypto, or BoringSSL?

That large table itself seems problematic because it prevents dead code
elimination, so I'm totally sensitive to that. Can we just have a common
constant accessible inter-TU?

[davidben, 2016/03/03 21:59:36]

The string wouldn't be duplicated. That's largely the point. My branch (not yet
uploaded) in BoringSSL never needs to define a single constant more than once.
If it did, I would add a helper function for that type of OID (or higher-level
structure).

Nothing but an SPKI parser ever needs to process SPKI key type OIDs. So we put
them in the SPKI parser/encoder. Nothing but a parser for hash algorithm OIDs
ever needs to process hash algorithm OIDs. So we put them in the hash algorithm
parser/encoder, etc.
I do not follow. We either make it so the function implicitly accepts everything
(which you convinced me was bad, and I agree), or code which accepts MD5 needs
to be able to dispatch around that.
It's certainly possible BoringSSL needs to have a master list of these
constants. Thus far, I haven't found this to be the case. I'd rather be able to
keep this stuff internal to the modules if possible. I think that's a much
better design as it avoids consumers having to know about OIDs.

I'd rather they simply know about things like "parse out a hash AlgID and give
me an EVP_MD" and "given an EVP_MD, write out the AlgID for it".

Likewise, you never actually need to write out id-rsaEncryption. You need to
write out an SPKI. Thus it makes much more sense for us to fold the
id-rsaEncryption definition into the SPKI and PKCS#8 APIs.

Unfortunately, net/cert/internal doesn't have a DER encoder or any of that
stack, so it has to be duplicated right now. See below.

[Ryan Sleevi, 2016/03/03 22:08:43]

I think we're in agreement here. Specifically, my concern was wanting to have
either something like

extern const uint8_t kMD5Encryption[stuff];

or something like

void GetMD5EncryptionOID(uint8_t*, size_t*);

or something like

enum SignatureAlgorithms {
  MD5,
  SHA-1
  SHA-256
};

void GetSignatureAlgorithm(SignatureAlgorithms, uint8_t*, size_t*);

or basically anything so that only 1 of the TUs has the full expansion. Is that
what you were thinking?
+1, it's this comment that made me think we're on the same page.

To be clear, my goal was to avoid something like:
a.cc
static const uint8_t kMyMD5WithRSA[] = { ... };
b.cc
static const uint8_t kAnotherMD5WithRSA[] = { ... };
c.cc
static const uint8_t kThirdMD5WithRSA[] = { ... };


Because that's split across TUs, there's no guarantee that the linker will be
allowed to fold those strings into a single address.
I think we're on the same page here.

[davidben, 2016/03/03 22:25:40]

Yup. I haven't done that here since signature_algorithm.cc doesn't even know
about MD5 anyway and it doesn't have a DER encoder which makes it awkward.
Cool. Yeah, I'll be sure to send the start of the "detach from crypto/obj" CLs
your way.

+
+  // Write the algorithm identifier and then the signature.
+  CBB sig_bitstring;
+  uint8_t* sig;
+  size_t sig_len;
+  if (!CBB_add_bytes(&spkac, kMd5WithRsaEncryption,
+                     sizeof(kMd5WithRsaEncryption)) ||
+      !CBB_add_asn1(&spkac, &sig_bitstring, CBS_ASN1_BITSTRING) ||
+      !CBB_add_u8(&sig_bitstring, 0 /* padding */) ||

[Ryan Sleevi, 2016/03/03 18:32:06]

? Why is this necessary? Because the assumption is there's no unused bits in
sig_bitstring?

This isn't really padding then, is it?

[davidben, 2016/03/03 19:58:15]

I think we've had this conversation before. :-) It's not an assumption.
Signatures are byte strings full-stop. I do not know of a single crypto
specification that legitimately uses bit strings to encode signatures.
Everything that specifies an encoding gives back octet strings.

I say "legitimately" because X.509 and related specs nominally do, but don't
actually. They just encode them as BIT STRING but never actually use them that
way. Even RSA in X.509, whose signatures are a number mod N, encodes to byte
strings first and than passes through the common byte string to BIT STRING
conversion.

A 2051-bit RSA signature in X.509 will still be encoded in a BIT STRING with a
multiple of 8 bytes.

(The common byte string to BIT STRING conversion just happens to be what you'd
get if you order the bits within a byte in the same order as the DER encoding of
a BIT STRING. Then you have to prepend a zero because of how BIT STRINGs are
encoding in DER.)
This is how BIT STRINGs are encoded in DER. You zero-pad at the *end* and then
*prepend* a single byte telling you how many bits to remove from the *end*.
(Indeed because of that, it actually doesn't make sense to encode a 2051-bit RSA
signature with a partial number of bits. The signature is encoded big-endian and
you pad at the end, so you'll end up actually having to shift everything by 3
bits.)

In X.509's byte string to BIT STRING conversion, this works out to always
sticking a zero byte in front of everything. I figured "padding" was one way to
condense that nonsense, I'm not very happy with it. I sometimes call it a "phase
byte" for BIT STRING in general, but I think I'm the only one who says that and
started calling it "padding" for that reason.

What do you think it should be called?



Appending of citations:
---

https://tools.ietf.org/html/rfc3279#section-2.2.1
at the bottom:

   The RSA signature generation process and the encoding of the result
   is described in detail in PKCS #1 [RFC 2313].

Latest version of RFC 2313 is RFC 3448.

https://tools.ietf.org/html/rfc3447#section-8.2.1
   Output:
   S        signature, an octet string of length k, where k is the
            length in octets of the RSA modulus n

Alas, RFC 3279 and a few others didn't quite realize they made type error.
(ASN.1 specifies no order of bits within a byte for a BIT STRING. One typically
uses the order in the DER encoding, but, abstractly, one is a
std::vector<uint8_t> and the other a std::vector<bool>.)

By RFC 4055, they seem to have figured this out and actually spelled this out
more clearly:
https://tools.ietf.org/html/rfc4055#section-3.2

   To convert a signature value to a bit string, the most significant
   bit of the first octet of the signature value SHALL become the first
   bit of the bit string, and so on through the least significant bit of
   the last octet of the signature value, which SHALL become the last
   bit of the bit string.

This is, coincidentally, the same as the DER encoding, except for the phase
byte.

[Ryan Sleevi, 2016/03/03 20:27:41]

That was much more than needed, but still doesn't answer my question, because I
feel it misunderstood what I ask.

Both "padding" and "phase byte" don't make sense, *IF* this is the DER-encoding
signifier of the number of unused bits. Is this that byte?

[davidben, 2016/03/03 21:59:36]

Yes, this is that byte. I tend to call it the "phase byte", but I think I'm the
only one who does. It is not an assumption that it is zero. It is simply how you
encode signatures in X.509.

Would you be happy with

  !CBB_add_u8(&sig_bitstring, 0 /* no unused bits */)

[Ryan Sleevi, 2016/03/03 22:08:43]

Yup. That's all I was trying to understand - to make sure that this is the /* no
unused bits */ byte :)

[davidben, 2016/03/03 22:25:40]

Done.

+      // Determine the maximum length of the signature.
+      !EVP_DigestSignFinal(ctx.get(), nullptr, &sig_len) ||
+      // Reserve |sig_len| bytes and write the signature to |spkac|.
+      !CBB_reserve(&sig_bitstring, &sig, sig_len) ||
+      !EVP_DigestSignFinal(ctx.get(), sig, &sig_len) ||
+      !CBB_did_write(&sig_bitstring, sig_len)) {

[Ryan Sleevi, 2016/03/03 18:32:06]

This feels like such a gross way to do it. Is this really how you envision the
new BoringSSL signing world? This seems like so much boilerplate and so much
opportunity to get things wrong.

[davidben, 2016/03/03 19:58:15]

Which piece of boilerplate in particular?

If you mean that we have to transcribe the ASN.1 type, someone has to do it. We
have to transcribe it in the Mac code too. If you prefer, I can make BoringSSL
provide a new version of the NETSCAPE_SPKI type. I figured this was such a
one-off, it may as well live in Chromium.


If you mean the reserve + did_write dance, it just avoids allocating a buffer
and stuff. *shrug* You can call it two step like that. You can also just use
EVP_PKEY_size to find the max size, though I find this confusing. (I think
historically they were lax about that vs. "key size in bytes" because RSA is the
same, and then they retconned EVP_PKEY_size to mean maximum signature size due
to DSA.) You can also allocate a separate buffer and stuff.

I can also add some EVP_DigestSignFinal variant that appends to a CBB. That
wouldn't be crazy.


If you mean the X.509-level assembly with signature algorithm and stuff, I am
hoping that most people will not have to deal with such nonsense. Hence the enum
and whatnot. :-)

At one point, I had something resembling AlgorithmIdentifier APIs in EVP. But
they still had ties to the old stack and you convinced me they were wrong
because net/cert will need to parse signature algorithms *and* enforce policies
on them. Accordingly, I've left BoringSSL core's APIs a layer below that. We
handle parsing SPKIs because that's used generally, but consumers that need
X.509 bits should use the X.509 stack built on top of it.

Except the new net/cert X.509 stack doesn't actually have signatureAlgorithm
mechanisms for creating X.509 signatures, only consuming them, hence this code
needs to do it itself until we have a clearer story there. (If we need one. I
actually think this code is pretty straight-forward.)

(Relatedly, the BoringSSL vs net/cert split is kind of awkward. We'll probably
need to figure out some of this at some point.)

[Ryan Sleevi, 2016/03/03 20:27:41]

Specifically, that the act of transcribing a SignatureAlgorithm + Signature pair
(which is consistent for all of the SignedData variants - X.509, CRL, OCSP,
SPKAC) into ASN.1; it seems like a common enough pattern that you already had to
duplicate it twice, but I anticipate we'd need to duplicate it more.

[davidben, 2016/03/03 21:59:36]

I don't believe I've ever had to write this before. Verifying certificates, but
the net/cert stack seems to not bother with creating them. Certainly, this bit
really should be part of net/cert. I'm hoping someone can factor that out and
put it into net/cert where it belongs if we ever get a second copy.

In the meantime, net/cert seems to believe X.509 signature algorithms are its
prerogative, so I don't want to also have X.509 signature algorithm
representations in BoringSSL. Hence this code is Chromium.

[Ryan Sleevi, 2016/03/03 22:08:43]

Oh, I thought you were saying you had a second copy (for the Mac code) - that
is, in this CL.

If this is the only use, I agree; and I suspect we *should* factor this out into
a common function in //net/cert (ok, //net/base can't depend on //net/cert so
we'll figure that out later) to handle other SignedData constructs.

[davidben, 2016/03/03 22:25:40]

Oh! No, I mean, the Mac code has an existing separate copy of serializing this
structure because historically we used the Mac APIs for this. I didn't want to
bother with deduplicating that here. Even then, it'd largely be about factoring
out the SPKAC construction and not the signature because Mac doesn't use
EVP_PKEY.
Added a TODO about that.

+    return std::string();
+  }
+
+  // Finally, the structure is base64-encoded.
+  uint8_t* der;
+  size_t der_len;
+  if (!CBB_finish(cbb.get(), &der, &der_len)) {
+    return std::string();
+  }
+  std::string result;
+  base::Base64Encode(
+      base::StringPiece(reinterpret_cast<const char*>(der), der_len), &result);
+  OPENSSL_free(der);
   return result;
 }
 
 }  // namespace net
-