Bug 7057 - Buffer overflow when sending a file with long name
Summary: Buffer overflow when sending a file with long name
Alias: None
Product: rsync
Classification: Unclassified
Component: core (show other bugs)
Version: 3.0.7
Hardware: All Linux
: P3 normal (vote)
Target Milestone: ---
Assignee: Wayne Davison
QA Contact: Rsync QA Contact
URL: https://bugzilla.redhat.com/show_bug....
Depends on:
Reported: 2010-01-22 16:05 UTC by Jan Zeleny
Modified: 2010-03-31 13:39 UTC (History)
0 users

See Also:

Patch adding string boundary check (455 bytes, patch)
2010-01-22 16:06 UTC, Jan Zeleny
no flags Details
Reproducer (434 bytes, application/x-bzip2)
2010-03-23 19:32 UTC, Matt McCutchen
no flags Details
Rewritten patch doing boundary check (423 bytes, patch)
2010-03-24 08:54 UTC, Jan Zeleny
no flags Details
My fix for the issue. (1.12 KB, patch)
2010-03-26 19:06 UTC, Wayne Davison
no flags Details
Vary the final dir len (creates 3 final dirs) and puts 3 files in those dirs with varying name lengths. (765 bytes, text/plain)
2010-03-26 19:16 UTC, Wayne Davison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Zeleny 2010-01-22 16:05:20 UTC
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.
Comment 1 Jan Zeleny 2010-01-22 16:06:31 UTC
Created attachment 5215 [details]
Patch adding string boundary check
Comment 2 Wayne Davison 2010-01-26 17:45:47 UTC
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().
Comment 3 Jan Zeleny 2010-01-27 03:20:50 UTC
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.
Comment 4 Matt McCutchen 2010-03-17 03:16:56 UTC
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.
Comment 5 Jan Zeleny 2010-03-17 08:26:46 UTC
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.
Comment 6 Matt McCutchen 2010-03-23 17:34:36 UTC
Re the Fedora situation, see my comment at https://bugzilla.redhat.com/show_bug.cgi?id=557916#c9 .
Comment 7 Matt McCutchen 2010-03-23 19:32:54 UTC
Created attachment 5529 [details]

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.
Comment 8 Jan Zeleny 2010-03-24 08:54:51 UTC
Created attachment 5530 [details]
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?
Comment 9 Wayne Davison 2010-03-26 19:02:30 UTC
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!
Comment 10 Wayne Davison 2010-03-26 19:06:21 UTC
Created attachment 5551 [details]
My fix for the issue.

Here's the patch that was committed to both the b3.0.x and master branches.
Comment 11 Wayne Davison 2010-03-26 19:16:46 UTC
Created attachment 5553 [details]
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
Comment 12 Matt McCutchen 2010-03-31 13:39:43 UTC
IIUC, the "skipping long-named directory" check in send_if_directory is now redundant and can be removed.