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

Fixed Sensitive Data Exposure at /tmp directory #5375

Merged

Conversation

fazledyn-or
Copy link
Contributor

Summary

This PR fixes a case of sensitive data exposure by using mkstemp method to create dbus.log file and a custom opener to create anaconda-tb-all.log.

Description

While triaging your project, our bug fixing tool generated the following message(s)-

In file: dbus_launcher.py, there is a method that creates a temporary file using a hard coded path to a public writable directory (CWE-379). This exposes the temporarily created file to race condition attacks. An attacker can access, modify or even delete a file. iCR suggested that a temporary file should be created using a safe API in a secured directory instead.

In file: anaconda.py, there is a method that creates a temporary file using a hard coded path to a public writable directory (CWE-379). This exposes the temporarily created file to race condition attacks. An attacker can access, modify or even delete a file. iCR suggested that a temporary file should be created using a safe API in a secured directory instead.

There were two cases which required individual fixes for each of them. I've updated them as it seemed correct to me. Please have a look and let me know what you think. If required, I can update the PR accordingly.

Previously Found & Fixed

Below is a list of open-source projects where this same bug was found and fixed-

CLA Requirements

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

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

Sponsorship and Support

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security.

The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.

pyanaconda/anaconda.py Outdated Show resolved Hide resolved
KKoukiou
KKoukiou previously approved these changes Jan 3, 2024
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

This is fine I just would prefer the commits squashed, as the change from the second commit is introduced in the first commit.

@fazledyn-or
Copy link
Contributor Author

This is fine I just would prefer the commits squashed, as the change from the second commit is introduced in the first commit.

Done. Can you please check now?

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

I just noticed that we hardcode this dbus.log in more places. See pyanaconda/exception.py

We need to match the file matching the pattern there and include it to the file list.

@KKoukiou KKoukiou dismissed their stale review January 3, 2024 15:55

Needs a bit more work.

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Similarly pyanaconda/modules/boss/installation.py and tests/unit_tests/pyanaconda_tests/modules/boss/test_copy_logs_task.py can't have hardcoded the file name anymore.

@fazledyn-or
Copy link
Contributor Author

Similarly pyanaconda/modules/boss/installation.py and tests/unit_tests/pyanaconda_tests/modules/boss/test_copy_logs_task.py can't have hardcoded the file name anymore.

How about we use open_with_perm for the dbus.log too?

@KKoukiou
Copy link
Contributor

KKoukiou commented Jan 4, 2024

Similarly pyanaconda/modules/boss/installation.py and tests/unit_tests/pyanaconda_tests/modules/boss/test_copy_logs_task.py can't have hardcoded the file name anymore.

How about we use open_with_perm for the dbus.log too?

Sounds like the easy way to go forward indeed!

@fazledyn-or
Copy link
Contributor Author

Similarly pyanaconda/modules/boss/installation.py and tests/unit_tests/pyanaconda_tests/modules/boss/test_copy_logs_task.py can't have hardcoded the file name anymore.

How about we use open_with_perm for the dbus.log too?

Sounds like the easy way to go forward indeed!

Please have a look.

This PR fixes a case of sensitive data exposure by
using the existing helped method `open_with_perm`.

Signed-off-by: fazledyn-or <ataf@openrefactory.com>
@KKoukiou
Copy link
Contributor

/kickstart-test --testtype smoke

@KKoukiou KKoukiou merged commit 3d1e979 into rhinstaller:master Jan 10, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants