You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/07/29 13:26:36 UTC
[GitHub] [commons-io] JLLeitschuh opened a new pull request, #371: [SECURITY] Fix Partial Path Traversal Vulnerability
JLLeitschuh opened a new pull request, #371:
URL: https://github.com/apache/commons-io/pull/371
# Security Vulnerability Fix
This pull request fixes a partial-path traversal vulnerability due to an insufficient path traversal guard.
## 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](https://github.com/aws/aws-sdk-java/security/advisories/GHSA-c28r-hw5m-5gv3) - aws/aws-sdk-java
- [CVE-2022-23457](https://securitylab.github.com/advisories/GHSL-2022-008_The_OWASP_Enterprise_Security_API/) - ESAPI/esapi-java-legacy
# :arrow_right: Vulnerability Disclosure :arrow_left:
:wave: 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!
## 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](https://docs.github.com/en/code-security/repository-security-advisories/creating-a-repository-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](mailto:report@snyk.io). 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](https://github.com/github/codeql-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](https://github.com/openrewrite/rewrite) [refactoring recipe](https://docs.openrewrite.org/), 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](https://github.com/openrewrite/rewrite-java-security/blob/main/src/main/java/org/openrewrite/java/security/PartialPathTraversalVulnerability.java)
## Why didn't you disclose privately (ie. coordinated disclosure)?
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.
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, you are proving a maintainer something highly actionable to actually fix the identified vulnerability. The pull request.
There's a larger discussion on this topic that can be found here: https://github.com/JLLeitschuh/security-research/discussions/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](https://moz.com/learn/seo/robotstxt) format for future contributions.
Alternatively, if this project is no longer actively maintained, consider [archiving](https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-archiving-repositories) 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/](https://developercertificate.org/) for more information).
>
> \- [Git Commit Signoff documentation](https://developercertificate.org/)
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](https://www.moderne.io/), 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: https://github.com/JLLeitschuh/security-research/issues/13
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1199391853
This PR needs a test that fails without the main change, otherwise it's just one commit away from a regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1199926236
Well, we are all busy people, most of us here volunteers to boot; without a test I am -1 to merge, I think this should go on the queue for study and hopefully someone will take the time to create a test.i might even look over the weekend.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory closed pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
garydgregory closed pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
URL: https://github.com/apache/commons-io/pull/371
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] JLLeitschuh commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
JLLeitschuh commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1199847583
> Uh? You created the PR...
I used a bot to create the PR on my behalf, as well as generate the fix, as well as generated fixes for 30 other OSS projects with this same bot. I'm currently working through a collection of other vulnerabilities I'm aiming to fix including generating 108 pull request to fix a bunch of OSS projects that are vulnerable to Zip-Slip.
> you can create the test.
Unfortunately, this is not something I have time for right now. However, I believe this may fix a security vulnerability, do you agree? If so, consider this a security report from a security researcher, and consider treating it as such, following the Apache standard vulnerability handling profess. If you don't consider this a security vulnerability, and are unwilling to accept this change without a test (which is completely understandable) please feel free to either close this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] JLLeitschuh commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
JLLeitschuh commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1200526484
> Otherwise, even though I believe fixing these issues is beneficial to users, in the end your pull requests may end up stalled for a long time until a volunteer has time to reproduce, add tests, prepare the CVE/disclosure, etc. And it wouldn't be doing any benefit to users or maintainers in this case.
This is an unfortunate trade off of this methodology. I don't have any control over the maintainer vulnerability handling timeline. I agree that, the net benifit when looking at this on a project-by-project basis is less than ideal. But when looked at at scale, the value proposition to the entire internet becomes clearer.
I can relate to the plight of the OSS maintainer. I am one myself, although not on one with quite as many users as this project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1199839017
Uh? You created the PR... you can create the test. Don't blame the tool you chose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] JLLeitschuh commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
JLLeitschuh commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1199678551
> This PR needs a test that fails without the main change, otherwise it's just one commit away from a regression.
I agree! The code generator used to generate this PR doesn't have the capabilities to also generate tests. Any of the maintainers of this project are more than welcome to push additional changes to this branch to add tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] pjfanning commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
pjfanning commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1228927498
This change affects code that tries to exclude some files when copying a directory (to avoid https://issues.apache.org/jira/browse/IO-141). I can see how the change can help to avoid stopping some files being added to the exclusion list when they shouldn't. I tried to take the existing FileUtilsTest for IO-141 (testCopyDirectoryToChild) and to add one based on it but where the source dir is not really a parent dir of the dest dir but the existing check mistakenly thinks it is. I have such a test (below) but it works with and without this PR change.
I don't think there is any security issue here.
```
/**
* Test for https://github.com/apache/commons-io/pull/371. The dir name 'par' is a substring of
* the dir name 'parent' which is the parent of the 'parent/child' dir.
*/
@Test
public void testCopyDirectoryWithPotentialFalsePartialMatch() throws Exception {
final File grandParentDir = new File(tempDirFile, "grandparent");
final File parentDir = new File(grandParentDir, "parent");
final File parDir = new File(grandParentDir, "par");
final File childDir = new File(parentDir, "child");
createFilesForTestCopyDirectory(grandParentDir, parDir, childDir);
final List<File> initFiles = LIST_WALKER.list(grandParentDir);
final List<File> parFiles = LIST_WALKER.list(parDir);
final long expectedCount = initFiles.size() + parFiles.size();
final long expectedSize = FileUtils.sizeOfDirectory(grandParentDir) + FileUtils.sizeOfDirectory(parDir);
FileUtils.copyDirectory(parDir, childDir);
final List<File> latestFiles = LIST_WALKER.list(grandParentDir);
assertEquals(expectedCount, latestFiles.size());
assertEquals(expectedSize, FileUtils.sizeOfDirectory(grandParentDir));
assertTrue(expectedCount > 0, "Count > 0");
assertTrue(expectedSize > 0, "Size > 0");
Set<String> initFilePaths = getFilePathSet(initFiles);
Set<String> newFilePaths = getFilePathSet(latestFiles);
newFilePaths.removeAll(initFilePaths);
assertEquals(parFiles.size(), newFilePaths.size());
}
private Set<String> getFilePathSet(List<File> files) {
return files.stream().map(f -> {
try {
return f.getCanonicalPath();
} catch (IOException e) {
throw new RuntimeException(e);
}
}).collect(Collectors.toSet());
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] JLLeitschuh commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
JLLeitschuh commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1200142202
> I am sure you are well-intentioned but the path you chose (arg, pun) is not the best one for the community,
I do appreciate the pun 😂
> please take the time to read https://commons.apache.org/security.html, specifically, an issue like this one should be reported to the security mailing list before being published in the open.
Although I had not read the Apache Commons Security disclosure page, I assure you I am very familiar with the Apache Software Foundation's vulnerability disclosure process. I've disclosed numerous vulnerabilities to the ASF in the past. I also have over 50 CVEs to my name.
The problem that I encounter is a matter of scale. I can find tens, hundreds, or sometimes thousands of vulnerabilities in OSS with tools like CodeQL, and GitHub Code Search. The problem that I face is how to actually get those vulnerabilities fixed. I have actually attempted private disclosure to organizations when I encounterd a large scale security vulnerability in the past. It took me months of work ([example](https://infosecwriteups.com/want-to-take-over-the-java-ecosystem-all-you-need-is-a-mitm-1fc329d898fb)).
Reporting these vulnerabilities one at a time is impractical due to the scale of the problem. I believe that, when compared with other options, full disclosure with a fix is far superior no disclosure at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] codecov-commenter commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1199980381
# [Codecov](https://codecov.io/gh/apache/commons-io/pull/371?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#371](https://codecov.io/gh/apache/commons-io/pull/371?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8065a17) into [master](https://codecov.io/gh/apache/commons-io/commit/857a8a25fa85bf8ca8cdd99ee63e1e1a97b17edd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (857a8a2) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #371 +/- ##
============================================
- Coverage 85.97% 85.95% -0.02%
+ Complexity 3089 3088 -1
============================================
Files 200 200
Lines 7330 7329 -1
Branches 905 905
============================================
- Hits 6302 6300 -2
Misses 778 778
- Partials 250 251 +1
```
| [Impacted Files](https://codecov.io/gh/apache/commons-io/pull/371?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [src/main/java/org/apache/commons/io/FileUtils.java](https://codecov.io/gh/apache/commons-io/pull/371/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vRmlsZVV0aWxzLmphdmE=) | `94.49% <100.00%> (-0.01%)` | :arrow_down: |
| [.../main/java/org/apache/commons/io/input/Tailer.java](https://codecov.io/gh/apache/commons-io/pull/371/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vaW5wdXQvVGFpbGVyLmphdmE=) | `85.57% <0.00%> (-0.50%)` | :arrow_down: |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1230340074
Hi All:
I added the above (slightly modified https://github.com/apache/commons-io/pull/371#issuecomment-1228927498) test to git master.
I am still -1 to merge this PR until a test is created to prove it indeed fixes something
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] kinow commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1199978976
@ JLLeitschuh I think I understand the time limitation you have to work privately with organizations, and also to work on tests. What about integrating your tool with another existing tool like Google OSS Fuzz? I am not sure if your tool is a fuzzer, or a static or runtime analyzer that tries to use existing exploits against code bases. Maybe there are other tools like Sonarcloud that also support these analyzers.
The advantage of integrating with a tool like OSS Fuzz, for instance, is that they already have a workflow that is used by organizations like ASF, and a process for private disclosure of issues.
Otherwise, even though I believe fixing these issues is beneficial to users, in the end your pull requests may end up stalled for a long time until a volunteer has time to reproduce, add tests, prepare the CVE/disclosure, etc. And it wouldn't be doing any benefit to users or maintainers in this case.
Cheers
-Bruno
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] JLLeitschuh commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
JLLeitschuh commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1200077856
> I am not sure if your tool is a fuzzer, or a static or runtime analyzer that tries to use existing exploits against code bases.
OpenRewrite is a Format Preserving, Abstract Syntax Tree Transformer. Basically, it's performing static code analysis to detect the vulnerability, then creating a fix by transforming the code to fix the vulnerability.
There are Gradle & Maven plugins available: https://docs.openrewrite.org/reference
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #371: [SECURITY] Fix Partial Path Traversal Vulnerability
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #371:
URL: https://github.com/apache/commons-io/pull/371#issuecomment-1200140128
@JLLeitschuh
I am sure you are well-intentioned but the path you chose (arg, pun) is not the best one for the community, please take the time to read https://commons.apache.org/security.html, specifically, an issue like this one should be reported to the security mailing list before being published in the open.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org