d/freebsd-dev
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Make suid/sgid restore be "opportunistic" if

Browse Source 
owner restore is not requested.  If you ask
for permissions to be restored but not owner,
you will now get no error if suid/sgid bits
cannot be set.  (It's a security hole to restore
suid/sgid bits if the owner/group aren't restored.)

This fixes an obscure problem where a simple
"tar -xf" with no other options will sometimes
fail gratuitously because of suid/sgid bits.
This is causing occasional problems for people
using bsdtar as a drop-in replacement for
"that other tar program." ;-)

Note: If you do ask for owner restore, then suid/sgid
restore failures still issue an error.  This
only suppresses the error in the case where an
suid/sgid bit restore fails because of an owner
mismatch and owner restore was not requested.

Approved by: re (bmah)
MFC after: 7 days
This commit is contained in:
Tim Kientzle 2007-08-12 17:35:05 +00:00
parent 77912eb26b
commit 9136384dc2
Notes: svn2git 2020-12-20 02:59:44 +00:00 
svn path=/head/; revision=171811
2 changed files with 82 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
lib/libarchive/archive_write_disk.c
View File

@ -1541,15 +1541,28 @@ set_mode(struct archive_write_disk *a, int mode)
}
if (a->pst->st_gid != a->gid) {
mode &= ~ S_ISGID;
archive_set_error(&a->archive, -1, "Can't restore SGID bit");
r = ARCHIVE_WARN;
if (a->flags & ARCHIVE_EXTRACT_OWNER) {
/*
* This is only an error if you
* requested owner restore. If you
* didn't, we'll try to restore
* sgid/suid, but won't consider it a
* problem if we can't.
*/
archive_set_error(&a->archive, -1,
"Can't restore SGID bit");
r = ARCHIVE_WARN;
}
}
/* While we're here, double-check the UID. */
if (a->pst->st_uid != a->uid
&& (a->todo & TODO_SUID)) {
mode &= ~ S_ISUID;
archive_set_error(&a->archive, -1, "Can't restore SUID bit");
r = ARCHIVE_WARN;
if (a->flags & ARCHIVE_EXTRACT_OWNER) {
archive_set_error(&a->archive, -1,
"Can't restore SUID bit");
r = ARCHIVE_WARN;
}
}
a->todo &= ~TODO_SGID_CHECK;
a->todo &= ~TODO_SUID_CHECK;
@ -1561,8 +1574,11 @@ set_mode(struct archive_write_disk *a, int mode)
*/
if (a->user_uid != a->uid) {
mode &= ~ S_ISUID;
archive_set_error(&a->archive, -1, "Can't make file SUID");
r = ARCHIVE_WARN;
if (a->flags & ARCHIVE_EXTRACT_OWNER) {
archive_set_error(&a->archive, -1,
"Can't make file SUID");
r = ARCHIVE_WARN;
}
}
a->todo &= ~TODO_SUID_CHECK;
}

70
lib/libarchive/test/test_write_disk_perms.c
View File

