Server crashes with specific GET request

[serjek]

Following server setup:

var container:NodeContainer = new NodeContainer(3567);
var router:Router<Root> = new Router<Root>(new Root());

container.run(req -> {
    router
        .route(Context.ofRequest(req))
        .recover(OutgoingResponse.reportError);
    }
);

In browser navigate to: http://0.0.0.0:3567//%80../%80../%80../%80../%80../%80../windows/win.ini

server will crash with following error:

 return decodeURIComponent(this1.split("+").join(" "));
                       ^
URIError: URI malformed
    at decodeURIComponent (<anonymous>)
    at Object.tink_url__$Portion_Portion_$Impl_$.toString 

[serjek]

wrapping router.route into try...catch solves the problem:

try router.route(Context.ofRequest(req)).recover(OutgoingResponse.reportError)
catch (e:Dynamic) Future.sync(OutgoingResponse.reportError(new Error(Std.string(e)));

[back2dos]

This workaround only catches the error if routing is fully synchronous.

We could:

1. Catch the error in tink.url.Portion.toString (return original if urlDecode fails), which will lead to a 404 in routing or pass gibberish to a path parameter.
2. Catch the error during routing and report a 422. (preferable)

[grepsuzette]

~Regarding above bug (it was moved since it happened in tink_http_middleware), I wonder if there is something that runs before middlewares that could prematurely reject uris containing null bytes e.g. with a 422.~

Because:

1. null bytes in urls suck (in C language, string end with null bytes, creating security threats, this one for example is crazy).
2. Also there is no good way to fix them, if code A removes them and code B keeps them, it creates inconsistencies and some tests may in turn create more security threats.
3. Middlewares are chained and are optional anyway, so it seems all pointless to do something at this level. And to make matter worse people can write their own middlewares.

~In short, is it possible to have a something like a if s.indexOf("\x00") > -1 then 422 (or another code, I don't mind), before middlewares processing happens?~

Doing some more research this morning on whether such null bytes should be accepted by webservers in general.

[grepsuzette]

Appears web servers actually seem to never reject uris with params containing null bytes. In node express for example you can even have a route containing null bytes e.g. /%00hi%00. So back to square 1.

[back2dos]

Null bytes are "fine" I guess. The thing is percent encoding itself is only valid upto 0x7F.

[grepsuzette]

Hum. Yes I agree %80 and above don't have the excuses for himself that %00 has. A rejection seems ok in that case. Though it is always tough to answer those questions in a hurry.

[grepsuzette]

Actually:

• GET /onehundred%
• GET /%WTF

both crash as well as the above %80. Same place (or in Static if you use it but with the same exception thrown from decodeURIComponent, which is called from StringTools.urlDecode().

StringTools.urlDecode() is absent from tink_web and tink_http but at least present in:

1. tink.url.Portion
2. tink.http.middleware.Static

Personally if URI is malformed in the first place I see no reason to go deeper in the middleware or the router.

[grepsuzette]

Just modified SimpleHandler (locally, won't make a PR before discussing it) in tink.http.Handler like so:

public function process(req:IncomingRequest):Future<OutgoingResponse>
    return StringTools.urlDecode.bind(req.header.url).catchExceptions().isSuccess()
        ? f(req)
        : OutgoingResponse.reportError(new Error(UnprocessableEntity, "Unprocessable Entity"))
    ;

The above fixes the malformed URI problem. But I am not sure if this is an acceptable solution (do we have many types of handlers?), so I just propose this for discussion. Anyway for now the 7688 attacks from the nikto suite don't make my server crash anymore.

[back2dos]

Ok, so using tink_url >= 0.4.2 this won't throw anymore (invalid portions are decoded as empty strings). If anyone feels strongly in favor of producing a 422, PRs are welcome ;)

[grepsuzette]

Before this gets cold and we both forget, by this | If anyone feels strongly in favor of producing a 422, PRs are welcome ;) were you talking about my snippet just above or another solution?

[back2dos]

were you talking about my snippet just above or another solution?

From my side, I think it's fine. Not exactly glorious, but it gets the job done.

[grepsuzette]

Yes I think just the same. I will perform more tests soon, e.g. with unicode uris. So will see if the same recipe has to be applied again. For now I think is is not urgently needed.