GitPython icon indicating copy to clipboard operation
GitPython copied to clipboard

Safer tempfiles

Open whokilleddb opened this issue 3 years ago • 3 comments

Description

Replaced mktemp() with safer mkstemp() to prevent Race Conditions

whokilleddb avatar May 28 '22 05:05 whokilleddb

Also, I found this amazing stack overflow answer. I hope this is helpful. Thank You :D

whokilleddb avatar May 28 '22 15:05 whokilleddb

Hi @Byron, thank you for being so patient. The reason mktemp is vulnerable to the race condition is that an attacker can simply predict and replace the temporary file with a symlink to a sensitive file.

Later when the program opens the symlink for operations like read or write, they end up writing or reading from the sensitive file instead, which is enough to tamper with a system or leak information about it.

However, mkstemp and NamedTemporaryFile (they internally call the same functions) solve this issue by:

  • mkstemp returns a handle to the file as well as the filename. So, there is no race condition between creation and opening. It also employs additional checks to make sure that there are no conflicts in name while creation. Hence, an attacker can no longer get the opportunity to replace the file with a symlink, thereby effectively dismissing the race condition.

  • NamedTemporaryFile internally calls the mkstemp function as well, thereby encapsulating all it's safety features, with the added benefit that it returns a python File object and with delete=True set, the temporary files so created will be deleted once it's closed, thereby preventing any information leak.

I hope this helps with the explanation. Thank you for being so patient and kind with the issue. It's been a pleasure to contribute and learn 😄

whokilleddb avatar May 29 '22 05:05 whokilleddb

Can you please make a statement related to the resource leaks?

I don't know what to do to get an answer about this. If leaking resources is OK, the python process will crash after a while which seems a lot like denial of access if caused by an attacker. In this case, it looks like a supply-chain attack, doesn't it?

Without an answer to the question above, there doesn't seem to be a reason to understand if each of these cases actually represents a potential issue, and if so why exactly that is.

There is no security issue just because mktemp is used, in these cases, and with even more patience we will both arrive there or you manage to show exactly what has to be done to be exploited. And by that I don't mean to repeat general truisms about the matter but to actually look at what happens. I recommend to start with git/index/util.py:43.

Byron avatar May 29 '22 12:05 Byron

Closed due to inactivity.

Byron avatar Aug 30 '22 07:08 Byron