Implement ECDSA_SIG_{parse,marshal} with crypto/bytestring.

This is the first structure to be implemented with the new BIGNUM ASN.1
routines. Object reuse in the legacy d2i/i2d functions is implemented by
releasing whatever was in *out before and setting it to the
newly-allocated object. As with the new d2i_SSL_SESSION, this is a
weaker form of object reuse, but should suffice for reasonable callers.

As ECDSA_SIG is more likely to be parsed alone than as part of another
structure (and using CBB is slightly tedious), add convenient functions
which take byte arrays. For consistency with SSL_SESSION, they are named
to/from_bytes. from_bytes, unlike the CBS variant, rejects trailing
data.

Note this changes some test expectations: BER signatures now push an
error code. That they didn't do this was probably a mistake.

BUG=499653

Change-Id: I9ec74db53e70d9a989412cc9e2b599be0454caec
Reviewed-on: https://boringssl-review.googlesource.com/5269
Reviewed-by: Adam Langley <agl@google.com>

2 files changed, 135 insertions, 23 deletions

diff --git a/crypto/ecdsa/ecdsa.c b/crypto/ecdsa/ecdsa.c
index b71799eb..7021ceb6 100644
--- a/crypto/ecdsa/ecdsa.c
+++ b/crypto/ecdsa/ecdsa.c
@@ -52,9 +52,11 @@
 
 #include <openssl/ecdsa.h>
 
+#include <assert.h>
 #include <string.h>
 
 #include <openssl/bn.h>
+#include <openssl/bytestring.h>
 #include <openssl/err.h>
 #include <openssl/mem.h>
 
@@ -76,21 +78,24 @@ int ECDSA_verify(int type, const uint8_t *digest, size_t digest_len,
   ECDSA_SIG *s;
   int ret = 0;
   uint8_t *der = NULL;
+  size_t der_len;
 
   if (eckey->ecdsa_meth && eckey->ecdsa_meth->verify) {
     return eckey->ecdsa_meth->verify(digest, digest_len, sig, sig_len, eckey);
   }
 
-  s = ECDSA_SIG_new();
-  const uint8_t *sigp = sig;
-  if (s == NULL || d2i_ECDSA_SIG(&s, &sigp, sig_len) == NULL ||
-      sigp != sig + sig_len) {
+  /* Decode the ECDSA signature. */
+  s = ECDSA_SIG_from_bytes(sig, sig_len);
+  if (s == NULL) {
     goto err;
   }
 
-  /* Ensure that the signature uses DER and doesn't have trailing garbage. */
-  const int der_len = i2d_ECDSA_SIG(s, &der);
-  if (der_len < 0 || (size_t) der_len != sig_len || memcmp(sig, der, sig_len)) {
+  /* Defend against potential laxness in the DER parser. */
+  size_t der_len;
+  if (!ECDSA_SIG_to_bytes(&der, &der_len, s) ||
+      der_len != sig_len || memcmp(sig, der, sig_len) != 0) {
+    /* This should never happen. crypto/bytestring is strictly DER. */
+    OPENSSL_PUT_ERROR(ECDSA, ECDSA_verify, ERR_R_INTERNAL_ERROR);
     goto err;
   }
 
@@ -455,20 +460,36 @@ err:
 int ECDSA_sign_ex(int type, const uint8_t *digest, size_t digest_len,
                   uint8_t *sig, unsigned int *sig_len, const BIGNUM *kinv,
                   const BIGNUM *r, EC_KEY *eckey) {
+  int ret = 0;
   ECDSA_SIG *s = NULL;
 
   if (eckey->ecdsa_meth && eckey->ecdsa_meth->sign) {
     OPENSSL_PUT_ERROR(ECDSA, ECDSA_sign_ex, ECDSA_R_NOT_IMPLEMENTED);
     *sig_len = 0;
-    return 0;
+    goto err;
   }
 
   s = ECDSA_do_sign_ex(digest, digest_len, kinv, r, eckey);
   if (s == NULL) {
     *sig_len = 0;
-    return 0;
+    goto err;
   }
-  *sig_len = i2d_ECDSA_SIG(s, &sig);
+
+  CBB cbb;
+  CBB_zero(&cbb);
+  size_t len;
+  if (!CBB_init_fixed(&cbb, sig, ECDSA_size(eckey)) ||
+      !ECDSA_SIG_marshal(&cbb, s) ||
+      !CBB_finish(&cbb, NULL, &len)) {
+    OPENSSL_PUT_ERROR(ECDSA, ECDSA_sign_ex, ECDSA_R_ENCODE_ERROR);
+    CBB_cleanup(&cbb);
+    *sig_len = 0;
+    goto err;
+  }
+  *sig_len = (unsigned)len;
+  ret = 1;
+
+err:
   ECDSA_SIG_free(s);
-  return 1;
+  return ret;
 }
diff --git a/crypto/ecdsa/ecdsa_asn1.c b/crypto/ecdsa/ecdsa_asn1.c
index 93be405d..7f6b7760 100644
--- a/crypto/ecdsa/ecdsa_asn1.c
+++ b/crypto/ecdsa/ecdsa_asn1.c
@@ -52,24 +52,18 @@
 
 #include <openssl/ecdsa.h>
 
-#include <openssl/asn1.h>
-#include <openssl/asn1t.h>
+#include <limits.h>
+#include <string.h>
+
+#include <openssl/bn.h>
+#include <openssl/bytestring.h>
+#include <openssl/err.h>
 #include <openssl/ec_key.h>
 #include <openssl/mem.h>
 
 #include "../ec/internal.h"
 
 
-DECLARE_ASN1_FUNCTIONS_const(ECDSA_SIG);
-DECLARE_ASN1_ENCODE_FUNCTIONS_const(ECDSA_SIG, ECDSA_SIG);
-
-ASN1_SEQUENCE(ECDSA_SIG) = {
-    ASN1_SIMPLE(ECDSA_SIG, r, CBIGNUM),
-    ASN1_SIMPLE(ECDSA_SIG, s, CBIGNUM),
-} ASN1_SEQUENCE_END(ECDSA_SIG);
-
-IMPLEMENT_ASN1_ENCODE_FUNCTIONS_const_fname(ECDSA_SIG, ECDSA_SIG, ECDSA_SIG);
-
 size_t ECDSA_size(const EC_KEY *key) {
   if (key == NULL) {
     return 0;
@@ -124,6 +118,61 @@ void ECDSA_SIG_free(ECDSA_SIG *sig) {
   OPENSSL_free(sig);
 }
 
+ECDSA_SIG *ECDSA_SIG_parse(CBS *cbs) {
+  ECDSA_SIG *ret = ECDSA_SIG_new();
+  if (ret == NULL) {
+    return NULL;
+  }
+  CBS child;
+  if (!CBS_get_asn1(cbs, &child, CBS_ASN1_SEQUENCE) ||
+      !BN_cbs2unsigned(&child, ret->r) ||
+      !BN_cbs2unsigned(&child, ret->s) ||
+      CBS_len(&child) != 0) {
+    OPENSSL_PUT_ERROR(ECDSA, ECDSA_SIG_parse, ECDSA_R_BAD_SIGNATURE);
+    ECDSA_SIG_free(ret);
+    return NULL;
+  }
+  return ret;
+}
+
+ECDSA_SIG *ECDSA_SIG_from_bytes(const uint8_t *in, size_t in_len) {
+  CBS cbs;
+  CBS_init(&cbs, in, in_len);
+  ECDSA_SIG *ret = ECDSA_SIG_parse(&cbs);
+  if (ret == NULL || CBS_len(&cbs) != 0) {
+    OPENSSL_PUT_ERROR(ECDSA, ECDSA_SIG_from_bytes, ECDSA_R_BAD_SIGNATURE);
+    ECDSA_SIG_free(ret);
+    return NULL;
+  }
+  return ret;
+}
+
+int ECDSA_SIG_marshal(CBB *cbb, const ECDSA_SIG *sig) {
+  CBB child;
+  if (!CBB_add_asn1(cbb, &child, CBS_ASN1_SEQUENCE) ||
+      !BN_bn2cbb(&child, sig->r) ||
+      !BN_bn2cbb(&child, sig->s) ||
+      !CBB_flush(cbb)) {
+    OPENSSL_PUT_ERROR(ECDSA, ECDSA_SIG_marshal, ECDSA_R_ENCODE_ERROR);
+    return 0;
+  }
+  return 1;
+}
+
+int ECDSA_SIG_to_bytes(uint8_t **out_bytes, size_t *out_len,
+                       const ECDSA_SIG *sig) {
+  CBB cbb;
+  CBB_zero(&cbb);
+  if (!CBB_init(&cbb, 0) ||
+      !ECDSA_SIG_marshal(&cbb, sig) ||
+      !CBB_finish(&cbb, out_bytes, out_len)) {
+    OPENSSL_PUT_ERROR(ECDSA, ECDSA_SIG_to_bytes, ECDSA_R_ENCODE_ERROR);
+    CBB_cleanup(&cbb);
+    return 0;
+  }
+  return 1;
+}
+
 /* der_len_len returns the number of bytes needed to represent a length of |len|
  * in DER. */
 static size_t der_len_len(size_t len) {
@@ -157,3 +206,45 @@ size_t ECDSA_SIG_max_len(size_t order_len) {
   }
   return ret;
 }
+
+ECDSA_SIG *d2i_ECDSA_SIG(ECDSA_SIG **out, const uint8_t **inp, long len) {
+  if (len < 0) {
+    return NULL;
+  }
+  CBS cbs;
+  CBS_init(&cbs, *inp, (size_t)len);
+  ECDSA_SIG *ret = ECDSA_SIG_parse(&cbs);
+  if (ret == NULL) {
+    return NULL;
+  }
+  if (out != NULL) {
+    ECDSA_SIG_free(*out);
+    *out = ret;
+  }
+  *inp += (size_t)len - CBS_len(&cbs);
+  return ret;
+}
+
+int i2d_ECDSA_SIG(const ECDSA_SIG *sig, uint8_t **outp) {
+  uint8_t *der;
+  size_t der_len;
+  if (!ECDSA_SIG_to_bytes(&der, &der_len, sig)) {
+    return -1;
+  }
+  if (der_len > INT_MAX) {
+    OPENSSL_PUT_ERROR(ECDSA, i2d_ECDSA_SIG, ERR_R_OVERFLOW);
+    OPENSSL_free(der);
+    return -1;
+  }
+  if (outp != NULL) {
+    if (*outp == NULL) {
+      *outp = der;
+      der = NULL;
+    } else {
+      memcpy(*outp, der, der_len);
+      *outp += der_len;
+    }
+  }
+  OPENSSL_free(der);
+  return (int)der_len;
+}