Proposal: removeDirectoryRecursive should not follow symlinks

[Simon Hengel]

And for what it's worth, -1 for a deprecation cycle.

On Tue, Jan 06, 2015 at 01:53:58PM +0800, Simon Hengel wrote:

> > I propose to backwards incompatibly change the behavior of
> > removeDirectoryRecursive to not follow symlinks.
>
> +1
>
> > We could optionally add a replacement function under a new name which
> > does follow symlinks.
>
> -1 (I don't see the need for this)
>
> Cheers,
> Simon
> _______________________________________________
> Libraries mailing list
> [hidden email]
> http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Ben Gamari]

"Edward Z. Yang" <[hidden email]> writes:

> Discussion period: one month
>
> I propose to backwards incompatibly change the behavior of
> removeDirectoryRecursive to not follow symlinks.  We could optionally
> add a replacement function under a new name which does follow symlinks.
>
+1. The fact that this function (which I've reached for regularly)
behaves like this is unexpected and a bit terrifying.  It's mildly
amazing that there haven't been more complaints of lost data due to this
behavior.

Cheers,

- Ben

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Bardur Arantsson-2]

On 2015-01-06 02:16, Johan Tibell wrote:
> Let me make a wider comment about backwards compatibility. Many successful
> programming languages (e.g. Java) *never* break backwards compatibility.

That is not quite correct. They're certainly averse to it, but for
example the JDK developers changed String's fundamental behavior wrt.
whether "substring" should copy the relevant string slice or whether it
should just point into the original string. You might argue that the
semantics didn't change, but this change *did* (predictably) break quite
a few programs which suddenly experienced pathological memory allocation
behavior and crashed with OoM's where they had none before.

> They deprecate (and only if the old API is too error prone for the
> programmer) and add a new API. In my opinion breaking backwards
> compatibility is almost never worth it*. Our libraries are already full of
> #ifdefs and maintaining our core libraries (which I maintain some of) is a
> headache because the code gets worse every time we "clean it up".
>
> * And it's only worth it sometimes because we're still a relatively small
> language, by usage.
>

I don't disagree with the general point, but this in this case we're
talking about absurdly dangerous and incorrect behavior which (as Austin
points out is trivially exploitable by messing around in /tmp and
waiting until e.g. a "clean-tmp" cron jobs starts running). AFAICT there
isn't even a function that *does the right thing* in System.Directory!
If someone wants this crazy behavior they can damn well code it
themselves ;)

Regards,

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Ganesh Sittampalam]

On 06/01/2015 05:30, Greg Weber wrote:

> When I suggested deprecation, I assumed that following a symlink was a
> desirable behavior for someone. If it is not and it is 100% the case
> that this behavior is a defect, then this is just a bugfix then
> deprecation is not needed.

My general feeling is that it is just a bug.

> However, since this is considered dangerous, I think Eric makes an
> excellent point that it makes it possible for the function to be used
> properly for one compilation of a library that depends on `directory`,
> but for another compilation to pick up an older version of `directory`.
> That means that just fixing the behavior in the newest version of
> `directory` is not very satisfactory.
>
> So I see 2 approaches to address these issues
>
> 1) the deprecation warning approach
> 2) produce an updated point releases with the bugfix for every major
> version of directory. It still could be a good idea to do the
> deprecation warning on top of this because there are still older
> versions of the function out there with the bad behavior.

Will (2) this be enough for old GHCs, which will have a buggy version of
directory already installed? I think cabal will prefer the installed one
over a point release.

Ganesh

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Bardur Arantsson-2]

On 2015-01-06 08:03, Ganesh Sittampalam wrote:
> On 06/01/2015 05:30, Greg Weber wrote:
>
>> When I suggested deprecation, I assumed that following a symlink was a
>> desirable behavior for someone. If it is not and it is 100% the case
>> that this behavior is a defect, then this is just a bugfix then
>> deprecation is not needed.
>
> My general feeling is that it is just a bug.
>

