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

Server path cleanup incorrectly processes query string #247

Closed
jtjbrady opened this issue Feb 23, 2022 · 3 comments
Closed

Server path cleanup incorrectly processes query string #247

jtjbrady opened this issue Feb 23, 2022 · 3 comments

Comments

@jtjbrady
Copy link

Given a path of /soap?query=../test the code for handling paths will treat this as /test

This is because the code in KDSoapServerSocket.cpp parseHeaders calls QDir::cleanPath on the entirety of the passed path including the query.

This could lead to interesting results when using processFileRequest as /soap?query=../../../../../etc/passwd results in a path of ../../../../etc/passwd being passed.

Suggested fix is to split on the query.

-    const QByteArray path = QDir::cleanPath(QString::fromLatin1(firstLine.at(1).constData())).toLatin1();
+    const int queryLocation = firstLine.at(1).indexOf('?');
+    QByteArray path = QDir::cleanPath(QString::fromLatin1(firstLine.at(1).constData(), queryLocation)).toLatin1();
+    if (queryLocation > 0)
+            path += firstLine.at(1).mid(queryLocation);
@dfaure-kdab
Copy link
Member

Hi, thanks for the report. I have now investigated and fixed this issue, see the referenced commit.

In addition to stripping out the query like you suggested, I also added a security check that the final path being requested must start with a '/', which excludes paths such as "../../etc/passwd".

Please let me know if any of my changes creates problems for you.

@jtjbrady
Copy link
Author

It looks like the new code fixes the security issue, but it looks like it strips out the query string completely.
I use /soapservice?wsdl as the path for the wsdl document and before this change it was possible to implement handling of queries via processFileRequest.

@dfaure-kdab
Copy link
Member

Thanks for the quick test. I have amended by commit in commit e474a58 so that queries are preserved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants