Fix string compare functions

Author: padelsbach

.gitignore

@@ -140,6 +140,7 @@ tools/unit-tests/unit-aes128
 tools/unit-tests/unit-aes256
 tools/unit-tests/unit-chacha20
 tools/unit-tests/unit-delta
+tools/unit-tests/unit-disk
 tools/unit-tests/unit-enc-nvm
 tools/unit-tests/unit-enc-nvm-flagshome
 tools/unit-tests/unit-extflash
@@ -153,7 +154,9 @@ tools/unit-tests/unit-pci
 tools/unit-tests/unit-pkcs11_store
 tools/unit-tests/unit-sectorflags
 tools/unit-tests/unit-spi-flash
+tools/unit-tests/unit-string
 tools/unit-tests/unit-update-flash
+tools/unit-tests/unit-update-flash-enc
 tools/unit-tests/unit-update-ram
 
 

lib/wolfssl

@@ -861,51 +861,74 @@ void RAMFUNCTION wolfBoot_success(void)
  */
 uint16_t wolfBoot_find_header(uint8_t *haystack, uint16_t type, uint8_t **ptr)
 {
-    uint8_t *p = haystack;
+    uint8_t *p;
     uint16_t len, htype;
-    const volatile uint8_t *max_p = (haystack - IMAGE_HEADER_OFFSET) +
-                                                    IMAGE_HEADER_SIZE;
+    uintptr_t p_addr, max_addr;
+
     *ptr = NULL;
-    if (p > max_p) {
+
+    if (haystack == NULL) {
+        unit_dbg("Illegal address (NULL)\n");
+        return 0;
+    }
+
+    p_addr = (uintptr_t)haystack;
+    if (p_addr < IMAGE_HEADER_OFFSET) {
+        unit_dbg("Illegal address (too low)\n");
+        return 0;
+    }
+
+    max_addr = p_addr - IMAGE_HEADER_OFFSET;
+    if (max_addr > (UINTPTR_MAX - IMAGE_HEADER_SIZE)) {
+        unit_dbg("Illegal address (overflow)\n");
+        return 0;
+    }
+    max_addr += IMAGE_HEADER_SIZE;
+
+    if (p_addr > max_addr) {
         unit_dbg("Illegal address (too high)\n");
         return 0;
     }
-    while ((p + 4) < max_p) {
+
+    while (p_addr < max_addr) {
+        if ((max_addr - p_addr) < 4U) {
+            break;
+        }
+        p = (uint8_t *)p_addr;
         htype = p[0] | (p[1] << 8);
         if (htype == 0) {
             unit_dbg("Explicit end of options reached\n");
             break;
         }
         /* skip unaligned half-words and padding bytes */
-        if ((p[0] == HDR_PADDING) || ((((size_t)p) & 0x01) != 0)) {
-            p++;
+        if ((p[0] == HDR_PADDING) || ((p_addr & 0x01U) != 0U)) {
+            p_addr++;
             continue;
         }
 
         len = p[2] | (p[3] << 8);
         /* check len */
-        if ((4 + len) > (uint16_t)(IMAGE_HEADER_SIZE - IMAGE_HEADER_OFFSET)) {
+        if ((4U + len) > (uint16_t)(IMAGE_HEADER_SIZE - IMAGE_HEADER_OFFSET)) {
             unit_dbg("This field is too large (bigger than the space available "
                      "in the current header)\n");
-            unit_dbg("%d %d %d\n", len, IMAGE_HEADER_SIZE, IMAGE_HEADER_OFFSET);
+            unit_dbg("%u %u %u\n", (unsigned int)len,
+                     (unsigned int)IMAGE_HEADER_SIZE,
+                     (unsigned int)IMAGE_HEADER_OFFSET);
             break;
         }
         /* check max pointer */
-        if (p + 4 + len > max_p) {
+        if ((max_addr - p_addr) < (uintptr_t)(4U + len)) {
             unit_dbg("This field is too large and would overflow the image "
                      "header\n");
             break;
         }
 
-        /* skip header [type|len] */
-        p += 4;
-
         if (htype == type) {
             /* found, return pointer to data portion */
-            *ptr = p;
+            *ptr = (uint8_t *)(p_addr + 4U);
             return len;
         }
-        p += len;
+        p_addr += (uintptr_t)(4U + len);
     }
     return 0;
 }

src/libwolfboot.c

@@ -109,54 +109,41 @@ char *strcat(char *dest, const char *src)
 
 int strcmp(const char *s1, const char *s2)
 {
-    int diff = 0;
-
-    while (!diff && *s1) {
-        diff = (int)*s1 - (int)*s2;
-        s1++;
-        s2++;
+    while (*s1 && *s2) {
+        int c1 = ((unsigned char)*s1++);
+        int c2 = ((unsigned char)*s2++);
+        if (c1 != c2)
+            return c1 - c2;
     }
-
-    return diff;
+    return ((unsigned char)*s1) - ((unsigned char)*s2);
 }
 #endif /* Renesas CCRX */
 
 int strcasecmp(const char *s1, const char *s2)
 {
-    int diff = 0;
-
-    while (!diff && *s1) {
-        diff = (int)*s1 - (int)*s2;
-
-        if (((diff == 'A' - 'a') || (diff == 'a' - 'A')) &&
-                (isalpha((unsigned char)*s1) && isalpha((unsigned char)*s2)))
-            diff = 0;
-
-        s1++;
-        s2++;
+    while (*s1 && *s2) {
+        int c1 = tolower((unsigned char)*s1++);
+        int c2 = tolower((unsigned char)*s2++);
+        if (c1 != c2)
+            return c1 - c2;
     }
-
-    return diff;
+    return tolower((unsigned char)*s1) - tolower((unsigned char)*s2);
 }
 
 int strncasecmp(const char *s1, const char *s2, size_t n)
 {
-    int diff = 0;
-    size_t i = 0;
-
-    while (!diff && *s1) {
-        diff = (int)*s1 - (int)*s2;
+    if (n == 0)
+        return 0;
 
-        if (((diff == 'A' - 'a') || (diff == 'a' - 'A')) &&
-                (isalpha((unsigned char)*s1) && isalpha((unsigned char)*s2)))
-            diff = 0;
-
-        s1++;
-        s2++;
-        if (++i >= n)
-            break;
+    while (n--) {
+        int c1 = tolower((unsigned char)*s1++);
+        int c2 = tolower((unsigned char)*s2++);
+        if (c1 != c2)
+            return c1 - c2;
+        if (c1 == '\0')
+            return 0;
     }
-    return diff;
+    return 0;
 }
 
 #if !defined(__CCRX__) /* Renesas CCRX */
@@ -206,7 +193,7 @@ char *strcpy(char *dst, const char *src)
 {
    size_t i = 0;
 
-    while(1) {
+    while (1) {
         dst[i] = src[i];
         if (src[i] == '\0')
             break;

src/string.c

@@ -1146,11 +1146,11 @@ test-all: clean
 test-size-all:
 	make test-size SIGN=NONE LIMIT=5040 NO_ARM_ASM=1
 	make keysclean
-	make test-size SIGN=ED25519 LIMIT=11700 NO_ARM_ASM=1
+	make test-size SIGN=ED25519 LIMIT=11724 NO_ARM_ASM=1
 	make keysclean
 	make test-size SIGN=ECC256  LIMIT=18944 NO_ARM_ASM=1
 	make clean
-	make test-size SIGN=ECC256 NO_ASM=1 LIMIT=13840 NO_ARM_ASM=1
+	make test-size SIGN=ECC256 NO_ASM=1 LIMIT=13856 NO_ARM_ASM=1
 	make keysclean
 	make test-size SIGN=RSA2048 LIMIT=11916 NO_ARM_ASM=1
 	make clean
@@ -1162,21 +1162,21 @@ test-size-all:
 	make keysclean
 	make test-size SIGN=ECC384 LIMIT=19888 NO_ARM_ASM=1
 	make clean
-	make test-size SIGN=ECC384 NO_ASM=1 LIMIT=15216 NO_ARM_ASM=1
+	make test-size SIGN=ECC384 NO_ASM=1 LIMIT=15232 NO_ARM_ASM=1
 	make keysclean
-	make test-size SIGN=ED448 LIMIT=13760 NO_ARM_ASM=1
+	make test-size SIGN=ED448 LIMIT=13776 NO_ARM_ASM=1
 	make keysclean
 	make test-size SIGN=RSA3072 LIMIT=12056 NO_ARM_ASM=1
 	make clean
 	make test-size SIGN=RSA3072 NO_ASM=1 LIMIT=12600 NO_ARM_ASM=1
 	make keysclean
 	make test-size SIGN=LMS LMS_LEVELS=2 LMS_HEIGHT=5 LMS_WINTERNITZ=8 \
 		WOLFBOOT_SMALL_STACK=0 IMAGE_SIGNATURE_SIZE=2644 \
-		IMAGE_HEADER_SIZE?=5288 LIMIT=7696 NO_ARM_ASM=1
+		IMAGE_HEADER_SIZE?=5288 LIMIT=7712 NO_ARM_ASM=1
 	make keysclean
 	make test-size SIGN=XMSS XMSS_PARAMS='XMSS-SHA2_10_256' \
 		IMAGE_SIGNATURE_SIZE=2500 IMAGE_HEADER_SIZE?=4096 \
-		LIMIT=8560 NO_ARM_ASM=1
+		LIMIT=8568 NO_ARM_ASM=1
 	make keysclean
 	make clean
 	make test-size SIGN=ML_DSA ML_DSA_LEVEL=2 LIMIT=19362 \

tools/test.mk

@@ -22,6 +22,12 @@ CFLAGS+=-DUNIT_TEST -DWOLFSSL_USER_SETTINGS
 LDFLAGS+=-fprofile-arcs
 LDFLAGS+=-ftest-coverage
 
+ASAN?=0
+ifeq ($(ASAN),1)
+	CFLAGS+=-fsanitize=address -fno-omit-frame-pointer -O1
+	LDFLAGS+=-fsanitize=address
+endif
+
 
 
 
@@ -74,6 +80,7 @@ unit-update-flash:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TEST_AUTH
 unit-update-ram:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TEST_AUTH \
 	-DWOLFBOOT_HASH_SHA256 -DPRINTF_ENABLED -DEXT_FLASH -DPART_UPDATE_EXT \
 	-DPART_SWAP_EXT -DPART_BOOT_EXT -DWOLFBOOT_DUALBOOT -DNO_XIP
+unit-string:CFLAGS+=-fno-builtin
 
 
 WOLFCRYPT_CFLAGS+=-DWOLFBOOT_SIGN_ECC256 -DWOLFBOOT_SIGN_ECC256 -DHAVE_ECC_KEY_IMPORT -D__WOLFBOOT

tools/unit-tests/Makefile

@@ -92,6 +92,83 @@ START_TEST(test_case_insensitive_alpha_only)
 }
 END_TEST
 
+START_TEST(test_strcasecmp_mixed_alnum_punct)
+{
+    ck_assert_int_eq(strcasecmp("Boot-123_OK!", "bOot-123_ok!"), 0);
+    ck_assert_int_eq(strcasecmp("v1.2.3-rc1", "V1.2.3-RC1"), 0);
+    ck_assert_int_eq(strcasecmp("A_B-C.D", "a_b-c.d"), 0);
+}
+END_TEST
+
+START_TEST(test_strcasecmp_non_alpha_ordering)
+{
+    ck_assert_int_lt(strcasecmp("abc-1", "abc_1"), 0);
+    ck_assert_int_gt(strcasecmp("abc_1", "abc-1"), 0);
+    ck_assert_int_lt(strcasecmp("abc1", "abc2"), 0);
+    ck_assert_int_gt(strcasecmp("abc2", "abc1"), 0);
+}
+END_TEST
+
+START_TEST(test_strncasecmp_non_alpha_n_boundaries)
+{
+    ck_assert_int_eq(strncasecmp("Boot-123_OK!", "bOot-123_ok?", 11), 0);
+    ck_assert_int_lt(strncasecmp("Boot-123_OK!", "bOot-123_ok?", 12), 0);
+    ck_assert_int_gt(strncasecmp("bOot-123_ok?", "Boot-123_OK!", 12), 0);
+    ck_assert_int_eq(strncasecmp("A1.B2-C3", "a1.b2-c3", 8), 0);
+}
+END_TEST
+
+START_TEST(test_strcasecmp_prefix_regression)
+{
+    ck_assert_int_lt(strcasecmp("a", "ab"), 0);
+    ck_assert_int_gt(strcasecmp("ab", "a"), 0);
+    ck_assert_int_lt(strcasecmp("", "a"), 0);
+    ck_assert_int_gt(strcasecmp("a", ""), 0);
+}
+END_TEST
+
+START_TEST(test_strncasecmp_n_limit_regression)
+{
+    ck_assert_int_eq(strncasecmp("ABC", "abc", 0), 0);
+    ck_assert_int_eq(strncasecmp("", "a", 0), 0);
+    ck_assert_int_eq(strncasecmp("AbCd", "aBcE", 3), 0);
+    ck_assert_int_lt(strncasecmp("AbCd", "aBcE", 4), 0);
+}
+END_TEST
+
+START_TEST(test_strncasecmp_stop_at_null_regression)
+{
+    const char s1[] = { 'A', '\0', 'x', '\0' };
+    const char s2[] = { 'a', '\0', 'Y', '\0' };
+
+    ck_assert_int_eq(strncasecmp(s1, s2, 2), 0);
+    ck_assert_int_eq(strncasecmp(s1, s2, 3), 0);
+    ck_assert_int_eq(strncasecmp(s1, s2, 8), 0);
+}
+END_TEST
+
+START_TEST(test_strncasecmp_prefix_large_n_regression)
+{
+    ck_assert_int_lt(strncasecmp("a", "ab", 8), 0);
+    ck_assert_int_gt(strncasecmp("ab", "a", 8), 0);
+    ck_assert_int_lt(strncasecmp("A", "aB", 8), 0);
+}
+END_TEST
+
+START_TEST(test_strncasecmp_no_read_past_null_when_n_remaining)
+{
+    const char s1[] = { '\0' };
+    const char s2[] = { '\0' };
+
+    /*
+     * Regression target: if implementation does not stop on '\0' when n > 1,
+     * the next loop iteration reads past both 1-byte buffers.
+     */
+    ck_assert_int_eq(strncasecmp(s1, s2, 2), 0);
+    ck_assert_int_eq(strncasecmp(s1, s2, 8), 0);
+}
+END_TEST
+
 START_TEST(test_isalpha_helpers)
 {
     ck_assert_int_eq(islower('a'), 1);
@@ -145,6 +222,16 @@ START_TEST(test_strlen_strcmp)
 }
 END_TEST
 
+START_TEST(test_strcmp_prefix_termination)
+{
+    ck_assert_int_lt(strcmp("a", "abc"), 0);
+    ck_assert_int_lt(strcmp("ab", "abc"), 0);
+    ck_assert_int_gt(strcmp("abc", "ab"), 0);
+    ck_assert_int_gt(strcmp("abc", "a"), 0);
+    ck_assert_int_eq(strcmp("", ""), 0);
+}
+END_TEST
+
 START_TEST(test_strcpy_strncpy_strcat_strncat)
 {
     char buf[8];
@@ -329,9 +416,18 @@ Suite *string_suite(void)
     tcase_add_test(tcase_strncasecmp, test_strncasecmp_n_exact);
     tcase_add_test(tcase_strncasecmp, test_strncasecmp_diff_before_n);
     tcase_add_test(tcase_strncasecmp, test_case_insensitive_alpha_only);
+    tcase_add_test(tcase_strncasecmp, test_strcasecmp_mixed_alnum_punct);
+    tcase_add_test(tcase_strncasecmp, test_strcasecmp_non_alpha_ordering);
+    tcase_add_test(tcase_strncasecmp, test_strncasecmp_non_alpha_n_boundaries);
+    tcase_add_test(tcase_strncasecmp, test_strcasecmp_prefix_regression);
+    tcase_add_test(tcase_strncasecmp, test_strncasecmp_n_limit_regression);
+    tcase_add_test(tcase_strncasecmp, test_strncasecmp_stop_at_null_regression);
+    tcase_add_test(tcase_strncasecmp, test_strncasecmp_prefix_large_n_regression);
+    tcase_add_test(tcase_strncasecmp, test_strncasecmp_no_read_past_null_when_n_remaining);
     tcase_add_test(tcase_misc, test_isalpha_helpers);
     tcase_add_test(tcase_misc, test_memset_memcmp_memchr);
     tcase_add_test(tcase_misc, test_strlen_strcmp);
+    tcase_add_test(tcase_misc, test_strcmp_prefix_termination);
     tcase_add_test(tcase_misc, test_strcpy_strncpy_strcat_strncat);
     tcase_add_test(tcase_misc, test_strncmp);
     tcase_add_test(tcase_misc, test_memcpy_memmove);