That's what I thought too -- it's a typical rookie mistake to forget to
check if "isDirecory?" will return "true" for symlinks to directories.
But the documentation actually states the expected behavior correctly --
it's not nearly explicit enough about how dangerous it is, but the
documentation is technically correct.

However, even so, this is CVE-worthy behavior on its own (as pointed out
by Brandon), and should be removed pronto. Perhaps with new minor
versions for all affected major versions (excellent point by Greg
Weber), depending on how much work that is.


_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Roman Cheplyaka-2]

+1 for fixing this asap.


_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Milan Straka]

Hi all,

> -----Original message-----
> From: "Edward Z. Yang" <[hidden email]>
> Sent: 5 Jan 2015, 14:25
>
> I propose to backwards incompatibly change the behavior of
> removeDirectoryRecursive to not follow symlinks.  We could optionally
> add a replacement function under a new name which does follow symlinks.

+1 to changing behaviour of removeDirectoryRecursive.

-1 to depreciation cycle and a replacement function.

To me, this feels like bugfixing, not backwards incompatible change, as
the current behaviour is unexpected and potentially harmful.

Cheers,
Milan Straka
_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Herbert Valerio Riedel]

Hello *,

I'd like to hijack this discussion to bring attention to two IMO
strongly related tickets filed against directory which could benefit from
some feedback/comments:

 - removeFile and directories and symlinks to directories #3
   https://github.com/haskell/directory/issues/3

 - removeDirectory and symlinks #2
   https://github.com/haskell/directory/issues/2


Cheers,
  hvr
_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[David Feuer]

One approach (extreme, perhaps) would be to simply write

{-# DEPRECATED removeDirectoryRecursive #-}
removeDirectoryRecursive = error "This function was horribly dangerous and has been removed. Use _____ instead."

On Jan 5, 2015 8:22 PM, "Eric Mertens" <[hidden email]> wrote:

I'm concerned that changing the behavior of the existing function would make it too easy to write vulnerable programs when compiled with older GHCs. Having a new safe function along with a deprecation warning on the old one would clue people in and avoid functionality varying subtly/dangerously based on the compiler used.

On Mon, Jan 5, 2015, 5:17 PM Johan Tibell <[hidden email]> wrote:

Let me make a wider comment about backwards compatibility. Many successful programming languages (e.g. Java) *never* break backwards compatibility. They deprecate (and only if the old API is too error prone for the programmer) and add a new API. In my opinion breaking backwards compatibility is almost never worth it*. Our libraries are already full of #ifdefs and maintaining our core libraries (which I maintain some of) is a headache because the code gets worse every time we "clean it up".

* And it's only worth it sometimes because we're still a relatively small language, by usage.

On Mon, Jan 5, 2015 at 8:08 PM, Austin Seipp <[hidden email]> wrote:

On Mon, Jan 5, 2015 at 5:51 PM, David Feuer <[hidden email]> wrote:
> In case some people don't understand just how horrible this is: imagine a
> privileged user uses this to erase a user's home directory. It could easily
> hit a symbolic link to a critical system directory and hose the whole
> machine.

I think it's potentially even worse than that: this subtle behavior
could easily be abused for this exact scenario by a hostile entity.
Imagine a scenario where an attacker may have permission to place a
symlink somewhere that a privileged process recursively removes; then
your / (or any number of things) goes 'boom'. This could easily happen
through a combination of a race condition (say, placing junk files in
/tmp you later remove, and the attacker races to place the symlink
there) and an improper umask setting. Or if the attacker simply may be
part of a group that allows access to something a program will remove,
etc etc.

I agree this behavior is fundamentally wrong, and I'm strongly +1 on
changing the existing behavior, which I think is the only sane thing
to do. Otherwise any existing callers of this function in old code
could very easily do The Wrong Thing if they don't get the memo.

I have no opinion on warnings or adding a function that preserves the
old behavior.

> On Jan 5, 2015 6:44 PM, "Brandon Allbery" <[hidden email]> wrote:
>>
>> On Mon, Jan 5, 2015 at 5:54 PM, Johan Tibell <[hidden email]>
>> wrote:
>>>
>>> Aside: can we look at what other languages with similar functions do?
>>
>>
>> You will find that essentially all other implementations do the right
>> thing and not follow symlinks, because the other behavior is a severe bug. I
>> really do not understand why anyone believes the current behavior is
>> defensible.
>>
>> --
>> brandon s allbery kf8nh                               sine nomine
>> associates
>> [hidden email]
>> [hidden email]
>> unix, openafs, kerberos, infrastructure, xmonad
>> http://sinenomine.net
>>
>> _______________________________________________
>> Libraries mailing list
>> [hidden email]
>> http://www.haskell.org/mailman/listinfo/libraries
>>
>
> _______________________________________________
> Libraries mailing list
> [hidden email]
> http://www.haskell.org/mailman/listinfo/libraries
>

--
Regards,

Austin Seipp, Haskell Consultant
Well-Typed LLP, http://www.well-typed.com/

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Andreas Abel]

