rsync - Jan 2010 - DO NOT REPLY [Bug 7057] New: Buffer overflow when sending a file with long name

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

           Summary: Buffer overflow when sending a file with long name
           Product: rsync
           Version: 3.0.7
          Platform: All
               URL: https://bugzilla.redhat.com/show_bug.cgi?id=557916
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: P3
         Component: core
        AssignedTo: wayned at samba.org
        ReportedBy: jzeleny at redhat.com
         QAContact: rsync-qa at samba.org


There is a description of the issue in the bug report given in URL. What I
found out is that most likely there is a bug in function f_name(). There is no
string bounding checked when making a copy of a file path. That leads to buffer
overflow in function send1extra() and possibly in other functions. Attaching a
patch, which should be resolving this, but I'm not sure if I took the right
approach. Please check it.


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

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





------- Comment #1 from jzeleny at redhat.com  2010-01-22 16:06 CST -------
Created an attachment (id=5215)
 --> (https://bugzilla.samba.org/attachment.cgi?id=5215&action=view)
Patch adding string boundary check


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

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





------- Comment #2 from wayned at samba.org  2010-01-26 17:45 CST -------
Take note of the comment right before the function you patched:

No size-checking is done because we checked the size when creating the
file_struct entry.

If you have a test case where dirname >= MAXPATHLEN, I'd love to see it
since
that would mean something very wrong happened long before the call to f_name().


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

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





------- Comment #3 from jzeleny at redhat.com  2010-01-27 03:20 CST -------
I noticed the comment and I also realize it is indeed strange behavior. But if
you look at referenced redhat bugzilla and backtrace provided there, it is the
only option that seems to be even remotely possible. If you have other idea
what could cause that traceback, please let me know. I will try to create
reproducer for this, so it can be investigated further.


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

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





------- Comment #4 from matt at mattmccutchen.net  2010-03-17 03:16 CST -------
The patch has been pushed to Fedora stable, though we have no evidence besides
that one abrt report that the problem is real.  Can't say I'm happy
about that.


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

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





------- Comment #5 from jzeleny at redhat.com  2010-03-17 08:26 CST -------
The patch has been pushed there automatically because nobody complained about
it during testing phase and there were several positive evaluations in Bodhi.
Now I'm waiting for the original reporter to confirm the "fix". If
the
situation changes and the patch will be proven wrong, of course I'll
withdraw
it.


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

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





------- Comment #6 from matt at mattmccutchen.net  2010-03-23 17:34 CST -------
Re the Fedora situation, see my comment at
https://bugzilla.redhat.com/show_bug.cgi?id=557916#c9 .


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

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





------- Comment #7 from matt at mattmccutchen.net  2010-03-23 19:32 CST -------
Created an attachment (id=5529)
 --> (https://bugzilla.samba.org/attachment.cgi?id=5529&action=view)
Reproducer

I think I found the problem, and no, it isn't fixed by the proposed patch. 
If
send_directory is called with dlen == MAXPATHLEN - 1, it will append a slash
and then write a null byte just beyond the buffer.  Attached is a reproducer. 
I reproduced the fortify failure on i686.  For some reason I did not get a
fortify failure on x86_64, but I got a valgrind error if I changed the buffer
to be heap allocated.


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

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


jzeleny at redhat.com changed:

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




------- Comment #8 from jzeleny at redhat.com  2010-03-24 08:54 CST -------
Created an attachment (id=5530)
 --> (https://bugzilla.samba.org/attachment.cgi?id=5530&action=view)
Rewritten patch doing boundary check

I didn't manage to reproduce the issue either, but I went ahead and traced
the
issue step by step. You were right, the problem is when the path is
MAXPATHLEN-1 characters long, I just didn't pinpoint the problematic place
in
code accurately before. I put together another patch, I'm sending in
attachment. What do you think of it? Output of fixed rsync run by your
reproducer is:

$ rsync -n -a src/ dest/
Directory path too long
rsync error: some files/attrs were not transferred (see previous errors) (code
23) at main.c(1042) [sender=3.0.7]

I know it's the simplest solution, but I think it's acceptable. Of
course I
based it on an assumption, that OS will have problems with paths longer than
MAXPATHLEN anyway. Or am I mistaken?


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

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


wayned at samba.org changed:

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




------- Comment #9 from wayned at samba.org  2010-03-26 19:02 CST -------
The latest patch should fix the bug, though it should really call closedir(d)
before returning.

I've chosen to fix the issue in a different manner that will make rsync only
output about files inside the directory that it cannot send (since the dir
itself is short enough that we can send it).  I also fixed up the output so
that it reports the real filenames (not truncated filenames) and mentions how
big the overflow actually is.

Thanks for the analysis/patch/reproducer!


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

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





------- Comment #10 from wayned at samba.org  2010-03-26 19:06 CST -------
Created an attachment (id=5551)
 --> (https://bugzilla.samba.org/attachment.cgi?id=5551&action=view)
My fix for the issue.

Here's the patch that was committed to both the b3.0.x and master branches.


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

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





------- Comment #11 from wayned at samba.org  2010-03-26 19:16 CST -------
Created an attachment (id=5553)
 --> (https://bugzilla.samba.org/attachment.cgi?id=5553&action=view)
Vary the final dir len (creates 3 final dirs) and puts 3 files in those dirs
with varying name lengths.

My version of the make-chain script puts the files "1" "22"
and "333" into the
final dir, and also into 2 other dirs (so they are 13, 14, and 15 chars long). 
This results in:

<13-char>/1        succeeds
<13-char>/22       fails by 1
<13-char>/333      fails by 2
<14-char>/1        fails by 1
<14-char>/22       fails by 2
<14-char>/333      fails by 3
<15-char>/1        fails by 2
<15-char>/22       fails by 3
<15-char>/333      fails by 4


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

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





------- Comment #12 from matt at mattmccutchen.net  2010-03-31 13:39 CST -------
IIUC, the "skipping long-named directory" check in send_if_directory
is now
redundant and can be removed.


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