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

[SECURITY] Fix Temporary File Information Disclosure Vulnerability #61

Conversation

JLLeitschuh
Copy link
Contributor

Security Vulnerability Fix

This pull request fixes a Temporary File Information Disclosure Vulnerability, which existed in this project.

Preamble

The system temporary directory is shared between all users on most unix-like systems (not MacOS, or Windows). Thus, code interacting with the system temporary directory must be careful about file interactions in this directory, and must ensure that the correct file posix permissions are set.

This PR was generated because a call to File.createTempFile(..) was detected in this repository in a way that makes this project vulnerable to local information disclosure.
With the default uname configuration, File.createTempFile(..) creates a file with the permissions -rw-r--r--. This means that any other user on the system can read the contents of this file.

Impact

Information in this file is visible to other local users, allowing a malicious actor co-resident on the same machine to view potentially sensitive files.

Other Examples

The Fix

The fix has been to convert the logic above to use the following API that was introduced in Java 1.7.

File tmpDir = Files.createTempFile("temp dir").toFile();

The API both creates the file securely, ie. with a random, non-conflicting name, with file permissions that only allow the currently executing user to read or write the contents of this file.
By default, Files.createTempFile("temp dir") will create a file with the permissions -rw-------, which only allows the user that created the file to view/write the file contents.

➡️ 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!

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.

Detecting this and Future Vulnerabilities

This vulnerability was automatically detected by GitHub's CodeQL using this CodeQL Query.

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:
SecureTempFileCreation

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#18

This fixes temporary file information disclosure vulnerability due to the use
of the vulnerable `File.createTempFile()` method. The vulnerability is fixed by
using the `Files.createTempFile()` method which sets the correct posix permissions.

Weakness: CWE-377: Insecure Temporary File
Severity: Medium
CVSSS: 5.5
Detection: CodeQL & OpenRewrite (https://public.moderne.io/recipes/org.openrewrite.java.security.SecureTempFileCreation)

Reported-by: Jonathan Leitschuh <[email protected]>
Signed-off-by: Jonathan Leitschuh <[email protected]>

Bug-tracker: JLLeitschuh/security-research#18


Co-authored-by: Moderne <[email protected]>
Copy link
Owner

@davidmoten davidmoten left a comment

Choose a reason for hiding this comment

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

Thanks!

@codecov-commenter
Copy link

Codecov Report

Base: 69.47% // Head: 69.61% // Increases project coverage by +0.14% 🎉

Coverage data is based on head (5f898a8) compared to base (d5ed1ce).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master      #61      +/-   ##
============================================
+ Coverage     69.47%   69.61%   +0.14%     
- Complexity      419      420       +1     
============================================
  Files            69       69              
  Lines          3531     3531              
  Branches        402      402              
============================================
+ Hits           2453     2458       +5     
+ Misses          945      943       -2     
+ Partials        133      130       -3     
Impacted Files Coverage Δ
...com/github/davidmoten/rx/buffertofile/Options.java 70.58% <100.00%> (ø)
...en/rx/internal/operators/OperatorBufferToFile.java 82.17% <0.00%> (+0.77%) ⬆️
...oten/rx/internal/operators/FileBasedSPSCQueue.java 74.45% <0.00%> (+1.45%) ⬆️
...dmoten/rx/internal/operators/RollingSPSCQueue.java 54.28% <0.00%> (+1.90%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@davidmoten davidmoten merged commit 7f82f49 into davidmoten:master Nov 23, 2022
@JLLeitschuh
Copy link
Contributor Author

Hi,

Do you believe this fixed a valid security vulnerability? Do you need assistance with vulnerability disclosure and CVE issuance?

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

Successfully merging this pull request may close these issues.

3 participants