rsync - Dec 2011 - [Bug 8654] New: link-by-hash: Fix (non-exploitable) buffer overflow

https://bugzilla.samba.org/show_bug.cgi?id=8654

           Summary: link-by-hash: Fix (non-exploitable) buffer overflow
           Product: rsync
           Version: 3.1.0
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: minor
          Priority: P5
         Component: core
        AssignedTo: wayned at samba.org
        ReportedBy: chris at onthe.net.au
         QAContact: rsync-qa at samba.org


Created attachment 7167
  --> https://bugzilla.samba.org/attachment.cgi?id=7167
link-by-hash: Fix (non-exploitable) buffer overflow

The link-by-hash.diff patch contains a buffer overflow: the size of the
'hash'
buffer on the stack allows for one extra character beyond the size needed for
the text version of the hash but it needs two: one for the '/' directory
separator and another for the trailing null.

This was caught by noticing a compile warning:

gcc -std=gnu99 -I. -I../rsync -g -O2 -DHAVE_CONFIG_H -Wall -W  -c
../rsync/hashlink.c -o hashlink.o
../rsync/hashlink.c: In function ?make_hash_name?:
../rsync/hashlink.c:46: warning: array subscript is above array bounds

The overflow is non-exploitable as it harmlessly overwrites the following dst
variable with a null just before using asprintf() to set the dst variable
again.

But it should be fixed - see attached patch, against
"packaging/branch-from-patch ../rsync-patches/link-by-hash.diff"

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=8654

--- Comment #1 from Chris Dunlop <chris at onthe.net.au> 2011-12-12
03:07:19 UTC ---
Created attachment 7168
  --> https://bugzilla.samba.org/attachment.cgi?id=7168
Improve generation of hash file name

...and whilst we're in there fixing the buffer overflow, perhaps consider
the
attached patch to clean up the same make_hash_name() function.

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=8654

--- Comment #2 from Chris Dunlop <chris at onthe.net.au> 2011-12-12
03:44:11 UTC ---
Created attachment 7169
  --> https://bugzilla.samba.org/attachment.cgi?id=7169
Improve documentation

...another clean up, to clarify that link may be done by MD5 hash

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=8654

--- Comment #3 from Chris Dunlop <chris at onthe.net.au> 2011-12-12
06:16:28 UTC ---
Created attachment 7170
  --> https://bugzilla.samba.org/attachment.cgi?id=7170
Expand the --link-by-hash manual entry

Another documentation improvement

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=8654

Chris Dunlop <chris at onthe.net.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7168|0                           |1
        is obsolete|                            |

--- Comment #4 from Chris Dunlop <chris at onthe.net.au> 2011-12-13
00:02:55 UTC ---
Created attachment 7173
  --> https://bugzilla.samba.org/attachment.cgi?id=7173
Improve generation of hash file name

An improved version of the improvement to make_hash_name()

Changes from previous version:

Kill unused variable
Use assert() to check for buffer overflow
Further code clean up to reduce "increment noise"

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=8654

Chris Dunlop <chris at onthe.net.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7173|0                           |1
        is obsolete|                            |

--- Comment #5 from Chris Dunlop <chris at onthe.net.au> 2011-12-13
00:13:09 UTC ---
Created attachment 7174
  --> https://bugzilla.samba.org/attachment.cgi?id=7174
Improve generation of hash file name

Augh! Sorry for the noise. The previous "Improve generation of hash file
name"
changed the length of the top level directory name from 8 characters to 2
characters (part of some tests I was doing). This version doesn't have that
inadvertent change but contains the other improvements.

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=8654

Chris Dunlop <chris at onthe.net.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7174|0                           |1
        is obsolete|                            |

--- Comment #6 from Chris Dunlop <chris at onthe.net.au> 2011-12-13
02:48:36 UTC ---
Created attachment 7175
  --> https://bugzilla.samba.org/attachment.cgi?id=7175
Improve generation of hash file name

That will teach me to patch on the run.

The previous version didn't compile as it left out an external variable. And
the assert() was incorrect and would trigger every time.

This version is compile and run tested.

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=8654

Chris Dunlop <chris at onthe.net.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7169|0                           |1
        is obsolete|                            |

--- Comment #7 from Chris Dunlop <chris at onthe.net.au> 2011-12-13
03:23:19 UTC ---
Comment on attachment 7169
  --> https://bugzilla.samba.org/attachment.cgi?id=7169
Improve documentation

Correction: it's always an MD4 hash

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=8654

Chris Dunlop <chris at onthe.net.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7170|0                           |1
        is obsolete|                            |

--- Comment #8 from Chris Dunlop <chris at onthe.net.au> 2011-12-13
03:38:27 UTC ---
Created attachment 7176
  --> https://bugzilla.samba.org/attachment.cgi?id=7176
Expand the --link-by-hash manual entry

Correction: it's always an MD4 hash

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=8654

--- Comment #9 from Chris Dunlop <chris at onthe.net.au> 2011-12-14
03:40:08 UTC ---
Created attachment 7177
  --> https://bugzilla.samba.org/attachment.cgi?id=7177
Code cleanup

Make local functions static

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=8654

Wayne Davison <wayned at samba.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #10 from Wayne Davison <wayned at samba.org> 2011-12-16
03:11:55 UTC ---
Thanks for pointing out the various problems with the patch, and providing some
suggested fixes.

I made several changes:

- I changed the code to use the file's MD5 checksum when talking to a modern
rsync (3.0.0 and newer).  This means one less checksum to compute, and most
people don't use an rsync older than 3.x these days.  It does allow the use
of
an MD4 checksum for supporting an older sender, but it requires the user to
force this mode using --checksum-seed=1 --protocol=29 (which will make the
old-style MD4 checksums consistent).  The documentation recommends using one or
the other -- obviously using both would waste space.  This change is
incompatible with an old directory setup, but someone could convert the link
hierarchy via a simple script.

- I unified the checksum's conversion to hex in a single function that log.c
and linkhash.c use.  I don't like the use of sprintf() in a loop, so the
code
continues to do its own hex conversion, just with better size checking.

- I included your daemon setting support, your manpage additions, your
static-function tweaks, and hopefully covered all the issues you found.

- I made the hashlinks.c routines less chatty by putting the output into a new
--debug=hashlink option.

You can see the latest version of the patch via gitweb:

http://gitweb.samba.org/?p=rsync-patches.git

Thanks!

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=8654

--- Comment #11 from Chris Dunlop <chris at onthe.net.au> 2011-12-17
03:06:25 UTC ---
Created attachment 7202
  --> https://bugzilla.samba.org/attachment.cgi?id=7202
link-by-hash: checksum hash required when copying file

(In reply to comment #10)
> Thanks for pointing out the various problems with the patch, and providing
some
> suggested fixes.
> 
> I made several changes:
Looks good!

Your additional --debug=hashlink option was almost word for word identical to a
patch I had sitting here. (I'm learning to not sent off patches as soon as I
create them!)

BTW, referencing your rsync-patches commit message, the surname is
"Dunlop"
(like the tyres) rather than "Dunlap" :-)

See additional patch here: link-by-hash requires the checksum hash when it's
copying a file so that it can properly link into the link farm. In the absence
of this patch I'm seeing it trying to link to the all-zeros hash name
(perhaps
if a previous file had carried a checksum the copy file would link to that
previous checksum?).


Given existing link farms are already affected by the change to the MD5 hash,
it's worth considering another change...

There's a general issue with the link farm directory structure, currently
using
an 8-char top level directory and 24-char second level directory, before
getting to the actual link files.

As the whole point of md5 is to evenly distribute the hash, this means
you'll
essentially end up with a bucket-load of top level directories (up to 16^8),
each containing one second level directory, each containing one link file. 
That's pretty expensive in terms of directory inodes and blocks and
associated
I/O etc.

Perhaps a better structure would be a two-char top level directory (giving 256
in all) and 30-char second level directory, so the top level directories each
contain a reasonable number of second level directories.

That would still leave all your second level directories with one or very few
link files, so maybe even better would be to replace the second level
directories directly with the link files, with a file extension to indicate
different files mapping to the same hash. E.g. instead of
hash[0-7]/hash[8-32]/0, have hash[0-1]/hash[2-32]_00. Even though the top level
directories could end up with large numbers of files, if you have that many
files in the first place you're likely using a non-relic file system with
b-tree indexed directories.

-- 
Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.