Bug#332434: storebackup: Several security problems (already fixed in sid/testing)
Hi Martin Schulze schrieb: http://cve.mitre.org/cgi-bin/cvename.cgi?name=CAN-2005-3146 seems fixed by the newly introduced checkDelSymlink() function, which was added to ten different places in the code (not all of which might be security sensitive, but at least two operate directly on temporary files). This does not eliminate the vulnerability but only shortens the vulnerable window. An attacker can still re-create the link between the unlink() and the open() calls. The proper action would be to use File::Temp or something similar. Though the whole passing around temporary file names and reopening them in another function seems broken to me, see -outRandom and others. Searching for uses of $tmpdir in StoreBackup.pl and reveals a dozend or so places where filenames in /tmp are passed to functions which then open them writable without any checks (though with randomized suffixes). Some of these files are then opened again later. I'm not competent on this whole tempfile race issue but I don't like this. I'm now building a 1.18 package for stable with your fixes, one day I have to replace the filename passing stuff by filehandle passing, but this will happen in a current version. Shall I build a deb with your patches for you? regards, 2ri -- Secure email, spread GPG, clearsign all mail. http://www.gnupg.org . Reality is that which, when you stop believing in it, doesn't go away. -- Philip K. Dick signature.asc Description: Digital signature
Bug#332434: storebackup: Several security problems (already fixed in sid/testing)
Arthur Korn wrote: Hi 1.19-1 source and binary packages work on stable, and the differences to 1.18.4-2 are all local bugfixes, so I figure it doesn't make any sense to separate bugfixes from bugfixes for a special security fix for stable. Well, we could split out Since the diff between 1.18 and 1.19 is some 1385 lines large, I have some doubts that it only contains security corrections. Hence, using the new upstream version does not look like the way to go. Regards, Joey -- Everybody talks about it, but nobody does anything about it! -- Mark Twain -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#332434: storebackup: Several security problems (already fixed in sid/testing)
Moritz Muehlenhoff wrote:
1.19-1 source and binary packages work on stable, and the
differences to 1.18.4-2 are all local bugfixes, so I figure it
doesn't make any sense to separate bugfixes from bugfixes for a
special security fix for stable. Well, we could split out
storeBackupSync, though that new script is explicitely marked as
experimental.
Security fixes for stable are typically minimal.
I've extracted the patches from the new upstream version.
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CAN-2005-3146
seems fixed by the newly introduced checkDelSymlink() function,
which was added to ten different places in the code (not all of which
might be security sensitive, but at least two operate directly
on temporary files).
This does not eliminate the vulnerability but only shortens the vulnerable
window. An attacker can still re-create the link between the unlink()
and the open() calls. The proper action would be to use File::Temp
or something similar.
I'm not sure about
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CAN-2005-3148,
which would require some more studying of the code.
It's the chown call.
It seems that the old version executed chown uid gid link which doesn't
work. The new version executes chown -h uid:gid link. My manpage doesn't
document -h though.
Regards,
Joey
--
Everybody talks about it, but nobody does anything about it! -- Mark Twain
Please always Cc to me when replying to me on the lists.
diff -u -p -Nr --exclude CVS storebackup-1.18.4.orig/bin/storeBackup.pl
storebackup-1.18.4/bin/storeBackup.pl
--- storebackup-1.18.4.orig/bin/storeBackup.pl 2004-07-23 05:58:47.0
+0200
+++ storebackup-1.18.4/bin/storeBackup.pl 2005-10-07 09:32:59.0
+0200
@@ -4819,6 +4819,7 @@ sub new
if ($self-{'debug'} == 0 or $self-{'debug'} == 1)
{
local *FILE;
+ ::checkDelSymLink($self-{'tmpfile'}, $prLog, 0x01);
open(FILE, . $self-{'tmpfile'}) or
$prLog-print('-kind' = 'E',
'-str' =
@@ -4933,6 +4934,7 @@ sub new
my $tmpfile = $self-{'tmpDir'} . /storeBackup-dirs.$$;
$self-{'tmpfile'} = $tmpfile;
local *FILE;
+::checkDelSymLink($tmpfile, $prLog, 0x01);
open(FILE, $tmpfile) or
$prLog-print('-kind' = 'E',
'-str' = [cannot open $tmpfile, exiting],
diff -u -p -Nr --exclude CVS storebackup-1.18.4.orig/lib/fileDir.pl
storebackup-1.18.4/lib/fileDir.pl
--- storebackup-1.18.4.orig/lib/fileDir.pl 2004-07-23 05:58:47.0
+0200
+++ storebackup-1.18.4/lib/fileDir.pl 2005-10-07 09:33:42.0 +0200
@@ -28,6 +28,46 @@ require 'forkProc.pl';
use strict;
+# checks if a file is a symlink and deletes it if wanted
+# return values (if not exiting):
+# 0: no symlink
+# -1: found symlink
+
+sub checkDelSymLink
+{
+my $file = shift; # name of the file
+my $prLog = shift;
+my $delExit = shift;# set bits:
+# bit 0: 0 = do not delete
+# 1 = delete synlink
+# if not possible, exit
+# bit 1: 0 = do not exit (if exists)
+# 1 = exit
+
+return 0 unless -l $file;
+
+if ($delExit 0x02)
+{
+ $prLog-print('-kind' = 'E',
+ '-str' =
+ [found symbolic link at $file, exiting ],
+ '-exit' = 1);
+}
+
+if ($delExit 0x01)
+{
+ $prLog-print('-kind' = 'W',
+ '-str' = [unlinking symbolic link $file]);
+ unlink $file or
+ $prLog-print('-kind' = 'E',
+ '-str' = [cannot unlink $file, exiting],
+ '-exit' = 1);
+}
+
+return -1;
+}
+
+
sub splitFileDir
{
--- storebackup-1.18.4/bin/storeBackup.pl 2004-07-23 05:58:47.0
+0200
+++ storebackup-1.19/bin/storeBackup.pl 2005-08-12 21:11:18.0 +0200
@@ -3164,6 +3183,7 @@
[cannot create $aktDir, exiting],
'-exit' = 1)
unless (mkdir $aktDir);
+chmod 0755, $aktDir;
my $chmodDir = $chmodMD5File;
$chmodDir |= 0100 if $chmodDir 0400;
$chmodDir |= 0010 if $chmodDir 0040;
diff -Nur storebackup-1.18.4.orig/bin/storeBackup.pl
storebackup-1.19.orig/bin/storeBackup.pl
--- storebackup-1.18.4.orig/bin/storeBackup.pl 2004-07-23 05:58:47.0
+0200
+++ storebackup-1.19.orig/bin/storeBackup.pl2005-08-12 21:11:18.0
+0200
@@ -3606,7 +3626,7 @@
# geaendert, sondern die Datei, auf die er verweist.
# (dann muss lchown genommen werden - Inkompatibilitaeten!?)
my $chown = forkProc-new('-exec' = 'chown',
-
Bug#332434: storebackup: Several security problems (already fixed in sid/testing)
Martin Schulze wrote:
I'm not sure about
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CAN-2005-3148,
which would require some more studying of the code.
It's the chown call.
It seems that the old version executed chown uid gid link which doesn't
work. The new version executes chown -h uid:gid link. My manpage doesn't
document -h though.
Sounds correct, my manpage says:
-h, --no-dereference
affect each symbolic link instead of any referenced file (useful only on
systems that can change the ownership of a symlink)
However, I think that this hunk is missing for CAN-2005-3148:
diff -Naur storebackup-1.18.4/bin/storeBackupRecover.pl
storebackup-1.19/bin/storeBackupRecover.pl
--- storebackup-1.18.4/bin/storeBackupRecover.pl2005-10-06
17:37:09.0 +0200
+++ storebackup-1.19/bin/storeBackupRecover.pl 2005-10-06 17:36:32.0
+0200
@@ -364,7 +371,7 @@
# geaendert, sondern die Datei, auf die er verweist.
# (dann muss lchown genommen werden - Inkompatibilitaeten!?)
my $chown = forkProc-new('-exec' = 'chown',
- '-param' = [$uid, $gid,
+ '-param' = ['-h', $uid:$gid,
$targetFile],
'-outRandom' = $tmpdir/chown-,
'-prLog' = $prLog);
Otherwise permissions might be incorrectly restored.
Cheers,
Moritz
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#332434: storebackup: Several security problems (already fixed in sid/testing)
Moritz Muehlenhoff wrote:
Sounds correct, my manpage says:
-h, --no-dereference
affect each symbolic link instead of any referenced file (useful only on
systems that can change the ownership of a symlink)
However, I think that this hunk is missing for CAN-2005-3148:
diff -Naur storebackup-1.18.4/bin/storeBackupRecover.pl
storebackup-1.19/bin/storeBackupRecover.pl
--- storebackup-1.18.4/bin/storeBackupRecover.pl2005-10-06
17:37:09.0 +0200
+++ storebackup-1.19/bin/storeBackupRecover.pl 2005-10-06 17:36:32.0
+0200
@@ -364,7 +371,7 @@
# geaendert, sondern die Datei, auf die er verweist.
# (dann muss lchown genommen werden - Inkompatibilitaeten!?)
my $chown = forkProc-new('-exec' = 'chown',
- '-param' = [$uid, $gid,
+ '-param' = ['-h', $uid:$gid,
$targetFile],
'-outRandom' = $tmpdir/chown-,
'-prLog' = $prLog);
Otherwise permissions might be incorrectly restored.
Oops, indeed. Thanks.
Regards,
Joey
--
Everybody talks about it, but nobody does anything about it! -- Mark Twain
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#332434: storebackup: Several security problems (already fixed in sid/testing)
Package: storebackup Version: 1.18.4-2 Severity: grave Tags: security Justification: user security hole Although it's not really mentioned in the changelog storebackup 1.19 fixed several security problems, which are still present in Sarge, they've been assigned CAN-2005-3150, CAN-2005-3149 and CAN-2005-3148: Quoting upstream's changelog: - uid and gid were not set correctly for symbolic links in the backups (in the files, not the description of the files) - check for symbolic links before opening temporary files - set permissions of backup root directory to 0755 (independent of umask) - uid and gid were not set correctly for symbolic links when restoring, instead they were changed in the file where the symlink pointed to Cheers, Moritz -- System Information: Debian Release: testing/unstable APT prefers unstable APT policy: (500, 'unstable') Architecture: i386 (i686) Shell: /bin/sh linked to /bin/bash Kernel: Linux 2.6.14-rc1 Locale: LANG=C, [EMAIL PROTECTED] (charmap=ISO-8859-15) -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#332434: storebackup: Several security problems (already fixed in sid/testing)
Hi 1.19-1 source and binary packages work on stable, and the differences to 1.18.4-2 are all local bugfixes, so I figure it doesn't make any sense to separate bugfixes from bugfixes for a special security fix for stable. Well, we could split out storeBackupSync, though that new script is explicitely marked as experimental. I don't know the details of the security issues, but might have some time over the weekend to look at it if needed. Moritz Muehlenhoff schrieb: Package: storebackup Version: 1.18.4-2 Severity: grave Tags: security Justification: user security hole Although it's not really mentioned in the changelog storebackup 1.19 fixed several security problems, which are still present in Sarge, they've been assigned CAN-2005-3150, CAN-2005-3149 and CAN-2005-3148: Quoting upstream's changelog: - uid and gid were not set correctly for symbolic links in the backups (in the files, not the description of the files) - check for symbolic links before opening temporary files - set permissions of backup root directory to 0755 (independent of umask) - uid and gid were not set correctly for symbolic links when restoring, instead they were changed in the file where the symlink pointed to ciao, 2ri -- signature.asc Description: Digital signature
Bug#332434: storebackup: Several security problems (already fixed in sid/testing)
Arthur Korn wrote: BTW, I made an error in my initial bug report, it's CAN-2005-314[876]. 1.19-1 source and binary packages work on stable, and the differences to 1.18.4-2 are all local bugfixes, so I figure it doesn't make any sense to separate bugfixes from bugfixes for a special security fix for stable. Well, we could split out storeBackupSync, though that new script is explicitely marked as experimental. Security fixes for stable are typically minimal. I don't know the details of the security issues, but might have some time over the weekend to look at it if needed. A quick view at the interdiff between 18.4-2 and 19-1 shows that http://cve.mitre.org/cgi-bin/cvename.cgi?name=CAN-2005-3147 seems fixed by this hunk: --- storebackup-1.18.4/bin/storeBackup.pl 2004-07-23 05:58:47.0 +0200 +++ storebackup-1.19/bin/storeBackup.pl 2005-08-12 21:11:18.0 +0200 @@ -3164,6 +3183,7 @@ [cannot create $aktDir, exiting], '-exit' = 1) unless (mkdir $aktDir); +chmod 0755, $aktDir; my $chmodDir = $chmodMD5File; $chmodDir |= 0100 if $chmodDir 0400; $chmodDir |= 0010 if $chmodDir 0040; http://cve.mitre.org/cgi-bin/cvename.cgi?name=CAN-2005-3146 seems fixed by the newly introduced checkDelSymlink() function, which was added to ten different places in the code (not all of which might be security sensitive, but at least two operate directly on temporary files). I'm not sure about http://cve.mitre.org/cgi-bin/cvename.cgi?name=CAN-2005-3148, which would require some more studying of the code. Cheers, Moritz -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
