Skip Menu |
 

This queue is for tickets about the File-Temp CPAN distribution.

Report information
The Basics
Id: 69106
Status: stalled
Priority: 0/
Queue: File-Temp

People
Owner: Nobody in particular
Requestors: john [...] nixnuts.net
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: (no value)
Fixed in: (no value)



Subject: File:Temp::_is_safe() allows unsafe traversal of symlinks
Download (untitled) / with headers
text/plain 296b
Example... As user "attacker": ln -s /tmp /tmp/exploit As user "victim": perl -MFile::Temp -e 'File::Temp->safe_level(File::Temp::HIGH); print File::Temp::tempdir("/tmp/exploit/meXXXX") . "\n";' The temporary directory path that is returned includes the symlink owned by the "attacker" user.
Subject: symlink-safety.patch
From b0fc1b4b83b545c09d2fccb34486052654abca20 Mon Sep 17 00:00:00 2001 From: John Lightsey <jd@cpanel.net> Date: Mon, 27 Jun 2011 13:07:44 -0500 Subject: [PATCH] symlink safety Add check for unsafe symbolic links to _is_safe() directory check. --- Temp.pm | 20 +++++++++++++++++++- 1 files changed, 19 insertions(+), 1 deletions(-) diff --git a/Temp.pm b/Temp.pm index a2d4ae0..0d515a9 100644 --- a/Temp.pm +++ b/Temp.pm @@ -668,7 +668,25 @@ sub _is_safe { my $err_ref = shift; # Stat path - my @info = stat($path); + my @info = lstat($path); + my $symlink_test_path = $path; + my $symlink_loop_count = 0; + while (-l _) { + if (++$symlink_loop_count >= 50) { + $$err_ref = "50 levels of symlinks encountered at $path"; + return 0; + } + if ( $info[4] <= File::Temp->top_system_uid() || $info[4] == $>) { + # safe to traverse + $symlink_test_path = readlink($symlink_test_path); + @info = lstat($symlink_test_path); + } + else { + $$err_ref = "Unsafe symlink at $path"; + return 0; + } + } + unless (scalar(@info)) { $$err_ref = "stat(path) returned no values"; return 0; -- 1.7.2.5
This bug has been assigned CVE-2011-4116
Download (untitled) / with headers
text/plain 452b
Solar Designer on OSS-Sec had some great comments on this bug. The full thread starts here: http://seclists.org/oss-sec/2011/q4/231 The patch I sent before is not adequate to address this bug. In particular, it would still allow using a chain of hardlinked symlinks to construct an insecure path. Making _is_safe() return false if it hits any symlinks would be a better simple fix. Solar Designer gave a more detailed analysis in the linked thread.
Subject: Re: [rt.cpan.org #69106] File:Temp::_is_safe() allows unsafe traversal of symlinks
Date: Mon, 7 Nov 2011 16:12:28 -1000
To: bug-File-Temp [...] rt.cpan.org
From: Tim Jenness <tjenness [...] cpan.org>
Download (untitled) / with headers
text/plain 781b
On Fri, Nov 4, 2011 at 9:46 AM, John D. Lightsey via RT < bug-File-Temp@rt.cpan.org> wrote: Show quoted text
> Queue: File-Temp > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=69106 > > > Solar Designer on OSS-Sec had some great comments on this bug. > > The full thread starts here: > > http://seclists.org/oss-sec/2011/q4/231 > > The patch I sent before is not adequate to address this bug. In > particular, it would still allow using a chain of hardlinked symlinks to > construct an insecure path. > > Making _is_safe() return false if it hits any symlinks would be a better > simple fix. Solar Designer gave a more detailed analysis in the linked > thread. >
So _is_safe() should check each directory in $path and return false if any of them are soft links? -- Tim Jenness
Subject: Re: [rt.cpan.org #69106] File:Temp::_is_safe() allows unsafe traversal of symlinks
Date: Sun, 13 Nov 2011 17:03:55 -0600
To: bug-File-Temp [...] rt.cpan.org
From: John Lightsey <john [...] nixnuts.net>
Download (untitled) / with headers
text/plain 2.6k
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 11/07/2011 08:12 PM, TJENNESS via RT wrote: Show quoted text
> So _is_safe() should check each directory in $path and return false if any > of them are soft links?
IMHO, that would indeed be safe. Solar Designer expressed concern that /tmp itself might be a symlink (making it impossible to use File::Temp.) Some other alternatives could be: 1) If you hit a symlink, verify the link side is owned correctly, then start walking the directories again towards the link target. IE: A points to foo/bar and is owned by the correct ID B points to /tmp and is owned by the correct ID Attacker hardlinks A to /tmp/exploit Attacker creates /tmp/foo directory Attacker hardlinks B to /tmp/foo/bar Now the victim asks for /tmp/exploit/xxxx, but the attacker controls the real path since she owns /tmp/foo To protect against it: - - check /tmp (directory, owned correctly, move on.) - - check /tmp/exploit (symlink, owned correctly, read its target) - -- check all the paths leading up to the symlink target - -- check /tmp (directory, owned correctly, move on.) - -- check /tmp/foo (directory, not owned correctly, stop) Perhaps for a bit of optimization, figure out the full link target path and start at the first portion of the path that hasn't been verified rather than walking up the complete link target path from the bottom. 2) Dont check the parent directory at all and document that any request like File::Temp::tempdir("/tmp/exploit/meXXXX") implies that the caller has verified /tmp/exploit already. 3) Dont allow any symlinks in the directory chain. you could make this more tolerable by allowing a symlink in the $ENV{'TMP'} portion of the path under the assumption that $ENV{'TMP'} should be set sensibly by the caller. Solution #1 is probably the best. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQIcBAEBAgAGBQJOwEzXAAoJEORPgBbTYw+JnjQQAJ/TdUCLsa3IsoObsWdbvbwx zMAogHadSLA69Sl6pxkJvaPOCxDCmrNUrmKu6ujwC2nl0cD62ub7fZNcsN6Niec+ u3U/mOqHWz/acI471yiaGr7paqnU+lJEHTwLdCqgO9bLMLaqs7gLQYBW5dzKPko5 gMqYRfSAxw9e1es8AV5qdt1C8Y2X9BYCe5+Vb3jqYqx4GuwmI+EOToiHWbF1azBF K9VGpmLMySDkkpAcEDkboW43IGTjphXmoFY4LNpJSj3AIqzKftdxTXihAUb+z3D8 s0AHm28j7Dgfh3UWUarV/9a9DNlEIXEgT1kHrmGdkLwDHP4L2eSuTLgGgEOLDy3B RWj2HoYC5bPfK5g4U7w1pvFuv9CUeGJzd55DCaCz7Vkj/Z2lxRfgCuQM2itV82Sl CP4AUbxRtAC+VYhPNFIzEpAgw3SoAuWEZnndlZ5AuDnK2ZiDCPtT1+LGSijA7SO2 g0898JZ+Nq9UbnLj3KPWB6BL90j9L/8YTAx9m+t7lt+2lreYMME9tzCrPBCx/BNf o0frcvQc7wFVZ4YDlRoekcjfOv209vBRwEPIj/uNpyy2YFyKLYI77jVaH0pZhGej cMFkITcDUfK7DMnTzzx1ofQEFrBBuL2UmWMFXA7VopNam8nDtg8Ki3WM8LYAcgKf iexZ8DJdi6QCD4qSYOXh =A3rn -----END PGP SIGNATURE-----
Download (untitled) / with headers
text/plain 491b
On Sun Nov 13 18:01:49 2011, lightsey wrote: Show quoted text
> 1) If you hit a symlink, verify the link side is owned correctly, then > start walking the directories again towards the link target.
What if all symlinks in the parent were resolved using Cwd::abs_path() prior to the existing checks? E.g. /tmp/exploit resolves to /tmp/foo/bar and that gets checked and finds that foo is incorrectly owned? That would allow the case where /tmp is a symlink since it would just get resolved to the real path.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.