[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: pre-approval for perl/5.10.0-19



On Thu, Dec 18, 2008 at 10:01:33PM +0200, Niko Tyni wrote:
> Hi release team,
> 
> I understand we're not quite in the deep freeze yet, so please consider
> pre-approving or disapproving these changes I'd like to get in lenny:
> 
>  perl (5.10.0-19) UNRELEASED; urgency=low
>  .
>    * Downgrade the perl-doc recommendation to a suggestion.
>      (Closes: #496770, #442805)
>    * Make File::Temp warn on cleaning up the current working directory at
>      exit instead of bailing out. (Closes: #479317)
>    * Fix $? when dumping core. (Closes: #509041)
>    * Fix a memory leak with Scalar::Util::weaken(). (Closes: #506324)

Here's one more: a non-RC security fix I'd like to get in too.

   * [SECURITY] "second half of CVE-2007-4829": Archive::Tar no longer
     follows symlinks when unpacking. Upstream fix backported by Ubuntu.
     (Closes: #509802)

Patch attached. There are a couple of not strictly necessary cosmetic
fixes in there, but I think there's some value to keeping in sync with
what Ubuntu put in their security update first.

Thanks,
-- 
Niko Tyni   ntyni@debian.org
[SECURITY] "second half of CVE-2007-4829": Archive::Tar no longer follows
symlinks when unpacking.  Upstream fix backported by Ubuntu. (Closes: #509802)

http://rt.cpan.org/Public/Bug/Display.html?id=30380#txn-436899
second half of unpack issue CVE-2007-4829, from 1.39_01 of Archive::Tar

Original patch from Ubuntu version 5.10.0-11.1ubuntu2.2.
diff -uNrp perl-5.10.0~/lib/Archive/Tar.pm perl-5.10.0/lib/Archive/Tar.pm
--- perl-5.10.0~/lib/Archive/Tar.pm	2007-12-18 02:47:07.000000000 -0800
+++ perl-5.10.0/lib/Archive/Tar.pm	2008-12-03 12:56:19.000000000 -0800
@@ -561,26 +561,61 @@ sub _extract_file {
 
     ### it's a relative path ###
     } else {
-        my $cwd     = (defined $self->{cwd} ? $self->{cwd} : cwd());
+        my $cwd     = (ref $self and defined $self->{cwd}) 
+                        ? $self->{cwd} 
+                        : cwd();
 
         my @dirs = defined $alt
             ? File::Spec->splitdir( $dirs )         # It's a local-OS path
             : File::Spec::Unix->splitdir( $dirs );  # it's UNIX-style, likely
                                                     # straight from the tarball
 
-        ### paths that leave the current directory are not allowed under
-        ### strict mode, so only allow it if a user tells us to do this.
         if( not defined $alt            and 
-            not $INSECURE_EXTRACT_MODE  and 
-            grep { $_ eq '..' } @dirs
-        ) {
-            $self->_error(
-                q[Entry ']. $entry->full_path .q[' is attempting to leave the ].
-                q[current working directory. Not extracting under SECURE ].
-                q[EXTRACT MODE]
-            );
-            return;
-        }            
+            not $INSECURE_EXTRACT_MODE 
+        ) {            
+
+            ### paths that leave the current directory are not allowed under
+            ### strict mode, so only allow it if a user tells us to do this.
+            if( grep { $_ eq '..' } @dirs ) {
+    
+                $self->_error(
+                    q[Entry ']. $entry->full_path .q[' is attempting to leave ].
+                    q[the current working directory. Not extracting under ].
+                    q[SECURE EXTRACT MODE]
+                );
+                return;
+            } 
+        
+            ### the archive may be asking us to extract into a symlink. This
+            ### is not sane and a possible security issue, as outlined here:
+            ### https://rt.cpan.org/Ticket/Display.html?id=30380
+            ### https://bugzilla.redhat.com/show_bug.cgi?id=295021
+            ### https://issues.rpath.com/browse/RPL-1716
+            my $full_path = $cwd;
+            for my $d ( @dirs ) {
+                $full_path = File::Spec->catdir( $full_path, $d );
+                
+                ### we've already checked this one, and it's safe. Move on.
+                next if ref $self and $self->{_link_cache}->{$full_path};
+
+                if( -l $full_path ) {
+                    my $to   = readlink $full_path;
+                    my $diag = "symlinked directory ($full_path => $to)";
+
+                    $self->_error(
+                        q[Entry ']. $entry->full_path .q[' is attempting to ].
+                        qq[extract to a $diag. This is considered a security ].
+                        q[vulnerability and not allowed under SECURE EXTRACT ].
+                        q[MODE]
+                    );
+                    return;
+                }
+                
+                ### XXX keep a cache if possible, so the stats become cheaper:
+                $self->{_link_cache}->{$full_path} = 1 if ref $self;
+            }
+        }
+
         
         ### '.' is the directory delimiter, of which the first one has to
         ### be escaped/changed.
@@ -622,7 +657,8 @@ sub _extract_file {
     unless ( -d _ ) {
         eval { File::Path::mkpath( $dir, 0, 0777 ) };
         if( $@ ) {
-            $self->_error( qq[Could not create directory '$dir': $@] );
+            my $fp = $entry->full_path;
+            $self->_error(qq[Could not create directory '$dir' for '$fp': $@]);
             return;
         }
         
@@ -672,8 +708,13 @@ sub _extract_file {
         $self->_make_special_file( $entry, $full ) or return;
     }
 
-    utime time, $entry->mtime - TIME_OFFSET, $full or
-        $self->_error( qq[Could not update timestamp] );
+    ### only update the timestamp if it's not a symlink; that will change the
+    ### timestamp of the original. This addresses bug #33669: Could not update
+    ### timestamp warning on symlinks
+    if( not -l $full ) {
+        utime time, $entry->mtime - TIME_OFFSET, $full or
+            $self->_error( qq[Could not update timestamp] );
+    }
 
     if( $CHOWN && CAN_CHOWN ) {
         chown $entry->uid, $entry->gid, $full or
@@ -707,8 +748,8 @@ sub _make_special_file {
                 or $fail++;
         }
 
-        $err =  qq[Making symbolink link from '] . $entry->linkname .
-                qq[' to '$file' failed] if $fail;
+        $err =  qq[Making symbolic link '$file' to '] .
+                $entry->linkname .q[' failed] if $fail;
 
     } elsif ( $entry->is_hardlink ) {
         my $fail;

Reply to: