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

Simplify path handling, use safe_join #12

Merged
merged 1 commit into from Oct 6, 2020

Conversation

ChristianTacke
Copy link
Contributor

The current implementation of sanitized_join did not handle ".." properly. The problem is, that .absolute() does not do what .resolve() does, but .resolve() does not work on non existant paths.

Anyway, flask has a function exactly for this: safe_join.

So let's use that one.

While at it, simplified the whole path handling a bit.

The current implementation of sanitized_join did not handle
".." properly. The problem is, that .absolute() does not do
what .resolve() does, but .resolve() does not work on non
existant paths.

Anyway, flask has a function exactly for this: safe_join.

So let's use that one.

While at it, simplified the whole path handling a bit.
@horazont horazont added the bug label Oct 6, 2020
@horazont horazont self-assigned this Oct 6, 2020
@horazont
Copy link
Owner

horazont commented Oct 6, 2020

@ChristianTacke Thank you very much for your contribution!

This looks as if the current code is vulnerable to a directory traversal attack, is that correct?

What mitigates the impact is:

  • There must exist both .meta and .data files for a path to be served.
  • The .meta file must parse as JSON.
  • The JSON in the .meta file needs to have the Content-Type key.

I am inclined to not go through the bureaucracy of allocating a CVE for this, but I’ll take other opinions on that matter. What do you think? Vote with 👍 if you agree with my position, 👎 if you disagree or comment if you have additional thoughts. In the meantime, I’ll prepare a release.

@horazont horazont added this to the 0.4 milestone Oct 6, 2020
@horazont
Copy link
Owner

horazont commented Oct 6, 2020

After further discussion, some vectors came to mind in a shared hosting scenario (no idea if anyone uses it in that way) which are considerable.

I’ll try to get a CVE for this, but I won’t block releasing the fix.

@ChristianTacke
Copy link
Contributor Author

This looks as if the current code is vulnerable to a directory traversal attack, is that correct?

Yes, that's fully correct. I was able to test this by doing:

$ telnet localhost 5000
GET /../1.txt HTTP/1.0
Host: localhost

and putting appropriate files (.data, .meta) in place.

Additional thoughts about mitigations

  • To abuse this for writing, one needs to posses the HMAC key, so that part is really unlikely.

About shared hosting:

  • An attacker with local access possibly could create a matching .meta file and make the .data file a symlink and use all of this to read files with the permissions of the xhu daemon.

Please excuse for not properly doing the disclosure. I had realized most of the mitigations and had assumed (as you did in your first comment), that they would make the problem small enough. I hadn't included shared hosting in the scenario.

Yes, please do a patch release/etc early (possibly merge my PR soonish, so that people using the master branch get the fix quickly).

@horazont
Copy link
Owner

horazont commented Oct 6, 2020

To abuse this for writing, one needs to posses the HMAC key, so that part is really unlikely.

Oh wow, that vector didn’t even cross my mind yet. This means that the XMPP server gains file write privileges outside of the data directory. The XMPP server may not be trusted to that extent.

Regarding shared hosting: The attack vector which came to mind was more along the lines of accessing data from another xhu instance.

E.g. if you have one at /var/lib/customers/domain1/ and one at /var/lib/customers/domain2/, you can access data from domain1 via domain2. This may have impact if there are egress traffic quotas or additional HTTP authentication in place for domain1, because both can be circumvented that way.

@horazont horazont merged commit 741157c into horazont:master Oct 6, 2020
@ChristianTacke ChristianTacke deleted the pr/safe_join branch October 6, 2020 16:06
@ChristianTacke
Copy link
Contributor Author

Thanks for merging!

To summarize:
We have multiple vectors. All of them do not look very likely to me till now. So the impact seems to be low, IMHO.

@horazont
Copy link
Owner

horazont commented Oct 6, 2020

GHSA-hwv5-w8gm-fq9f

@horazont
Copy link
Owner

horazont commented Oct 6, 2020

Thanks again for the report. No worries about the responsible disclosure; That would’ve been appreciated, but I agree that this is a low-impact vector.

Repository owner deleted a comment from nonganson Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants