Improve security when handling requests to download files

[dfaure-kdab]

Make sure we never go outside the base directory

Fixes #314

[jtjbrady]

In case you're wondering why the unit test is failing on Windows but passing on Linux.

The first two unit tests start with /// this is unmodified by QDir::cleanPath on Windows (due to UNC paths I believe), on Linux this becomes a single /

So that the server performs the same on Linux and Windows I believe multiple leading slashes should be stripped in parseHeaders.

[dfaure-kdab]

In case you're wondering why the unit test is failing on Windows but passing on Linux.

The first two unit tests start with /// this is unmodified by QDir::cleanPath on Windows (due to UNC paths I believe), on Linux this becomes a single /

So that the server performs the same on Linux and Windows I believe multiple leading slashes should be stripped in parseHeaders.

Thanks, I've been failing to get the output from the failing test on Windows (and I'm on Linux myself).
However I don't really understand: given the Qt::SkipEmptyParts argument to split, a path with "///" as the result of cleanPath should still have no impact on the rest of the code. In fact the test passes on Linux even if I remove the call to QDir::cleanPath.

[jtjbrady]

However I don't really understand: given the Qt::SkipEmptyParts argument to split, a path with "///" as the result of cleanPath should still have no impact on the rest of the code. In fact the test passes on Linux even if I remove the call to QDir::cleanPath.

Ok, so there are at least two locations in this file where cleanPath is called.

One in: static HeadersMap parseHeaders(const QByteArray &headerData) line 86 - this produces the path which is eventually passed to everything else.
One in your new function: static bool isPathSecure(const QString& path) line 286 - this is superfluous really as the path is already cleaned by parseHeaders.

So, what I was suggesting was ensuring there is at most one leading slash stored in cleanPath function in the headersMap("_path") by parseHeaders. It's annoying that cleanPath provides a different output on Windows.

[dfaure-kdab]

Ok, so there are at least two locations in this file where cleanPath is called.

One in: static HeadersMap parseHeaders(const QByteArray &headerData) line 86 - this produces the path which is eventually passed to everything else. One in your new function: static bool isPathSecure(const QString& path) line 286 - this is superfluous really as the path is already cleaned by parseHeaders.

So, what I was suggesting was ensuring there is at most one leading slash stored in cleanPath function in the headersMap("_path") by parseHeaders. It's annoying that cleanPath provides a different output on Windows.

Ooops I meant to remove the cleanPath call in parseHeaders().
Done now locally. The issue remains, right?
EDIT: I guess the implementations of processFileRequest would still like the input to be cleaned, so I should rather keep that one and remove the one in isPathSecure...

Are you sure about your assessment that QDir::cleanPath("///foo") returns ///foo on Windows?
Looking at the code, the only Windows-specific bit I see is keeping the / in C:/, so I still have the feeling it returns /foo on all platforms...

[jtjbrady]

Are you sure about your assessment that QDir::cleanPath("///foo") returns ///foo on Windows? Looking at the code, the only Windows-specific bit I see is keeping the / in C:/, so I still have the feeling it returns /foo on all platforms...

Here is my amazing program:

#include <QDir>

int main()
{
    qDebug("%s", qPrintable(QDir::cleanPath("///../hello/../world")));
    return 0;
}

On Linux: "/../world"
On Windows: "///../world"

[jtjbrady]

qdir.cpp line 66 in rootLength has Windows specific rules to do with UNC paths. Those are normally //servername/
Apparently a blank server name is allowed which gives us three slashes.

[dfaure-kdab]

Oh, I missed the fact that qt_normalizePathSegments called rootLength.
Thanks for the analysis. All clear now, here's an updated MR.

[dfaure-kdab]

OK, it passed. What do you think of this solution?

[jtjbrady]

More verbose than my regular expression solution, but much easier to understand.
Whereas my solution corrected urls by removing any leading /../ I think reporting a 403 error instead is the correct solution.

Given the use of QDir::cleanPath in parseHeaders I believe the whole test could probably be:

return path.startsWith(QLatin1Char('/')) && path.section('/', 1, 1) != "..";

i.e. ensure the string starts with a / and the first section is not '..'
What you have however is more explicit in what it is trying to accomplish and easy to understand.

[dfaure-kdab]

Oh I see what you mean. We can't end up with /foo/bar/../.. after cleanPath. So there's no need to count going in and out, cleanPath has already cleaned as much as possible for us. I like your idea (simpler = less risks of errors), I'll change it that way.

[jtjbrady]

I've also just realised that this path contains the query string. So, the test should be only operating on the part before the query, e.g. path.section('?', 0, 0)

So...
return path.startsWith(QLatin1Char('/')) && path.section('?', 0, 0).section('/', 1, 1) != "..";

Actually that's not necessary the path.section('?', 0, 0) is only necessary using your loop method.

[dfaure-kdab]

Good point. But we've already split out the query in parseHeaders, and then reassembled it, I think I'll rather store them split and reassemble afterwards in handleRequest. No point in parsing twice (with risks of slight differences).

Also, since consecutive slashes have been cleaned up, we can just do path.startsWith(QLatin1String("/.."))

[jtjbrady]

I assume you mean return path.startsWith(QLatin1Char('/')) && !path.startsWith(QLatin1String('/..'))?
The first test is still necessary.

[dfaure-kdab]

Yes of course, after the first test. Pushed. But too early, forgot about splitting the query. Doing too many things at the same time.

[jtjbrady]

Yes of course, after the first test. Pushed. But too early, forgot about splitting the query. Doing too many things at the same time.

I believe splitting the query was necessary only using your previous loop method, this method is looking for specific characters at the start of the string.

[dfaure-kdab]

misunderstanding, I mean the splitting of the query from the path ;)

[dfaure-kdab]

Are you OK with this version? Should I merge it?

[jtjbrady]

Only remaining issue I see is the one in the code review above.

The unit test "leading_double_slash" is identical to "leading_triple_slash".

[dfaure-kdab]

Oh I haven't seen such a code review comment....
Indeed they are the same (this comes from #314 BTW).
Amending.

[jtjbrady]

Ok, looks good to me.