lib: check for strcpy() overflows to fixed length buffers

PATCH In review Dan Carpenter 5 participants 10 messages Apr 30, 2014 – May 6, 2014

Subsystems (5)

ACPI Library code ACPI component architecture (acpica) Sparse checker +1 more subsystems

lore

Share & tools

lore

Dan Carpenter

author

2014-04-30 15:08 UTC #0 lore raw

PATCH 1/1 lib: check for strcpy() overflows to fixed length buffers

There are sometimes where we know that we are doing an strcpy() into a fixed length buffer. In those cases, we could verify that the strcpy() doesn't overflow. This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS if you want to check for that. The downside is that it makes strcpy slower.

I introduced __compiletime_size() to make this work. It returns the size of the destination buffer or zero if the size isn't known. The __compiletime_object_size() is similar but if you pass it a struct member then it returns the size of everything from there to the end of the struct. Another difference is __compiletime_object_size() returns -1 for unknown sizes.

If you pass a char pointer to __compiletime_size() then it returns zero.

The strcpy() check ignores buffers with just one byte because people often use those for variable length strings at the end of a struct.

I have tested this patch lightly and created some bugs for it to detect but I have not detected any real life overflows.

Signed-off-by Dan Carpenter <[email protected]>

include/acpi/platform/acenv.h +1 -1

index e863dd5..5e0fc2b 100644
@@ -320,7 +320,7 @@
 #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
 #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
 #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
-#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
+#define ACPI_STRCPY(d,s)        strcpy((d), (s))
 #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
 #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
 #define ACPI_STRCMP(d,s)        strcmp((d), (s))

include/linux/compiler-gcc4.h +3 -0

index 2507fd2..1fb7fd0 100644
@@ -16,6 +16,9 @@
 #if GCC_VERSION >= 40100 && GCC_VERSION < 40600
 # define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
 #endif
+#if GCC_VERSION > 40600
+# define __compiletime_size(obj) __builtin_object_size(obj, 3)
+#endif
 
 #if GCC_VERSION >= 40300
 /* Mark functions as cold. gcc will assume any path leading to a call

include/linux/compiler.h +3 -0

index ee7239e..b615964 100644
@@ -318,6 +318,9 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 #ifndef __compiletime_object_size
 # define __compiletime_object_size(obj) -1
 #endif
+#ifndef __compiletime_size
+# define __compiletime_size(obj) 0
+#endif
 #ifndef __compiletime_warning
 # define __compiletime_warning(message)
 #endif

include/linux/string.h +9 -0

index ac889c5..fc126a1 100644
@@ -154,4 +154,13 @@ static inline const char *kbasename(const char *path)
 	return tail ? tail + 1 : path;
 }
 
+#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
+#define strcpy(dest, src) do {						\
+	int len = __compiletime_size(dest);				\
+	if (len > 1 && strlen(src) >= len)				\
+		WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);	\
+	strcpy(dest, src);						\
+} while (0)
+#endif
+
 #endif /* _LINUX_STRING_H_ */

lib/Kconfig.debug +9 -0

index 819ac51..94db086 100644
@@ -1431,6 +1431,15 @@ config DEBUG_STRICT_USER_COPY_CHECKS
 
 	  If unsure, say N.
 
+config DEBUG_STRICT_SLOW_STRCPY_CHECKS
+	bool "Strict checks for strcpy() overflows"
+	depends on DEBUG_KERNEL
+	help
+	  Enabling this option adds some extra sanity checks when strcpy() is
+	  called().  This will slow down the kernel a bit.
+
+	  If unsure, say N.
+
 source kernel/trace/Kconfig
 
 menu "Runtime Testing"

Kees Cook

2014-04-30 15:33 UTC #1 lore raw

Quoted 13 lines · Dan Carpenter

There are sometimes where we know that we are doing an strcpy() into a fixed length buffer. In those cases, we could verify that the strcpy() doesn't overflow. This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS if you want to check for that. The downside is that it makes strcpy slower.
I introduced __compiletime_size() to make this work. It returns the size of the destination buffer or zero if the size isn't known. The __compiletime_object_size() is similar but if you pass it a struct member then it returns the size of everything from there to the end of the struct. Another difference is __compiletime_object_size() returns -1 for unknown sizes.

Quoted 7 lines

If you pass a char pointer to __compiletime_size() then it returns zero.
The strcpy() check ignores buffers with just one byte because people often use those for variable length strings at the end of a struct.
I have tested this patch lightly and created some bugs for it to detect but I have not detected any real life overflows.

Cool, we should absolutely have this. And that's good news that it didn't trip anything. :)

Quoted 49 lines

Signed-off-by Dan Carpenter <[email protected]>

include/acpi/platform/acenv.h +1 -1

index e863dd5..5e0fc2b 100644
@@ -320,7 +320,7 @@
 #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
 #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
 #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
-#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
+#define ACPI_STRCPY(d,s)        strcpy((d), (s))
 #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
 #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
 #define ACPI_STRCMP(d,s)        strcmp((d), (s))

include/linux/compiler-gcc4.h +3 -0

index 2507fd2..1fb7fd0 100644
@@ -16,6 +16,9 @@
 #if GCC_VERSION >= 40100 && GCC_VERSION < 40600
 # define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
 #endif
+#if GCC_VERSION > 40600
+# define __compiletime_size(obj) __builtin_object_size(obj, 3)
+#endif
 
 #if GCC_VERSION >= 40300
 /* Mark functions as cold. gcc will assume any path leading to a call

include/linux/compiler.h +3 -0

index ee7239e..b615964 100644
@@ -318,6 +318,9 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 #ifndef __compiletime_object_size
 # define __compiletime_object_size(obj) -1
 #endif
+#ifndef __compiletime_size
+# define __compiletime_size(obj) 0
+#endif
 #ifndef __compiletime_warning
 # define __compiletime_warning(message)
 #endif

include/linux/string.h +0 -0

index ac889c5..fc126a1 100644

include/linux/string.h static inline const char *kbasename(const char *path) · line 161

Quoted 8 lines

include/linux/string.h +5 -0

@@ -154,4 +154,13 @@ static inline const char *kbasename(const char *path)
        return tail ? tail + 1 : path;
 }
 
+#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
+#define strcpy(dest, src) do {                                         \
+       int len = __compiletime_size(dest);                             \
+       if (len > 1 && strlen(src) >= len)                              \
+               WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);        \

This should probably BUG. An overflowing strcpy shouldn't be allowed to continue.

Quoted 10 lines

include/linux/string.h +4 -0

+       strcpy(dest, src);                                              \
+} while (0)
+#endif
+
 #endif /* _LINUX_STRING_H_ */

lib/Kconfig.debug +0 -0

index 819ac51..94db086 100644

lib/Kconfig.debug config DEBUG_STRICT_USER_COPY_CHECKS · line 1439

Quoted 9 lines

lib/Kconfig.debug +6 -0

@@ -1431,6 +1431,15 @@ config DEBUG_STRICT_USER_COPY_CHECKS
 
          If unsure, say N.
 
+config DEBUG_STRICT_SLOW_STRCPY_CHECKS
+       bool "Strict checks for strcpy() overflows"
+       depends on DEBUG_KERNEL
+       help
+         Enabling this option adds some extra sanity checks when strcpy() is
+         called().  This will slow down the kernel a bit.

Isn't this an entirely compile-time check? I would expect it to be entirely optimized by the compiler. In fact, could this be turned into a build failure instead?

lib/Kconfig.debug config DEBUG_STRICT_USER_COPY_CHECKS · line 1445

Quoted 6 lines

lib/Kconfig.debug +3 -0

+
+         If unsure, say N.
+
 source kernel/trace/Kconfig
 
 menu "Runtime Testing"

-Kees

Signature · 2 lines

Kees Cook
Chrome OS Security

Dan Carpenter

author

2014-04-30 16:19 UTC #2 lore raw

Quoted 9 lines · Kees Cook

include/linux/string.h static inline const char *kbasename(const char *path) · line 161
Quoted 5 lines · Dan Carpenter
include/linux/string.h +5 -0
+#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
+#define strcpy(dest, src) do {                                         \
+       int len = __compiletime_size(dest);                             \
+       if (len > 1 && strlen(src) >= len)                              \
+               WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);        \
This should probably BUG. An overflowing strcpy shouldn't be allowed to continue.

I was worried about false positives.

Speaking of false positives the STRICT checks on copy_from_user() have been disabled for a year now because of a three year old GCC bug. I wonder if the GCC people realize the security impact it has. See commit 2fb0815c9ee6 ('gcc4: disable __compiletime_object_size for GCC 4.6+') and http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48880

Quoted 10 lines

lib/Kconfig.debug config DEBUG_STRICT_USER_COPY_CHECKS · line 1439
Quoted 6 lines
lib/Kconfig.debug +6 -0
+config DEBUG_STRICT_SLOW_STRCPY_CHECKS
+       bool "Strict checks for strcpy() overflows"
+       depends on DEBUG_KERNEL
+       help
+         Enabling this option adds some extra sanity checks when strcpy() is
+         called().  This will slow down the kernel a bit.
Isn't this an entirely compile-time check? I would expect it to be entirely optimized by the compiler. In fact, could this be turned into a build failure instead?

No. The problem is when we don't know the size of the src string. Also if GCC is able to find the compile time size of both the src and dest string then Smatch and other static checkers are able to as well so I'm not very concerned about that case because we already catch them.

regards,
dan carpenter

Kees Cook

2014-04-30 16:44 UTC #3 lore raw

Quoted 11 lines · Dan Carpenter

Quoted 9 lines · Kees Cook
include/linux/string.h static inline const char *kbasename(const char *path) · line 161
Quoted 5 lines · Dan Carpenter
include/linux/string.h +5 -0
+#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
+#define strcpy(dest, src) do {                                         \
+       int len = __compiletime_size(dest);                             \
+       if (len > 1 && strlen(src) >= len)                              \
+               WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);        \
This should probably BUG. An overflowing strcpy shouldn't be allowed to continue.

Quoted 1 line

I was worried about false positives.

Sure, good to be initially cautious but I think if this goes in, it should BUG.

Quoted 5 lines

Speaking of false positives the STRICT checks on copy_from_user() have been disabled for a year now because of a three year old GCC bug. I wonder if the GCC people realize the security impact it has. See commit 2fb0815c9ee6 ('gcc4: disable __compiletime_object_size for GCC 4.6+') and http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48880

Yeah, lots of badness here. I'll see if I can find someone to look at solutions for this.

Quoted 8 lines

Quoted 7 lines

lib/Kconfig.debug +6 -0

+config DEBUG_STRICT_SLOW_STRCPY_CHECKS
+       bool "Strict checks for strcpy() overflows"
+       depends on DEBUG_KERNEL
+       help
+         Enabling this option adds some extra sanity checks when strcpy() is
+         called().  This will slow down the kernel a bit.

Quoted 8 lines

Quoted 3 lines
Isn't this an entirely compile-time check? I would expect it to be entirely optimized by the compiler. In fact, could this be turned into a build failure instead?
No. The problem is when we don't know the size of the src string. Also if GCC is able to find the compile time size of both the src and dest string then Smatch and other static checkers are able to as well so I'm not very concerned about that case because we already catch them.

Ah, right, the source. But that shouldn't make it "slow". How about naming this DEBUG_STRICT_STRCPY_SIZE_CHECKS or something? I can't imagine the performance from adding a single compare to be bad. You can even branch-hint it with "if (unlikely(...."

-Kees

Signature · 2 lines

Kees Cook
Chrome OS Security

Rafael J. Wysocki

2014-04-30 19:32 UTC #4 lore raw

In reply to Dan Carpenter

include/acpi/platform/acenv.h

Quoted 27 lines · Dan Carpenter