I also consider this a bugfix.  Bugfixes (hopefully) change semantics
and break backwards-compatibility (but in a good way).

+1 for fixing and backporting as far as possible.

On 06.01.2015 08:53, Milan Straka wrote:

> Hi all,
>
>> -----Original message-----
>> From: "Edward Z. Yang" <[hidden email]>
>> Sent: 5 Jan 2015, 14:25
>>
>> I propose to backwards incompatibly change the behavior of
>> removeDirectoryRecursive to not follow symlinks.  We could optionally
>> add a replacement function under a new name which does follow symlinks.
>
> +1 to changing behaviour of removeDirectoryRecursive.
>
> -1 to depreciation cycle and a replacement function.
>
> To me, this feels like bugfixing, not backwards incompatible change, as
> the current behaviour is unexpected and potentially harmful.
>
> Cheers,
> Milan Straka
> _______________________________________________
> Libraries mailing list
> [hidden email]
> http://www.haskell.org/mailman/listinfo/libraries
>

--
Andreas Abel  <><      Du bist der geliebte Mensch.

Department of Computer Science and Engineering
Chalmers and Gothenburg University, Sweden

[hidden email]
http://www2.tcs.ifi.lmu.de/~abel/
_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Aloïs Cochard]

+1 for fixing this

-1 for the deprecation cycle

I don't think it's sane to rely on the current behavior anyway.

Cheers

On 5 January 2015 at 23:25, Edward Z. Yang <[hidden email]> wrote:

Discussion period: one month

I propose to backwards incompatibly change the behavior of
removeDirectoryRecursive to not follow symlinks.  We could optionally
add a replacement function under a new name which does follow symlinks.

This would bring its behavior inline with rm -rf.

I also opened a ticket here: https://ghc.haskell.org/trac/ghc/ticket/9266

Thanks,
Edward
_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

--

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Johan Tibell-2]

This is not a bugfix. A bug is failing to follow the functions specification, which *does* include following symlinks.

On Tue, Jan 6, 2015 at 7:16 AM, Alois Cochard <[hidden email]> wrote:

+1 for fixing this

-1 for the deprecation cycle

I don't think it's sane to rely on the current behavior anyway.

Cheers

On 5 January 2015 at 23:25, Edward Z. Yang <[hidden email]> wrote:

Discussion period: one month

I propose to backwards incompatibly change the behavior of
removeDirectoryRecursive to not follow symlinks.  We could optionally
add a replacement function under a new name which does follow symlinks.

This would bring its behavior inline with rm -rf.

I also opened a ticket here: https://ghc.haskell.org/trac/ghc/ticket/9266

Thanks,
Edward
_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

--

Λ\ois

http://twitter.com/aloiscochard

http://github.com/aloiscochard

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Mike Meyer]

On Tue, Jan 6, 2015 at 7:48 AM, Johan Tibell <[hidden email]> wrote:

This is not a bugfix. A bug is failing to follow the functions specification, which *does* include following symlinks.

It's a bug in the design, not the code. The API it provides is useful, but dangerous and less useful than the safe version.

Breaking the API has been proposed. If we're going to do that, why not add a followSymlinks ::Bool argument as well, and document why the API was broken in this way. Going forward, at least. Not sure about backwards. 

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Bardur Arantsson-2]

