Starting with a base path (e.g. base = Pathname.new
'/some/controlled/directory/path'), I want to append to it in a way that only
descends in the directory, without going up.
I've searched doing this using the Ruby standard library because this is
tipically better addressed in a reviewed, battle tested code, but I could not
find anything.
Is there something like this that I overlooked?
If not, do you think the following is safe enough?
Is just trying to get if one string is a substring of the other or not.
2.0.0p195 :003 > base = File.expand_path '/some/controlled/directory/path'
=> "/some/controlled/directory/path"
2.0.0p195 :004 > user = File.expand_path '../../malicious/attempt', base
=> "/some/controlled/malicious/attempt"
2.0.0p195 :006 > user.start_with? base
=> false
Jesus.
···
On Mon, Jan 12, 2015 at 2:41 PM, Alejandro Exojo <suy@badopi.org> wrote:
Hi.
Starting with a base path (e.g. base = Pathname.new
'/some/controlled/directory/path'), I want to append to it in a way that only
descends in the directory, without going up.
I've searched doing this using the Ruby standard library because this is
tipically better addressed in a reviewed, battle tested code, but I could not
find anything.
Is there something like this that I overlooked?
If not, do you think the following is safe enough?
Is just trying to get if one string is a substring of the other or not.
base = Pathname.new '/some/controlled/directory/path'
from_tricky_user = '../../malicious/attempt/some/controlled/directory/path'
final3 = base + from_user_tricky
puts "Unsafe? #{final3.to_s[base.to_s].nil?}"
# => false (o-oh!)
Of course, this is only a problem is the base pathname gets exposed and can
be abused.
···
On Mon, Jan 12, 2015 at 3:41 PM, Alejandro Exojo <suy@badopi.org> wrote:
Hi.
Starting with a base path (e.g. base = Pathname.new
'/some/controlled/directory/path'), I want to append to it in a way that
only
descends in the directory, without going up.
I've searched doing this using the Ruby standard library because this is
tipically better addressed in a reviewed, battle tested code, but I could
not
find anything.
Is there something like this that I overlooked?
If not, do you think the following is safe enough?
Is just trying to get if one string is a substring of the other or not.
You could also make use of Dir::chroot. This will make it impossible to
break out of the target directory just by entering file pathes (other
means to break out of a Chroot exist).
Newbie question. Is it possible to resolve the path name and then
compare it? That is, if you append "../../../etc/passwd" to the base,
the string would be "/etc/passwd". You could then see if the base
string was in the resolved path.
···
On Mon, Jan 12, 2015 at 8:58 AM, Augusts Bautra <augusts@creo.mobi> wrote:
Consider:
base = Pathname.new '/some/controlled/directory/path'
from_tricky_user = '../../malicious/attempt/some/controlled/directory/path'
final3 = base + from_user_tricky
puts "Unsafe? #{final3.to_s[base.to_s].nil?}"
# => false (o-oh!)
Of course, this is only a problem is the base pathname gets exposed and can
be abused.
On Mon, Jan 12, 2015 at 3:41 PM, Alejandro Exojo <suy@badopi.org> wrote:
Hi.
Starting with a base path (e.g. base = Pathname.new
'/some/controlled/directory/path'), I want to append to it in a way that
only
descends in the directory, without going up.
I've searched doing this using the Ruby standard library because this is
tipically better addressed in a reviewed, battle tested code, but I could
not
find anything.
Is there something like this that I overlooked?
If not, do you think the following is safe enough?
Is just trying to get if one string is a substring of the other or not.
Agreed. The File.expand_path seems best. There are lots of ways to
trick a regex, it seems.
···
On Mon, Jan 12, 2015 at 9:39 AM, Alejandro Exojo <suy@badopi.org> wrote:
El Monday 12 January 2015, Ammar Ali escribió:
You can also check for the presence of two consecutive dots anywhere in the
path string:
raise 'Invalid path' if path =~ %r|/?\.\./|
I've considered this as well, but I thought I might ending up redoing all the
logic inside Pathname instead. Maybe I'm being overcautious, but I've seen so
many times how difficult is to make input validation bulletproof, that I
wanted to be safe and use as much as possible battle tested code.
I came here to say this. Using the right tool for the job is the better way to go. Writing a TON of fragile and error-prone "security" code is the WRONG way to do this. Look into BSD jails if you need something more sophisticated, otherwise chroot is the right thing to do.
···
On Jan 12, 2015, at 11:17, Quintus <quintus@quintilianus.eu> wrote:
You could also make use of Dir::chroot. This will make it impossible to
break out of the target directory just by entering file pathes (other
means to break out of a Chroot exist).
I've considered this as well, but I thought I might ending up redoing all the
logic inside Pathname instead. Maybe I'm being overcautious, but I've seen so
many times how difficult is to make input validation bulletproof, that I
wanted to be safe and use as much as possible battle tested code.
Thank you again.
···
El Monday 12 January 2015, Ammar Ali escribió:
You can also check for the presence of two consecutive dots anywhere in the
path string:
I'm not sure if you are referring here to my solution, which is the
one which used start_with?, but it's not quoted, so I don't know :).
The reasoning is that normalizing the base directory on one hand, and
normalizing the concatenation of the normalized base and the user
relative part,
should result in a normalized path that must start with the normalized
version of the base. Maybe I'm missing something which breaks my
version. I'd appreciate enlightenment :).
Jesus.
···
On Mon, Jan 12, 2015 at 4:05 PM, Robert Klemme <shortcutter@googlemail.com> wrote:
On Mon, Jan 12, 2015 at 3:15 PM, Ammar Ali <ammarabuali@gmail.com> wrote:
> On Jan 12, 2015, at 3:41 PM, Alejandro Exojo <suy@badopi.org> wrote:
>
> base = Pathname.new '/some/controlled/directory/path'
> from_user = 'foo/bar'
> from_bad_user = '../../malicious/attempt'
You can also check for the presence of two consecutive dots anywhere in
the path string:
raise 'Invalid path' if path =~ %r|/?\.\./|
But if you only want to confine one path inside another path then this check
is too strict! Note
/foo/bar/baz/../buz is contained in /foo/bar.
Testing with String#start_with? is also unreliable because "/foo/bar" is not
contained in "/foo/ba". This test is surprisingly tricky to get right.
Being strict was my intention. If these paths are coming from user input then disallowing '..' in the path reduces the risk and simplifies the test and the usage.
The example you gave is perfect; instead of allowing '/foo/bar/baz/../buz', the user is forced to submit '/foo/bar/buz'. If that's what they want then that's what they should submit. Why allow them to go up and down the path?
As for folder names with two dots, like 'foo..bar', they wouldn't be matched by that regular expression, but I would disallow them as well. I don't think they make good directory names. They increase the complexity of the check and I can't think of any real value coming from allowing them.
Of course it all depends on the context and the application in question, but in general I find simpler strict rules on user input to be the most effective and the most readable/maintainable.
Cheers,
Ammar
···
On Jan 12, 2015, at 5:05 PM, Robert Klemme <shortcutter@googlemail.com> wrote:
On Mon, Jan 12, 2015 at 3:15 PM, Ammar Ali <ammarabuali@gmail.com> wrote:
> On Jan 12, 2015, at 3:41 PM, Alejandro Exojo <suy@badopi.org> wrote:
>
> base = Pathname.new '/some/controlled/directory/path'
> from_user = 'foo/bar'
> from_bad_user = '../../malicious/attempt'
You can also check for the presence of two consecutive dots anywhere in the path string:
raise 'Invalid path' if path =~ %r|/?\.\./|
But if you only want to confine one path inside another path then this check is too strict! Note
/foo/bar/baz/../buz is contained in /foo/bar.
cannot jump inside and outside of a chroot jail which may or may not work
depending on the application. On solution would be to fork prior to using
this but that is rather expensive.
Kind regards
robert
···
On Mon, Jan 12, 2015 at 8:17 PM, Quintus <quintus@quintilianus.eu> wrote:
Hi,
You could also make use of Dir::chroot. This will make it impossible to
break out of the target directory just by entering file pathes (other
means to break out of a Chroot exist).
If you're worried about the OP's problem in the first place, then this is an appropriate solution. As I said in my original reply, if you need something more complicated (like being able to exec other subcommands, etc), then something like jails are for you.
Doing this via manual path-checking is like trying to bail out an ocean of water with a teaspoon.
···
On Jan 12, 2015, at 14:30, Robert Klemme <shortcutter@googlemail.com> wrote:
FORTUNATELY that is "all other code in this process and children". You cannot jump inside and outside of a chroot jail which may or may not work depending on the application. On solution would be to fork prior to using this but that is rather expensive.
> > You can also check for the presence of two consecutive dots anywhere in
the path string:
> >
> > raise 'Invalid path' if path =~ %r|/?\.\./|
>
>
>
> But if you only want to confine one path inside another path then this
> check is too strict! Note /foo/bar/baz/../buz is contained in /foo/bar.
Good point. Thanks for bringing them up.
Being strict was my intention. If these paths are coming from user input
then disallowing '..' in the path reduces the risk and simplifies the test
and the usage.
The example you gave is perfect; instead of allowing '/foo/bar/baz/../buz',
the user is forced to submit '/foo/bar/buz'. If that's what they want then
that's what they should submit. Why allow them to go up and down the path?
As for folder names with two dots, like 'foo..bar', they wouldn't be
matched by that regular expression, but I would disallow them as well. I
don't think they make good directory names. They increase the complexity
of the check and I can't think of any real value coming from allowing
them.
Of course it all depends on the context and the application in question,
but in general I find simpler strict rules on user input to be the most
effective and the most readable/maintainable.
Indeed, this is even trickier that I originally expected. What makes it more
puzzling is the fact that seems a common need for lots of applications (e.g.
mapping a URL path to a file path).
This is the best approach I can think of right now:
base = '/some/path'
joined = base.join(file)
joined.to_s.start_with?(base.to_s + '/') ? true : false
This should be good enough given that I'm also doing other checks, but I think
I'll block from the user input the '../' in addition.
>
>
>>
>>
>> >
>> > base = Pathname.new '/some/controlled/directory/path'
>> > from_user = 'foo/bar'
>> > from_bad_user = '../../malicious/attempt'
>>
>> You can also check for the presence of two consecutive dots anywhere in
>> the path string:
>>
>> raise 'Invalid path' if path =~ %r|/?\.\./|
>
>
> But if you only want to confine one path inside another path then this
check
> is too strict! Note
> /foo/bar/baz/../buz is contained in /foo/bar.
>
> Testing with String#start_with? is also unreliable because "/foo/bar" is
not
> contained in "/foo/ba". This test is surprisingly tricky to get right.
I'm not sure if you are referring here to my solution, which is the
one which used start_with?, but it's not quoted, so I don't know :).
I was not referring to your solution. Just the approach to convert anything
to full paths and then check prefixes. Sorry, I should have been more clear.
The reasoning is that normalizing the base directory on one hand, and
normalizing the concatenation of the normalized base and the user
relative part,
should result in a normalized path that must start with the normalized
version of the base. Maybe I'm missing something which breaks my
version. I'd appreciate enlightenment :).
If I'm not mistaken my approach using Pathname#relative_path is making use
of the same feature underneath: to resolve the relative path to another
path which is what File#expand_path also does when given a second argument.
Kind regards
robert
···
On Mon, Jan 12, 2015 at 4:58 PM, Jesús Gabriel y Galán < jgabrielygalan@gmail.com> wrote:
On Mon, Jan 12, 2015 at 4:05 PM, Robert Klemme > <shortcutter@googlemail.com> wrote:
> On Mon, Jan 12, 2015 at 3:15 PM, Ammar Ali <ammarabuali@gmail.com> > wrote:
>> > On Jan 12, 2015, at 3:41 PM, Alejandro Exojo <suy@badopi.org> wrote:
>
> >
> > base = Pathname.new '/some/controlled/directory/path'
> > from_user = 'foo/bar'
> > from_bad_user = '../../malicious/attempt'
>
> You can also check for the presence of two consecutive dots anywhere in
the path string:
>
> raise 'Invalid path' if path =~ %r|/?\.\./|
>
> But if you only want to confine one path inside another path then this
check is too strict! Note
> /foo/bar/baz/../buz is contained in /foo/bar.
Good point. Thanks for bringing them up.
You're welcome.
Being strict was my intention. If these paths are coming from user input
then disallowing '..' in the path reduces the risk and simplifies the test
and the usage.
Fair enough.
The example you gave is perfect; instead of allowing
'/foo/bar/baz/../buz', the user is forced to submit '/foo/bar/buz'. If
that's what they want then that's what they should submit. Why allow them
to go up and down the path?
As for folder names with two dots, like 'foo..bar', they wouldn't be
matched by that regular expression, but I would disallow
Sorry, this is wrong:
irb(main):001:0> %r|/?\.\./| =~ "/foo../bar"
=> 4
If you want to make the leading slash optional you need to use an
alternative as I did and match with \A OR use an optional match from the
beginning of the string:
them as well. I don't think they make good directory names. They increase
the complexity of the check and I can't think of any real value coming from
allowing them.
Of course it all depends on the context and the application in question,
but in general I find simpler strict rules on user input to be the most
effective and the most readable/maintainable.
In that case it may be even better to _positively_ match what you want to
allow, e.g.
%r{\A\w*(?:/\w+)+\z}
instead of trying to match forbidden sequences. That is also easier to
document.
Kind regards
robert
···
On Mon, Jan 12, 2015 at 5:16 PM, Ammar Ali <ammarabuali@gmail.com> wrote:
> On Jan 12, 2015, at 5:05 PM, Robert Klemme <shortcutter@googlemail.com> > wrote:
> On Mon, Jan 12, 2015 at 3:15 PM, Ammar Ali <ammarabuali@gmail.com> > wrote:
> > On Jan 12, 2015, at 3:41 PM, Alejandro Exojo <suy@badopi.org> wrote:
>
> FORTUNATELY that is "all other code in this process and children". You
cannot jump inside and outside of a chroot jail which may or may not work
depending on the application. On solution would be to fork prior to using
this but that is rather expensive.
Please quote correctly! This gives the impression that I wrote
"FORTUNATELY" which I did not.
If you're worried about the OP's problem in the first place, then this is
an appropriate solution.
What _is_ the OP's problem? I cannot see a clear statement about what these
paths are used for. What did I miss? If it is just a location for a file
upload then checking the path may well be sufficient because it avoids the
complexities and restrictions of chroot (see below).
As I said in my original reply, if you need something more complicated
(like being able to exec other subcommands, etc), then something like jails
are for you.
One important aspect to watch out for is that all paths in the application
will have to be changed for chroot. Plus, you must ensure either all code
of the program is loaded OR missing parts are available in the chrooted
environment.
Regards
robert
···
On Tue, Jan 13, 2015 at 3:34 AM, Ryan Davis <ryand-ruby@zenspider.com> wrote:
> On Jan 12, 2015, at 14:30, Robert Klemme <shortcutter@googlemail.com> > wrote:
For this case, I completely disagree. Is not about launching a process in a
sandbox. Then yes, you would need some fancy sandbox (chroots might not even
be enough then).
Is this case, I want to read from a directory that is trusted. I just need to
make trusted as well the mapping of URL to file name inside the directory. I
just want avoid that an untrusted URL fragment points accidentally to
/dev/urandom or something that might crash the process or worse (the file will
have to read and parsed afterwards).
A chroot is not even allowed by unprivileged processes, and is a complex,
expensive call with side effects. I want a simple check on the input string.
Not a single file system access should be needed, I think.
Thank you for the advice, though.
···
El Tuesday 13 January 2015, Ryan Davis escribió:
If you're worried about the OP's problem in the first place, then this is
an appropriate solution. As I said in my original reply, if you need
something more complicated (like being able to exec other subcommands,
etc), then something like jails are for you.
