Proposal: removeDirectoryRecursive should not follow symlinks

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
52 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Proposal: removeDirectoryRecursive should not follow symlinks

Edward Z. Yang
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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

David Feuer
+1. Following symlinks in such a potentially-destructive operation is
fundamentally wrong.

On Mon, Jan 5, 2015 at 5:25 PM, 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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Greg Weber
How about being backwards-compatible friendly by adding a new function with the friendly behavior, adding a deprecation notice to the existing function, and putting the existing function under a new name that signifies the -rf? That would put me at a +1

On Mon, Jan 5, 2015 at 2:31 PM, David Feuer <[hidden email]> wrote:
+1. Following symlinks in such a potentially-destructive operation is
fundamentally wrong.

On Mon, Jan 5, 2015 at 5:25 PM, 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


_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Brandon Allbery
On Mon, Jan 5, 2015 at 5:33 PM, Greg Weber <[hidden email]> wrote:
How about being backwards-compatible friendly by adding a new function with the friendly behavior, adding a deprecation notice to the existing function, and putting the existing function under a new name that signifies the -rf? That would put me at a +1

To be honest, that it followed symlinks in the first place is arguably a severe bug and also violates people's expectations. I suspect most existing users would either be (a) applying it to something they created with a known safe structure, so the change is irrelevant, or (b) horrified at the unexpected lurking bug.

Functions like this should not follow symlinks.

--
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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

David Feuer
In reply to this post by Greg Weber
This is much, much worse than -rf. GNU rm, at least, offers no way at
all to get the current behavior of removeDirectoryRecursive. I don't
think it's okay to leave it in the library by this name, and I'm a
moderate -1 on giving it a new name.

On Mon, Jan 5, 2015 at 5:33 PM, Greg Weber <[hidden email]> wrote:

> How about being backwards-compatible friendly by adding a new function with
> the friendly behavior, adding a deprecation notice to the existing function,
> and putting the existing function under a new name that signifies the -rf?
> That would put me at a +1
>
> On Mon, Jan 5, 2015 at 2:31 PM, David Feuer <[hidden email]> wrote:
>>
>> +1. Following symlinks in such a potentially-destructive operation is
>> fundamentally wrong.
>>
>> On Mon, Jan 5, 2015 at 5:25 PM, 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
>
>
_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Edward Z. Yang
In reply to this post by Greg Weber
Sure, I'm happy for someone to put this forward as a counterproposal.

My reasoning for changing the meaning of the identifier: I'm pretty
sure almost everyone using the current function doesn't actually
want to follow symlinks.  To actually verify this I'd have to do
some Hackage spelunking.

Edward

Excerpts from Greg Weber's message of 2015-01-05 14:33:41 -0800:

> How about being backwards-compatible friendly by adding a new function with
> the friendly behavior, adding a deprecation notice to the existing
> function, and putting the existing function under a new name that signifies
> the -rf? That would put me at a +1
>
> On Mon, Jan 5, 2015 at 2:31 PM, David Feuer <[hidden email]> wrote:
>
> > +1. Following symlinks in such a potentially-destructive operation is
> > fundamentally wrong.
> >
> > On Mon, Jan 5, 2015 at 5:25 PM, 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
> >
_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Bardur Arantsson-2
In reply to this post by Edward Z. Yang
On 2015-01-05 23:25, Edward Z. Yang 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
>

+1 as is.

-1 on adding a deprecation cycle unless someone can come up with a
REALLY good reason for that.


_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Andreas Abel
+1 to change implementation as proposed.

On 05.01.2015 23:46, Bardur Arantsson wrote:

> On 2015-01-05 23:25, Edward Z. Yang 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
>>
>
> +1 as is.
>
> -1 on adding a deprecation cycle unless someone can come up with a
> REALLY good reason for that.
>
>
> _______________________________________________
> 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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Johan Tibell-2
-1 For changing the existing function.

+1 For adding it under a different name. +1 to adding a deprecation pragma to the old one, if we consider the old behavior harmful to users.

Aside: can we look at what other languages with similar functions do?

On Mon, Jan 5, 2015 at 5:51 PM, Andreas Abel <[hidden email]> wrote:
+1 to change implementation as proposed.


On 05.01.2015 23:46, Bardur Arantsson wrote:
On 2015-01-05 23:25, Edward Z. Yang 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


+1 as is.

-1 on adding a deprecation cycle unless someone can come up with a
REALLY good reason for that.


_______________________________________________
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


_______________________________________________
Libraries mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/libraries
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Brandon Allbery
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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

David Feuer

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.

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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

amindfv
In reply to this post by Brandon Allbery
Wow, I did not realize this was the behavior. I would consider it a major bug.

+1 to removing, -1 to any deprecation periods or delay, +0 to adding the existing function with a new name (as long as warnings are clearly visible)

I would also support backporting a warning to haddocks for versions before the "fix"

Tom


El Jan 5, 2015, a las 18:44, Brandon Allbery <[hidden email]> escribió:

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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Austin Seipp-5
In reply to this post by David Feuer
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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Johan Tibell-2
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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Eric Mertens

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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Brandon Allbery
In reply to this post by Johan Tibell-2
On Mon, Jan 5, 2015 at 8:16 PM, Johan Tibell <[hidden email]> wrote:
Let me make a wider comment about backwards compatibility.

The community has already messed up backward compatibility in far more obvious and wide-reaching ways, for far worse reasons, many times. Suddenly deciding to stand on principle, in a case where the current behavior is *clearly* wrong and dangerous, seems "off" to me.

Of course, you're welcome to do it... I'll just have to make sure it's understood that the decision was made to enshrine actively dangerous behavior.

(I could make an argument for this being CVE-worthy. In fact, Austin Seipp has already made most of it. Should a major security hole also be protected and propagated by a sudden need to stand on backward compatibility principles that have never been given much more than lip service in the past?)

--
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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Brandon Allbery
In reply to this post by Eric Mertens
On Mon, Jan 5, 2015 at 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.

Only really helpful if you can go back and retrofit that deprecation into already deployed older versions. Also, it's using the obvious name for the function. So the correctly working one needs to have some unobvious name and a 'warning you should not use this, use some_other_function instead' form this point on?

Indeed, the Java community does do things that way. One of many reasons the Java ecosystem is an absolute, irredeemable mess.

--
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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Michael Snoyman
In reply to this post by Johan Tibell-2

I strongly support this position. Put me as +1 on the deprecation and new function approach.


On Tue, Jan 6, 2015, 3:17 AM 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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Greg Weber
In reply to this post by Brandon Allbery


On Mon, Jan 5, 2015 at 5:28 PM, Brandon Allbery <[hidden email]> wrote:
On Mon, Jan 5, 2015 at 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.

Only really helpful if you can go back and retrofit that deprecation into already deployed older versions. Also, it's using the obvious name for the function. So the correctly working one needs to have some unobvious name and a 'warning you should not use this, use some_other_function instead' form this point on?

I don't know why 'removeDirectoryRecursive' would be considered by someone to be the only obvious name available. `removeDirectoryTree` would work also. Also, the warning does not have to be placed on older versions for it to have effect with older versions. The point is that the library author will be taught not to use this function at all.

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.

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.
 

Indeed, the Java community does do things that way. One of many reasons the Java ecosystem is an absolute, irredeemable mess.

--
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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: removeDirectoryRecursive should not follow symlinks

Simon Hengel
In reply to this post by Edward Z. Yang
> 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
123