On 2015-01-06 14:57, Mike Meyer wrote:

> On Tue, Jan 6, 2015 at 7:48 AM, Johan Tibell <[hidden email]> wrote:
>
>> This is not a bugfix. A bug is failing to follow the functions
>> specification, which *does* include following symlinks.
>>
>
> It's a bug in the design, not the code. The API it provides is useful, but
> dangerous and less useful than the safe version.
>
> Breaking the API has been proposed. If we're going to do that, why not add
> a followSymlinks ::Bool argument as well, and document why the API was
> broken in this way. Going forward, at least. Not sure about backwards.
>

Because *nobody* wants to follow symlinks when doing "rm -rf". Even if
they think they do, they *really* don't. Any design which relies on it
is probably fundamentally broken. (If they're *really really* sure, they
can write their own.)

Btw, a Bool argumnent usually isn't a good idea. Better API design would be

   data SymlinkBehavior =
      FollowSymlinks
     | NoFollowSymlinks

(so that it's easy to tell at all call sites what it's doing.). However,
without default arguments this is forcing a pointless decision upon the
user, i.e. 99.9%+ of all usages should be using NoFollow, so why force
the user of the API to decide?

Even with such a change, I don't think the functionality should exist in
a "standard" library. (I won't repeat all the arguments; see the rest of
the thread :))

Regards,


_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Mike Meyer]

On Tue, Jan 6, 2015 at 8:59 AM, Bardur Arantsson <[hidden email]> wrote:

Because *nobody* wants to follow symlinks when doing "rm -rf". Even if
they think they do, they *really* don't. Any design which relies on it
is probably fundamentally broken. (If they're *really really* sure, they
can write their own.)

Don't need to reread the arguments - I've been aware of them since CSRG introduced symlinks in 4BSD, and the utilities didn't know about them in the beta releases.

You pretty much never want to call it on a file tree you didn't build.  But if I built the file tree, then I know what's going on. Making me write my own is probably going to result in less robust code, or copying the code out of the library and tweaking it, neither of which is desirable.

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Bardur Arantsson-2]

On 2015-01-06 16:43, Mike Meyer wrote:

> On Tue, Jan 6, 2015 at 8:59 AM, Bardur Arantsson <[hidden email]>
> wrote:
>>
>> Because *nobody* wants to follow symlinks when doing "rm -rf". Even if
>> they think they do, they *really* don't. Any design which relies on it
>> is probably fundamentally broken. (If they're *really really* sure, they
>> can write their own.)
>>
>
> Don't need to reread the arguments - I've been aware of them since CSRG
> introduced symlinks in 4BSD, and the utilities didn't know about them in
> the beta releases.
>
> You pretty much never want to call it on a file tree you didn't build.  But
> if I built the file tree, then I know what's going on.

Indeed, but even in *that* case it's incredibly error-prone...

> Making me write my
> own is probably going to result in less robust code, or copying the code
> out of the library and tweaking it, neither of which is desirable.
>

It's such a rare need and so dangerous that I'd argue that forcing a
copy + tweak is actually desirable in this case.

Regards,

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[David Feuer]

So stick it in a directory-for-insane-people package and call it removeDirectoryRecursiveFollowSymlinksISwearIReallyWantThat.

On Jan 6, 2015 10:43 AM, "Mike Meyer" <[hidden email]> wrote:

On Tue, Jan 6, 2015 at 8:59 AM, Bardur Arantsson <[hidden email]> wrote:

Because *nobody* wants to follow symlinks when doing "rm -rf". Even if
they think they do, they *really* don't. Any design which relies on it
is probably fundamentally broken. (If they're *really really* sure, they
can write their own.)

Don't need to reread the arguments - I've been aware of them since CSRG introduced symlinks in 4BSD, and the utilities didn't know about them in the beta releases.

You pretty much never want to call it on a file tree you didn't build.  But if I built the file tree, then I know what's going on. Making me write my own is probably going to result in less robust code, or copying the code out of the library and tweaking it, neither of which is desirable.

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Malcolm Wallace-2]

