GNU bug report logs -
#12947
[brlink@debian.org: Bug#598018: install: temporary insecure file permissions]
Previous Next
Reported by: Samuel Bronson <naesten <at> gmail.com>
Date: Tue, 20 Nov 2012 19:07:01 UTC
Severity: normal
Tags: patch, security
Found in version 8.5
Done: Paul Eggert <eggert <at> cs.ucla.edu>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 12947 in the body.
You can then email your comments to 12947 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-coreutils <at> gnu.org:
bug#12947; Package
coreutils.
(Tue, 20 Nov 2012 19:07:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Samuel Bronson <naesten <at> gmail.com>:
New bug report received and forwarded. Copy sent to
bug-coreutils <at> gnu.org.
(Tue, 20 Nov 2012 19:07:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Package: coreutils
Version: 8.5
Tags: security patch
From <http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=598018>:
[Message part 2 (message/rfc822, inline)]
[Message part 3 (text/plain, inline)]
Package: coreutils
Version: 8.5-1
Tags: security
X-Debbugs-CC: team <at> security.debian.org
Install a regular file with install creates the file with the same
permissions as the original file, copies the contents,
then changes the permissions of that file to 0600 and finally changes
ownerships and sets permissions to the ones requested with -m.
This means that if the target directory is more accessibly than the
original directory, or if the group will be set, the file can
for a short time be accessible to users it should not be accessible to.
Consider for example someone doing
install -m 750 -g shadow /etc/shadow /backup/shadow
results in:
stat64("/etc/shadow", {st_mode=S_IFREG|0640, st_size=778, ...}) = 0
lstat64("/backup/shadow", 0xffd932b4) = -1 ENOENT (No such file or directory)
open("/etc/shadow", O_RDONLY|O_LARGEFILE) = 3
fstat64(3, {st_mode=S_IFREG|0640, st_size=778, ...}) = 0
open("/backup/shadow", O_WRONLY|O_CREAT|O_EXCL|O_LARGEFILE, 0640) = 4
fstat64(4, {st_mode=S_IFREG|0640, st_size=0, ...}) = 0
[...]
read(...)
write(...)
[...]
fchmod(4, 0600) = 0
close(4) = 0
close(3) = 0
lchown32("/backup/shadow", -1, 42) = 0
chmod("/backup/shadow", 0600) = 0
Which means the generated file will for a short time be readable by
accounts in group root (which should only be able to get the contests
if they also know the root password).
Other examples where this can be an issue are copying a file with mode
0644 in a directory only accessible to the current user to a directory
other people can access with install -m 600: again for a short time the
file will be accessible with mode 644.
The following patch fixes that (also attached to avoid transport problems):
diff -r -u -N a/src/copy.c b/src/copy.c
--- a/src/copy.c 2010-04-20 21:52:04.000000000 +0200
+++ b/src/copy.c 2010-09-25 13:44:01.000000000 +0200
@@ -2007,7 +2007,7 @@
used as the 3rd argument in the open call. Historical
practice passed all the source mode bits to 'open', but the extra
bits were ignored, so it should be the same either way. */
- if (! copy_reg (src_name, dst_name, x, src_mode & S_IRWXUGO,
+ if (! copy_reg (src_name, dst_name, x, dst_mode_bits & S_IRWXUGO,
omitted_permissions, &new_dst, &src_sb))
goto un_backup;
}
This patch should be safe as dst_mode_bits is src_mode unless set_mode
is set, which only install seems to set (and for install that behaviour
is always better).
Bernhard R. Link
[diff.diff (text/x-diff, attachment)]
[Message part 5 (text/plain, inline)]
I don't claim to understand it (copy_internal() is gigantic!), but it
seems like this should have been forwarded two years ago...
--
Hi! I'm a .signature virus! Copy me into your ~/.signature to help me spread!
Reply sent
to
Paul Eggert <eggert <at> cs.ucla.edu>:
You have taken responsibility.
(Tue, 20 Nov 2012 21:23:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Samuel Bronson <naesten <at> gmail.com>:
bug acknowledged by developer.
(Tue, 20 Nov 2012 21:23:02 GMT)
Full text and
rfc822 format available.
Message #10 received at 12947-done <at> debbugs.gnu.org (full text, mbox):
Thanks, I installed this patch into the coreutils master branch,
and I'm marking the upstream coreutils bug as done.
From 7ee71d9ddad1435bbea00779bcd4c62482ea3473 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Tue, 20 Nov 2012 13:15:34 -0800
Subject: [PATCH] install: fix security race
* src/copy.c (copy_internal): Use DST_MODE_BITS, not SRC_MODE.
See Bernhard R. Link in <http://bugs.gnu.org/12947> and in
<http://bugs.debian.org/598018>.
---
src/copy.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/copy.c b/src/copy.c
index 16aed03..7a35414 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2394,8 +2394,13 @@ copy_internal (char const *src_name, char const *dst_name,
/* POSIX says the permission bits of the source file must be
used as the 3rd argument in the open call. Historical
practice passed all the source mode bits to 'open', but the extra
- bits were ignored, so it should be the same either way. */
- if (! copy_reg (src_name, dst_name, x, src_mode & S_IRWXUGO,
+ bits were ignored, so it should be the same either way.
+
+ This call uses DST_MODE_BITS, not SRC_MODE. These are
+ normally the same, and the exception (where x->set_mode) is
+ used only by 'install', which POSIX does not specify and
+ where DST_MODE_BITS is what's wanted. */
+ if (! copy_reg (src_name, dst_name, x, dst_mode_bits & S_IRWXUGO,
omitted_permissions, &new_dst, &src_sb))
goto un_backup;
}
--
1.7.11.7
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#12947; Package
coreutils.
(Tue, 20 Nov 2012 21:44:02 GMT)
Full text and
rfc822 format available.
Message #13 received at 12947 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/20/2012 02:20 PM, Paul Eggert wrote:
> Thanks, I installed this patch into the coreutils master branch,
> and I'm marking the upstream coreutils bug as done.
>
>>From 7ee71d9ddad1435bbea00779bcd4c62482ea3473 Mon Sep 17 00:00:00 2001
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Tue, 20 Nov 2012 13:15:34 -0800
> Subject: [PATCH] install: fix security race
>
> * src/copy.c (copy_internal): Use DST_MODE_BITS, not SRC_MODE.
> See Bernhard R. Link in <http://bugs.gnu.org/12947> and in
> <http://bugs.debian.org/598018>.
> ---
> src/copy.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
This also needs a NEWS entry. I'm not sure how easy or hard it would be
to write a test case, though.
--
Eric Blake eblake <at> redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#12947; Package
coreutils.
(Wed, 21 Nov 2012 02:14:02 GMT)
Full text and
rfc822 format available.
Message #16 received at 12947 <at> debbugs.gnu.org (full text, mbox):
On 11/20/2012 01:41 PM, Eric Blake wrote:
> This also needs a NEWS entry. I'm not sure how easy or hard it would be
> to write a test case, though.
Jim's the expert on writing test cases for race conditions.
Not sure that this one is worth a lot of work, though.
I pushed this NEWS patch:
From 791a9c05122a1031820eebf58c04c4f157e36cfd Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Tue, 20 Nov 2012 18:10:21 -0800
Subject: [PATCH] install: fix security race
* NEWS: Document this.
---
NEWS | 3 +++
1 file changed, 3 insertions(+)
diff --git a/NEWS b/NEWS
index 713f761..15fddd4 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,9 @@ GNU coreutils NEWS -*- outline -*-
Instead, cut now fails and emits an appropriate diagnostic.
[This bug was present in "the beginning".]
+ install -m M SOURCE DEST no longer has a race condition where DEST's
+ permissions are temporarily derived from SOURCE instead of from M.
+
pr -n no longer crashes when passed values >= 32. Also line numbers are
consistently padded with spaces, rather than with zeros for certain widths.
[bug introduced in TEXTUTILS-1_22i]
--
1.7.11.7
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#12947; Package
coreutils.
(Wed, 21 Nov 2012 10:29:01 GMT)
Full text and
rfc822 format available.
Message #19 received at 12947 <at> debbugs.gnu.org (full text, mbox):
On November 21, 2012 at 3:12 AM Paul Eggert <eggert <at> cs.ucla.edu> wrote:
> I pushed this [...]
This is more of a question, and I may be wrong,
but isn't here still a race afterwards?
execve("src/ginstall", ["src/ginstall", "-g", "video", "-m",
"664", "src/ginstall", "/tmp/g"], ...) = 0
...
stat("src/ginstall", {st_dev=makedev(8, 16), st_ino=134447,
st_mode=S_IFREG|0755, st_nlink=1,
st_uid=1000, st_gid=100, ...}) = 0
lstat("/tmp/g", 0x7fff6458b750) = -1 ENOENT (No such file or
directory)
open("src/ginstall", O_RDONLY) = 3
fstat(3, {st_dev=makedev(8, 16), st_ino=134447,
st_mode=S_IFREG|0755, st_nlink=1,
st_uid=1000, st_gid=100, ...}) = 0
open("/tmp/g", O_WRONLY|O_CREAT|O_EXCL, 0600) = 4
fstat(4, {st_dev=makedev(8, 2), st_ino=18846,
st_mode=S_IFREG|0600, st_nlink=1,
st_uid=1000, st_gid=100, ...}) = 0
fadvise64(3, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
read(3, ..., 65536) = 65536
write(4, ..., 65536) = 65536
...
fchmod(4, 0600) = 0
close(4) = 0
close(3) = 0
<== ... race? ... ==>
lchown("/tmp/g", 4294967295, 33) = 0
chmod("/tmp/g", 0664) = 0
I.e., after closing FDs 4 and 3, the file "/tmp/g" could
have been replaced. Why aren't we using fchown and
fchmod_or_lchmod before the close() call?
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#12947; Package
coreutils.
(Wed, 21 Nov 2012 15:43:02 GMT)
Full text and
rfc822 format available.
Message #22 received at 12947 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2012 02:27 AM, Bernhard Voelker wrote:
> Why aren't we using fchown and
> fchmod_or_lchmod before the close() call?
The code used to do that, if memory serves, but then
the code was modified to deal with ACLs or SELinux
or whatever and it turned into a big mess, which
I've been afraid to deal with. I vaguely recall that
it had something to do with the relevant ACL and/or
SELinux calls requiring file names (which seemed like
a huge mistake to me at the time).
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Thu, 20 Dec 2012 12:24:04 GMT)
Full text and
rfc822 format available.
This bug report was last modified 11 years and 123 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.