On Fri, Apr 29, 2011 at 10:02 AM, Eli Bendersky firstname.lastname@example.org wrote:
I completely understand this "other code/thread deletes the path between exists() and unlink()" case - it indeed is a race condition waiting to happen. What I didn't understand was Antoine's example of "attacker creates targetpath between os.path.exists and os.unlink", and was asking for a more detailed example, since I'm not really experienced with security-oriented thinking.
If targetpath is created after the os.path.exists() check, then os.unlink() will not be called, so os.symlink() will raise an exception when it sees that targetpath already exists.
On Thu, Apr 28, 2011 at 5:44 PM, Antoine Pitrou email@example.com wrote:
On Thu, 28 Apr 2011 17:40:05 +0200 Nadeem Vawda firstname.lastname@example.org wrote:
The deletion case could be handled like this:
except OSError as e:
if e.errno != errno.ENOENT:
raise os.symlink(tarinfo.linkname, targetpath)
Someone can still create "targetpath" between the calls to unlink() and symlink() though.
Like I said, that code only handles the case of targetpath being deleted. I can't think of a similarly easy fix for the creation case. You could solve that particular form of the problem with something like:
if tarinfo.issym(): while True: try: os.symlink(tarinfo.linkname, targetpath) except OSError as e: if e.errno != errno.EEXIST: raise else: break try: os.unlink(targetpath) except OSError as e: if e.errno != errno.ENOENT: raise
... but that would open up another DOS vulnerability - if an attacker manages to keep re-creating targetpath in the window between unlink() and symlink(), the loop would never terminate. Also, the code is rather ugly ;-)