Fix bad usage of strtol()

Someone didn't RTFM.

1. Error code wasn't being catched.
2. Second argument was NULL, so the caller had no way of knowing
   whether the string was fully consumed.
2. The code wasn't making sure the input was a clean hex string.
3. The input string wasn't NULL-terminated.

In the end, I ditched strtol(). It's too dangerous and cluttered for
this purpose.

diff --git a/src/rrdp.c b/src/rrdp.c
index 7a522e9bb53bac9c4e9268cf304c1ab0a3e24abc..a8251b6e3cdc988f18b36319a9208503d99e5bb8 100644 (file)
--- a/src/rrdp.c
+++ b/src/rrdp.c
@@ -318,44 +318,74 @@ parse_string(xmlTextReaderPtr reader, char const *attr)
        return result;
 }
 
+static unsigned int
+hexchar2uint(xmlChar xmlchar)
+{
+       if ('0' <= xmlchar && xmlchar <= '9')
+               return xmlchar - '0';
+       if ('a' <= xmlchar && xmlchar <= 'f')
+               return xmlchar - 'a' + 10;
+       if ('A' <= xmlchar && xmlchar <= 'F')
+               return xmlchar - 'A' + 10;
+       return 32;
+}
+
+static int
+hexstr2sha256(xmlChar *hexstr, unsigned char **result)
+{
+       unsigned char *hash;
+       unsigned int digit;
+       size_t i;
+
+       if (xmlStrlen(hexstr) != 2 * SHA256_DIGEST_LENGTH)
+               return EINVAL;
+
+       hash = pmalloc(SHA256_DIGEST_LENGTH);
+
+       for (i = 0; i < SHA256_DIGEST_LENGTH; i++) {
+               digit = hexchar2uint(hexstr[2 * i]);
+               if (digit > 15)
+                       goto fail;
+               hash[i] = digit << 4;
+
+               digit = hexchar2uint(hexstr[2 * i + 1]);
+               if (digit > 15)
+                       goto fail;
+               hash[i] |= digit;
+       }
+
+       *result = hash;
+       return 0;
+
+fail:
+       free(hash);
+       return EINVAL;
+}
+
 static int
-parse_hex_string(xmlTextReaderPtr reader, hash_requirement hr, char const *attr,
+parse_hash(xmlTextReaderPtr reader, hash_requirement hr, char const *attr,
     unsigned char **result, size_t *result_len)
 {
-       xmlChar *xml_value;
-       unsigned char *tmp, *ptr;
-       char *xml_cur;
-       char buf[2];
-       size_t tmp_len;
-
-       xml_value = xmlTextReaderGetAttribute(reader, BAD_CAST attr);
-       if (xml_value == NULL)
+       xmlChar *xmlattr;
+       int error;
+
+       if (hr == HR_IGNORE)
+               return 0;
+
+       xmlattr = xmlTextReaderGetAttribute(reader, BAD_CAST attr);
+       if (xmlattr == NULL)
                return (hr == HR_MANDATORY)
-                   ? pr_val_err("RRDP file: Couldn't find xml attribute '%s'", attr)
+                   ? pr_val_err("Tag is missing the '%s' attribute.", attr)
                    : 0;
 
-       /* The rest of the checks are done at the schema */
-       if (xmlStrlen(xml_value) % 2 != 0) {
-               xmlFree(xml_value);
-               return pr_val_err("RRDP file: Attribute %s isn't a valid hex string",
-                   attr);
-       }
+       error = hexstr2sha256(xmlattr, result);
 
-       tmp_len = xmlStrlen(xml_value) / 2;
-       tmp = pzalloc(tmp_len);
+       xmlFree(xmlattr);
 
-       ptr = tmp;
-       xml_cur = (char *) xml_value;
-       while (ptr - tmp < tmp_len) {
-               memcpy(buf, xml_cur, 2);
-               *ptr = strtol(buf, NULL, 16);
-               xml_cur+=2;
-               ptr++;
-       }
-       xmlFree(xml_value);
-
-       *result = tmp;
-       (*result_len) = tmp_len;
+       if (error)
+               return pr_val_err("The '%s' xml attribute does not appear to be a SHA-256 hash.",
+                   attr);
+       *result_len = SHA256_DIGEST_LENGTH;
        return 0;
 }
 
@@ -487,7 +517,7 @@ parse_file_metadata(xmlTextReaderPtr reader, struct rpki_uri *notif,
        if (hr == HR_IGNORE)
                return 0;
 
-       error = parse_hex_string(reader, hr, RRDP_ATTR_HASH, &meta->hash,
+       error = parse_hash(reader, hr, RRDP_ATTR_HASH, &meta->hash,
            &meta->hash_len);
        if (error) {
                uri_refput(meta->uri);

diff --git a/test/rrdp_test.c b/test/rrdp_test.c
index 9debbacc3cc2d59fa7ce81790afccba8511b66dd..18ef83681acf07474d59129fa326b53730a351d4 100644 (file)
--- a/test/rrdp_test.c
+++ b/test/rrdp_test.c
@@ -39,6 +39,46 @@ MOCK_ABORT_PTR(validation_tal, tal, struct validation *state)
 
 /* Mocks end */
 
+START_TEST(test_hexstr2sha256)
+{
+       char *hex;
+       unsigned char *sha = NULL;
+       unsigned int i;
+
+       hex = "01";
+       ck_assert_int_eq(EINVAL, hexstr2sha256((xmlChar *) hex, &sha));
+       ck_assert_ptr_eq(NULL, sha);
+
+       hex = "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f";
+       ck_assert_int_eq(0, hexstr2sha256((xmlChar *) hex, &sha));
+       ck_assert_ptr_ne(NULL, sha);
+       for (i = 0; i < 32; i++)
+               ck_assert_uint_eq(i, sha[i]);
+       free(sha);
+       sha = NULL;
+
+       hex = "0x0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f";
+       ck_assert_int_eq(EINVAL, hexstr2sha256((xmlChar *) hex, &sha));
+       ck_assert_ptr_eq(NULL, sha);
+
+       hex = " 00102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f";
+       ck_assert_int_eq(EINVAL, hexstr2sha256((xmlChar *) hex, &sha));
+       ck_assert_ptr_eq(NULL, sha);
+
+       hex = "0001020g0405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f";
+       ck_assert_int_eq(EINVAL, hexstr2sha256((xmlChar *) hex, &sha));
+       ck_assert_ptr_eq(NULL, sha);
+
+       hex = "0001020g0405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1";
+       ck_assert_int_eq(EINVAL, hexstr2sha256((xmlChar *) hex, &sha));
+       ck_assert_ptr_eq(NULL, sha);
+
+       hex = "0001020g0405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f2";
+       ck_assert_int_eq(EINVAL, hexstr2sha256((xmlChar *) hex, &sha));
+       ck_assert_ptr_eq(NULL, sha);
+}
+END_TEST
+
 #define END 0xFFFF
 
 static int
@@ -144,13 +184,14 @@ END_TEST
 static Suite *xml_load_suite(void)
 {
        Suite *suite;
-       TCase *validate;
+       TCase *misc;
 
-       validate = tcase_create("Validate");
-       tcase_add_test(validate, test_sort_deltas);
+       misc = tcase_create("misc");
+       tcase_add_test(misc, test_hexstr2sha256);
+       tcase_add_test(misc, test_sort_deltas);
 
-       suite = suite_create("xml_test()");
-       suite_add_tcase(suite, validate);
+       suite = suite_create("RRDP");
+       suite_add_tcase(suite, misc);
 
        return suite;
 }