Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rsync crashes with "*** buffer overflow detected ***: terminated" #511

Closed
jirislaby opened this issue Aug 17, 2023 · 7 comments · Fixed by #513
Closed

rsync crashes with "*** buffer overflow detected ***: terminated" #511

jirislaby opened this issue Aug 17, 2023 · 7 comments · Fixed by #513

Comments

@jirislaby
Copy link

When _FORTIFY_SOURCE=2 is enabled during build, rsync crashes. gdb says:

#3  0x00007f2a31226917 in __GI_abort () at abort.c:79
#4  0x00007f2a312277e3 in __libc_message (fmt=fmt@entry=0x7f2a313b030c "*** %s ***: terminated\n") at ../sysdeps/posix/libc_fatal.c:150
#5  0x00007f2a31327bdb in __GI___fortify_fail (msg=msg@entry=0x7f2a313b02f3 "buffer overflow detected") at fortify_fail.c:24
#6  0x00007f2a31327506 in __GI___chk_fail () at chk_fail.c:28
#7  0x00007f2a31329279 in __strlcpy_chk (s1=<optimized out>, s2=<optimized out>, n=<optimized out>, s1len=<optimized out>) at strlcpy_chk.c:28
27        if (__glibc_unlikely (s1len < n))
28          __chk_fail ();
#8  0x0000559d0acf778a in strlcpy (__n=4096, __src=0x7ffece39ae20 "xslaby/pokus/Align-37-43/", __dest=0x559d0ad61886 <dirbuf.lto_priv+6> "")
    at /usr/include/bits/string_fortified.h:156
156         return __strlcpy_chk (__dest, __src, __n, __glibc_objsize (__dest));

How does it come __glibc_objsize(dirbuf.lto_priv+6) is less than 4096?

#9  setup_merge_file (mergelist_num=mergelist_num@entry=0, ex=ex@entry=0x559d0bf84b40, lp=lp@entry=0x559d0bf84b90) at /usr/src/debug/rsync-3.2.7/exclude.c:737

https://github.com/WayneD/rsync/blob/2f9b963abaa52e44891180fe6c0d1c2219f6686d/exclude.c#L737
Apparently, y can be incremented, so this MAXPATHLEN looks to be wrong. That's why.

#10 0x0000559d0acf7d94 in push_local_filters (dir=dir@entry=0x7ffece39c000 ".", dirlen=dirlen@entry=1) at /usr/src/debug/rsync-3.2.7/exclude.c:806
#11 0x0000559d0acf8259 in change_local_filter_dir (dname=0x7ffece39c000 ".", dlen=1, dir_depth=0) at /usr/src/debug/rsync-3.2.7/exclude.c:899
#12 0x0000559d0acef91c in send_file_list (f=4, argc=0, argv=0x559d0bf84898) at /usr/src/debug/rsync-3.2.7/flist.c:2453
#13 0x0000559d0ad07d4b in client_run (f_in=f_in@entry=5, f_out=f_out@entry=4, pid=pid@entry=6659, argc=argc@entry=1, argv=argv@entry=0x559d0bf84890)
    at /usr/src/debug/rsync-3.2.7/main.c:1315
#14 0x0000559d0ace2bdb in start_client (argv=0x559d0bf84890, argc=1) at /usr/src/debug/rsync-3.2.7/main.c:1613
#15 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/rsync-3.2.7/main.c:1873
t=0x559d0ad61886 <dirbuf.lto_priv+6> "")
    at /usr/include/bits/string_fortified.h:156
@mruprich
Copy link
Contributor

Adding a very simple reproducer on localhost:

# touch file
# mkdir dest
# /usr/bin/rsync -vvv --delay-updates -F --compress --delete-after --archive --no-owner --no-group  file dest/
[sender] add_rule(-p .~tmp~/)
[Receiver] add_rule(-p .~tmp~/)
building file list ... 
[sender] make_file(file,*,0)
done
send_file_list done
send_files starting
server_recv(2) starting pid=1736
recv_file_name(file)
received 1 names
recv_file_list done
get_local_name count=1 dest/
generator starting pid=1736
delta-transmission disabled for local transfer or --whole-file
recv_generator(file,0)
file is uptodate
send_files(0, file)
generate_files phase=1
send_files phase=1
recv_files(1) starting
recv_files(file)
recv_files phase=1
generate_files phase=2
send_files phase=2
recv_files phase=2
generate_files phase=3
send files finished
total: matches=0  hash_hits=0  false_alarms=0 data=0
recv_files finished
*** buffer overflow detected ***: terminated
rsync: connection unexpectedly closed (508 bytes received so far) [sender]
rsync error: error in rsync protocol data stream (code 12) at io.c(231) [sender=3.2.7]
[sender] _exit_cleanup(code=12, file=io.c, line=231): about to call exit(12)

