Skip to content

Commit

Permalink
Make verification match the new restricted directory symlink behavior
Browse files Browse the repository at this point in the history
Only follow directory symlinks owned by target directory owner or root
during verification to match the behavior of fsmVerify() in the new
CVE-2017-7500 world order.

The code is klunkier than it should and the logic should use common code
with fsmVerify() instead of duplicating it here, but that needs more
changes than is comfortable to backport so starting with this.

Also worth noting that the previous "follow the link" logic from
commit 3ccd774 was not quite right,
it'd fail with RPMVERIFY_LSTATFAIL on a broken symlink when it should've
ran verification on the symlink itself. This behavior is fixed here too.

Finally, once again fakechroot gets in the way and forces the related
verify testcase to be changed to be able to create a valid link. Reuse
the replacement testcase for the purpose and add another case for
verifying an invalid link.
  • Loading branch information
pmatilai committed Sep 28, 2017
1 parent f2d3be2 commit c815822
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 12 deletions.
15 changes: 12 additions & 3 deletions lib/verify.c
Expand Up @@ -94,9 +94,18 @@ rpmVerifyAttrs rpmfilesVerify(rpmfiles fi, int ix, rpmVerifyAttrs omitMask)
}

/* If we expected a directory but got a symlink to one, follow the link */
if (S_ISDIR(fmode) && S_ISLNK(sb.st_mode) && stat(fn, &sb) != 0) {
vfy |= RPMVERIFY_LSTATFAIL;
goto exit;
if (S_ISDIR(fmode) && S_ISLNK(sb.st_mode)) {
struct stat dsb;
/* ...if it actually points to a directory */
if (stat(fn, &dsb) == 0 && S_ISDIR(dsb.st_mode)) {
uid_t fuid;
/* ...and is by a legit user, to match fsmVerify() behavior */
if (sb.st_uid == 0 ||
(rpmugUid(rpmfilesFUser(fi, ix), &fuid) == 0 &&
sb.st_uid == fuid)) {
sb = dsb; /* struct assignment */
}
}
}

/* Links have no mode, other types have no linkto */
Expand Down
56 changes: 47 additions & 9 deletions tests/rpmverify.at
Expand Up @@ -69,18 +69,56 @@ AT_KEYWORDS([verify])
AT_CHECK([
RPMDB_CLEAR
RPMDB_INIT
tf="${RPMTEST}"/opt/foo
rm -rf "${RPMTEST}"/opt/*
rm -rf "${TOPDIR}"

runroot rpm -U --nodeps --noscripts --ignorearch --ignoreos \
/data/RPMS/hello-1.0-1.i386.rpm
mv "${RPMTEST}"/usr/share/doc/hello-1.0 "${RPMTEST}"/usr/share/doc/hello-1.0.orig
ln -s hello-1.0.orig "${RPMTEST}"/usr/share/doc/hello-1.0
runroot rpm -Vva --nodeps --nouser --nogroup
rm -rf "${RPMTEST}"/usr/share/doc/
runroot rpmbuild --quiet -bb \
--define "ver 1.0" \
--define "filetype datadir" \
--define "filedata README1" \
--define "user $(id -u -n)" \
/data/SPECS/replacetest.spec

runroot rpm -U /build/RPMS/noarch/replacetest-1.0-1.noarch.rpm
mv "${RPMTEST}"/opt/foo "${RPMTEST}"/opt/was
ln -s was "${RPMTEST}"/opt/foo
runroot rpm -Vv replacetest
],
[0],
[......... /usr/local/bin/hello
......... /usr/share/doc/hello-1.0
......... d /usr/share/doc/hello-1.0/FAQ
[......... /opt/foo
......... /opt/foo/README1
......... /opt/goo
......... /opt/zoo
],
[])
AT_CLEANUP

AT_SETUP([directory replaced with an invalid directory symlink])
AT_KEYWORDS([verify])
AT_CHECK([
RPMDB_CLEAR
RPMDB_INIT
tf="${RPMTEST}"/opt/foo
rm -rf "${RPMTEST}"/opt/*
rm -rf "${TOPDIR}"

runroot rpmbuild --quiet -bb \
--define "ver 1.0" \
--define "filetype datadir" \
--define "filedata README1" \
/data/SPECS/replacetest.spec

runroot rpm -U /build/RPMS/noarch/replacetest-1.0-1.noarch.rpm
mv "${RPMTEST}"/opt/foo "${RPMTEST}"/opt/was
ln -s was "${RPMTEST}"/opt/foo
runroot rpm -Vv --nouser --nogroup replacetest
],
[1],
[....L.... /opt/foo
......... /opt/foo/README1
......... /opt/goo
......... /opt/zoo
],
[])
AT_CLEANUP
Expand Down

0 comments on commit c815822

Please sign in to comment.