@ -235,6 +235,24 @@ DEFINE_TEST(test_write_disk_perms)
archive_entry_set_uid(ae, getuid() + 1);
archive_write_disk_set_options(a, ARCHIVE_EXTRACT_PERM);
assertA(0 == archive_write_header(a, ae));
/*
* Because we didn't ask for owner, the failure to
* restore SUID shouldn't return a failure.
* We check below to make sure SUID really wasn't set.
* See more detailed comments below.
*/
failure("Opportunistic SUID failure shouldn't return error.");
assertEqualInt(0, archive_write_finish_entry(a));
assert(archive_entry_clear(ae) != NULL);
archive_entry_copy_pathname(ae, "file_bad_suid2");
archive_entry_set_mode(ae, S_IFREG | S_ISUID | 0742);
archive_entry_set_uid(ae, getuid() + 1);
archive_write_disk_set_options(a,
ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER);
assertA(0 == archive_write_header(a, ae));
/* Owner change should fail here. */
failure("Non-opportunistic SUID failure should return error.");
assertEqualInt(ARCHIVE_WARN, archive_write_finish_entry(a));
/* Write a regular file with ARCHIVE_EXTRACT_PERM & SGID bit */
@ -254,16 +272,26 @@ DEFINE_TEST(test_write_disk_perms)
*/
printf("Current user can't test gid restore: must belong to more than one group.\n");
} else {
/* Write a regular file with ARCHIVE_EXTRACT_PERM & SGID bit */
/*
* Write a regular file with ARCHIVE_EXTRACT_PERM & SGID bit
* but without ARCHIVE_EXTRACT_OWNER.
*/
/*
* This is a weird case: The user has asked for permissions to
* be restored but not asked for ownership to be restored. As
* a result, the default file creation will create a file with
* the wrong group. There are two reasonable behaviors: warn
* and drop the SGID bit (the current libarchive behavior) or
* try to set the group. It is completely wrong to set the
* SGID bit with the wrong group (which is, incidentally,
* exactly what gtar 1.15 does).
* the wrong group. There are several possible behaviors for
* libarchive in this scenario:
* = Set the SGID bit. It is wrong and a security hole to
* set SGID with the wrong group. Even POSIX thinks so.
* = Implicitly set the group. I don't like this.
* = drop the SGID bit and warn (the old libarchive behavior)
* = drop the SGID bit and don't warn (the current libarchive
* behavior).
* The current behavior sees SGID/SUID restore when you
* don't ask for owner restore as an "opportunistic"
* action. That is, libarchive should do it if it can,
* but if it can't, it's not an error.
*/
assert(archive_entry_clear(ae) != NULL);
archive_entry_copy_pathname(ae, "file_alt_sgid");
@ -272,10 +300,13 @@ DEFINE_TEST(test_write_disk_perms)
archive_entry_set_gid(ae, altgid());
archive_write_disk_set_options(a, ARCHIVE_EXTRACT_PERM);
assert(0 == archive_write_header(a, ae));
failure("Setting SGID bit should not succeed here.");
assertEqualIntA(a, ARCHIVE_WARN, archive_write_finish_entry(a));
failure("Setting SGID bit should fail because of group mismatch but the failure should be silent because we didn't ask for the group to be set.");
assertEqualIntA(a, 0, archive_write_finish_entry(a));
/* As above, but add _EXTRACT_OWNER. */
/*
* As above, but add _EXTRACT_OWNER to verify that it
* does succeed.
*/
assert(archive_entry_clear(ae) != NULL);
archive_entry_copy_pathname(ae, "file_alt_sgid_owner");
archive_entry_set_mode(ae, S_IFREG | S_ISGID | 0742);
@ -303,7 +334,17 @@ DEFINE_TEST(test_write_disk_perms)
archive_entry_set_gid(ae, invalidgid());
archive_write_disk_set_options(a, ARCHIVE_EXTRACT_PERM);
assertA(0 == archive_write_header(a, ae));
failure("This SGID restore should fail.");
failure("This SGID restore should fail without an error.");
assertEqualIntA(a, 0, archive_write_finish_entry(a));
assert(archive_entry_clear(ae) != NULL);
archive_entry_copy_pathname(ae, "file_bad_sgid2");
archive_entry_set_mode(ae, S_IFREG | S_ISGID | 0742);
archive_entry_set_gid(ae, invalidgid());
archive_write_disk_set_options(a,
ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER);
assertA(0 == archive_write_header(a, ae));
failure("This SGID restore should fail with an error.");
assertEqualIntA(a, ARCHIVE_WARN, archive_write_finish_entry(a));
}
@ -362,6 +403,11 @@ DEFINE_TEST(test_write_disk_perms)
failure("file_bad_suid: st.st_mode=%o", st.st_mode);
assert((st.st_mode & 07777) == (0742));
/* SUID bit should NOT have been set here. */
assert(0 == stat("file_bad_suid2", &st));
failure("file_bad_suid2: st.st_mode=%o", st.st_mode);
assert((st.st_mode & 07777) == (0742));
/* SGID should be set here. */
assert(0 == stat("file_perm_sgid", &st));
failure("file_perm_sgid: st.st_mode=%o", st.st_mode);
@ -384,6 +430,10 @@ DEFINE_TEST(test_write_disk_perms)
assert(0 == stat("file_bad_sgid", &st));
failure("file_bad_sgid: st.st_mode=%o", st.st_mode);
assert((st.st_mode & 07777) == (0742));
/* SGID should NOT be set here. */
assert(0 == stat("file_bad_sgid2", &st));
failure("file_bad_sgid2: st.st_mode=%o", st.st_mode);
assert((st.st_mode & 07777) == (0742));
}
if (getuid() != 0) {