From d93aa10bac6b269d6c9c431d8dc58858ba690fac Mon Sep 17 00:00:00 2001
From: Miroslav Lichvar <mlichvar@redhat.com>
Date: Wed, 8 Jul 2020 12:02:12 +0200
Subject: [PATCH] cmac+hash: change parameter types

For consistency and safety, change the CMC and HSH functions to accept
signed lengths and handle negative values as errors. Also, change the
input data type to void * to not require casting in the caller.
---
 cmac.h           |  8 ++++----
 cmac_nettle.c    | 14 ++++++++------
 hash.h           |  6 ++----
 hash_intmd5.c    | 10 ++++++----
 hash_nettle.c    | 10 ++++++----
 hash_nss.c       | 10 ++++++----
 hash_tomcrypt.c  | 10 ++++++----
 keys.c           |  9 ++++-----
 keys.h           |  6 +++---
 ntp_auth.c       |  4 ++--
 stubs.c          |  9 ++++-----
 test/unit/cmac.c | 12 +++++++-----
 test/unit/hash.c |  9 ++++++---
 13 files changed, 64 insertions(+), 53 deletions(-)

diff --git a/cmac.h b/cmac.h
index 206edc1..935820d 100644
--- a/cmac.h
+++ b/cmac.h
@@ -37,11 +37,11 @@ typedef enum {
 
 typedef struct CMC_Instance_Record *CMC_Instance;
 
-extern unsigned int CMC_GetKeyLength(CMC_Algorithm algorithm);
+extern int CMC_GetKeyLength(CMC_Algorithm algorithm);
 extern CMC_Instance CMC_CreateInstance(CMC_Algorithm algorithm, const unsigned char *key,
-                                       unsigned int length);
-extern unsigned int CMC_Hash(CMC_Instance inst, const unsigned char *in, unsigned int in_len,
-                             unsigned char *out, unsigned int out_len);
+                                       int length);
+extern int CMC_Hash(CMC_Instance inst, const void *in, int in_len,
+                    unsigned char *out, int out_len);
 extern void CMC_DestroyInstance(CMC_Instance inst);
 
 #endif
