Anybody can hack the routes - is this an issue?

[Christian Bourgeois]

See https://github.com/playframework/playframework/issues/4892 for details...

[Naftoli Gugenheim]

What do you mean by hack? Can you give a scenario where it would lead to something insecure?

On Mon, Jul 27, 2015, 2:15 PM Christian Bourgeois <chbourg...@gmail.com> wrote:

See https://github.com/playframework/playframework/issues/4892 for details...

--
You received this message because you are subscribed to the Google Groups "play-framework" group.
To unsubscribe from this group and stop receiving emails from it, send an email to play-framewor...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/play-framework/ebf9fdba-ac3c-424a-8f37-81ad4577459c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Christian Bourgeois]

Hello, I'm not very fluent in describing complex things in english, let me know if something is not clear enough.

There are so many scenarios.

One of them being to serve file from a folder: if I have some static files in a folder (and yes I'm not using the Assets util for this), and I serve them with name coming from the route "/path/:name". In that very simple case, I (as a developer) just read the doc and understand that :name must contain "exactly one URI path segment", so I just trust it and use new File(folder, name). But this is wrong, as I can have something like ../../../file, which was unexpected, using the %2F trick (see my github post).

On the other framework we're working with, which has the same behavior, we were able to display the content of application.conf, with all passwords and secret keys ;)

https://www.playframework.com/documentation/2.4.x/JavaRouting 

The default matching strategy for a dynamic part is defined by the regular expression[^/]+, meaning that any dynamic part defined as :id will match exactly one URI path segment.

 

Another use case might be using the name parameter to call a REST service, where the name must be included in the URL. Again, with the %2F trick, the real called URL can be easily changed to something else, allowing a hacker to call whatever url they want to.

These two examples are perhaps very simple, but there are probably others I can't imagine.

[Greg Methvin]

Play's router just parses the path segments and gives you the decoded values. It doesn't know if a segment represents an actual path on the filesystem, a name (which maybe could contain slashes), an ID, or something else; each would require different validation. A path segment can contain encoded slashes as %2f, as well as many other encoded characters. If you're going to use the segment again in a URI, you should be encoding it properly, just as you would with any other value you put into a URI.

Maybe we should improve the documentation here to make it explicit that you are responsible for doing the encoding/validation.

Is there a reason why you're not using the built in Assets controller? If so it'd be interesting to understand why it doesn't suit your needs.

[Rich Dougherty]

On Tue, Jul 28, 2015 at 10:10 AM, Christian Bourgeois <chbourg...@gmail.com> wrote:

…I (as a developer) just read the doc and understand that :name must contain "exactly one URI path segment", so I just trust it and use new File(folder, name). But this is wrong, as I can have something like ../../../file, which was unexpected, using the %2F trick (see my github post).

I agree, this may be unexpected, but I don't think it is wrong. It is a perfectly valid for decoded path segments to contain "/"… just like it is valid for decoded query parameters to contain separators like "?" and "&" or decoded JSON strings to contain '"' characters. A web developer (unfortunately) needs to be educated about the input values they are accepting and validate them appropriately.

The Java File API is highly vulnerable to issues with ".." and "/" characters creeping into paths. It would be safer if the API would had more typesafety and did some escaping/validation when constructing paths.

https://www.playframework.com/documentation/2.4.x/JavaRouting 

The default matching strategy for a dynamic part is defined by the regular expression[^/]+, meaning that any dynamic part defined as :id will match exactly one URI path segment.

Perhaps we could update the docs to indicate that we match the bit between "/"s, but we return the decoded path segment, which could contain "/"s.

By the way, routes like GET /files/*name are confusing too, because they give the encoded path segments, which is the opposite way of doing things!

Maybe we could add some typesafety, e.g. change /:segment to return a PathSegment and /:path to return a Seq[PathSegment]. Users could pick which String value to get by calling either decoded or encoded on a PathSegment object.

Cheers

Rich

--

Rich Dougherty

Engineer, Typesafe, Inc

@richdougherty

[Christian Bourgeois]

I understand from a "technical developer of the product who knows the internals and flaws" that this is logic, and that it must be checked again and again. I personaly have deep interest in frameworks in general, and used to react this way.

But from a pure user point of view, if there is no "non-technical" difference between /:name and /*name, why is there two options for something that can get exactly the same results? Just to recall, the user could get path traversal in both case.

If it is decided to let the things like it is now, this is indeed important to update the documentation to explain that /:name could contain anything, not only a pure unique path segment, as proved in the unexpected part of my github post, like in /*name. And in that case, maybe removing the /:name would be a good idea, as it give false idea to the developer of the content of the :name value.

If we try to get a little higher in the way http routing works in many frameworks, I would say that this is a new type of injection, "path injection", that is not covered yet and maybe never, and that every developer should be aware of it.

We're back in the dark ages of SQL injection...

[Greg Methvin]

Maybe you could call it a new type of injection, but it's really just another form of not properly encoding/escaping user input.

I agree it may be confusing that the parameter in /:name is decoded, while /*name is not, and both are bound by default to String. Maybe for /*name we should parse to a special Path type by default, with explicit methods for getting segments and/or the full encoded path.

The behavior for /:name is exactly what I would expect, though. Play takes the path segment and decodes it for me to a regular string. Once it's decoded, I treat it like any other user input. If I happen to be using it in a URI, I'll encode as a URI. I think decoding the value is more useful than not doing so, as most of the time I'm not using it in a URI and I actually want the decoded value. The documentation could be clarified though.

If you wanted to do some additional validation on the path segment you could also implement a custom PathBindable (https://www.playframework.com/documentation/2.4.x/api/java/play/mvc/PathBindable.html). We should do a better job explaining this in the documentation.

--
You received this message because you are subscribed to a topic in the Google Groups "play-framework" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/play-framework/KCcWk72CwVU/unsubscribe.
To unsubscribe from this group and all its topics, send an email to play-framewor...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/play-framework/38b96042-71e0-4d56-a9ad-4f16d12e696c%40googlegroups.com.

[Naftoli Gugenheim]

On Tue, Jul 28, 2015, 3:55 AM Christian Bourgeois <chbourg...@gmail.com> wrote:

I understand from a "technical developer of the product who knows the internals and flaws" that this is logic, and that it must be checked again and again. I personaly have deep interest in frameworks in general, and used to react this way.

But from a pure user point of view, if there is no "non-technical" difference between /:name and /*name, why is there two options for something that can get exactly the same results? Just to recall, the user could get path traversal in both case.

The point of * is to support URLs like /docs/section1/subsection3/page6, where you want one controller to handle multiple path components.

If it is decided to let the things like it is now, this is indeed important to update the documentation to explain that /:name could contain anything, not only a pure unique path segment,

That is not true. It can only contain a pure path segment. Ab%2fcd is one path segment. Path segments can *represent* a string of any characters including /, they just have to pass it in in an escaped form. I'm not sure what's not clear about that.

as proved in the unexpected part of my github post, like in /*name. And in that case, maybe removing the /:name would be a good idea, as it give false idea to the developer of the content of the :name value.

Then how would you match /handle/62/action, or /pair/5/77, i.e., a non-last path components going into a parameter?

If we try to get a little higher in the way http routing works in many frameworks, I would say that this is a new type of injection, "path injection", that is not covered yet and maybe never, and that every developer should be aware of it.

Fair enough, but I'm not clear what you expect the framework to do. The common denominator of injections is "strings are a very unsafe type." In fact I would say you should almost never give controller actions String parameters, and instead use custom types that have a PathBindable etc. typeclass instance available.

We're back in the dark ages of SQL injection...

Le mardi 28 juillet 2015 02:01:25 UTC+2, Rich Dougherty a écrit :

On Tue, Jul 28, 2015 at 10:10 AM, Christian Bourgeois <chbourg...@gmail.com> wrote:

…I (as a developer) just read the doc and understand that :name must contain "exactly one URI path segment", so I just trust it and use new File(folder, name). But this is wrong, as I can have something like ../../../file, which was unexpected, using the %2F trick (see my github post).

I agree, this may be unexpected, but I don't think it is wrong. It is a perfectly valid for decoded path segments to contain "/"… just like it is valid for decoded query parameters to contain separators like "?" and "&" or decoded JSON strings to contain '"' characters. A web developer (unfortunately) needs to be educated about the input values they are accepting and validate them appropriately.

The Java File API is highly vulnerable to issues with ".." and "/" characters creeping into paths. It would be safer if the API would had more typesafety and did some escaping/validation when constructing paths.

Is there a better file api (are any of nio Path, sbt IO, ammonite ops, any better in this regard)?

https://www.playframework.com/documentation/2.4.x/JavaRouting 

The default matching strategy for a dynamic part is defined by the regular expression[^/]+, meaning that any dynamic part defined as :id will match exactly one URI path segment.

Perhaps we could update the docs to indicate that we match the bit between "/"s, but we return the decoded path segment, which could contain "/"s.

By the way, routes like GET /files/*name are confusing too, because they give the encoded path segments, which is the opposite way of doing things!

Maybe we could add some typesafety, e.g. change /:segment to return a PathSegment and /:path to return a Seq[PathSegment].

You mean *path? I think it's a good idea, depending on what you mean by "returning." The type is dictated by the action parameters, and the availability of a typeclass. Perhaps String is currently special-cased not to need typeclass. In any case doesn't the action get the "first say" as to the type?

Users could pick which String value to get by calling either decoded or encoded on a PathSegment object.

Cheers

Rich

--

Rich Dougherty

Engineer, Typesafe, Inc

@richdougherty

--
You received this message because you are subscribed to the Google Groups "play-framework" group.
To unsubscribe from this group and stop receiving emails from it, send an email to play-framewor...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/play-framework/38b96042-71e0-4d56-a9ad-4f16d12e696c%40googlegroups.com.

[Greg Methvin]

On Wed, Jul 29, 2015 at 4:57 PM, Naftoli Gugenheim <nafto...@gmail.com> wrote:

You mean *path? I think it's a good idea, depending on what you mean by "returning." The type is dictated by the action parameters, and the availability of a typeclass. Perhaps String is currently special-cased not to need typeclass. In any case doesn't the action get the "first say" as to the type?

String has a PathBindable (play.api.mvc.PathBindable.bindableString), and it is also the default type in the routes (if no type is specified). It might be possible to change this default type to something else, like PathSegment for /:segment and Seq[PathSegment] for /*path (or maybe just String and Seq[String]).

The same PathBindable is used for /*path and /:segment, which is a bit weird considering one is encoded and the other is not. I don't think regex path components are decoded either. Maybe it would also make sense to have a different typeclass to deal with the encoded (possibly multi-segment) parts. I think we should also be clearer in the documentation about when things are encoded.

[Naftoli Gugenheim]

On Wed, Jul 29, 2015, 11:31 PM Greg Methvin <greg.m...@gmail.com> wrote:

On Wed, Jul 29, 2015 at 4:57 PM, Naftoli Gugenheim <nafto...@gmail.com> wrote:

You mean *path? I think it's a good idea, depending on what you mean by "returning." The type is dictated by the action parameters, and the availability of a typeclass. Perhaps String is currently special-cased not to need typeclass. In any case doesn't the action get the "first say" as to the type?

String has a PathBindable (play.api.mvc.PathBindable.bindableString), and it is also the default type in the routes (if no type is specified).

Oh - you have to write out the types in the routes file? Can't it get the type from the controller action?

It might be possible to change this default type to something else, like PathSegment for /:segment and Seq[PathSegment] for /*path (or maybe just String and Seq[String]).

The same PathBindable is used for /*path and /:segment, which is a bit weird considering one is encoded and the other is not. I don't think regex path components are decoded either. Maybe it would also make sense to have a different typeclass to deal with the encoded (possibly multi-segment) parts. I think we should also be clearer in the documentation about when things are encoded.

--

You received this message because you are subscribed to the Google Groups "play-framework" group.
To unsubscribe from this group and stop receiving emails from it, send an email to play-framewor...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/play-framework/CAO2h8AyaWpOi81C-Geu_ncM1Z13Rn2zuGG2_w6hiagAMxJm-2g%40mail.gmail.com.