There are sometimes where we know that we are doing an strcpy() into a fixed length buffer. In those cases, we could verify that the strcpy() doesn't overflow. This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS if you want to check for that. The downside is that it makes strcpy slower.
I introduced __compiletime_size() to make this work. It returns the size of the destination buffer or zero if the size isn't known. The __compiletime_object_size() is similar but if you pass it a struct member then it returns the size of everything from there to the end of the struct. Another difference is __compiletime_object_size() returns -1 for unknown sizes.
If you pass a char pointer to __compiletime_size() then it returns zero.
The strcpy() check ignores buffers with just one byte because people often use those for variable length strings at the end of a struct.
I have tested this patch lightly and created some bugs for it to detect but I have not detected any real life overflows.
Signed-off-by Dan Carpenter <[email protected]>
include/acpi/platform/acenv.h +0 -0
index e863dd5..5e0fc2b 100644

This is an ACPICA header and changes to it need to be submitted to the ACPICA maintainers (as per MAINTAINERS). We only get ACPICA changes from the upstream project (except for really special situations).

include/acpi/platform/acenv.h line 326

Quoted 9 lines

include/acpi/platform/acenv.h +1 -1

@@ -320,7 +320,7 @@
 #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
 #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
 #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
-#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
+#define ACPI_STRCPY(d,s)        strcpy((d), (s))
 #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
 #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
 #define ACPI_STRCMP(d,s)        strcmp((d), (s))

Thanks!

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Dan Carpenter

author

2014-04-30 20:15 UTC #5 lore raw

Quoted 24 lines · Rafael J. Wysocki

Quoted 23 lines · Dan Carpenter
There are sometimes where we know that we are doing an strcpy() into a fixed length buffer. In those cases, we could verify that the strcpy() doesn't overflow. This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS if you want to check for that. The downside is that it makes strcpy slower.
I introduced __compiletime_size() to make this work. It returns the size of the destination buffer or zero if the size isn't known. The __compiletime_object_size() is similar but if you pass it a struct member then it returns the size of everything from there to the end of the struct. Another difference is __compiletime_object_size() returns -1 for unknown sizes.
If you pass a char pointer to __compiletime_size() then it returns zero.
The strcpy() check ignores buffers with just one byte because people often use those for variable length strings at the end of a struct.
I have tested this patch lightly and created some bugs for it to detect but I have not detected any real life overflows.
Signed-off-by Dan Carpenter <[email protected]>

Quoted 8 lines

include/acpi/platform/acenv.h
Quoted 4 lines
include/acpi/platform/acenv.h +0 -0
index e863dd5..5e0fc2b 100644
This is an ACPICA header and changes to it need to be submitted to the ACPICA maintainers (as per MAINTAINERS). We only get ACPICA changes from the upstream project (except for really special situations).

Ok. I should have added Robert and Lv to the CC list. My guess is that the (void) is needed to avoid compile warnings but it's needed for us to avoid compile breakage with this change.

Anyway, I'll wait for a couple days and resend that bit broken out.

regards,
dan carpenter

Quoted 16 lines

include/acpi/platform/acenv.h line 326

Quoted 10 lines

include/acpi/platform/acenv.h +1 -1

@@ -320,7 +320,7 @@
 #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
 #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
 #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
-#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
+#define ACPI_STRCPY(d,s)        strcpy((d), (s))
 #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
 #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
 #define ACPI_STRCMP(d,s)        strcmp((d), (s))

Thanks!

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Zheng, Lv

2014-05-05 00:19 UTC #6 lore raw

Hi,

Quoted 37 lines

From: Dan Carpenter [mailto:dan.carpenter(a)oracle.com]
Sent: Thursday, May 01, 2014 4:15 AM
Quoted 24 lines · Rafael J. Wysocki
Quoted 23 lines · Dan Carpenter
There are sometimes where we know that we are doing an strcpy() into a fixed length buffer. In those cases, we could verify that the strcpy() doesn't overflow. This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS if you want to check for that. The downside is that it makes strcpy slower.
I introduced __compiletime_size() to make this work. It returns the size of the destination buffer or zero if the size isn't known. The __compiletime_object_size() is similar but if you pass it a struct member then it returns the size of everything from there to the end of the struct. Another difference is __compiletime_object_size() returns -1 for unknown sizes.
If you pass a char pointer to __compiletime_size() then it returns zero.
The strcpy() check ignores buffers with just one byte because people often use those for variable length strings at the end of a struct.
I have tested this patch lightly and created some bugs for it to detect but I have not detected any real life overflows.
Signed-off-by Dan Carpenter <dan.carpenter(a)oracle.com>
Quoted 8 lines
include/acpi/platform/acenv.h
Quoted 4 lines
include/acpi/platform/acenv.h +0 -0
index e863dd5..5e0fc2b 100644
This is an ACPICA header and changes to it need to be submitted to the ACPICA maintainers (as per MAINTAINERS). We only get ACPICA changes from the upstream project (except for really special situations).