diff --git a/cmac_nettle.c b/cmac_nettle.c
index 239d006..5b2c0d4 100644
--- a/cmac_nettle.c
+++ b/cmac_nettle.c
@@ -44,7 +44,7 @@ struct CMC_Instance_Record {
 
 /* ================================================== */
 
-unsigned int
+int
 CMC_GetKeyLength(CMC_Algorithm algorithm)
 {
   if (algorithm == CMC_AES128)
@@ -57,11 +57,11 @@ CMC_GetKeyLength(CMC_Algorithm algorithm)
 /* ================================================== */
 
 CMC_Instance
-CMC_CreateInstance(CMC_Algorithm algorithm, const unsigned char *key, unsigned int length)
+CMC_CreateInstance(CMC_Algorithm algorithm, const unsigned char *key, int length)
 {
   CMC_Instance inst;
 
-  if (length == 0 || length != CMC_GetKeyLength(algorithm))
+  if (length <= 0 || length != CMC_GetKeyLength(algorithm))
     return NULL;
 
   inst = MallocNew(struct CMC_Instance_Record);
@@ -83,10 +83,12 @@ CMC_CreateInstance(CMC_Algorithm algorithm, const unsigned char *key, unsigned i
 
 /* ================================================== */
 
-unsigned int
-CMC_Hash(CMC_Instance inst, const unsigned char *in, unsigned int in_len,
-         unsigned char *out, unsigned int out_len)
+int
+CMC_Hash(CMC_Instance inst, const void *in, int in_len, unsigned char *out, int out_len)
 {
+  if (in_len < 0 || out_len < 0)
+    return 0;
+
   if (out_len > CMAC128_DIGEST_SIZE)
     out_len = CMAC128_DIGEST_SIZE;
 
diff --git a/hash.h b/hash.h
index 12167bc..290710f 100644
--- a/hash.h
+++ b/hash.h
@@ -48,10 +48,8 @@ typedef enum {
 
 extern int HSH_GetHashId(HSH_Algorithm algorithm);
 
-extern unsigned int HSH_Hash(int id,
-    const unsigned char *in1, unsigned int in1_len,
-    const unsigned char *in2, unsigned int in2_len,
-    unsigned char *out, unsigned int out_len);
+extern int HSH_Hash(int id, const void *in1, int in1_len, const void *in2, int in2_len,
+                    unsigned char *out, int out_len);
 
 extern void HSH_Finalise(void);
 
diff --git a/hash_intmd5.c b/hash_intmd5.c
index 5a0debf..4d101ea 100644
--- a/hash_intmd5.c
+++ b/hash_intmd5.c
@@ -45,11 +45,13 @@ HSH_GetHashId(HSH_Algorithm algorithm)
   return 0;
 }
 
-unsigned int
-HSH_Hash(int id, const unsigned char *in1, unsigned int in1_len,
-    const unsigned char *in2, unsigned int in2_len,
-    unsigned char *out, unsigned int out_len)
+int
+HSH_Hash(int id, const void *in1, int in1_len, const void *in2, int in2_len,
+         unsigned char *out, int out_len)
 {
+  if (in1_len < 0 || in2_len < 0 || out_len < 0)
+    return 0;
+
   MD5Init(&ctx);
   MD5Update(&ctx, in1, in1_len);
   if (in2)
diff --git a/hash_nettle.c b/hash_nettle.c
index 66fbe75..dba1717 100644
--- a/hash_nettle.c
+++ b/hash_nettle.c
@@ -84,14 +84,16 @@ HSH_GetHashId(HSH_Algorithm algorithm)
   return id;
 }
 
-unsigned int
-HSH_Hash(int id, const unsigned char *in1, unsigned int in1_len,
-         const unsigned char *in2, unsigned int in2_len,
-         unsigned char *out, unsigned int out_len)
+int
+HSH_Hash(int id, const void *in1, int in1_len, const void *in2, int in2_len,
+         unsigned char *out, int out_len)
 {
   const struct nettle_hash *hash;
   void *context;
 
+  if (in1_len < 0 || in2_len < 0 || out_len < 0)
+    return 0;
+
   hash = hashes[id].nettle_hash;
   context = hashes[id].context;
 
diff --git a/hash_nss.c b/hash_nss.c
index 81345c2..fe26627 100644
--- a/hash_nss.c
+++ b/hash_nss.c
@@ -74,14 +74,16 @@ HSH_GetHashId(HSH_Algorithm algorithm)
   return i;
 }
 
-unsigned int
-HSH_Hash(int id, const unsigned char *in1, unsigned int in1_len,
-    const unsigned char *in2, unsigned int in2_len,
-    unsigned char *out, unsigned int out_len)
+int
+HSH_Hash(int id, const void *in1, int in1_len, const void *in2, int in2_len,
+         unsigned char *out, int out_len)
 {
   unsigned char buf[MAX_HASH_LENGTH];
   unsigned int ret = 0;
 
+  if (in1_len < 0 || in2_len < 0 || out_len < 0)
+    return 0;
+
   NSSLOWHASH_Begin(hashes[id].context);
   NSSLOWHASH_Update(hashes[id].context, in1, in1_len);
   if (in2)
diff --git a/hash_tomcrypt.c b/hash_tomcrypt.c
index 5da09b2..5d126ca 100644
--- a/hash_tomcrypt.c
+++ b/hash_tomcrypt.c
@@ -89,15 +89,17 @@ HSH_GetHashId(HSH_Algorithm algorithm)
   return find_hash(hashes[i].int_name);
 }
 
-unsigned int
-HSH_Hash(int id, const unsigned char *in1, unsigned int in1_len,
-    const unsigned char *in2, unsigned int in2_len,
-    unsigned char *out, unsigned int out_len)
+int
+HSH_Hash(int id, const void *in1, int in1_len, const void *in2, int in2_len,
+         unsigned char *out, int out_len)
 {
   unsigned char buf[MAX_HASH_LENGTH];
   unsigned long len;
   int r;
 
+  if (in1_len < 0 || in2_len < 0 || out_len < 0)
+    return 0;
+
   len = sizeof (buf);
   if (in2)
     r = hash_memory_multi(id, buf, &len,
diff --git a/keys.c b/keys.c
index a08af97..69d7f89 100644
--- a/keys.c
+++ b/keys.c
@@ -376,8 +376,7 @@ KEY_GetKeyInfo(uint32_t key_id, int *type, int *bits)
 /* ================================================== */
 
 static int
-generate_auth(Key *key, const unsigned char *data, int data_len,
-              unsigned char *auth, int auth_len)
+generate_auth(Key *key, const void *data, int data_len, unsigned char *auth, int auth_len)
 {
   switch (key->class) {
     case NTP_MAC:
@@ -393,7 +392,7 @@ generate_auth(Key *key, const unsigned char *data, int data_len,
 /* ================================================== */
 
 static int
-check_auth(Key *key, const unsigned char *data, int data_len,
+check_auth(Key *key, const void *data, int data_len,
            const unsigned char *auth, int auth_len, int trunc_len)
 {
   unsigned char buf[MAX_HASH_LENGTH];
@@ -407,7 +406,7 @@ check_auth(Key *key, const unsigned char *data, int data_len,
 /* ================================================== */
 
 int
-KEY_GenerateAuth(uint32_t key_id, const unsigned char *data, int data_len,
+KEY_GenerateAuth(uint32_t key_id, const void *data, int data_len,
                  unsigned char *auth, int auth_len)
 {
   Key *key;
@@ -423,7 +422,7 @@ KEY_GenerateAuth(uint32_t key_id, const unsigned char *data, int data_len,
 /* ================================================== */
 
 int
-KEY_CheckAuth(uint32_t key_id, const unsigned char *data, int data_len,
+KEY_CheckAuth(uint32_t key_id, const void *data, int data_len,
               const unsigned char *auth, int auth_len, int trunc_len)
 {
   Key *key;
diff --git a/keys.h b/keys.h
index 7064590..c89fea9 100644
--- a/keys.h
+++ b/keys.h
@@ -39,9 +39,9 @@ extern int KEY_GetAuthLength(uint32_t key_id);
 extern int KEY_CheckKeyLength(uint32_t key_id);
 extern int KEY_GetKeyInfo(uint32_t key_id, int *type, int *bits);
 
-extern int KEY_GenerateAuth(uint32_t key_id, const unsigned char *data,
-    int data_len, unsigned char *auth, int auth_len);
-extern int KEY_CheckAuth(uint32_t key_id, const unsigned char *data, int data_len,
+extern int KEY_GenerateAuth(uint32_t key_id, const void *data, int data_len,
+                            unsigned char *auth, int auth_len);
+extern int KEY_CheckAuth(uint32_t key_id, const void *data, int data_len,
                          const unsigned char *auth, int auth_len, int trunc_len);
 
 #endif /* GOT_KEYS_H */
diff --git a/ntp_auth.c b/ntp_auth.c
index f7c31c4..c7f61d1 100644
--- a/ntp_auth.c
+++ b/ntp_auth.c
@@ -60,7 +60,7 @@ generate_symmetric_auth(uint32_t key_id, NTP_Packet *packet, NTP_PacketInfo *inf
   max_auth_len = (info->version == 4 ? NTP_MAX_V4_MAC_LENGTH : NTP_MAX_MAC_LENGTH) - 4;
   max_auth_len = MIN(max_auth_len, sizeof (NTP_Packet) - info->length - 4);
 
-  auth_len = KEY_GenerateAuth(key_id, (unsigned char *)packet, info->length,
+  auth_len = KEY_GenerateAuth(key_id, packet, info->length,
                               (unsigned char *)packet + info->length + 4, max_auth_len);
   if (!auth_len) {
     DEBUG_LOG("Could not generate auth data with key %"PRIu32, key_id);
@@ -86,7 +86,7 @@ check_symmetric_auth(NTP_Packet *packet, NTP_PacketInfo *info)
   trunc_len = info->version == 4 && info->auth.mac.length <= NTP_MAX_V4_MAC_LENGTH ?
               NTP_MAX_V4_MAC_LENGTH : NTP_MAX_MAC_LENGTH;
 
-  if (!KEY_CheckAuth(info->auth.mac.key_id, (void *)packet, info->auth.mac.start,
+  if (!KEY_CheckAuth(info->auth.mac.key_id, packet, info->auth.mac.start,
                      (unsigned char *)packet + info->auth.mac.start + 4,
                      info->auth.mac.length - 4, trunc_len - 4))
     return 0;
diff --git a/stubs.c b/stubs.c
index 73b9a95..43e6e3f 100644
--- a/stubs.c
+++ b/stubs.c
@@ -438,21 +438,20 @@ NSD_SignAndSendPacket(uint32_t key_id, NTP_Packet *packet, NTP_PacketInfo *info,
 
 #ifndef HAVE_CMAC
 
-unsigned int
+int
 CMC_GetKeyLength(CMC_Algorithm algorithm)
 {
   return 0;
 }
 
 CMC_Instance
-CMC_CreateInstance(CMC_Algorithm algorithm, const unsigned char *key, unsigned int length)
+CMC_CreateInstance(CMC_Algorithm algorithm, const unsigned char *key, int length)
 {
   return NULL;
 }
 
-unsigned int
-CMC_Hash(CMC_Instance inst, const unsigned char *in, unsigned int in_len,
-         unsigned char *out, unsigned int out_len)
+int
+CMC_Hash(CMC_Instance inst, const void *in, int in_len, unsigned char *out, int out_len)
 {
   return 0;
 }
diff --git a/test/unit/cmac.c b/test/unit/cmac.c
index 07fc3d5..be4ffbb 100644
--- a/test/unit/cmac.c
+++ b/test/unit/cmac.c
@@ -31,9 +31,9 @@
 struct cmac_test {
   const char *name;
   const unsigned char key[MAX_KEY_LENGTH];
-  unsigned int key_length;
+  int key_length;
   const unsigned char hash[MAX_HASH_LENGTH];
-  unsigned int hash_length;
+  int hash_length;
 };
 
 void
@@ -52,8 +52,7 @@ test_unit(void)
 
   CMC_Algorithm algorithm;
   CMC_Instance inst;
-  unsigned int length;
-  int i, j;
+  int i, j, length;
 
 #ifndef HAVE_CMAC
   TEST_REQUIRE(0);
@@ -68,7 +67,7 @@ test_unit(void)
 
     DEBUG_LOG("testing %s", tests[i].name);
 
-    for (j = 0; j <= 128; j++) {
+    for (j = -1; j <= 128; j++) {
       if (j == tests[i].key_length)
         continue;
       TEST_CHECK(!CMC_CreateInstance(algorithm, tests[i].key, j));
@@ -79,6 +78,9 @@ test_unit(void)
 
     TEST_CHECK(!CMC_CreateInstance(0, tests[i].key, tests[i].key_length));
 
+    TEST_CHECK(CMC_Hash(inst, data, -1, hash, sizeof (hash)) == 0);
+    TEST_CHECK(CMC_Hash(inst, data, sizeof (data) - 1, hash, -1) == 0);
+
     for (j = 0; j <= sizeof (hash); j++) {
       memset(hash, 0, sizeof (hash));
       length = CMC_Hash(inst, data, sizeof (data) - 1, hash, j);
diff --git a/test/unit/hash.c b/test/unit/hash.c
index 331dcac..d57c915 100644
--- a/test/unit/hash.c
+++ b/test/unit/hash.c
@@ -28,7 +28,7 @@
 struct hash_test {
   const char *name;
   const unsigned char out[MAX_HASH_LENGTH];
-  unsigned int length;
+  int length;
 };
 
 void
@@ -71,8 +71,7 @@ test_unit(void)
   };
 
   HSH_Algorithm algorithm;
-  unsigned int length;
-  int i, j, hash_id;
+  int i, j, hash_id, length;
 
   TEST_CHECK(HSH_INVALID == 0);
 
@@ -93,6 +92,10 @@ test_unit(void)
 
     DEBUG_LOG("testing %s", tests[i].name);
 
+    TEST_CHECK(HSH_Hash(hash_id, data1, -1, NULL, 0, out, sizeof (out)) == 0);
+    TEST_CHECK(HSH_Hash(hash_id, data1, sizeof (data1) - 1, data2, -1, out, sizeof (out)) == 0);
+    TEST_CHECK(HSH_Hash(hash_id, data1, sizeof (data1) - 1, NULL, 0, out, -1) == 0);
+
     for (j = 0; j <= sizeof (out); j++) {
       TEST_CHECK(HSH_GetHashId(algorithm) == hash_id);
       TEST_CHECK(HSH_GetHashId(0) < 0);