The Samba-Bugzilla – Bug 7057
Buffer overflow when sending a file with long name
Last modified: 2010-03-31 13:39:43 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.
Created attachment 5215 [details]
Patch adding string boundary check
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().
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.
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.
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.
Re the Fedora situation, see my comment at https://bugzilla.redhat.com/show_bug.cgi?id=557916#c9 .
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.
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?
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!
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.
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>/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
IIUC, the "skipping long-named directory" check in send_if_directory is now redundant and can be removed.