Quoted 3 lines

Ok. I should have added Robert and Lv to the CC list. My guess is that the (void) is needed to avoid compile warnings but it's needed for us to avoid compile breakage with this change.

In normal ACPICA build environment, I didn't suffer from new build errors after deleting this "void". But this might be required by lint users.
You can split ACPICA changes into a separate patch so that it could be easily reverted if someone complained.

Thanks
-Lv

Quoted 22 lines

Anyway, I'll wait for a couple days and resend that bit broken out.
regards,
dan carpenter
Quoted 16 lines
include/acpi/platform/acenv.h line 326
Quoted 10 lines
include/acpi/platform/acenv.h +1 -1
@@ -320,7 +320,7 @@
 #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
 #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
 #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
-#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
+#define ACPI_STRCPY(d,s)        strcpy((d), (s))
 #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
 #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
 #define ACPI_STRCMP(d,s)        strcmp((d), (s))
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Dan Carpenter

author

2014-05-06 12:41 UTC #7 lore raw

In reply to Dan Carpenter

Quoted 9 lines · Dan Carpenter

Quoted 8 lines
include/acpi/platform/acenv.h
Quoted 4 lines
include/acpi/platform/acenv.h +0 -0
index e863dd5..5e0fc2b 100644
This is an ACPICA header and changes to it need to be submitted to the ACPICA maintainers (as per MAINTAINERS). We only get ACPICA changes from the upstream project (except for really special situations).

Quoted 6 lines

Ok. I should have added Robert and Lv to the CC list. My guess is that the (void) is needed to avoid compile warnings but it's needed for us to avoid compile breakage with this change.
Anyway, I'll wait for a couple days and resend that bit broken out.

In the end, I won't need to modify the ACPICA headers if I use an expression statement ({ ... }) instead of a do { } while (0) statement.

Thanks, though. :)

regards,
dan carpenter

Solar Designer

2014-05-01 04:06 UTC #8 lore raw

In reply to Dan Carpenter

Quoted 4 lines · Dan Carpenter

There are sometimes where we know that we are doing an strcpy() into a fixed length buffer. In those cases, we could verify that the strcpy() doesn't overflow. This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS if you want to check for that.

FWIW, I had posted similar macros for userland strcpy() and friends to the security-audit list (now defunct) in 1998. Someone preserved a copy here (although the indentation is lost):

http://www.opennet.ru/soft/0432.html

In (weird) use, with proper indentation:

http://www.merit.edu/mail.archives/nanog/2000-02/msg00366.html
https://github.com/tureba/trinoo/blob/master/strfix.h

Personally, I was using this at the time for building known-broken software like wu-ftpd, where the risk of false positives felt lower than the risk of buffer overflow bugs being in fact present in the code.

I used gcc's Statement Exprs extension, which is also used in the Linux kernel a lot:

http://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

So maybe you should, too. (That is, if you want to go ahead with this approach for code that isn't meant to be as broken as wu-ftpd was.) This lets us propagate the original return value.

To determine the destination buffer size, I simply used sizeof() and skipped my added protection in case the size looked like that of a pointer. Now you have those nice new gcc features instead. :-)

Quoted 1 line

The downside is that it makes strcpy slower.

I guess the slowdown is mostly from the added strlen(). I avoided it by using strncat(), so I had truncation instead of detection. It is unclear which is better.

Other functions I did this for are strcat(), sprintf(), vsprintf().

Alexander

Dan Carpenter

author

2014-05-01 07:45 UTC #9 lore raw

Ah. Fantastic. That's all great stuff. I'm on holiday today but I'll send a new later in the week.

regards,
dan carpenter