[LEDE-DEV] [PATCH 1/2] firmware-utils: mkdapimg2: Fix potential buffer overflow

[Arjen de Korte]

Citeren Rosen Penev <rosenp at gmail.com>:

> If GCC is built with stack smashing protection enabled (SSP), it  
> errors when compiling lzma-loader.
>
> Switching to strncpy seems to be an easy way to fix this. It's  
> probably better to use snprintf or strlcpy but the latter is not  
> available for glibc systems.

This seems to be the wrong solution to a problem. For each of the  
strings involved, storage is allocated on the stack. There is a length  
check before the contents of the arguments provided, is copied.  
However, the values checked against are incorrect. The required  
storage length for a string will be strlen(string) + 1 (to allow for  
the terminating NUL byte).

So instead of

117	if (strlen(optarg) > MAX_SIGN_LEN + 1) {
125	if (strlen(optarg) > MAX_FW_VER_LEN + 1) {
133	if (strlen(optarg) > MAX_REG_LEN + 1) {

the correct check would have been

117	if (strlen(optarg) > MAX_SIGN_LEN - 1) {
125	if (strlen(optarg) > MAX_FW_VER_LEN - 1) {
133	if (strlen(optarg) > MAX_REG_LEN - 1) {

Good catch though.

> Output of crash below:
>
> *** buffer overflow detected ***:  
> /home/mangix/devstuff/openwrt/staging_dir/host/bin/mkdapimg2  
> terminated
> ======= Backtrace: =========
> /lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7f6318ea77e5]
> /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x5c)[0x7f6318f4915c]
> /lib/x86_64-linux-gnu/libc.so.6(+0x117160)[0x7f6318f47160]
> /home/mangix/devstuff/openwrt/staging_dir/host/bin/mkdapimg2[0x400ab7]
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6318e50830]
> /home/mangix/devstuff/openwrt/staging_dir/host/bin/mkdapimg2[0x400e69]
> ======= Memory map: ========
> 00400000-00402000 r-xp 00000000 00:00 223275                      
> /home/mangix/devstuff/openwrt/staging_dir/host/bin/mkdapimg200601000-00602000 r--p 00001000 00:00 223275                     /home/mangix/devstuff/openwrt/staging_dir/host/bin/mkdapimg200602000-00603000 rw-p 00002000 00:00 223275                     /home/mangix/devstuff/openwrt/staging_dir/host/bin/mkdapimg2018c2000-018e3000 rw-p 00000000 00:00 0                           
> [heap]
> 7f6318c10000-7f6318c26000 r-xp 00000000 00:00 122040              
> /lib/x86_64-linux-gnu/libgcc_s.so.1
> 7f6318c26000-7f6318e25000 ---p 00016000 00:00 122040              
> /lib/x86_64-linux-gnu/libgcc_s.so.1
> 7f6318e25000-7f6318e26000 rw-p 00015000 00:00 122040              
> /lib/x86_64-linux-gnu/libgcc_s.so.1
> 7f6318e30000-7f6318ff0000 r-xp 00000000 00:00 123971              
> /lib/x86_64-linux-gnu/libc-2.23.so
> 7f6318ff0000-7f6318ff9000 ---p 001c0000 00:00 123971              
> /lib/x86_64-linux-gnu/libc-2.23.so
> 7f6318ff9000-7f63191f0000 ---p 001c9000 00:00 123971              
> /lib/x86_64-linux-gnu/libc-2.23.so
> 7f63191f0000-7f63191f4000 r--p 001c0000 00:00 123971              
> /lib/x86_64-linux-gnu/libc-2.23.so
> 7f63191f4000-7f63191f6000 rw-p 001c4000 00:00 123971              
> /lib/x86_64-linux-gnu/libc-2.23.so
> 7f63191f6000-7f63191fa000 rw-p 00000000 00:00 0
> 7f6319200000-7f6319226000 r-xp 00000000 00:00 123969              
> /lib/x86_64-linux-gnu/ld-2.23.so
> 7f6319425000-7f6319426000 r--p 00025000 00:00 123969              
> /lib/x86_64-linux-gnu/ld-2.23.so
> 7f6319426000-7f6319427000 rw-p 00026000 00:00 123969              
> /lib/x86_64-linux-gnu/ld-2.23.so
> 7f6319427000-7f6319428000 rw-p 00000000 00:00 0
> 7f6319460000-7f6319461000 rw-p 00000000 00:00 0
> 7f6319470000-7f6319471000 rw-p 00000000 00:00 0
> 7f6319480000-7f6319481000 rw-p 00000000 00:00 0
> 7f6319490000-7f6319491000 rw-p 00000000 00:00 0
> 7fffca52e000-7fffcad2e000 rw-p 00000000 00:00 0                  [stack]
> 7fffcb121000-7fffcb122000 r-xp 00000000 00:00 0                  [vdso]
> make -r world: build failed. Please re-run make with -j1 V=s to see  
> what's going on
>
> Signed-off-by: Rosen Penev <rosenp at gmail.com>
> ---
>  tools/firmware-utils/src/mkdapimg2.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/firmware-utils/src/mkdapimg2.c  
> b/tools/firmware-utils/src/mkdapimg2.c
> index aef003c..59c9f00 100644
> --- a/tools/firmware-utils/src/mkdapimg2.c
> +++ b/tools/firmware-utils/src/mkdapimg2.c
> @@ -119,7 +119,7 @@ main(int ac, char *av[])
>  					progname, MAX_SIGN_LEN);
>  				exit(1);
>  			}
> -			strcpy(signature, optarg);
> +			strncpy(signature, optarg, MAX_SIGN_LEN);
>  			break;
>  		case 'v':
>  			if (strlen(optarg) > MAX_FW_VER_LEN + 1) {
> @@ -127,7 +127,7 @@ main(int ac, char *av[])
>  					progname, MAX_FW_VER_LEN);
>  				exit(1);
>  			}
> -			strcpy(version, optarg);
> +			strncpy(version, optarg, MAX_FW_VER_LEN);
>  			break;
>  		case 'r':
>  			if (strlen(optarg) > MAX_REG_LEN + 1) {
> @@ -135,7 +135,7 @@ main(int ac, char *av[])
>  					progname, MAX_REG_LEN);
>  				exit(1);
>  			}
> -			strcpy(region, optarg);
> +			strncpy(region, optarg, MAX_REG_LEN);
>  			break;
>  		case 'k':
>  			kernel = strtoul(optarg, &ptr, 0);