Removing -F helps:

# /usr/bin/rsync -vvv --delay-updates  --compress --delete-after --archive --no-owner --no-group  file dest/
[sender] add_rule(-p .~tmp~/)
[Receiver] add_rule(-p .~tmp~/)
building file list ... 
[sender] make_file(file,*,0)
done
send_file_list done
send_files starting
server_recv(2) starting pid=1750
recv_file_name(file)
received 1 names
recv_file_list done
get_local_name count=1 dest/
generator starting pid=1750
delta-transmission disabled for local transfer or --whole-file
recv_generator(file,0)
send_files(0, file)
send_files mapped file of size 0
calling match_sums file
file
sending file_sum
false_alarms=0 hash_hits=0 matches=0
sender finished file
generate_files phase=1
send_files phase=1
recv_files(1) starting
recv_files(file)
got file_sum
set modtime, atime of .file.1eyMWq to (1692278729) 2023/08/17 09:25:29, (1692278949) 2023/08/17 09:29:09
renaming .file.1eyMWq to .~tmp~/file
recv_files phase=1
generate_files phase=2
send_files phase=2
recv_files phase=2
renaming .~tmp~/file to file
generate_files phase=3
send files finished
total: matches=0  hash_hits=0  false_alarms=0 data=0
recv_files finished
[generator] pushing local filters for /root/dest/file/
generate_files finished

sent 83 bytes  received 679 bytes  1,524.00 bytes/sec
total size is 0  speedup is 0.00

jirislaby pushed a commit to jirislaby/rsync that referenced this issue Aug 18, 2023
Fortified (-D_FORTIFY_SOURCE=2 for gcc) builds make strlcpy() crash when
its third parameter (size) is larger than the buffer:
  $ rsync -FFXHav '--filter=merge global-rsync-filter' Align-37-43/ xxx
  sending incremental file list
  *** buffer overflow detected ***: terminated

It's in the exclude code in setup_merge_file():
  strlcpy(y, save, MAXPATHLEN);

Note the 'y' pointer was incremented, so it no longer points to memory
with MAXPATHLEN "owned" bytes.

Fix it by remembering the number of copied bytes into the 'save' buffer
and use that instead of MAXPATHLEN which is clearly incorrect.

Fixes RsyncProject#511.
@sergiodj
Copy link

I tried to reproduce this bug on Debian in order to confirm whether we were affected or not (and whether it made sense to backport the fix), and that got me into a rabbit hole. Allow me to explain.

Neither the bug description nor the PR description are very clear regarding the reproduction steps. My initial attempt was to run the reproducer provided by @mruprich , but that failed to trigger the issue. Debian uses _FORTIFY_SOURCE=2 by default, but we're still shipping glibc 2.37 on sid, which does not contain strlcpy (it was introduced only in glibc 2.38). But we do have glibc 2.38 on experimental, so I updated it, rebuilt rsync, and ran the reproducer again. And again, I could not make it crash.

After reading the implementation of glibc's __strlcpy_chk, I noticed that it calls __glibc_objsize in order to determine the length of the destination buffer. __glibc_objsize is macro defined as:

/* Fortify support.  */
#define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1)
#define __bos0(ptr) __builtin_object_size (ptr, 0)

/* Use __builtin_dynamic_object_size at _FORTIFY_SOURCE=3 when available.  */
#if __USE_FORTIFY_LEVEL == 3 && (__glibc_clang_prereq (9, 0)		      \
				 || __GNUC_PREREQ (12, 0))
# define __glibc_objsize0(__o) __builtin_dynamic_object_size (__o, 0)
# define __glibc_objsize(__o) __builtin_dynamic_object_size (__o, 1)
#else
# define __glibc_objsize0(__o) __bos0 (__o)
# define __glibc_objsize(__o) __bos (__o)
#endif

A little bit of debugging told me that __builtin_object_size was returning -1 when it received y as an argument, but __builtin_dynamic_object_size would return the expected value. With that, I finally understood that the bug would manifest only if I use _FORTIFY_SOURCE=3. After I recompiled rsync with the proper fortified level, I could reproduce the crash.

In summary, the conclusion is that the bug only happens when rsync is compiled against glibc >= 2.38 and with _FORTIFY_SOURCE=3.

I went through all this trouble because the rsync code that trips this crash actually made sense to me (y does not change between the two invocations of strlcpy; the code is effectively swapping the values of y and save), so I wanted to understand better why the buffer overflow was being detected in this scenario. Hopefully this comment makes things a bit clearer.

@samueloph
Copy link
Contributor

To add on to @sergiodj's comment, there's no real buffer overflow happening, this is a false positive from the fortified function. This is implied from his explanation but I wanted to make it clear.

@jirislaby
Copy link
Author

Yes, it's a false positive. I don't know why I didn't add a downstream link: https://bugzilla.suse.com/show_bug.cgi?id=1214249. See:
https://build.opensuse.org/package/view_file/network/rsync/rsync-fortified-strlcpy-fix.patch?expand=1
how this was worked around. I think the proper fix is just use strcpy() as it's guaranteed the buffer is long enough. Or maybe see also comment 12 in the bug above.

Mic92 added a commit to Mic92/nixpkgs that referenced this issue Nov 6, 2023
See RsyncProject/rsync#511 for details.
Not using fetchpatch, because I didn't found a raw link for opensuse's repository
ehmry pushed a commit to NixOS/nixpkgs that referenced this issue Nov 6, 2023
See RsyncProject/rsync#511 for details.
Not using fetchpatch, because I didn't found a raw link for opensuse's repository
Mic92 added a commit to Mic92/nixpkgs that referenced this issue Nov 6, 2023
See RsyncProject/rsync#511 for details.
Not using fetchpatch, because I didn't found a raw link for opensuse's repository
Mic92 added a commit to Mic92/nixpkgs that referenced this issue Nov 6, 2023
See RsyncProject/rsync#511 for details.
Not using fetchpatch, because I didn't found a raw link for opensuse's repository
Mic92 added a commit to NixOS/nixpkgs that referenced this issue Nov 7, 2023
See RsyncProject/rsync#511 for details.
Not using fetchpatch, because I didn't found a raw link for opensuse's repository
Mic92 added a commit to NixOS/nixpkgs that referenced this issue Nov 7, 2023
See RsyncProject/rsync#511 for details.
Not using fetchpatch, because I didn't found a raw link for opensuse's repository
nyabinary pushed a commit to nyabinary/nixpkgs that referenced this issue Nov 14, 2023
See RsyncProject/rsync#511 for details.
Not using fetchpatch, because I didn't found a raw link for opensuse's repository
@siddhesh
Copy link

siddhesh commented Dec 6, 2023

Yes, it's a false positive.

Not really, the fortification check is explicit about the destination and size relationship in the strlcpy function, which it derives from the specification of strlcpy. The strlcpy(y, save, MAXPATHLEN); call invokes undefined behaviour because y does not point to storage of at least MAXPATHLEN size.

The fortification abort error messages could benefit some nuance though, because unlike what they claim, they don't trigger at the point of (or just before) a buffer overflow. The fortifications trigger when they detect a potential for buffer overflow, which is subtly different.

@heitbaum
Copy link

Using rsync 3.3.0pre1 compiled on Ubuntu:jammy and gcc-12 I have had no issue. But compiling the same rsync with gcc-13 on Ubuntu:noble. And then using rsync in a kernel build gives me the same error. (This is with the patch #513) included. So there must be another code with a similar issue. With rsync-3.2.7 I did not get this error (on either jammy or noble.)

execve("/var/media/DATA/home-rudi/LibreELEC.tv/build.LibreELEC-Generic.x86_64-12.0-devel/toolchain/bin/rsync", ["rsync", "-mrl", "--include=*/", "--include=*\\.h", "--exclude=*", "usr/include", "dest"], 0x7ffdfa601810 /* 192 vars */) = 0
brk(NULL)                               = 0x557effc6f000
arch_prctl(0x3001 /* ARCH_??? */, 0x7ffc110f5cd0) = -1 EINVAL (Invalid argument)
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fc0c407c000
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/var/media/DATA/home-rudi/LibreELEC.tv/build.LibreELEC-Generic.x86_64-12.0-devel/toolchain/lib/glibc-hwcaps/x86-64-v3/libz.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/var/media/DATA/home-rudi/LibreELEC.tv/build.LibreELEC-Generic.x86_64-12.0-devel/toolchain/lib/glibc-hwcaps/x86-64-v3/", 0x7ffc110f4f00, 0) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/var/media/DATA/home-rudi/LibreELEC.tv/build.LibreELEC-Generic.x86_64-12.0-devel/toolchain/lib/glibc-hwcaps/x86-64-v2/libz.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/var/media/DATA/home-rudi/LibreELEC.tv/build.LibreELEC-Generic.x86_64-12.0-devel/toolchain/lib/glibc-hwcaps/x86-64-v2/", 0x7ffc110f4f00, 0) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/var/media/DATA/home-rudi/LibreELEC.tv/build.LibreELEC-Generic.x86_64-12.0-devel/toolchain/lib/libz.so.1", O_RDONLY|O_CLOEXEC) = 5
read(5, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\0\0\0\0\0\0\0\0"..., 832) = 832
newfstatat(5, "", {st_mode=S_IFREG|0644, st_size=109392, ...}, AT_EMPTY_PATH) = 0
mmap(NULL, 102792, PROT_READ, MAP_PRIVATE|MAP_DENYWRITE, 5, 0) = 0x7fc0c4062000
mmap(0x7fc0c4065000, 57344, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 5, 0x3000) = 0x7fc0c4065000
mmap(0x7fc0c4073000, 28672, PROT_READ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 5, 0x11000) = 0x7fc0c4073000
mmap(0x7fc0c407a000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 5, 0x17000) = 0x7fc0c407a000
close(5)                                = 0
openat(AT_FDCWD, "/var/media/DATA/home-rudi/LibreELEC.tv/build.LibreELEC-Generic.x86_64-12.0-devel/toolchain/lib/libc.so.6", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 5
newfstatat(5, "", {st_mode=S_IFREG|0644, st_size=13153, ...}, AT_EMPTY_PATH) = 0
mmap(NULL, 13153, PROT_READ, MAP_PRIVATE, 5, 0) = 0x7fc0c405e000
close(5)                                = 0
openat(AT_FDCWD, "/lib/x86_64-linux-gnu/libc.so.6", O_RDONLY|O_CLOEXEC) = 5
read(5, "\177ELF\2\1\1\3\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\220\202\2\0\0\0\0\0"..., 832) = 832
pread64(5, "\6\0\0\0\4\0\0\0@\0\0\0\0\0\0\0@\0\0\0\0\0\0\0@\0\0\0\0\0\0\0"..., 784, 64) = 784
newfstatat(5, "", {st_mode=S_IFREG|0755, st_size=2104632, ...}, AT_EMPTY_PATH) = 0
pread64(5, "\6\0\0\0\4\0\0\0@\0\0\0\0\0\0\0@\0\0\0\0\0\0\0@\0\0\0\0\0\0\0"..., 784, 64) = 784
mmap(NULL, 2149776, PROT_READ, MAP_PRIVATE|MAP_DENYWRITE, 5, 0) = 0x7fc0c3e51000
mmap(0x7fc0c3e77000, 1568768, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 5, 0x26000) = 0x7fc0c3e77000
mmap(0x7fc0c3ff6000, 348160, PROT_READ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 5, 0x1a5000) = 0x7fc0c3ff6000
mmap(0x7fc0c404b000, 24576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 5, 0x1f9000) = 0x7fc0c404b000
mmap(0x7fc0c4051000, 52624, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7fc0c4051000
close(5)                                = 0
mmap(NULL, 12288, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fc0c3e4e000
arch_prctl(ARCH_SET_FS, 0x7fc0c3e4e740) = 0
set_tid_address(0x7fc0c3e4ea10)         = 130915
set_robust_list(0x7fc0c3e4ea20, 24)     = 0
rseq(0x7fc0c3e4f060, 0x20, 0, 0x53053053) = 0
mprotect(0x7fc0c404b000, 16384, PROT_READ) = 0
mprotect(0x7fc0c407a000, 4096, PROT_READ) = 0
mprotect(0x557efde5d000, 8192, PROT_READ) = 0
mprotect(0x7fc0c40b3000, 8192, PROT_READ) = 0
prlimit64(0, RLIMIT_STACK, NULL, {rlim_cur=8192*1024, rlim_max=RLIM64_INFINITY}) = 0
munmap(0x7fc0c405e000, 13153)           = 0
rt_sigaction(SIGUSR1, {sa_handler=0x557efde0db40, sa_mask=[], sa_flags=SA_RESTORER|SA_NOCLDSTOP, sa_restorer=0x7fc0c3e93900}, NULL, 8) = 0
rt_sigaction(SIGUSR2, {sa_handler=0x557efde0e620, sa_mask=[], sa_flags=SA_RESTORER|SA_NOCLDSTOP, sa_restorer=0x7fc0c3e93900}, NULL, 8) = 0
rt_sigaction(SIGCHLD, {sa_handler=0x557efde0daa0, sa_mask=[], sa_flags=SA_RESTORER|SA_NOCLDSTOP, sa_restorer=0x7fc0c3e93900}, NULL, 8) = 0
rt_sigaction(SIGVTALRM, {sa_handler=0x557efde0e030, sa_mask=[], sa_flags=SA_RESTORER|SA_NOCLDSTOP, sa_restorer=0x7fc0c3e93900}, NULL, 8) = 0
geteuid()                               = 1000
getegid()                               = 1000
umask(000)                              = 022
umask(022)                              = 000
getrandom("\x60\x15\x56\xc1\x7d\xdd\x6f\x81", 8, GRND_NONBLOCK) = 8
brk(NULL)                               = 0x557effc6f000
brk(0x557effc90000)                     = 0x557effc90000
openat(AT_FDCWD, "/usr/lib/locale/locale-archive", O_RDONLY|O_CLOEXEC) = 5
newfstatat(5, "", {st_mode=S_IFREG|0644, st_size=3052896, ...}, AT_EMPTY_PATH) = 0
mmap(NULL, 3052896, PROT_READ, MAP_PRIVATE, 5, 0) = 0x7fc0c3b64000
close(5)                                = 0
openat(AT_FDCWD, "/etc/popt", O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/home/docker/.popt", O_RDONLY) = -1 ENOENT (No such file or directory)
writev(2, [{iov_base="*** ", iov_len=4}, {iov_base="buffer overflow detected", iov_len=24}, {iov_base=" ***: terminated\n", iov_len=17}], 3*** buffer overflow detected ***: terminated
) = 45
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fc0c4061000
rt_sigprocmask(SIG_UNBLOCK, [ABRT], NULL, 8) = 0
gettid()                                = 130915
getpid()                                = 130915
tgkill(130915, 130915, SIGABRT)         = 0
--- SIGABRT {si_signo=SIGABRT, si_code=SI_TKILL, si_pid=130915, si_uid=1000} ---
+++ killed by SIGABRT +++

WayneD pushed a commit that referenced this issue Apr 6, 2024
Fortified (-D_FORTIFY_SOURCE=2 for gcc) builds make strlcpy() crash when
its third parameter (size) is larger than the buffer:
  $ rsync -FFXHav '--filter=merge global-rsync-filter' Align-37-43/ xxx
  sending incremental file list
  *** buffer overflow detected ***: terminated

It's in the exclude code in setup_merge_file():
  strlcpy(y, save, MAXPATHLEN);

Note the 'y' pointer was incremented, so it no longer points to memory
with MAXPATHLEN "owned" bytes.

Fix it by remembering the number of copied bytes into the 'save' buffer
and use that instead of MAXPATHLEN which is clearly incorrect.

Fixes #511.
@WayneD WayneD reopened this Apr 6, 2024
@WayneD
Copy link
Member

WayneD commented Apr 6, 2024

This should be resolved with the merge of #513.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants