[SECURITY] Fix Partial Path Traversal Vulnerability

[JLLeitschuh]

Security Vulnerability Fix

This pull request fixes a partial-path traversal vulnerability due to an insufficient path traversal guard.

Even if you deem, as the maintainer of this project, this is not necessarily fixing a security vulnerability, it is still a valid security hardening.

Preamble

Impact

This issue allows a malicious actor to potentially break out of the expected directory. The impact is limited to sibling directories. For example, userControlled.getCanonicalPath().startsWith("/usr/out") will allow an attacker to access a directory with a name like /usr/outnot.

Why?

To demonstrate this vulnerability, consider "/usr/outnot".startsWith("/usr/out").
The check is bypassed although /outnot is not under the /out directory.
It's important to understand that the terminating slash may be removed when using various String representations of the File object.
For example, on Linux, println(new File("/var")) will print /var, but println(new File("/var", "/") will print /var/;
however, println(new File("/var", "/").getCanonicalPath()) will print /var.

The Fix

Comparing paths with the java.nio.files.Path#startsWith will adequately protect againts this vulnerability.

For example: file.getCanonicalFile().toPath().startsWith(BASE_DIRECTORY) or file.getCanonicalFile().toPath().startsWith(BASE_DIRECTORY_FILE.getCanonicalFile().toPath())

Other Examples

• CVE-2022-31159 - aws/aws-sdk-java
• CVE-2022-23457 - ESAPI/esapi-java-legacy

➡️ Vulnerability Disclosure ⬅️

👋 Vulnerability disclosure is a super important part of the vulnerability handling process and should not be skipped! This may be completely new to you, and that's okay, I'm here to assist!

First question, do we need to perform vulnerability disclosure? It depends!

1. Is the vulnerable code only in tests or example code? No disclosure required!
2. Is the vulnerable code in code shipped to your end users? Vulnerability disclosure is probably required!

For partial path traversal, consider if user-supplied input could ever flow to this logic. If user supplied input could reach this conditional, it's insufficient and, as such, most likely a vulnerability.

Vulnerability Disclosure How-To

You have a few options options to perform vulnerability disclosure. However, I'd like to suggest the following 2 options:

1. Request a CVE number from GitHub by creating a repository-level GitHub Security Advisory. This has the advantage that, if you provide sufficient information, GitHub will automatically generate Dependabot alerts for your downstream consumers, resolving this vulnerability more quickly.
2. Reach out to the team at Snyk to assist with CVE issuance. They can be reached at the Snyk's Disclosure Email. Note: Please include JLLeitschuh Disclosure in the subject of your email so it is not missed.

Detecting this and Future Vulnerabilities

You can automatically detect future vulnerabilities like this by enabling the free (for open-source) GitHub Action.

I'm not an employee of GitHub, I'm simply an open-source security researcher.

Source

This contribution was automatically generated with an OpenRewrite refactoring recipe, which was lovingly hand crafted to bring this security fix to your repository.

The source code that generated this PR can be found here:
PartialPathTraversalVulnerability

Why didn't you disclose privately (ie. coordinated disclosure)?

This PR was automatically generated, in-bulk, and sent to this project as well as many others, all at the same time.

This is technically what is called a "Full Disclosure" in vulnerability disclosure, and I agree it's less than ideal. If GitHub offered a way to create private pull requests to submit pull requests, I'd leverage it, but that infrastructure, sadly, doesn't exist yet.

The problem is that as an open source software security researcher, I (exactly like open source maintainers), I only have so much time in a day. I'm able to find vulnerabilities impacting hundreds, or sometimes thousands of open source projects with tools like GitHub Code Search and CodeQL. The problem is that my knowledge of vulnerabilities doesn't scale very well.

Individualized vulnerability disclosure takes time and care. It's a long and tedious process, and I have a significant amount of experience with it (I have over 50 CVEs to my name). Even tracking down the reporting channel (email, Jira, ect..) can take time and isn't automatable. Unfortunately, when facing prblems of this scale, individual reporting doesn't work well either.

Additionally, if I just spam out emails or issues, I'll just overwhelm already over taxed maintainers, I don't want to do this either.

By creating a pull request, I am aiming to provide maintainers something highly actionable to actually fix the identified vulnerability; a pull request.

There's a larger discussion on this topic that can be found here: JLLeitschuh/security-research#12

Opting-Out

If you'd like to opt-out of future automated security vulnerability fixes like this, please consider adding a file called
.github/GH-ROBOTS.txt to your repository with the line:

User-agent: JLLeitschuh/security-research
Disallow: *

This bot will respect the ROBOTS.txt format for future contributions.

Alternatively, if this project is no longer actively maintained, consider archiving the repository.

CLA Requirements

This section is only relevant if your project requires contributors to sign a Contributor License Agreement (CLA) for external contributions.

It is unlikely that I'll be able to directly sign CLAs. However, all contributed commits are already automatically signed-off.

The meaning of a signoff depends on the project, but it typically certifies that committer has the rights to submit this work under the same license and agrees to a Developer Certificate of Origin
(see https://developercertificate.org/ for more information).

- Git Commit Signoff documentation

If signing your organization's CLA is a strict-requirement for merging this contribution, please feel free to close this PR.

Sponsorship & Support

This contribution is sponsored by HUMAN Security Inc. and the new Dan Kaminsky Fellowship, a fellowship created to celebrate Dan's memory and legacy by funding open-source work that makes the world a better (and more secure) place.

This PR was generated by Moderne, a free-for-open source SaaS offering that uses format-preserving AST transformations to fix bugs, standardize code style, apply best practices, migrate library versions, and fix common security vulnerabilities at scale.

Tracking

All PR's generated as part of this fix are tracked here: JLLeitschuh/security-research#13

[juanpablo-santos]

Hi @JLLeitschuh,

speaking for myself, not on behalf of the project, while most of the time I welcome any PR, I have to say that I find this kind of PRs disrespectful and irresponsible. Don't get me wrong, I appreciate your security concerns, but I sincerely think this is not the way.

Sending bulk e-mails is very similar to how spam works. Furthermore, proposed code fix has been detected mass scaning OSS projects, without a check that indeed there is a vulnerability (details on this at the end of this comment). As a security researcher/whatever you are expected to do at least that. Every other vulnerability report that we have received has done that, so sending a security report without checking is somewhat disrepectful to other security researchers. How the vulnerability can be exploited is also a nice to have, so the people receiving your reports are able to fix the issue as fast as possible, but since you have gone the public way I suppose it is fine as it is.

You say that you know/hope/think that there is a vulnerability, so you choose to disclose publicly because you don't have time to go to every project and check how do they manage these kind of issues. Please respect everybody else's time, which will probably be as scarce and valuable as yours, and play nice. Think of it as if part of your security research involves time on those tasks. Same for verifying whatever the tooling you use has found. Yes, you would probably not reach as many projects, but that's just a matter of quality over quantity. Also, since you are targetting GitHub projects, here's a hint: most of them contain a Security tab at the top of each repo with further instructions. For ASF projects, you can directly follow up with these instructions; since there are more than 200 ASF projects, plus the incubating ones, surely that's worth automating.

Really, full public disclosure does not help anyone: best case scenario, there isn't really a security vulnerability, and some mantainer will have to write back to someone who thinks that mass e-mailing projects is fine, after wasting his/her time looking if the report is really valid. Worst case scenario, let's say you find the next log4shell issue; I fail to see how it would be a good idea to publicly disclose that. Seriously. Don't blame lack of GH infrastructure and just be responsible. Doing otherwise is not cool.

As for the proposed one line fix, there's a comparison between a path's file and a directory whose path can't be reached or managed by an end-user, so I can't see how the reported vulnerability could be exploited, nor the need to security-harden it. Would be nice if you could follow up with this, most preferably following these guidelines.

best regards,

[JLLeitschuh]

As for the proposed one line fix, there's a comparison between a path's file and a directory whose path can't be reached or managed by an end-user, so I can't see how the reported vulnerability could be exploited, nor the need to security-harden it.

To clarify, the uid is not ever controlled by an outside actor? It is only ever an internal value not supplied by user controlled data?

[JLLeitschuh]

I have to say that I find this kind of PRs disrespectful and irresponsible

I'm sorry to hear that you feel disrespected. That was not my intention. Please forgive me.

Sending bulk e-mails is very similar to how spam works.

For ASF projects, you can directly follow up with these instructions; since there are more than 200 ASF projects, plus the incubating ones, surely that's worth automating.

I'm struggling to understand what you're asking for here with these two comments. They seem to contradict eachother.

Please respect everybody else's time, which will probably be as scarce and valuable as yours, and play nice. Think of it as if part of your security research involves time on those tasks.

I fully recognize that Open Source is "free as in free puppy". I've been an open source developer for many years. I believe that users of open source can't have any expectations of open source maintainers (the software is freely made available without a contract), but unfortunately that also means that there can be no expectations around anyone in the OSS community, including security researchers.

Every other vulnerability report that we have received has done that, so sending a security report without checking is somewhat disrepectful to other security researchers.

AFAIK, no other security researcher has attempted to disclose and fix vulnerabilities at this scale before. I'm forging a new path, and I fully admit I may have gotten it wrong in places, but I am taking feedback like yours into consideration. I'm sorry that this upset you so much.

If you'd like to setup some time to discuss your feelings and potential solutions in more detail, feel free to grab a slot on my calendar. I'm more than happy to chat.

https://calendly.com/d/g5x-jtk-653/one-off-meeting

[juanpablo-santos]

Hi,

as of the PR:

To clarify, the uid is not ever controlled by an outside actor? It is only ever an internal value not supplied by user controlled data?

I'm saying it doesn't matter. Worst case scenario, you'll try to traverse directories inside the jspwiki working directory (set up by an administrator) so most probably you'd be able to see lucene files or things like that, if you guess the autogenerated file names. But that shouldn't happen because the requested/traversed file path is compared against a computer generated value (cookieDir inside jspwiki working dir), and if the first doesn't start with the second, the file isn't served.

---

as for the way of working/disclosing vulnerabilities:
my main concern is with public disclosure because there's no time / the scale is too big / YOLO.

IMHO, working at that scale is no excuse for public disclosure. Suppose you find next log4shell or whatever, do you think publicly disclosing that is responsible? Lack of tooling is no excuse - just build that.

For ASF projects, you can directly follow up with these instructions; since there are more than 200 ASF projects, plus the incubating ones, surely that's worth automating.

I'm struggling to understand what you're asking for here with these two comments. They seem to contradict eachother.

I (re)wrote the comment several times, in order to just cool down. The original thought was that if you prefer to continue mass disclosing, at least please consider for ASF projects changing your tooling so that instead of opening a PR, thus publicly disclosing, just follow the aforementioned instructions. Surely other big projects have their own security vulnerabilities reporting, out of the head I'm thinking of Spring or Eclipse Foundation projects... they'll surelly appreciate that too.

I am taking feedback like yours into consideration. I'm sorry that this upset you so much.

If you'd like to setup some time to discuss your feelings and potential solutions in more detail, feel free to grab a slot on my calendar. I'm more than happy to chat.

No hard feelings, so no worry about that (although I don't wish to pursue this further).

I'll proceed with closing the issue, however if you feel the PR needs further discussion feel free to reopen.

best regards,