Bug 13105 - 1byte heap overflow in sanitize_path
Summary: 1byte heap overflow in sanitize_path
Status: RESOLVED FIXED
Alias: None
Product: rsync
Classification: Unclassified
Component: core (show other bugs)
Version: 3.1.3
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Wayne Davison
QA Contact: Rsync QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-27 03:54 UTC by jeriko.one
Modified: 2017-10-29 22:57 UTC (History)
0 users

See Also:


Attachments
Use MAX to ensure at least 2 bytes are allocated for dest (763 bytes, patch)
2017-10-27 03:54 UTC, jeriko.one
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description jeriko.one 2017-10-27 03:54:14 UTC
Created attachment 13733 [details]
Use MAX to ensure at least 2 bytes are allocated for dest

$ ./rsync  --version
rsync  version 3.1.3dev  protocol version 31

The path that'll hit this issue is when using rsyncd and you receive arguments from the client like:
"--server" "--sender" "." " test/"

Where "test" is the name of a module. 

sanitize_path doesn't handle the case where the argument p is == "\0" and p != dest

This leads to a 1 byte overwrite as the allocation of dest is too small. 

 sanitize_path (dest=0x0, p=0x60200000ebb0 "", 
    rootdir=0x4f51e0 "", depth=0, flags=1) at util.c:1009

1011     if (dest != p) {
1012         int plen = strlen(p); <-- returns 0 

1023         } else if (!(dest = new_array(char, rlen + plen + 1))) <-- 1 byte requested

1037     start = sanp = dest + rlen;

1073     if (sanp == dest) {
1074         /* ended up with nothing, so put in "." component */
1075         *sanp++ = '.';
1076     }
1077     *sanp = '\0'; <-- overwrite by 1 


The patch I've attached will ensures that at least 2 bytes are allocated for dest. 


Below is the ASan output
=================================================================
==24556==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000eb71 at pc 0x00000044b486 bp 0x7fffffffabd0 sp 0x7fffffffabc0
WRITE of size 1 at 0x60200000eb71 thread T0
    #0 0x44b485 in sanitize_path /home/raj/rsync/rsync/util.c:1077
    #1 0x449484 in glob_expand /home/raj/rsync/rsync/util.c:717
    #2 0x449af8 in glob_expand_module /home/raj/rsync/rsync/util.c:783
    #3 0x478b61 in read_args /home/raj/rsync/rsync/io.c:1266
    #4 0x4b12b6 in rsync_module /home/raj/rsync/rsync/clientserver.c:869
    #5 0x4b2a49 in start_daemon /home/raj/rsync/rsync/clientserver.c:1135
    #6 0x48f56e in start_accept_loop /home/raj/rsync/rsync/socket.c:618
    #7 0x4b320a in daemon_main /home/raj/rsync/rsync/clientserver.c:1237
    #8 0x4582da in main /home/raj/rsync/rsync/main.c:1627
    #9 0x7ffff64d866f in __libc_start_main (/lib64/libc.so.6+0x2066f)
    #10 0x4047c8 in _start (/home/raj/rsync/asan/bin/rsync+0x4047c8)

0x60200000eb71 is located 0 bytes to the right of 1-byte region [0x60200000eb70,0x60200000eb71)
allocated by thread T0 here:
    #0 0x7ffff6f04572 in malloc (/usr/lib/gcc/x86_64-pc-linux-gnu/5.4.0/libasan.so.2+0x98572)
    #1 0x44e480 in _new_array /home/raj/rsync/rsync/util2.c:68
    #2 0x44aed9 in sanitize_path /home/raj/rsync/rsync/util.c:1023
    #3 0x449484 in glob_expand /home/raj/rsync/rsync/util.c:717
    #4 0x449af8 in glob_expand_module /home/raj/rsync/rsync/util.c:783
    #5 0x478b61 in read_args /home/raj/rsync/rsync/io.c:1266
    #6 0x4b12b6 in rsync_module /home/raj/rsync/rsync/clientserver.c:869
    #7 0x4b2a49 in start_daemon /home/raj/rsync/rsync/clientserver.c:1135
    #8 0x48f56e in start_accept_loop /home/raj/rsync/rsync/socket.c:618
    #9 0x4b320a in daemon_main /home/raj/rsync/rsync/clientserver.c:1237
    #10 0x4582da in main /home/raj/rsync/rsync/main.c:1627
    #11 0x7ffff64d866f in __libc_start_main (/lib64/libc.so.6+0x2066f)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/raj/rsync/rsync/util.c:1077 sanitize_path
Shadow bytes around the buggy address:
  0x0c047fff9d10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9d20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9d30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9d40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9d50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c047fff9d60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa[01]fa
  0x0c047fff9d70: fa fa 07 fa fa fa 07 fa fa fa fd fa fa fa 02 fa
  0x0c047fff9d80: fa fa 00 01 fa fa 00 01 fa fa 00 07 fa fa fd fd
  0x0c047fff9d90: fa fa fd fd fa fa fd fa fa fa 00 00 fa fa 00 00
  0x0c047fff9da0: fa fa 00 00 fa fa 05 fa fa fa 05 fa fa fa 07 fa
  0x0c047fff9db0: fa fa fd fd fa fa 00 04 fa fa fd fd fa fa fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
==24556==ABORTING
Comment 1 Wayne Davison 2017-10-29 22:57:01 UTC
I committed a fix to git. Thanks!