Skip to content

Python: Add query for unsafe use of tempfile.mktemp.#785

Merged
markshannon merged 12 commits intogithub:masterfrom
tausbn:python-unsafe-use-of-mktemp
Feb 27, 2019
Merged

Python: Add query for unsafe use of tempfile.mktemp.#785
markshannon merged 12 commits intogithub:masterfrom
tausbn:python-unsafe-use-of-mktemp

Conversation

@taus-semmle
Copy link
Contributor

Title says it all.

@felicity-semmle for the change note and qhelp.

Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

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

Thanks for pinging me. I've made a few text suggestions, but nothing major.

@@ -0,0 +1,20 @@
/**
* @name Insecure temporary file
* @description Creating a temporary file using mktemp may be insecure.
Copy link
Contributor

Choose a reason for hiding this comment

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

The convention is to put code elements in single quotes in the description, so mktemp -> 'mktemp'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rewritten the description, since the query now supports two other insecure functions (but perhaps it's too vague now?). Please let me know if you think it needs further changes.

<code>mktemp</code> returns. Opening a file with this name must then happen
separately, and there is no guarantee that these operations will happen
atomically. Because of this, it may be possible for an attacker to interfere
with the file before it is opened.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest revising along the following lines to reduce sentence length and, I hope, improve clarity:

Using the mktemp function in the tempfile module to create a file is insecure because the function doesn't ensure exclusive access to the file. It returns a filename that is guaranteed to be unique on creation but the file must be opened in a separate operation. There is no guarantee that the creation and open operations will happen atomically. This provides an opportunity for an attacker to interfere with the file before it is opened.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have rewritten this based on your suggestions.

Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. One small suggestion over the description, but otherwise the text seems fine (although it would be nice to see a preview of the qhelp once the conflicts are resolved).

/**
* @name Insecure temporary file
* @description Creating a temporary file using mktemp may be insecure.
* @description Creating a temporary file name may be insecure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably there are some ways to create a temporary file securely, so perhaps: "Creating a temporary file using this method may be insecure."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

markshannon
markshannon previously approved these changes Feb 26, 2019
Copy link
Contributor

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

Code and tests look fine now.

@markshannon markshannon merged commit f7d7b8e into github:master Feb 27, 2019
@tausbn tausbn deleted the python-unsafe-use-of-mktemp branch February 12, 2021 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments