[Python-Dev] [Python-checkins] cpython (2.7): Fix closes issue10761: tarfile.extractall failure when symlinked files are

[Nadeem Vawda]

On Fri, Apr 29, 2011 at 10:02 AM, Eli Bendersky <eliben at gmail.com> 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 <solipsis at pitrou.net> wrote:
> On Thu, 28 Apr 2011 17:40:05 +0200
> Nadeem Vawda <nadeem.vawda at gmail.com> wrote:
>>
>> The deletion case could be handled like this:
>>
>>              if tarinfo.issym():
>> +                try:
>> +                    os.unlink(targetpath)
>> +                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 ;-)

Cheers,
Nadeem