Document and fix up name hashing functions

These return 32-bit hashes, so they should return a platform-independent
uint32_t. I've categorized X509_issuer_name_hash and friends under
"convenience" functions. X509_NAME_hash and X509_NAME_hash_old are as
yet unclassified. Since the hash function is only relevant to
X509_LOOKUP_hash_dir, I'm thinking I'll put them with that, once that's
organized.

While I'm here, simplify the implementations of these functions. The
hash operation itself can be made infallible and allocation-free easily.
However the function itself is still fallible (and non-const, and not
thread-safe) due to the cached encoding mess. X509Test.NameHash captures
existing hash values, so we'd notice if this changed the output.

Update-Note: This is source-compatible for C/C++, including with
-Wconversion, but some bindings need a patch in cl/588632028 to be
compatible.

Bug: 426
Change-Id: I9bfd3f1093ab15c44d8cb2d81d53aeb3d6e49fc9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64647
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

crypto/x509/by_dir.c

@@ -54,6 +54,7 @@
  * copied and put under another distribution licence
  * [including the GNU Public Licence.] */
 
+#include <inttypes.h>
 #include <string.h>
 #include <sys/stat.h>
 #include <sys/types.h>
@@ -68,7 +69,7 @@
 #include "internal.h"
 
 typedef struct lookup_dir_hashes_st {
-  unsigned long hash;
+  uint32_t hash;
   int suffix;
 } BY_DIR_HASH;
 
@@ -246,8 +247,8 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name,
   int ok = 0;
   size_t i;
   int j, k;
-  unsigned long h;
-  unsigned long hash_array[2];
+  uint32_t h;
+  uint32_t hash_array[2];
   int hash_index;
   BUF_MEM *b = NULL;
   X509_OBJECT stmp, *tmp;
