Revolution: No response body when null-byte character in query string (%00)

[pixelchutes]

Summary

If passing a null-byte character (%00) in the URL to a MODX website, the request is immediately terminated with a HTTP/1.1 200 OK response, but _no_ response body.

Step to reproduce

Browse to MODX website with null-byte character in the URL:

Example: https://modx.com/?%00

Observed behavior

HTTP/1.1 200 OK
Connection: keep-alive
Content-Encoding: gzip
Content-Type: text/html; charset=UTF-8
Date: Mon, 13 Feb 2017 21:14:51 GMT
MC: 2Mru1ZFR/JMsNYMAR6VMFC5F3zNmMHNF7EfTrM+JyGvvVVA7Bk+CJhC8hu17ZySp
Server: nginx/1.10.0 (Ubuntu)
Transfer-Encoding: chunked
Vary: Accept-Encoding

// no response body here

Example: https://modxcloud.com/?%00

HTTP/1.1 200 OK
Connection: keep-alive
Content-Encoding: gzip
Content-Type: text/html; charset=UTF-8
Date: Mon, 13 Feb 2017 21:19:33 GMT
MC: RdPkGQARO+eA54v8wZoGfXPmoazL4Uc8YT/K60fe+99+jz1EZWVvKbCnpATyRlGa
Server: nginx/1.8.1
Strict-Transport-Security: max-age=31536000;
Transfer-Encoding: chunked

// no response body here

Testing this against the exact same server _(but bypassing MODX entirely)_ suggests that this is an internal handling issue within MODX core.

Expected behavior

I would expect some accompanying response body if the URI was valid, and if not, then perhaps the 404 Error page, for example. Something more than just the HTTP headers.

Environment

MODX 2.5.5, PHP 5.6.30, NGINX 1.8.* or 1.10.* or 1.11.*

[Mark-H]

Here's the cause. Seems to have been there since the first revo commit.

Weird to just die() the process though, I can find some resources that say null byte injection can be a security risk, but it seems a simple str_replace on the values (perhaps in modX::sanitize or modX::protect should do the trick?

[pixelchutes]

Interesting, good find @Mark-H. I did confirm this against an Evo instance, as well (same results) -- so probably originated there.

Yes, I definitely don't want to make things any less secure 😉 but the handling as-is seems a bit abrupt.

[Mark-H]

If we want to replace it with a str_replace, I think we'll have to place that into modX::protect - modX::sanitize isn't called until much later compared to where it halts now.

Alternatively, if we want to keep it, we could add a header (400 or 422 perhaps) with a short text description in the existing conditional.

[pixelchutes]

I would personally lean towards replacing (sanitizing/cleansing), allowing the traffic to continue through to the site. To me, this seems like a better end result, assuming same protection would effectively still be in place vs. halting the entire request.

Devil's advocate: Is there a reason it wasn't simply replaced originally? Perhaps Evo didn't have as good controls around protection at the time maybe?

[mrhaw]

Very interesting find! Wonder if it can be exploited in any sense. http://security.stackexchange.com/questions/48187/null-byte-injection-on-php/74660#74660

[OptimusCrime]

If MODX bumps their requirements to 5.3.4 this could be removed. But as far as I know the only check is for 5.1.x?

[pixelchutes]

Sounds like we _may_ be bumping PHP minimum version to 5.4.x for upcoming MODX 2.6. That said, I've flagged this for 2.6.0 where the offending check could safely be removed.