Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean path from containing backslashes in the webserver #18625

Closed

Conversation

sledgehammer999
Copy link
Member

This will prevent the webserver from serving files outside of the webui folder.

Closes #18618

@sledgehammer999 sledgehammer999 added WebUI WebUI-related issues/changes WebAPI WebAPI-related issues/changes labels Feb 27, 2023
@sledgehammer999 sledgehammer999 added Security Related to software vulnerability in qbt (don't overuse this) and removed WebUI WebUI-related issues/changes labels Feb 27, 2023
@sledgehammer999
Copy link
Member Author

After path is cleaned the following check should work properly and deny access outside webui root folder

const QStringList pathItems {request().path.split(u'/', Qt::SkipEmptyParts)};
if (pathItems.contains(u".") || pathItems.contains(u".."))
throw InternalServerErrorHTTPError();

This will prevent the webserver from serving files outside
of the webui folder.

Closes qbittorrent#18618
@dgw
Copy link

dgw commented Feb 27, 2023

Was about to say, QDir::cleanPath() would have resolved . and .. in the path before the web UI code checked for those.

@sledgehammer999
Copy link
Member Author

Was about to say, QDir::cleanPath() would have resolved . and .. in the path before the web UI code checked for those.

👍
That's why I redid my patch.

@DrunkenSeafarer
Copy link

DrunkenSeafarer commented Feb 27, 2023

Tested with 5e48d8a
It seems to fix the issue, the server now returns HTTP/1.1 500 Internal Server Error.

>curl -i "http://127.0.0.1:8080/..\..\..\..\..\windows\win.ini"
HTTP/1.1 500 Internal Server Error
connection: keep-alive
content-length: 21
content-security-policy: default-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; script-src 'self' 'unsafe-inline'; object-src 'none'; form-action 'self'; frame-ancestors 'self';
content-type: text/plain; charset=UTF-8
date: Mon, 27 Feb 2023 02:04:22 GMT
referrer-policy: same-origin
x-content-type-options: nosniff
x-frame-options: SAMEORIGIN
x-xss-protection: 1; mode=block

Internal Server Error

Against a Linux qbit, I am getting HTTP/1.1 404 Not Found.

Still, the error code being 500 is a bit weird, it suggests some error is not getting handled properly.

@dgw
Copy link

dgw commented Feb 27, 2023

Still, the error code being 500 is a bit weird, it suggests some error is not getting handled properly.

The InternalServerErrorHTTPError here matches 500, to me:

if (pathItems.contains(u".") || pathItems.contains(u".."))
throw InternalServerErrorHTTPError();

Still raises the question of where the 404 comes from, then.

@DrunkenSeafarer
Copy link

Against a Linux qbit, I am getting HTTP/1.1 404 Not Found.

Still raises the question of where the 404 comes from, then.

Ah, nvm, I need the --path-as-is option for curl, e.g.:
$ curl -ik "https://192.168.xx.yy:zzz/../../../../config_ssl/server.crt" --path-as-is

I was getting the 404 because it was looking inside the public folder directly...

@@ -39,6 +39,7 @@
#include <QUrlQuery>

#include "base/global.h"
#include "base/path.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover?

// Convert all '\' to '/'
// This helps the server down the line to guard against path traversal using '.' or '..'
m_request.path = QString::fromUtf8(QByteArray::fromPercentEncoding(pathComponent))
.replace(u"\\"_qs, u"/"_qs);
Copy link
Member

@Chocobo1 Chocobo1 Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use another overload: https://doc.qt.io/qt-6/qstring.html#replace-3
The last time I checked its source, it was simpler (and therefore faster I presume).

@sledgehammer999
Copy link
Member Author

Closed in favor of #18626

@sledgehammer999 sledgehammer999 deleted the fix_webui_security branch February 27, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Security Related to software vulnerability in qbt (don't overuse this) WebAPI WebAPI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants