Bug 343571: Dragging and dropping a directory containing a symlink results in the symlink being ign…

Bug 343571: Dragging and dropping a directory containing a symlink results in the symlink being ignored.

DraggedFileUtil inherits some of its operations from LocalFileUtil and its LocalFileEnumerator, both performing checks to avoid following symbolic links for security reasons (as it should be for generic local files).

However, files coming from a drag & drop operation can be considered "trusted files" (after all, the user is who is dropping them there on purpose) and should honor symbolic links.

This patch modifies DraggedFileUtil to override that behaviour, acting as LocalFileUtil does, but without skipping symbolic links.

To achieve this, a new FollowSymlinksLocalFileEnumerator is defined, which also does the same as LocalFileEnumerator but without the checks.

BUG=343571

[kinuko, 2014-03-10 02:33:05]

not lgtm. We skip symlinks intentionally (crbug.com/229849) to avoid potential
security risks (e.g. a dragged folder may have a link to /etc) and infinite
recursive calls (e.g. if a dragged folder has a symlink pointing to its ancestor
recursive traversal of the dragged folder can't finish forever).  We can't just
add this feature without addressing these issues.

[eocanha, 2014-03-10 17:22:17]

On 2014/03/10 02:33:05, kinuko wrote:

> not lgtm.

I understand that, as owner, you have the last word on the security implications
of a patch like this one. However, the following arguments can add some context
to the discussion:

> We skip symlinks intentionally (crbug.com/229849) to avoid potential
> security risks (e.g. a dragged folder may have a link to /etc) and infinite

The system-provided "open file" dialog used when uploading a file in a form also
has that problem and nobody seems to care. In my case, it's the KDE "open file"
dialog.

> recursive calls (e.g. if a dragged folder has a symlink pointing to its
ancestor
> recursive traversal of the dragged folder can't finish forever).

Recursive symlinks cause an ELOOP error. Therefore, there's no actual infinite
recursion:

[17212:17243:0310/175328:25984909737:ERROR:file_enumerator_posix.cc(148)]
Couldn't stat
/tmp/test/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/android2.png:
Too many levels of symbolic links

>  We can't just
> add this feature without addressing these issues.

Do you think this patch could have an opportunity to be accepted if I try to
address the concerns you mention (early detection of cycles and forbidding
access to selected system directories)? Or going down that road doesn't worth
the effort?

Thank you.

[kinuko, 2014-03-11 03:56:06]

(+jschuh@)

I'm not the best person to judge the security implication here, let me add
jschuh@ to hear how this sounds to him.

Justin, we're talking about the possibility to symlinks support in directory
drag-and-drop feature. I'm being afraid that it may open up another security
concern as one could drag-and-drop a folder which has a symlink to '/', while
it's also true that that's what the user dropped and s/he might have actually
wanted to do. What's your opinion here?

On 2014/03/10 17:22:17, eocanha wrote:
> On 2014/03/10 02:33:05, kinuko wrote:
> > not lgtm.
> 
> I understand that, as owner, you have the last word on the security
implications
> of a patch like this one. However, the following arguments can add some
context
> to the discussion:
> 
> > We skip symlinks intentionally (crbug.com/229849) to avoid potential
> > security risks (e.g. a dragged folder may have a link to /etc) and infinite
> 
> The system-provided "open file" dialog used when uploading a file in a form
also
> has that problem and nobody seems to care. In my case, it's the KDE "open
file"
> dialog.

I understand your point here, but we can't naively compare what native apps
(which has access to system-provided dialog) and web apps do, since users could
run arbitrary web apps far more easily than native apps without noticing it
could do something wrong.

> > recursive calls (e.g. if a dragged folder has a symlink pointing to its
> ancestor
> > recursive traversal of the dragged folder can't finish forever).
> 
> Recursive symlinks cause an ELOOP error. Therefore, there's no actual infinite
> recursion:
> [17212:17243:0310/175328:25984909737:ERROR:file_enumerator_posix.cc(148)]
> Couldn't stat
>
/tmp/test/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/test2/android2.png:
> Too many levels of symbolic links

'infinite recursion' was inaccurate, I should have probably said 'too many
levels of recursion'.
This makes chrome try to upload too many duplicated entries for directory upload
by drag-and-drop, which was the issue we fixed in http://crbug.com/229849 (which
virtually made it skip symlinks), and we need not to regress on this. (So this
one is not really about security, but about preferred behavior)

> >  We can't just
> > add this feature without addressing these issues.
> 
> Do you think this patch could have an opportunity to be accepted if I try to
> address the concerns you mention (early detection of cycles and forbidding
> access to selected system directories)? Or going down that road doesn't worth
> the effort?

Let's see how security folks respond.

[jschuh, 2014-03-11 21:26:16]

The root problem here is that the sandbox architecture and
ChildProcessSecurityPolicy base their security decisions on the assumption of
distinct absolute paths. So, things get fuzzy if we introduce symbolic links,
which can change that.

@tsepez - You probably have better context here.

[Tom Sepez, 2014-03-13 18:34:18]

I agree with Justin and kinuko in that the assumptions are sprinkled throughout
chrome, and we're better off without this.

[eocanha, 2014-03-13 19:01:02]

On 2014/03/13 18:34:18, Tom Sepez wrote:
> I agree with Justin and kinuko in that the assumptions are sprinkled
throughout
> chrome, and we're better off without this.

Thank you all for having had a look at this. Now I understand the full
consequences of enabling symbolic links.

I've left a comment in the bug explaining the situation. It would be great if
somebody could mark the bug as WONTFIX (I don't have permission).