On 6 Jan 2015, at 14:59, Bardur Arantsson wrote:

> On 2015-01-06 14:57, Mike Meyer wrote:
>> On Tue, Jan 6, 2015 at 7:48 AM, Johan Tibell <[hidden email]> wrote:
>>
>>> This is not a bugfix. A bug is failing to follow the functions
>>> specification, which *does* include following symlinks.
>>>
>>
>> It's a bug in the design, not the code.

> Because *nobody* wants to follow symlinks when doing "rm -rf". Even if
> they think they do, they *really* don't.

I agree 100%.  Even time I use this function, I worry briefly about whether it follows symlinks, then think to myself "no, no-one would be so stupid to implement that deliberately in a publically available API".  So it was a real shock to discover in this thread that I was wrong, and furthermore that the function is documented as doing the wrong thing.  We should fix both spec and implementation, as soon as possible.

Regards,
    Malcolm
_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Edward Kmett-2]

I'm +1 for fixing this, in place, on the current function.

The specification we have here is doing a very very bad thing and needs to be fixed, not slavishly copied forward because someone sometime once made a mistake. 

The current behavior grievously violates the expectations of anyone who would be in a situation to go and reach for it and has any prior experience with any other such tool.

-Edward

On Tue, Jan 6, 2015 at 11:14 AM, Malcolm Wallace <[hidden email]> wrote:

On 6 Jan 2015, at 14:59, Bardur Arantsson wrote:

> On 2015-01-06 14:57, Mike Meyer wrote:
>> On Tue, Jan 6, 2015 at 7:48 AM, Johan Tibell <[hidden email]> wrote:
>>
>>> This is not a bugfix. A bug is failing to follow the functions
>>> specification, which *does* include following symlinks.
>>>
>>
>> It's a bug in the design, not the code.

> Because *nobody* wants to follow symlinks when doing "rm -rf". Even if
> they think they do, they *really* don't.

I agree 100%.  Even time I use this function, I worry briefly about whether it follows symlinks, then think to myself "no, no-one would be so stupid to implement that deliberately in a publically available API".  So it was a real shock to discover in this thread that I was wrong, and furthermore that the function is documented as doing the wrong thing.  We should fix both spec and implementation, as soon as possible.

Regards,
    Malcolm

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

[Gabriel Gonzalez]

I think it's safer to remove the old function altogether (perhaps after one deprecation cycle) and provide a new one under a different name, rather than modify it in place.

Modifying it in place risks the behavior that others mentioned where your program is unsafe to compile against older library versions.  Yes, the user could explicitly enforce that by putting a lower bound on the library, but most users won't even realize that they need to do that.

On 1/6/15, 11:37 AM, Edward Kmett wrote:

I'm +1 for fixing this, in place, on the current function.

The specification we have here is doing a very very bad thing and needs to be fixed, not slavishly copied forward because someone sometime once made a mistake. 

The current behavior grievously violates the expectations of anyone who would be in a situation to go and reach for it and has any prior experience with any other such tool.

-Edward

On Tue, Jan 6, 2015 at 11:14 AM, Malcolm Wallace <[hidden email]> wrote:

On 6 Jan 2015, at 14:59, Bardur Arantsson wrote:

> On 2015-01-06 14:57, Mike Meyer wrote:
>> On Tue, Jan 6, 2015 at 7:48 AM, Johan Tibell <[hidden email]> wrote:
>>
>>> This is not a bugfix. A bug is failing to follow the functions
>>> specification, which *does* include following symlinks.
>>>
>>
>> It's a bug in the design, not the code.

> Because *nobody* wants to follow symlinks when doing "rm -rf". Even if
> they think they do, they *really* don't.

I agree 100%.  Even time I use this function, I worry briefly about whether it follows symlinks, then think to myself "no, no-one would be so stupid to implement that deliberately in a publically available API".  So it was a real shock to discover in this thread that I was wrong, and furthermore that the function is documented as doing the wrong thing.  We should fix both spec and implementation, as soon as possible.

Regards,
    Malcolm

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries

_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries