Reimplement PKCS #3 DH parameter parsing with crypto/bytestring.

Also add a test.

This is the last of the openssl/asn1.h includes from the directories that are
to be kept in the core libcrypto library. (What remains is to finish sorting
out the crypto/obj stuff. We'll also want to retain a decoupled version of the
PKCS#12 stuff.)

Functions that need to be audited for reuse:
i2d_DHparams

BUG=54

Change-Id: Ibef030a98d3a93ae26e8e56869f14858ec75601b
Reviewed-on: https://boringssl-review.googlesource.com/7900
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

crypto/dh/dh_asn1.c

@@ -55,30 +55,106 @@
 
 #include <openssl/dh.h>
 
-#include <openssl/asn1.h>
-#include <openssl/asn1t.h>
-
-#include "internal.h"
-
-/* Override the default free and new methods */
-static int dh_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
-                 void *exarg) {
-  if (operation == ASN1_OP_NEW_PRE) {
-    *pval = (ASN1_VALUE *)DH_new();
-    if (*pval) {
-      return 2;
+#include <assert.h>
+#include <limits.h>
+
+#include <openssl/bn.h>
+#include <openssl/bytestring.h>
+#include <openssl/err.h>
+
+#include "../bytestring/internal.h"
+
+
+static int parse_integer(CBS *cbs, BIGNUM **out) {
+  assert(*out == NULL);
+  *out = BN_new();
+  if (*out == NULL) {
+    return 0;
+  }
+  return BN_parse_asn1_unsigned(cbs, *out);
+}
+
+static int marshal_integer(CBB *cbb, BIGNUM *bn) {
+  if (bn == NULL) {
+    /* A DH object may be missing some components. */
+    OPENSSL_PUT_ERROR(DH, ERR_R_PASSED_NULL_PARAMETER);
+    return 0;
+  }
+  return BN_marshal_asn1(cbb, bn);
+}
+
+DH *DH_parse_parameters(CBS *cbs) {
+  DH *ret = DH_new();
+  if (ret == NULL) {
+    return NULL;
+  }
+
+  CBS child;
+  if (!CBS_get_asn1(cbs, &child, CBS_ASN1_SEQUENCE) ||
+      !parse_integer(&child, &ret->p) ||
+      !parse_integer(&child, &ret->g)) {
+    goto err;
+  }
+
+  uint64_t priv_length;
+  if (CBS_len(&child) != 0) {
+    if (!CBS_get_asn1_uint64(&child, &priv_length) ||
+        priv_length > UINT_MAX) {
+      goto err;
     }
+    ret->priv_length = (unsigned)priv_length;
+  }
+
+  if (CBS_len(&child) != 0) {
+    goto err;
+  }
+
+  return ret;
+
+err:
+  OPENSSL_PUT_ERROR(DH, DH_R_DECODE_ERROR);
+  DH_free(ret);
+  return NULL;
+}
+
+int DH_marshal_parameters(CBB *cbb, const DH *dh) {
+  CBB child;
+  if (!CBB_add_asn1(cbb, &child, CBS_ASN1_SEQUENCE) ||
+      !marshal_integer(&child, dh->p) ||
+      !marshal_integer(&child, dh->g) ||
+      (dh->priv_length != 0 &&
+       !CBB_add_asn1_uint64(&child, dh->priv_length)) ||
+      !CBB_flush(cbb)) {
+    OPENSSL_PUT_ERROR(DH, DH_R_ENCODE_ERROR);
     return 0;
-  } else if (operation == ASN1_OP_FREE_PRE) {
-    DH_free((DH *)*pval);
-    *pval = NULL;
-    return 2;
   }
   return 1;
 }
 
-ASN1_SEQUENCE_cb(DHparams, dh_cb) = {
-    ASN1_SIMPLE(DH, p, BIGNUM), ASN1_SIMPLE(DH, g, BIGNUM),
-    ASN1_OPT(DH, priv_length, ZLONG)} ASN1_SEQUENCE_END_cb(DH, DHparams);
+DH *d2i_DHparams(DH **out, const uint8_t **inp, long len) {
+  if (len < 0) {
+    return NULL;
+  }
+  CBS cbs;
+  CBS_init(&cbs, *inp, (size_t)len);
+  DH *ret = DH_parse_parameters(&cbs);
+  if (ret == NULL) {
+    return NULL;
+  }
+  if (out != NULL) {
+    DH_free(*out);
+    *out = ret;
+  }
+  *inp = CBS_data(&cbs);
+  return ret;
+}
 
-IMPLEMENT_ASN1_ENCODE_FUNCTIONS_const_fname(DH, DHparams, DHparams)
+int i2d_DHparams(const DH *in, uint8_t **outp) {
+  CBB cbb;
+  if (!CBB_init(&cbb, 0) ||
+      !DH_marshal_parameters(&cbb, in)) {
+    CBB_cleanup(&cbb);
+    return -1;
+  }
+  return CBB_finish_i2d(&cbb, outp);
+}

crypto/dh/dh_test.cc

@@ -62,6 +62,7 @@
 #include <vector>
 
 #include <openssl/bn.h>
+#include <openssl/bytestring.h>
 #include <openssl/crypto.h>
 #include <openssl/err.h>
 #include <openssl/mem.h>
@@ -73,13 +74,15 @@
 static bool RunBasicTests();
 static bool RunRFC5114Tests();
 static bool TestBadY();
+static bool TestASN1();
 
 int main(int argc, char *argv[]) {
   CRYPTO_library_init();
 
   if (!RunBasicTests() ||
       !RunRFC5114Tests() ||
-      !TestBadY()) {
+      !TestBadY() ||
+      !TestASN1()) {
     ERR_print_errors_fp(stderr);
     return 1;
   }
@@ -533,3 +536,91 @@ static bool TestBadY() {
 
   return true;
 }
+
+static bool BIGNUMEqualsHex(const BIGNUM *bn, const char *hex) {
+  BIGNUM *hex_bn = NULL;
+  if (!BN_hex2bn(&hex_bn, hex)) {
+    return false;
+  }
+  ScopedBIGNUM free_hex_bn(hex_bn);
+  return BN_cmp(bn, hex_bn) == 0;
+}
+
+static bool TestASN1() {
+  // kParams are a set of Diffie-Hellman parameters generated with
+  // openssl dhparam 256
+  static const uint8_t kParams[] = {
+      0x30, 0x26, 0x02, 0x21, 0x00, 0xd7, 0x20, 0x34, 0xa3, 0x27,
+      0x4f, 0xdf, 0xbf, 0x04, 0xfd, 0x24, 0x68, 0x25, 0xb6, 0x56,
+      0xd8, 0xab, 0x2a, 0x41, 0x2d, 0x74, 0x0a, 0x52, 0x08, 0x7c,
+      0x40, 0x71, 0x4e, 0xd2, 0x57, 0x93, 0x13, 0x02, 0x01, 0x02,
+  };
+
+  CBS cbs;
+  CBS_init(&cbs, kParams, sizeof(kParams));
+  ScopedDH dh(DH_parse_parameters(&cbs));
+  if (!dh || CBS_len(&cbs) != 0 ||
+      !BIGNUMEqualsHex(
+          dh->p,
+          "d72034a3274fdfbf04fd246825b656d8ab2a412d740a52087c40714ed2579313") ||
+      !BIGNUMEqualsHex(dh->g, "2") || dh->priv_length != 0) {
+    return false;
+  }
+
+  ScopedCBB cbb;
+  uint8_t *der;
+  size_t der_len;
+  if (!CBB_init(cbb.get(), 0) ||
+      !DH_marshal_parameters(cbb.get(), dh.get()) ||
+      !CBB_finish(cbb.get(), &der, &der_len)) {
+    return false;
+  }
+  ScopedOpenSSLBytes free_der(der);
+  if (der_len != sizeof(kParams) || memcmp(der, kParams, der_len) != 0) {
+    return false;
+  }
+
+  // kParamsDSA are a set of Diffie-Hellman parameters generated with
+  // openssl dhparam 256 -dsaparam
+  static const uint8_t kParamsDSA[] = {
+      0x30, 0x81, 0x89, 0x02, 0x41, 0x00, 0x93, 0xf3, 0xc1, 0x18, 0x01, 0xe6,
+      0x62, 0xb6, 0xd1, 0x46, 0x9a, 0x2c, 0x72, 0xea, 0x31, 0xd9, 0x18, 0x10,
+      0x30, 0x28, 0x63, 0xe2, 0x34, 0x7d, 0x80, 0xca, 0xee, 0x82, 0x2b, 0x19,
+      0x3c, 0x19, 0xbb, 0x42, 0x83, 0x02, 0x70, 0xdd, 0xdb, 0x8c, 0x03, 0xab,
+      0xe9, 0x9c, 0xc4, 0x00, 0x4d, 0x70, 0x5f, 0x52, 0x03, 0x31, 0x2c, 0xa4,
+      0x67, 0x34, 0x51, 0x95, 0x2a, 0xac, 0x11, 0xe2, 0x6a, 0x55, 0x02, 0x40,
+      0x44, 0xc8, 0x10, 0x53, 0x44, 0x32, 0x31, 0x63, 0xd8, 0xd1, 0x8c, 0x75,
+      0xc8, 0x98, 0x53, 0x3b, 0x5b, 0x4a, 0x2a, 0x0a, 0x09, 0xe7, 0xd0, 0x3c,
+      0x53, 0x72, 0xa8, 0x6b, 0x70, 0x41, 0x9c, 0x26, 0x71, 0x44, 0xfc, 0x7f,
+      0x08, 0x75, 0xe1, 0x02, 0xab, 0x74, 0x41, 0xe8, 0x2a, 0x3d, 0x3c, 0x26,
+      0x33, 0x09, 0xe4, 0x8b, 0xb4, 0x41, 0xec, 0xa6, 0xa8, 0xba, 0x1a, 0x07,
+      0x8a, 0x77, 0xf5, 0x5f, 0x02, 0x02, 0x00, 0xa0,
+  };
+
+  CBS_init(&cbs, kParamsDSA, sizeof(kParamsDSA));
+  dh.reset(DH_parse_parameters(&cbs));
+  if (!dh || CBS_len(&cbs) != 0 ||
+      !BIGNUMEqualsHex(dh->p,
+                       "93f3c11801e662b6d1469a2c72ea31d91810302863e2347d80caee8"
+                       "22b193c19bb42830270dddb8c03abe99cc4004d705f5203312ca467"
+                       "3451952aac11e26a55") ||
+      !BIGNUMEqualsHex(dh->g,
+                       "44c8105344323163d8d18c75c898533b5b4a2a0a09e7d03c5372a86"
+                       "b70419c267144fc7f0875e102ab7441e82a3d3c263309e48bb441ec"
+                       "a6a8ba1a078a77f55f") ||
+      dh->priv_length != 160) {
+    return false;
+  }
+
+  if (!CBB_init(cbb.get(), 0) ||
+      !DH_marshal_parameters(cbb.get(), dh.get()) ||
+      !CBB_finish(cbb.get(), &der, &der_len)) {
+    return false;
+  }
+  ScopedOpenSSLBytes free_der2(der);
+  if (der_len != sizeof(kParamsDSA) || memcmp(der, kParamsDSA, der_len) != 0) {
+    return false;
+  }
+
+  return true;
+}

crypto/err/dh.errordata

@@ -1,4 +1,6 @@
 DH,100,BAD_GENERATOR
+DH,104,DECODE_ERROR
+DH,105,ENCODE_ERROR
 DH,101,INVALID_PUBKEY
 DH,102,MODULUS_TOO_LARGE
 DH,103,NO_PRIVATE_VALUE

include/openssl/dh.h

@@ -174,22 +174,15 @@ OPENSSL_EXPORT DH *DHparams_dup(const DH *dh);
 
 /* ASN.1 functions. */
 
-/* d2i_DHparams parses an ASN.1, DER encoded Diffie-Hellman parameters
- * structure from |len| bytes at |*inp|. If |ret| is not NULL then, on exit, a
- * pointer to the result is in |*ret|. If |*ret| is already non-NULL on entry
- * then the result is written directly into |*ret|, otherwise a fresh |DH| is
- * allocated. However, one should not depend on writing into |*ret| because
- * this behaviour is likely to change in the future.
- *
- * On successful exit, |*inp| is advanced past the DER structure. It
- * returns the result or NULL on error. */
-OPENSSL_EXPORT DH *d2i_DHparams(DH **ret, const unsigned char **inp, long len);
+/* DH_parse_parameters decodes a DER-encoded DHParameter structure (PKCS #3)
+ * from |cbs| and advances |cbs|. It returns a newly-allocated |DH| or NULL on
+ * error. */
+OPENSSL_EXPORT DH *DH_parse_parameters(CBS *cbs);
 
-/* i2d_DHparams marshals |in| to an ASN.1, DER structure. If |outp| is not NULL
- * then the result is written to |*outp| and |*outp| is advanced just past the
- * output. It returns the number of bytes in the result, whether written or
- * not, or a negative value on error. */
-OPENSSL_EXPORT int i2d_DHparams(const DH *in, unsigned char **outp);
+/* DH_marshal_parameters marshals |dh| as a DER-encoded DHParameter structure
+ * (PKCS #3) and appends the result to |cbb|. It returns one on success and zero
+ * on error. */
+OPENSSL_EXPORT int DH_marshal_parameters(CBB *cbb, const DH *dh);
 
 
 /* ex_data functions.
@@ -213,6 +206,26 @@ OPENSSL_EXPORT DH *DH_generate_parameters(int prime_len, int generator,
                                           void (*callback)(int, int, void *),
                                           void *cb_arg);
 
+/* d2i_DHparams parses an ASN.1, DER encoded Diffie-Hellman parameters structure
+ * from |len| bytes at |*inp|. If |ret| is not NULL then, on exit, a pointer to
+ * the result is in |*ret|. Note that, even if |*ret| is already non-NULL on
+ * entry, it will not be written to. Rather, a fresh |DH| is allocated and the
+ * previous one is freed.
+ *
+ * On successful exit, |*inp| is advanced past the DER structure. It
+ * returns the result or NULL on error.
+ *
+ * Use |DH_parse_parameters| instead. */
+OPENSSL_EXPORT DH *d2i_DHparams(DH **ret, const unsigned char **inp, long len);
+
+/* i2d_DHparams marshals |in| to an ASN.1, DER structure. If |outp| is not NULL
+ * then the result is written to |*outp| and |*outp| is advanced just past the
+ * output. It returns the number of bytes in the result, whether written or
+ * not, or a negative value on error.
+ *
+ * Use |DH_marshal_parameters| instead. */
+OPENSSL_EXPORT int i2d_DHparams(const DH *in, unsigned char **outp);
+
 
 struct dh_st {
   BIGNUM *p;
@@ -248,5 +261,7 @@ struct dh_st {
 #define DH_R_INVALID_PUBKEY 101
 #define DH_R_MODULUS_TOO_LARGE 102
 #define DH_R_NO_PRIVATE_VALUE 103
+#define DH_R_DECODE_ERROR 104
+#define DH_R_ENCODE_ERROR 105
 
 #endif  /* OPENSSL_HEADER_DH_H */