[PATCH 0/3] net: use strnstr() for lwip_strnstr()
Using strstr() instead of strnstr() creates a security concern.
* Implement missing library function strnstr() and add unit tests. * Use it for lwIP.
Heinrich Schuchardt (3): lib: implement strnstr() test: unit tests for strstr() and strnstr() net: use strnstr() for lwip_strnstr()
include/linux/string.h | 3 +++ lib/lwip/u-boot/arch/cc.h | 2 +- lib/string.c | 49 +++++++++++++++++++++++++++------------ test/lib/string.c | 40 ++++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 16 deletions(-)
Implement library function strnstr(). Implement strstr() using strnstr(). Sort the includes.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- include/linux/string.h | 3 +++ lib/string.c | 49 +++++++++++++++++++++++++++++------------- 2 files changed, 37 insertions(+), 15 deletions(-)
diff --git a/include/linux/string.h b/include/linux/string.h index 27b2beb9ddb..8a377a8176a 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -72,6 +72,9 @@ extern char * strrchr(const char *,int); #ifndef __HAVE_ARCH_STRSTR extern char * strstr(const char *,const char *); #endif +#ifndef __HAVE_ARCH_STRNSTR +extern char *strnstr(const char *, const char *, size_t); +#endif #ifndef __HAVE_ARCH_STRLEN extern __kernel_size_t strlen(const char *); #endif diff --git a/lib/string.c b/lib/string.c index feae9519f2f..d5a486fb89d 100644 --- a/lib/string.c +++ b/lib/string.c @@ -16,11 +16,12 @@ */
#include <config.h> +#include <malloc.h> +#include <stdint.h> #include <linux/compiler.h> +#include <linux/ctype.h> #include <linux/types.h> #include <linux/string.h> -#include <linux/ctype.h> -#include <malloc.h>
/** * strncasecmp - Case insensitive, length-limited string comparison @@ -678,30 +679,48 @@ char *memdup(const void *src, size_t len) return p; }
-#ifndef __HAVE_ARCH_STRSTR +#ifndef __HAVE_ARCH_STRNSTR /** - * strstr - Find the first substring in a %NUL terminated string - * @s1: The string to be searched - * @s2: The string to search for + * strnstr() - find the first substring occurrence in a NUL terminated string + * + * @s1: string to be searched + * @s2: string to search for + * @len: maximum number of characters in s2 to consider + * + * Return: pointer to the first occurrence or NULL */ -char * strstr(const char * s1,const char * s2) +char *strnstr(const char *s1, const char *s2, size_t len) { - int l1, l2; + size_t l1, l2;
+ l1 = strnlen(s1, len); l2 = strlen(s2); - if (!l2) - return (char *) s1; - l1 = strlen(s1); - while (l1 >= l2) { - l1--; - if (!memcmp(s1,s2,l2)) + + for (; l1 >= l2; --l1, ++s1) { + if (!memcmp(s1, s2, l2)) return (char *) s1; - s1++; } + return NULL; } #endif
+#ifndef __HAVE_ARCH_STRSTR +/** + * strstr() - find the first substring occurrence in a NUL terminated string + * + * @s1: string to be searched + * @s2: string to search for + * @len: maximum number of characters in s2 to consider + * + * Return: pointer to the first occurrence or NULL + */ +char *strstr(const char *s1, const char *s2) +{ + return strnstr(s1, s2, SIZE_MAX); +} +#endif + #ifndef __HAVE_ARCH_MEMCHR /** * memchr - Find a character in an area of memory.
On 1/4/25 00:21, Heinrich Schuchardt wrote:
Implement library function strnstr(). Implement strstr() using strnstr(). Sort the includes.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
include/linux/string.h | 3 +++ lib/string.c | 49 +++++++++++++++++++++++++++++------------- 2 files changed, 37 insertions(+), 15 deletions(-)
diff --git a/include/linux/string.h b/include/linux/string.h index 27b2beb9ddb..8a377a8176a 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -72,6 +72,9 @@ extern char * strrchr(const char *,int); #ifndef __HAVE_ARCH_STRSTR extern char * strstr(const char *,const char *); #endif +#ifndef __HAVE_ARCH_STRNSTR +extern char *strnstr(const char *, const char *, size_t); +#endif #ifndef __HAVE_ARCH_STRLEN extern __kernel_size_t strlen(const char *); #endif diff --git a/lib/string.c b/lib/string.c index feae9519f2f..d5a486fb89d 100644 --- a/lib/string.c +++ b/lib/string.c @@ -16,11 +16,12 @@ */
#include <config.h> +#include <malloc.h> +#include <stdint.h> #include <linux/compiler.h> +#include <linux/ctype.h> #include <linux/types.h> #include <linux/string.h> -#include <linux/ctype.h> -#include <malloc.h>
/**
- strncasecmp - Case insensitive, length-limited string comparison
@@ -678,30 +679,48 @@ char *memdup(const void *src, size_t len) return p; }
-#ifndef __HAVE_ARCH_STRSTR +#ifndef __HAVE_ARCH_STRNSTR /**
- strstr - Find the first substring in a %NUL terminated string
- @s1: The string to be searched
- @s2: The string to search for
- strnstr() - find the first substring occurrence in a NUL terminated string
- @s1: string to be searched
- @s2: string to search for
- @len: maximum number of characters in s2 to consider
You mean s1?
*/
- Return: pointer to the first occurrence or NULL
-char * strstr(const char * s1,const char * s2) +char *strnstr(const char *s1, const char *s2, size_t len) {
- int l1, l2;
size_t l1, l2;
l1 = strnlen(s1, len); l2 = strlen(s2);
- if (!l2)
return (char *) s1;- l1 = strlen(s1);
- while (l1 >= l2) {
l1--;if (!memcmp(s1,s2,l2))
- for (; l1 >= l2; --l1, ++s1) {
if (!memcmp(s1, s2, l2)) return (char *) s1;
s1++;
- return NULL;
} #endif
This won't return s1 when s2 == NULL, will it? Why not use a known good implementation such as [1]?
[1] https://github.com/freebsd/freebsd-src/blob/main/lib/libc/string/strnstr.c
+#ifndef __HAVE_ARCH_STRSTR +/**
- strstr() - find the first substring occurrence in a NUL terminated string
- @s1: string to be searched
- @s2: string to search for
- @len: maximum number of characters in s2 to consider
There is no len parameter
- Return: pointer to the first occurrence or NULL
- */
+char *strstr(const char *s1, const char *s2) +{
- return strnstr(s1, s2, SIZE_MAX);
+} +#endif
#ifndef __HAVE_ARCH_MEMCHR /**
- memchr - Find a character in an area of memory.
Thanks,
On Mon, Jan 06 2025, Jerome Forissier jerome.forissier@linaro.org wrote:
*/
- Return: pointer to the first occurrence or NULL
-char * strstr(const char * s1,const char * s2) +char *strnstr(const char *s1, const char *s2, size_t len) {
- int l1, l2;
size_t l1, l2;
l1 = strnlen(s1, len); l2 = strlen(s2);
- if (!l2)
return (char *) s1;- l1 = strlen(s1);
- while (l1 >= l2) {
l1--;if (!memcmp(s1,s2,l2))
- for (; l1 >= l2; --l1, ++s1) {
if (!memcmp(s1, s2, l2)) return (char *) s1;
s1++;
- return NULL;
} #endif
This won't return s1 when s2 == NULL, will it?
Why should it? That's a broken caller. You can't call str* functions with NULL pointers. It's ok for s2 to point at an empty string, in which case l2 is 0, so the very first memcmp() is guaranteed to succeed and yes, this will return s1 in that case.
As passing "" as s2 is likely quite rare, the current short-circuiting of that case is pointless.
Why not use a known good implementation such as [1]?
[1] https://github.com/freebsd/freebsd-src/blob/main/lib/libc/string/strnstr.c
Licensing? Also, that's quite unreadable, so please don't.
Rasmus
On 1/6/25 11:14, Rasmus Villemoes wrote:
On Mon, Jan 06 2025, Jerome Forissier jerome.forissier@linaro.org wrote:
*/
- Return: pointer to the first occurrence or NULL
-char * strstr(const char * s1,const char * s2) +char *strnstr(const char *s1, const char *s2, size_t len) {
- int l1, l2;
size_t l1, l2;
l1 = strnlen(s1, len); l2 = strlen(s2);
- if (!l2)
return (char *) s1;- l1 = strlen(s1);
- while (l1 >= l2) {
l1--;if (!memcmp(s1,s2,l2))
- for (; l1 >= l2; --l1, ++s1) {
if (!memcmp(s1, s2, l2)) return (char *) s1;
s1++;
- return NULL;
} #endif
This won't return s1 when s2 == NULL, will it?
Why should it? That's a broken caller. You can't call str* functions with NULL pointers. It's ok for s2 to point at an empty string, in which case l2 is 0, so the very first memcmp() is guaranteed to succeed and yes, this will return s1 in that case.
Sorry I read the strncmp() man page too quickly and mistook empty string for NULL :-/
As passing "" as s2 is likely quite rare, the current short-circuiting of that case is pointless.
Agreed.
Why not use a known good implementation such as [1]?
[1] https://github.com/freebsd/freebsd-src/blob/main/lib/libc/string/strnstr.c
Licensing?
Isn't BSD compatible with GPL?
Also, that's quite unreadable, so please don't.
OK.
Rasmus
Add unit tests for the library functions.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- lib/string.c | 2 +- test/lib/string.c | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/lib/string.c b/lib/string.c index d5a486fb89d..66aae583940 100644 --- a/lib/string.c +++ b/lib/string.c @@ -16,8 +16,8 @@ */
#include <config.h> +#include <limits.h> #include <malloc.h> -#include <stdint.h> #include <linux/compiler.h> #include <linux/ctype.h> #include <linux/types.h> diff --git a/test/lib/string.c b/test/lib/string.c index 8d22f3fd68f..31391a387b9 100644 --- a/test/lib/string.c +++ b/test/lib/string.c @@ -11,6 +11,7 @@
#include <command.h> #include <log.h> +#include <string.h> #include <test/lib.h> #include <test/test.h> #include <test/ut.h> @@ -221,3 +222,42 @@ static int lib_memdup(struct unit_test_state *uts) return 0; } LIB_TEST(lib_memdup, 0); + +/** lib_strnstr() - unit test for strnstr() */ +static int lib_strnstr(struct unit_test_state *uts) +{ + const char *s1 = "Itsy Bitsy Teenie Weenie"; + const char *s2 = "eenie"; + const char *s3 = "eery"; + + ut_asserteq_ptr(&s1[12], strnstr(s1, s2, SIZE_MAX)); + ut_asserteq_ptr(&s1[12], strnstr(s1, s2, 17)); + ut_assertnull(strnstr(s1, s2, 16)); + ut_assertnull(strnstr(s1, s2, 0)); + ut_asserteq_ptr(&s1[13], strnstr(&s1[3], &s2[1], SIZE_MAX)); + ut_asserteq_ptr(&s1[13], strnstr(&s1[3], &s2[1], 14)); + ut_assertnull(strnstr(&s1[3], &s2[1], 13)); + ut_assertnull(strnstr(&s1[3], &s2[1], 0)); + ut_assertnull(strnstr(s1, s3, SIZE_MAX)); + ut_assertnull(strnstr(s1, s3, 0)); + + return 0; +} +LIB_TEST(lib_strnstr, 0); + +/** lib_strstr() - unit test for strstr() */ +static int lib_strstr(struct unit_test_state *uts) +{ + const char *s1 = "Itsy Bitsy Teenie Weenie"; + const char *s2 = "eenie"; + const char *s3 = "easy"; + + ut_asserteq_ptr(&s1[12], strstr(s1, s2)); + ut_asserteq_ptr(&s1[13], strstr(&s1[3], &s2[1])); + ut_assertnull(strstr(s1, s3)); + ut_asserteq_ptr(&s1[2], strstr(s1, &s3[2])); + ut_asserteq_ptr(&s1[8], strstr(&s1[5], &s3[2])); + + return 0; +} +LIB_TEST(lib_strstr, 0);
On Sat, 4 Jan 2025 at 01:21, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Add unit tests for the library functions.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
lib/string.c | 2 +- test/lib/string.c | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/lib/string.c b/lib/string.c index d5a486fb89d..66aae583940 100644 --- a/lib/string.c +++ b/lib/string.c @@ -16,8 +16,8 @@ */
#include <config.h> +#include <limits.h> #include <malloc.h> -#include <stdint.h> #include <linux/compiler.h> #include <linux/ctype.h> #include <linux/types.h> diff --git a/test/lib/string.c b/test/lib/string.c index 8d22f3fd68f..31391a387b9 100644 --- a/test/lib/string.c +++ b/test/lib/string.c @@ -11,6 +11,7 @@
#include <command.h> #include <log.h> +#include <string.h> #include <test/lib.h> #include <test/test.h> #include <test/ut.h> @@ -221,3 +222,42 @@ static int lib_memdup(struct unit_test_state *uts) return 0; } LIB_TEST(lib_memdup, 0);
+/** lib_strnstr() - unit test for strnstr() */ +static int lib_strnstr(struct unit_test_state *uts) +{
const char *s1 = "Itsy Bitsy Teenie Weenie";const char *s2 = "eenie";const char *s3 = "eery";ut_asserteq_ptr(&s1[12], strnstr(s1, s2, SIZE_MAX));ut_asserteq_ptr(&s1[12], strnstr(s1, s2, 17));ut_assertnull(strnstr(s1, s2, 16));ut_assertnull(strnstr(s1, s2, 0));ut_asserteq_ptr(&s1[13], strnstr(&s1[3], &s2[1], SIZE_MAX));ut_asserteq_ptr(&s1[13], strnstr(&s1[3], &s2[1], 14));ut_assertnull(strnstr(&s1[3], &s2[1], 13));ut_assertnull(strnstr(&s1[3], &s2[1], 0));ut_assertnull(strnstr(s1, s3, SIZE_MAX));ut_assertnull(strnstr(s1, s3, 0));return 0;+} +LIB_TEST(lib_strnstr, 0);
+/** lib_strstr() - unit test for strstr() */ +static int lib_strstr(struct unit_test_state *uts) +{
const char *s1 = "Itsy Bitsy Teenie Weenie";const char *s2 = "eenie";const char *s3 = "easy";ut_asserteq_ptr(&s1[12], strstr(s1, s2));ut_asserteq_ptr(&s1[13], strstr(&s1[3], &s2[1]));ut_assertnull(strstr(s1, s3));ut_asserteq_ptr(&s1[2], strstr(s1, &s3[2]));ut_asserteq_ptr(&s1[8], strstr(&s1[5], &s3[2]));return 0;+}
+LIB_TEST(lib_strstr, 0);
2.47.1
Acked-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Using strstr() instead of strnstr() creates a security concern.
Fixes: 1c41a7afaa15 ("net: lwip: build lwIP") Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- lib/lwip/u-boot/arch/cc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/lwip/u-boot/arch/cc.h b/lib/lwip/u-boot/arch/cc.h index de138846358..6104c296f6f 100644 --- a/lib/lwip/u-boot/arch/cc.h +++ b/lib/lwip/u-boot/arch/cc.h @@ -34,7 +34,7 @@ x, __LINE__, __FILE__); } while (0)
#define atoi(str) (int)dectoul(str, NULL) -#define lwip_strnstr(a, b, c) strstr(a, b) +#define lwip_strnstr(a, b, c) strnstr(a, b, c)
#define LWIP_ERR_T int #define LWIP_CONST_CAST(target_type, val) ((target_type)((uintptr_t)val))
On 1/4/25 00:21, Heinrich Schuchardt wrote:
Using strstr() instead of strnstr() creates a security concern.
Fixes: 1c41a7afaa15 ("net: lwip: build lwIP") Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
lib/lwip/u-boot/arch/cc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/lwip/u-boot/arch/cc.h b/lib/lwip/u-boot/arch/cc.h index de138846358..6104c296f6f 100644 --- a/lib/lwip/u-boot/arch/cc.h +++ b/lib/lwip/u-boot/arch/cc.h @@ -34,7 +34,7 @@ x, __LINE__, __FILE__); } while (0)
#define atoi(str) (int)dectoul(str, NULL) -#define lwip_strnstr(a, b, c) strstr(a, b) +#define lwip_strnstr(a, b, c) strnstr(a, b, c)
#define LWIP_ERR_T int #define LWIP_CONST_CAST(target_type, val) ((target_type)((uintptr_t)val))
Reviewed-by: Jerome Forissier jerome.forissier@linaro.org
On Sat, Jan 04, 2025 at 12:21:18AM +0100, Heinrich Schuchardt wrote:
Using strstr() instead of strnstr() creates a security concern.
Fixes: 1c41a7afaa15 ("net: lwip: build lwIP") Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Reviewed-by: Jerome Forissier jerome.forissier@linaro.org
lib/lwip/u-boot/arch/cc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/lwip/u-boot/arch/cc.h b/lib/lwip/u-boot/arch/cc.h index de138846358..6104c296f6f 100644 --- a/lib/lwip/u-boot/arch/cc.h +++ b/lib/lwip/u-boot/arch/cc.h @@ -34,7 +34,7 @@ x, __LINE__, __FILE__); } while (0)
#define atoi(str) (int)dectoul(str, NULL) -#define lwip_strnstr(a, b, c) strstr(a, b) +#define lwip_strnstr(a, b, c) strnstr(a, b, c)
#define LWIP_ERR_T int #define LWIP_CONST_CAST(target_type, val) ((target_type)((uintptr_t)val))
This leads to: https://dev.azure.com/u-boot/u-boot/_build/results?buildId=10370&view=lo... as a failure, that I only end up seeing in Azure (I didn't track down if there's some good reason we don't see this in Gitlab).
On 21.01.25 02:05, Tom Rini wrote:
On Sat, Jan 04, 2025 at 12:21:18AM +0100, Heinrich Schuchardt wrote:
Using strstr() instead of strnstr() creates a security concern.
Fixes: 1c41a7afaa15 ("net: lwip: build lwIP") Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Reviewed-by: Jerome Forissier jerome.forissier@linaro.org
lib/lwip/u-boot/arch/cc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/lwip/u-boot/arch/cc.h b/lib/lwip/u-boot/arch/cc.h index de138846358..6104c296f6f 100644 --- a/lib/lwip/u-boot/arch/cc.h +++ b/lib/lwip/u-boot/arch/cc.h @@ -34,7 +34,7 @@ x, __LINE__, __FILE__); } while (0)
#define atoi(str) (int)dectoul(str, NULL) -#define lwip_strnstr(a, b, c) strstr(a, b) +#define lwip_strnstr(a, b, c) strnstr(a, b, c)
#define LWIP_ERR_T int #define LWIP_CONST_CAST(target_type, val) ((target_type)((uintptr_t)val))
This leads to: https://dev.azure.com/u-boot/u-boot/_build/results?buildId=10370&view=lo... as a failure, that I only end up seeing in Azure (I didn't track down if there's some good reason we don't see this in Gitlab).
Hello Tom,
It is not really clear how
Lab failure: Timeout executing 'tftpboot 40400000 u-boot.bin
could be related to this patch series.
git grep -ni strstr net/
only shows usage in wget but not in tftp.
Is this test result reproducible in Azure?
Best regards
Heinrich
On Tue, Jan 21, 2025 at 03:18:11AM +0100, Heinrich Schuchardt wrote:
On 21.01.25 02:05, Tom Rini wrote:
On Sat, Jan 04, 2025 at 12:21:18AM +0100, Heinrich Schuchardt wrote:
Using strstr() instead of strnstr() creates a security concern.
Fixes: 1c41a7afaa15 ("net: lwip: build lwIP") Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Reviewed-by: Jerome Forissier jerome.forissier@linaro.org
lib/lwip/u-boot/arch/cc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/lwip/u-boot/arch/cc.h b/lib/lwip/u-boot/arch/cc.h index de138846358..6104c296f6f 100644 --- a/lib/lwip/u-boot/arch/cc.h +++ b/lib/lwip/u-boot/arch/cc.h @@ -34,7 +34,7 @@ x, __LINE__, __FILE__); } while (0) #define atoi(str) (int)dectoul(str, NULL) -#define lwip_strnstr(a, b, c) strstr(a, b) +#define lwip_strnstr(a, b, c) strnstr(a, b, c) #define LWIP_ERR_T int #define LWIP_CONST_CAST(target_type, val) ((target_type)((uintptr_t)val))
This leads to: https://dev.azure.com/u-boot/u-boot/_build/results?buildId=10370&view=lo... as a failure, that I only end up seeing in Azure (I didn't track down if there's some good reason we don't see this in Gitlab).
Hello Tom,
It is not really clear how
Lab failure: Timeout executing 'tftpboot 40400000 u-boot.bincould be related to this patch series.
git grep -ni strstr net/only shows usage in wget but not in tftp.
Agreed.
Is this test result reproducible in Azure?
Yes, every time. And 1/3 + 2/3 pass, with 3/3 is when it fails.
On Sat, 04 Jan 2025 00:21:15 +0100, Heinrich Schuchardt wrote:
Using strstr() instead of strnstr() creates a security concern.
- Implement missing library function strnstr() and add unit tests.
- Use it for lwIP.
Heinrich Schuchardt (3): lib: implement strnstr() test: unit tests for strstr() and strnstr() net: use strnstr() for lwip_strnstr()
[...]
Applied to u-boot/master, thanks!
On Mon, Jan 20, 2025 at 02:52:24PM -0600, Tom Rini wrote:
On Sat, 04 Jan 2025 00:21:15 +0100, Heinrich Schuchardt wrote:
Using strstr() instead of strnstr() creates a security concern.
- Implement missing library function strnstr() and add unit tests.
- Use it for lwIP.
Heinrich Schuchardt (3): lib: implement strnstr() test: unit tests for strstr() and strnstr() net: use strnstr() for lwip_strnstr()
[...]
Applied to u-boot/master, thanks!
This was premature, sorry, I need to re-test this in Azure before pushing.
participants (5)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Jerome Forissier -
Rasmus Villemoes -
Tom Rini