search

jisaacks / GitGutter

A Sublime Text 2/3 plugin to see git diff in gutter
MIT License
3.88k stars 225 forks source link

Fixed Improper Method Call: Replaced `mktemp` #578

Closed fazledyn-or closed 12 months ago

fazledyn-or commented 12 months ago

Details

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

In file: temp.py, there is a method that creates a temporary file using an unsafe API mktemp. The use of this method is discouraged in the Python documentation. iCR suggested that a temporary file should be created using mkstemp which is a safe API. iCR replaced the usage of mktemp with mkstemp.

Changes

Previously Found & 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.

deathaxe commented 12 months ago

  1. This change would break existing code and can't be applied in proposed form!
    Return value of mkstemp() differs from mktemp().

  2. A comment alone does not turn a function into something unsafe.

    Files are actually created, opened and closed on demand by the calling class.

  3. mktemp() just returns a free temporary name. Nothing else. I don't see anything unsafe happening with that.

  4. The calling class uses user specific temporary directories which is /run/usr/xxxx/GitGutter on my Debian box or %USERPROFILE%\Appdata\Local\Temp on Windows. They are within user-specific folders which are accessible by owner, only.

  5. The GitGutter folder is explicitly created with 0o700. I don't see reasons to deny filesystem traversal to that directory via 0o600.

  6. Temporary files created by GitGutter don't have executable bit set on Unix.

Maybe better analyze code instead of dumping messages based on library comments!