Bug 225761

Summary: net/rsync long path causes buffer overflow (update to 3.1.3)
Product: Ports & Packages Reporter: os
Component: Individual Port(s)Assignee: Rodrigo Osorio <rodrigo>
Status: Closed Not A Bug    
Severity: Affects Some People CC: pi, w.schwarzenfeld
Priority: ---    
Version: Latest   
Hardware: amd64   
OS: Any   
Bug Depends on: 227635    
Bug Blocks:    
Attachments:
Description Flags
svn-diff-rsync
none
workaround-patch-rsync.h none

Description os 2018-02-08 15:08:00 UTC 
Hello,

running rsync will cause a buffer overflow when the following conditions are met:
* rsync syncs over the network (two hosts)
* path too long


expected behavior (output from local sync):

root@f:~ # rsync -aHAX /srv/test /tmp/asd/
filename overflows max-path len by 3: test/ttest.data/test.asd/ttest.data......ttest.data/test.asd/ttest.data/asd.dddddd
rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1200) [sender=3.1.2]

actual behavior:

root@f:~ # rsync -aHAX testsmba:/srv/test /tmp/
overflow: xflags=0x20fe l1=255 l2=769 lastname=test/ttest.data/............./ttest.data/test.asd/ttest.data [receiver]
ERROR: buffer overflow in recv_file_entry [receiver]
rsync error: error allocating core memory buffers (code 22) at util2.c(112) [receiver=3.1.2]
rsync: [generator] write error: Broken pipe (32)
rsync: [receiver] write error: Broken pipe (32)


versions used:
FreeBSD 11.1: rsync  version 3.1.2  protocol version 31
Debian Stretch: rsync  version 3.1.2  protocol version 31

In the example above the /srv/test directory contains only directories was created by the following command:

mkdir -p /srv/test/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/ttest.data/test.asd/
Comment 1 Emanuel Haupt freebsd_committer freebsd_triage 2018-02-11 08:45:03 UTC 
Back to the pool, I no longer maintain this port.
Comment 2 Walter Schwarzenfeld 2018-02-11 16:18:30 UTC 
Created attachment 190515 [details]
svn-diff-rsync

Update to 3.1.3 should fix that.

ChangeLog:
https://download.samba.org/pub/rsync/src/rsync-3.1.3-NEWS

Testbuilds 10.3amd64/i386 and 11.1amd64 ok.
Comment 3 Kurt Jaeger freebsd_committer freebsd_triage 2018-02-11 17:23:12 UTC 
Btw, testbuilds are fine and ehaupt said the upgrade was not the reason he dropped maintainer 8-}
Comment 4 commit-hook freebsd_committer freebsd_triage 2018-02-11 22:30:25 UTC 
A commit references this bug:

Author: joneum
Date: Sun Feb 11 22:30:08 UTC 2018
New revision: 461533
URL: https://svnweb.freebsd.org/changeset/ports/461533

Log:
  net/rsync: Update to 3.1.3

  Changelog:
   SECURITY FIXES:
      - Fixed a buffer overrun in the protocol's handling of xattr names and
        ensure that the received name is null terminated.
      - Fix an issue with --protect-args where the user could specify the arg in
        the protected-arg list and short-circuit some of the arg-sanitizing code.

    BUG FIXES:
      - Don't output about a new backup dir without appropriate info verbosity.
      - Fixed some issues with the sort functions in support/rsyncstats script.
      - Added a way to specify daemon config lists (e.g. users, groups, etc) that
        contain spaces (see "auth users" in the latest rsyncd.conf manpage).
      - If a backup fails (e.g. full disk) rsync exits with an error.
      - Fixed a problem with a doubled --fuzzy option combined with --link-dest.
      - Avoid invalid output in the summary if either the start or end time had
        an error.
      - We don't allow a popt alias to affect the --daemon or --server options.
      - Fix daemon exclude code to disallow attribute changes in addition to
        disallowing transfers.
      - Don't force nanoseconds to match if a non-transferred, non-checksummed
        file only passed the quick-check w/o comparing nanosecods.

    ENHANCEMENTS:
      - Added the ability for rsync to compare nanosecond times in its file-check
        comparisons, and added support nanosecond times on Mac OS X.
      - Added a short-option (-@) for --modify-window.
      - Added the --checksum-choice=NAME[,NAME] option to choose the checksum
        algorithms.
      - Added hashing of xattr names (with using -X) to improve the handling of
        files with large numbers of xattrs.
      - Added a way to filter xattr names using include/exclude/filter rules (see
        the --xattrs option in the manpage for details).
      - Added "daemon chroot|uid|gid" to the daemon config (in addition to the
        old chroot|uid|gid settings that affect the daemon's transfer process).
      - Added "syslog tag" to the daemon configuration.
      - Some manpage improvements.

   DEVELOPER RELATED:
      - Tweak the "make" output when yodl isn't around to create the man pages.
      - Changed an obsolete autoconf compile macro.
      - Support newer yodl versions when converting man pages.

  remove needless patch

  *While here switch to DISTVERSION

  PR:		225761
  Reported by:	 os@ist.ac.at
  Approved by:	tcberner (mentor)
  MFH:		2018Q1
  Differential Revision:	https://reviews.freebsd.org/D14324

Changes:
  head/net/rsync/Makefile
  head/net/rsync/distinfo
  head/net/rsync/files/patch-CVE-2017-16548
  head/net/rsync/files/patch-CVE-2017-17433
  head/net/rsync/files/patch-CVE-2017-17434-1
  head/net/rsync/files/patch-CVE-2017-17434-2
Comment 5 Jochen Neumeister freebsd_committer freebsd_triage 2018-02-11 22:30:58 UTC 
Landed. Thanks
Comment 6 commit-hook freebsd_committer freebsd_triage 2018-02-13 19:30:19 UTC 
A commit references this bug:

Author: joneum
Date: Tue Feb 13 19:30:01 UTC 2018
New revision: 461735
URL: https://svnweb.freebsd.org/changeset/ports/461735

Log:
  MFH: r461533

  net/rsync: Update to 3.1.3

  Changelog:
   SECURITY FIXES:
      - Fixed a buffer overrun in the protocol's handling of xattr names and
        ensure that the received name is null terminated.
      - Fix an issue with --protect-args where the user could specify the arg in
        the protected-arg list and short-circuit some of the arg-sanitizing code.

    BUG FIXES:
      - Don't output about a new backup dir without appropriate info verbosity.
      - Fixed some issues with the sort functions in support/rsyncstats script.
      - Added a way to specify daemon config lists (e.g. users, groups, etc) that
        contain spaces (see "auth users" in the latest rsyncd.conf manpage).
      - If a backup fails (e.g. full disk) rsync exits with an error.
      - Fixed a problem with a doubled --fuzzy option combined with --link-dest.
      - Avoid invalid output in the summary if either the start or end time had
        an error.
      - We don't allow a popt alias to affect the --daemon or --server options.
      - Fix daemon exclude code to disallow attribute changes in addition to
        disallowing transfers.
      - Don't force nanoseconds to match if a non-transferred, non-checksummed
        file only passed the quick-check w/o comparing nanosecods.

    ENHANCEMENTS:
      - Added the ability for rsync to compare nanosecond times in its file-check
        comparisons, and added support nanosecond times on Mac OS X.
      - Added a short-option (-@) for --modify-window.
      - Added the --checksum-choice=NAME[,NAME] option to choose the checksum
        algorithms.
      - Added hashing of xattr names (with using -X) to improve the handling of
        files with large numbers of xattrs.
      - Added a way to filter xattr names using include/exclude/filter rules (see
        the --xattrs option in the manpage for details).
      - Added "daemon chroot|uid|gid" to the daemon config (in addition to the
        old chroot|uid|gid settings that affect the daemon's transfer process).
      - Added "syslog tag" to the daemon configuration.
      - Some manpage improvements.

   DEVELOPER RELATED:
      - Tweak the "make" output when yodl isn't around to create the man pages.
      - Changed an obsolete autoconf compile macro.
      - Support newer yodl versions when converting man pages.

  remove needless patch

  *While here switch to DISTVERSION

  PR:		225761
  Reported by:	 os@ist.ac.at
  Approved by:	tcberner (mentor)
  Differential Revision:	https://reviews.freebsd.org/D14324

  Approved by:	ports-secteam (riggs)

Changes:
_U  branches/2018Q1/
  branches/2018Q1/net/rsync/Makefile
  branches/2018Q1/net/rsync/distinfo
  branches/2018Q1/net/rsync/files/patch-CVE-2017-16548
  branches/2018Q1/net/rsync/files/patch-CVE-2017-17433
  branches/2018Q1/net/rsync/files/patch-CVE-2017-17434-1
  branches/2018Q1/net/rsync/files/patch-CVE-2017-17434-2
Comment 7 os 2018-02-15 11:20:31 UTC 
Dear maintainers,

this problem is still reproducible with rsync 3.1.3. 
(The same test is not reproducible with 2 Linux hosts with rsync 3.1.2.)

root@f:~ # rsync --version
rsync  version 3.1.3  protocol version 31
Copyright (C) 1996-2018 by Andrew Tridgell, Wayne Davison, and others.
Web site: http://rsync.samba.org/
Capabilities:
    64-bit files, 32-bit inums, 64-bit timestamps, 64-bit long ints,
    socketpairs, hardlinks, symlinks, IPv6, batchfiles, inplace,
    append, ACLs, xattrs, iconv, symtimes, no prealloc, file-flags

rsync comes with ABSOLUTELY NO WARRANTY.  This is free software, and you
are welcome to redistribute it under certain conditions.  See the GNU
General Public Licence for details.


root@f:~ # rsync -aHAX testsmba.ista.local:/srv/test /tmp/
overflow: xflags=0x20fe l1=255 l2=769 lastname=test/ttest.data/test.asd....................test.asd/ttest.data/test.asd/ttest.data [receiver]
ERROR: buffer overflow in recv_file_entry [receiver]
rsync error: error allocating core memory buffers (code 22) at util2.c(111) [receiver=3.1.3]
rsync: [generator] write error: Broken pipe (32)
rsync: [receiver] write error: Broken pipe (32)
Comment 8 Walter Schwarzenfeld 2018-02-15 13:42:50 UTC 
Sorry, I was wrong, it does not fix it.

I have only a workaround.
make extract
change the size at least to 4096 in

work/rsync-3.1.3/rsync.h

 656 #ifndef MAXPATHLEN
 657 #define MAXPATHLEN 1024
 658 #endif
 659
 660 /* We want a roomy line buffer that can hold more than MAXPATHLEN,
 661  * and significantly more than an overly short MAXPATHLEN. */
 662 #if MAXPATHLEN < 4096
 663 #define BIGPATHBUFLEN (4096+1024)
 664 #else
 665 #define BIGPATHBUFLEN (MAXPATHLEN+1024)
 666 #endif

and build it new.
Comment 9 Walter Schwarzenfeld 2018-02-15 13:49:26 UTC 
Sorry, MAXPATHLEN must have more than 4096.
Comment 10 Walter Schwarzenfeld 2018-02-15 14:01:42 UTC 
Created attachment 190648 [details]
workaround-patch-rsync.h

Attached a patch. You can change in the patch the value of MAXPATHLEN as you need.
Comment 11 Jochen Neumeister freebsd_committer freebsd_triage 2018-02-15 14:44:14 UTC 
give Assignee to new Maintainer from net/rsync
Comment 12 Rodrigo Osorio freebsd_committer freebsd_triage 2018-09-22 08:12:27 UTC 
Hi,

I don't think the changes you request can be applied as port patch,
because is not a real porting behavior but an upstream issue you should
report to the upstream project.

Changing the MAX_PATH value here can impact the way rsync tool
works and interact with other tools/libs, and probably create more issues
than the one we are trying to solve.

I want to suggest you to open an issue into the rsync bugzilla (https://bugzilla.samba.org/)