Bug 8663 - deleting a symlink fails if the symlink target is outside of the share
deleting a symlink fails if the symlink target is outside of the share
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: File services
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
: 8696 (view as bug list)
Depends on:
Blocks: 8595
  Show dependency treegraph
 
Reported: 2011-12-15 14:32 UTC by samba-bugs
Modified: 2012-06-06 12:28 UTC (History)
4 users (show)

See Also:


Attachments
git-am fix for 3.5.next (4.33 KB, patch)
2011-12-16 00:07 UTC, Jeremy Allison
no flags Details
git-am fix for 3.5.next (5.24 KB, patch)
2011-12-16 20:46 UTC, Jeremy Allison
no flags Details
git-am fix for 3.5.next (5.31 KB, patch)
2011-12-16 23:45 UTC, Jeremy Allison
jra: review? (metze)
Details
git-am fix for 3.6.x (5.20 KB, patch)
2011-12-17 00:02 UTC, Jeremy Allison
jra: review? (metze)
ddiss: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description samba-bugs 2011-12-15 14:32:39 UTC
This bug is similar to bug 8541.
If i create a symlink on a samba share using a linux client (unix extensions enabled, wide links disabled) which points to a file that exists on the server it can be created and read but not deleted.

Steps to reproduce:
In a directory mounted via cifs from a samba-server:
ln -s . link1
ln -s / link2
ln -s /a link3
rm link1
(no error, is inside the share)
rm link3
(no error, does not exist on the server)
rm link2
(rm: cannot remove `link2': Permission denied)
Comment 1 Jeremy Allison 2011-12-16 00:07:18 UTC
Created attachment 7194 [details]
git-am fix for 3.5.next

This fixes it for me. I'll create fixes for master and 3.6.next.

Jeremy.
Comment 2 Jeremy Allison 2011-12-16 16:44:27 UTC
Comment on attachment 7194 [details]
git-am fix for 3.5.next

Gonna make one more check before I'm convinced this is correct. Bear with me.. :-).
Comment 3 Jeremy Allison 2011-12-16 20:46:07 UTC
Created attachment 7197 [details]
git-am fix for 3.5.next

Ok, I'm happy with this one !

Jeremy.
Comment 4 Volker Lendecke 2011-12-16 20:56:35 UTC
Sorry, but probably I just don't understand Posix permissions. symlinks do not have any permissions on their own at all? Do you have a reference for that?

Thanks,

Volker Lendecke
Comment 5 Jeremy Allison 2011-12-16 21:09:45 UTC
No, it isn't that they don't have permissions, it's just that we shouldn't check them as we'll never store Windows permissions separately on them. So we should just do the delete and allow the OS to decide.

Jeremy.
Comment 6 Jeremy Allison 2011-12-16 23:45:12 UTC
Created attachment 7199 [details]
git-am fix for 3.5.next

One last (hopefully :-) change. Ensure VALID_STAT before checking the ISLNK in patch #3.
Comment 7 Jeremy Allison 2011-12-17 00:02:02 UTC
Created attachment 7200 [details]
git-am fix for 3.6.x

Same fix for 3.6.next.
Comment 8 Volker Lendecke 2011-12-17 10:46:38 UTC
Ok, letting that grow for a week or two. That seems to be still under active development.

Volker
Comment 9 Jeremy Allison 2011-12-19 16:53:36 UTC
Think I'm done now :-). So long as it gets in for 3.5.next and 3.6.next I'm happy to wait over Christmas.
Jeremy.
Comment 10 Karolin Seeger 2012-01-08 19:55:48 UTC
Is there a chance to get the reviews done until Thursday?
The patch could be included in 3.6.2 then.

Thanks,
Karolin
Comment 11 Jeremy Allison 2012-01-11 21:32:18 UTC
*** Bug 8696 has been marked as a duplicate of this bug. ***
Comment 12 Jeremy Allison 2012-01-17 20:50:45 UTC
Comment on attachment 7200 [details]
git-am fix for 3.6.x

Adding David - need to get this reviewed for 3.6.next!
Comment 13 David Disseldorp 2012-01-17 21:56:51 UTC
(In reply to comment #12)
...
> Adding David - need to get this reviewed for 3.6.next!

Patch looks good at first glance Jeremy. I'd like to do some testing tomorrow before giving it the final +.
Comment 14 David Disseldorp 2012-01-19 11:37:20 UTC
(In reply to comment #13)
> (In reply to comment #12)
> ...
> > Adding David - need to get this reviewed for 3.6.next!
> 
> Patch looks good at first glance Jeremy. I'd like to do some testing tomorrow
> before giving it the final +.

Looking at the patch again, the check_name() removals from the open path have me concerned. IIUC it makes veto_files and widelink enforcement the responsibility of the IO path and as I see it these checks are currently missing.
Comment 15 Jeremy Allison 2012-01-19 18:32:44 UTC
check_name() is called inside filename_convert(), just after unix_convert(). This is the path that also handles the DFS redirect calls.

All paths that get to open_file() *must* have gone through filename_convert() (or in the case of rename or unlink calls where the name is pulled from a directory listing they call check_name() directly.

There is no codepath (*) where open can get called without going through check_name() which is why these changes are safe.

Jeremy.

(*) No Windows codepath - the UNIX codepath case for POSIX calls from a Linux client are safe as they always return the value of the LSTAT call, not a STAT call - so a link is never followed (that's what part of these changes are about).
Comment 16 David Disseldorp 2012-01-20 10:04:01 UTC
Comment on attachment 7200 [details]
git-am fix for 3.6.x

Thanks for the explanation Jeremy.
Comment 17 Jeremy Allison 2012-01-20 19:29:19 UTC
Re-assigning to Karolin for inclusion in 3.5.next and 3.6.2.

Jeremy.
Comment 18 Karolin Seeger 2012-01-21 20:07:50 UTC
Pushed to v3-5-test and v3-6-test.
Closing out bug report.

Thanks!