@@ -309,7 +310,8 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name,
         hent = NULL;
       }
       for (;;) {
-        snprintf(b->data, b->max, "%s/%08lx.%s%d", ent->dir, h, postfix, k);
+        snprintf(b->data, b->max, "%s/%08" PRIx32 ".%s%d", ent->dir, h, postfix,
+                 k);
 #ifndef OPENSSL_NO_POSIX_IO
 #if defined(_WIN32) && !defined(stat)
 #define stat _stat

crypto/x509/internal.h

@@ -105,7 +105,6 @@ struct X509_name_st {
   STACK_OF(X509_NAME_ENTRY) *entries;
   int modified;  // true if 'bytes' needs to be built
   BUF_MEM *bytes;
-  // unsigned long hash; Keep the hash around for lookups
   unsigned char *canon_enc;
   int canon_enclen;
 } /* X509_NAME */;

crypto/x509/x509_cmp.c

@@ -60,7 +60,9 @@
 #include <openssl/digest.h>
 #include <openssl/err.h>
 #include <openssl/mem.h>
+#include <openssl/md5.h>
 #include <openssl/obj.h>
+#include <openssl/sha.h>
 #include <openssl/stack.h>
 #include <openssl/x509.h>
 
@@ -88,11 +90,11 @@ X509_NAME *X509_get_issuer_name(const X509 *a) {
   return a->cert_info->issuer;
 }
 
-unsigned long X509_issuer_name_hash(X509 *x) {
-  return (X509_NAME_hash(x->cert_info->issuer));
+uint32_t X509_issuer_name_hash(X509 *x) {
+  return X509_NAME_hash(x->cert_info->issuer);
 }
 
-unsigned long X509_issuer_name_hash_old(X509 *x) {
+uint32_t X509_issuer_name_hash_old(X509 *x) {
   return (X509_NAME_hash_old(x->cert_info->issuer));
 }
 
@@ -108,12 +110,12 @@ const ASN1_INTEGER *X509_get0_serialNumber(const X509 *x509) {
   return x509->cert_info->serialNumber;
 }
 
-unsigned long X509_subject_name_hash(X509 *x) {
-  return (X509_NAME_hash(x->cert_info->subject));
+uint32_t X509_subject_name_hash(X509 *x) {
+  return X509_NAME_hash(x->cert_info->subject);
 }
 
-unsigned long X509_subject_name_hash_old(X509 *x) {
-  return (X509_NAME_hash_old(x->cert_info->subject));
+uint32_t X509_subject_name_hash_old(X509 *x) {
+  return X509_NAME_hash_old(x->cert_info->subject);
 }
 
 // Compare two certificates: they must be identical for this to work. NB:
@@ -165,44 +167,29 @@ int X509_NAME_cmp(const X509_NAME *a, const X509_NAME *b) {
   return OPENSSL_memcmp(a->canon_enc, b->canon_enc, a->canon_enclen);
 }
 
-unsigned long X509_NAME_hash(X509_NAME *x) {
-  unsigned long ret = 0;
-  unsigned char md[SHA_DIGEST_LENGTH];
-
-  // Make sure X509_NAME structure contains valid cached encoding
-  i2d_X509_NAME(x, NULL);
-  if (!EVP_Digest(x->canon_enc, x->canon_enclen, md, NULL, EVP_sha1(), NULL)) {
+uint32_t X509_NAME_hash(X509_NAME *x) {
+  // Make sure the X509_NAME structure contains a valid cached encoding.
+  if (i2d_X509_NAME(x, NULL) < 0) {
     return 0;
   }
 
-  ret = (((unsigned long)md[0]) | ((unsigned long)md[1] << 8L) |
-         ((unsigned long)md[2] << 16L) | ((unsigned long)md[3] << 24L)) &
-        0xffffffffL;
-  return ret;
+  uint8_t md[SHA_DIGEST_LENGTH];
+  SHA1(x->canon_enc, x->canon_enclen, md);
+  return CRYPTO_load_u32_le(md);
 }
 
 // I now DER encode the name and hash it.  Since I cache the DER encoding,
 // this is reasonably efficient.
 
-unsigned long X509_NAME_hash_old(X509_NAME *x) {
-  EVP_MD_CTX md_ctx;
-  unsigned long ret = 0;
-  unsigned char md[16];
-
-  // Make sure X509_NAME structure contains valid cached encoding
-  i2d_X509_NAME(x, NULL);
-  EVP_MD_CTX_init(&md_ctx);
-  // EVP_MD_CTX_set_flags(&md_ctx, EVP_MD_CTX_FLAG_NON_FIPS_ALLOW);
-  if (EVP_DigestInit_ex(&md_ctx, EVP_md5(), NULL) &&
-      EVP_DigestUpdate(&md_ctx, x->bytes->data, x->bytes->length) &&
-      EVP_DigestFinal_ex(&md_ctx, md, NULL)) {
-    ret = (((unsigned long)md[0]) | ((unsigned long)md[1] << 8L) |
-           ((unsigned long)md[2] << 16L) | ((unsigned long)md[3] << 24L)) &
-          0xffffffffL;
+uint32_t X509_NAME_hash_old(X509_NAME *x) {
+  // Make sure the X509_NAME structure contains a valid cached encoding.
+  if (i2d_X509_NAME(x, NULL) < 0) {
+    return 0;
   }
-  EVP_MD_CTX_cleanup(&md_ctx);
 
-  return ret;
+  uint8_t md[SHA_DIGEST_LENGTH];
+  MD5((const uint8_t *)x->bytes->data, x->bytes->length, md);
+  return CRYPTO_load_u32_le(md);
 }
 
 X509 *X509_find_by_issuer_and_serial(const STACK_OF(X509) *sk, X509_NAME *name,

crypto/x509/x509_test.cc

@@ -2582,8 +2582,8 @@ TEST(X509Test, X509NameSet) {
 TEST(X509Test, NameHash) {
   struct {
     std::vector<uint8_t> name_der;
-    unsigned long hash;
-    unsigned long hash_old;
+    uint32_t hash;
+    uint32_t hash_old;
   } kTests[] = {
       // SEQUENCE {
       //   SET {

include/openssl/x509.h

@@ -2831,6 +2831,22 @@ OPENSSL_EXPORT int X509_subject_name_cmp(const X509 *a, const X509 *b);
 // CRL, only the issuer fields using |X509_NAME_cmp|.
 OPENSSL_EXPORT int X509_CRL_cmp(const X509_CRL *a, const X509_CRL *b);
 
+// X509_issuer_name_hash returns the hash of |x509|'s issuer name with
+// |X509_NAME_hash|.
+OPENSSL_EXPORT uint32_t X509_issuer_name_hash(X509 *x509);
+
+// X509_subject_name_hash returns the hash of |x509|'s subject name with
+// |X509_NAME_hash|.
+OPENSSL_EXPORT uint32_t X509_subject_name_hash(X509 *x509);
+
+// X509_issuer_name_hash returns the hash of |x509|'s issuer name with
+// |X509_NAME_hash_old|.
+OPENSSL_EXPORT uint32_t X509_issuer_name_hash_old(X509 *x509);
+
+// X509_usjbect_name_hash returns the hash of |x509|'s usjbect name with
+// |X509_NAME_hash_old|.
+OPENSSL_EXPORT uint32_t X509_subject_name_hash_old(X509 *x509);
+
 
 // ex_data functions.
 //
@@ -3443,16 +3459,24 @@ OPENSSL_EXPORT const char *X509_get_default_private_dir(void);
 
 OPENSSL_EXPORT int X509_TRUST_set(int *t, int trust);
 
-OPENSSL_EXPORT unsigned long X509_issuer_name_hash(X509 *a);
-
-OPENSSL_EXPORT unsigned long X509_subject_name_hash(X509 *x);
+OPENSSL_EXPORT int X509_cmp(const X509 *a, const X509 *b);
 
-OPENSSL_EXPORT unsigned long X509_issuer_name_hash_old(X509 *a);
-OPENSSL_EXPORT unsigned long X509_subject_name_hash_old(X509 *x);
+// X509_NAME_hash returns a hash of |name|, or zero on error. This is the new
+// hash used by |X509_LOOKUP_hash_dir|.
+//
+// TODO(https://crbug.com/boringssl/407): This should be const and thread-safe
+// but currently is neither, notably if |name| was modified from its parsed
+// value.
+OPENSSL_EXPORT uint32_t X509_NAME_hash(X509_NAME *name);
 
-OPENSSL_EXPORT int X509_cmp(const X509 *a, const X509 *b);
-OPENSSL_EXPORT unsigned long X509_NAME_hash(X509_NAME *x);
-OPENSSL_EXPORT unsigned long X509_NAME_hash_old(X509_NAME *x);
+// X509_NAME_hash_old returns a hash of |name|, or zero on error. This is the
+// legacy hash used by |X509_LOOKUP_hash_dir|, which is still supported for
+// compatibility.
+//
+// TODO(https://crbug.com/boringssl/407): This should be const and thread-safe
+// but currently is neither, notably if |name| was modified from its parsed
+// value.
+OPENSSL_EXPORT uint32_t X509_NAME_hash_old(X509_NAME *name);
 
 OPENSSL_EXPORT int X509_CRL_match(const X509_CRL *a, const X509_CRL *b);