Bug 7081 - vfs_expand_msdfs doesn't work correctly (with fix identified)
Summary: vfs_expand_msdfs doesn't work correctly (with fix identified)
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.4
Classification: Unclassified
Component: File services (show other bugs)
Version: 3.4.3
Hardware: Other Linux
: P3 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
: 5998 7086 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-01-29 17:13 UTC by Eric Horst
Modified: 2010-02-28 07:57 UTC (History)
1 user (show)

See Also:


Attachments
Patch for master/3.5.0. (1.29 KB, patch)
2010-02-02 16:16 UTC, Jeremy Allison
no flags Details
git-am fix for 3.5.0. (1.79 KB, patch)
2010-02-02 18:46 UTC, Jeremy Allison
no flags Details
git-am fix for 3.4.6. (1.77 KB, patch)
2010-02-02 18:49 UTC, Jeremy Allison
no flags Details
Second part of patch for 3.5.0. (1.05 KB, patch)
2010-02-03 14:57 UTC, Jeremy Allison
no flags Details
Second part of patch for 3.4.6. (1.05 KB, patch)
2010-02-03 16:07 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Horst 2010-01-29 17:13:24 UTC
We've been using vfs_expand_msdfs for years with custom modifications. We recently updated from 3.0.x to 3.4.x and found the vfs_expand_msdfs didn't work. This is the error:

[2010/01/28 13:53:58,  0, pid=31373] lib/util_str.c:532(safe_strcpy_fn)
  ERROR: string overflow by 1 (6 - 5) in safe_strcpy [msdfs:servername.washington.edu\homes]

This appears to be the same as bug 5998 reported a long while ago against Samba 3.2. I'd guess that expand_msdfs doesn't get much use since this bug has been around without much complaint.

The bug is revealed because the function msdfs.c:is_msdfs_link_internal() provides a buffer of only 7 bytes to the VFS readlink() call in the case where it is simply trying to test if the link is an msdfs link. Given this 7 byte buf, the function vfs_expand_msdfs.c:expand_msdfs_readlink() does not comply with readlink() semantics. Notably, at the end of the function, it tries to put a string of possibly MAX_PATH length into a buffer that is too small, typically 7 bytes. The safe_strcpy call is what generates the error.

The readlink() semantics are:
1. places the contents of the symbolic link in buf which has bufsiz.
2. does not append a null to buf
3. will truncate the contents to bufsiz characters if buf is too small.
4. returns the number of bytes placed in buf.

In expand_msdfs_readlink(), safe_strcpy() is used to fill the buffer which isn't ideal, especially where the buffer is too small since strcpy requires the dest to be big enough and assumes it should null terminate. This is where the error message occurs. This shouldn't be done with strcpy and buf should be filled with as many bytes as possible and no nul.

Second, readlink() returns the number of bytes placed in the buffer. However, this function tries to do that with strlen() which would fail with a non-null terminated buf or one that was too small to hold the whole target.

I'm a rusty C programmer. Here is a fix that probably could be more elegant but makes expand_msdfs_readlink() comply with readlink() semantics and fixes the problem. Replace this:

        safe_strcpy(buf, target, bufsiz-1);
        return strlen(buf);

With this:

        result = (strlen(target) < bufsiz) ? strlen(target) : bufsiz;
        memcpy(buf, target, result);
        return result;

This will return the smaller of bufsiz or the length of the symlink target. It will fill buf with the full symlink target or truncate it to bufsiz. No nul termination, I might need that byte if my link target is MAX_PATH long.

-Eric
Comment 1 Eric Horst 2010-02-02 10:33:11 UTC
*** Bug 7086 has been marked as a duplicate of this bug. ***
Comment 2 Jeremy Allison 2010-02-02 16:16:37 UTC
Created attachment 5258 [details]
Patch for master/3.5.0.

Can you check this works for you please ? Should make the readlink call match the POSIX spec.
Jeremy.
Comment 3 Jeremy Allison 2010-02-02 16:17:04 UTC
*** Bug 5998 has been marked as a duplicate of this bug. ***
Comment 4 Jeremy Allison 2010-02-02 18:46:17 UTC
Created attachment 5263 [details]
git-am fix for 3.5.0.
Comment 5 Jeremy Allison 2010-02-02 18:49:35 UTC
Created attachment 5264 [details]
git-am fix for 3.4.6.
Comment 6 Eric Horst 2010-02-03 12:11:49 UTC
> Can you check this works for you please ? Should make the readlink call match
> the POSIX spec.
> Jeremy.

Yes, it looks good and works great. Thanks Jeremy.

-Eric 

Comment 7 Jeremy Allison 2010-02-03 14:57:48 UTC
Created attachment 5267 [details]
Second part of patch for 3.5.0.

From Volker - please add to 3.5.0 in addition to attachment #5263 [details].
Comment 8 Jeremy Allison 2010-02-03 16:07:44 UTC
Created attachment 5268 [details]
Second part of patch for 3.4.6.
Comment 9 Jeremy Allison 2010-02-03 16:08:12 UTC
Re-assigning to Karolin for inclusion in 3.4.6 and 3.5.0.
Jeremy.
Comment 10 Karolin Seeger 2010-02-04 04:02:12 UTC
Pushed to v3-5-test and v3-4-test.
Closing out bug report.

Thanks!