Bug: `secure-tempfile` uses `mkstemp` which is not drop-in replaceable for `mktemp`

[drdavella]

The return value of mktemp is a single string representing the filename, whereas mkstemp returns a tuple of an open OS file handle and the filename. See the docs.

A simple solution would be to replace the call like this:

-filename = tempfile.mktemp()
+filename = tempfile.mkstemp()[1]

However, that leaks the file handle in the first argument of the tuple, which needs to be closed.

A better solution might be to use TemporaryFile. However, that is also not API-compatible with mktemp.

In either case we need a more sophisticated solution.

[drdavella]

This also affects the sonar codemod: sonar:python/secure-tempfile-S5445

[clavedeluna]

I think to get this right we need to think about the code intent. Why would someone write tempfile.mktemp()? To get the name of a file that does not exist in the file system. From there, you would assume the code would actually use the file name to create the file, right? Well consider these two code examples:

# filename is created but never used to create and open a file
import tempfile
filename = tempfile.mktemp()
print(filename)
filename + ".pdf"

# filename is created and used to create and open file
import tempfile
tmp_file = open(tempfile.mktemp(), "w+")
tmp_file.write("text")
tmp_file.close() # code may actually not even have this line!

The code intent is simple but a big point here. Right now the codemod assumes it's all the same, but what we really care about is example 2 here. If a user asks for a tempfile name as a str and then just plays around with the filename str without actually doing any file opening with it, we don't really care. However, if the user does create a file, that's when security issues can come up.

So I propose we radically change this codemod to handle this. Ignore the first case but detect the second by checking if tempfile.mktemp() is used as a sink for open, io.open, etc. If so, we should re-write the example 2 code in the most secure way, like this:

import tempfile
with tempfile.NamedTemporaryFile("w+") as tmp_file:
	tmp_file.write("text")
	...

This handles for:

1. race condition. Cannot create a file with a filename that already exists
2. closing file. Using a CM ensures file closes even if there is some exception

There's going to be some tricky situations I've already identified. For example

filename = tempfile.mktemp()
print(filename) # where would this line go??
tmp_file = open(filename, "w+")

I'm not sure how to know that the print line needs to go inside the replacement CM.

We also need to handle any args passed in to mktemp() as well as different read/write modes a user may have picked.

[drdavella]

I wonder whether it's reasonable to do something like this:

-filename = tempfile.mktemp()
+with tempfile.NamedTemporaryFile(delete=False) as tf:
+    filename = tf.name

This doesn't leak the file handle because it gets closed by the context manager. The does leave the temporary file on disk but that's not really a problem. The only downside is it's slightly inefficient since most use-cases will involve something like this immediately after:

with open(filename, 'w') as ff:
    ...

This is redundant since the NamedTemporaryFile already had an open file handle within the context manager.