security scanner reports use of tempfile.mkstemp() in _termui_impl.py

[mohanbabubb]

When I ran a security scanner on my project, it returned the following report:

Click Toolkit for Python contains a flaw in the tempfile.mkstemp() function of _termui_impl.py as the program creates temporary files insecurely. It is possible for a local attacker to use a symlink attack against a file to cause the program to unexpectedly grant elevated privileges to the attacker.

It looks like this might be addressed by #947.

Environment

• Python version: any version
• Click version: 2.0 ≤ Version ≤ 7.1.2

[davidism]

Please see our guidelines for responsibly reporting security issues: https://github.com/pallets/click/security/policy

If this is in fact an issue, it appears to be related to Python's tempfile.mkstemp() function, not Click. Please report it to Python using appropriate channels for responsibly reporting.

[mohanbabubb]

This is the report returned by JFrog XRay.

Summary
Click Toolkit for Python _termui_impl.py tempfile.mkstemp() Function Symlink Local Privilege Escalation
Description
Click Toolkit for Python contains a flaw in the tempfile.mkstemp() function of _termui_impl.py as the program creates temporary files insecurely. It is possible for a local attacker to use a symlink attack against a file to cause the program to unexpectedly grant elevated privileges to the attacker.
Type Security
Provider JFrog
Severity Medium
Update Jul 24, 2020 3:28:17 PM
Package Type Pypi
References
https://github.com/pallets/click/pull/947
https://github.com/pallets/click/commit/f50bce0fd7ce972790869049774175dfa19ff05c
Infected Component click
Source Version 7.1.2
Infected Versions 2.0 ≤ Version ≤ 7.1.2
Matched Policies SEC_Medium (Violated Rule: Severity_Medium_no_Automatic_Actions)

[mohanbabubb]

It seems like #947 fixes this issue, why was it closed?

[ThiefMaster]

Please provide an actual example on how our usage of mkstemp is insecure. Otherwise we'll assume this is just an overzealous vulnerability scanner with false positives. When looking at the code using it, then the usage of mkstemp looks perfectly secure.

And #947 is a low-quality PR which does not even work (likely coming from someone who sent PRs created by automated tools):

>>> import tempfile
>>> fd, name = tempfile.TemporaryFile(prefix='editor-', suffix='test')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: not enough values to unpack (expected 2, got 0)

[davidism]

edit requires a filename to work, it spawns a subprocess for f"{editor} "{filename}", so mkstemp (or another function based on it, like NamedTemporaryFile) is required.

Your security scanner does seem to be overly aggressive here, I'd recommend adding an ignore rule for this finding. If you can't ignore it, edit takes either a filename or the text to edit. It only uses mkstemp if you pass the text. If you find mkstemp unacceptable, build your own way to securely generate a file name, write the text to it, then pass that filename to edit.

Based on the low quality of this report and the linked pull request, as well as the existing ability for edit to not use mkstemp, I'm not planning on making a change to this at this time.

I reiterate again that if you want to report security issues about any of the Pallets projects in the future, please follow https://github.com/pallets/click/security/policy.

[mohanbabubb]

That really make sense..