commit 51f536c9d961e81259713c3a56059a12d8c601e0 Author: Raphaƫl Hertzog Date: Fri Mar 5 18:19:33 2010 +0100 Dpkg::Source::Patch: add more sanity checks on patches patch will happily accept filenames like "../../../../stuff" and modify files outside of the expected destination directory. To avoid problems we error out when we detect a filename that contains "/../". Any leading "../" is not a problem since patches are applied with -p1 and it's stripped. We also verify that the file to be modified is not accessed through a symlink as a compromised source package could also provide a symlink in the orig.tar.gz that points outside of the destination directory. diff --git a/debian/changelog b/debian/changelog index 46a9c7b..a299e91 100644 --- a/debian/changelog +++ b/debian/changelog @@ -67,6 +67,12 @@ dpkg (1.15.6) UNRELEASED; urgency=low spaces. Closes: #572030 * dpkg-source does no longer fallback to other source formats if the requested one is not usable. Closes: #557459 + * Modify dpkg-source to error out when it would apply patches containing + insecure paths (with "/../") and also error out when it would apply a + patch through a symlink. Those checks are required as patch will happily + modify files outside of the target directory and unpacking a source package + should not be able to have any side-effect outside of the target + directory. [ Guillem Jover ] * Handle argument parsing in dpkg-checkbuilddeps and dpkg-scanpackages diff --git a/scripts/Dpkg/Source/Patch.pm b/scripts/Dpkg/Source/Patch.pm index 18130ec..25279c4 100644 --- a/scripts/Dpkg/Source/Patch.pm +++ b/scripts/Dpkg/Source/Patch.pm @@ -322,8 +322,9 @@ sub analyze { error(_g("expected ^--- in line %d of diff `%s'"), $., $diff); } $_ = strip_ts($_); - if ($_ eq '/dev/null' or s{^(\./)?[^/]+/}{$destdir/}) { + if ($_ eq '/dev/null' or s{^[^/]+/}{$destdir/}) { $fn = $_; + error(_g("diff %s contains insecure path: %s"), $diff, $_) if m{/../}; } if (/\.dpkg-orig$/) { error(_g("diff `%s' patches file with name ending .dpkg-orig"), $diff); @@ -336,8 +337,9 @@ sub analyze { error(_g("line after --- isn't as expected in diff `%s' (line %d)"), $diff, $.); } $_ = strip_ts($_); - if ($_ eq '/dev/null' or s{^(\./)?[^/]+/}{$destdir/}) { + if ($_ eq '/dev/null' or s{^[^/]+/}{$destdir/}) { $fn2 = $_; + error(_g("diff %s contains insecure path: %s"), $diff, $_) if m{/../}; } else { unless (defined $fn) { error(_g("none of the filenames in ---/+++ are relative in diff `%s' (line %d)"), @@ -363,6 +365,17 @@ sub analyze { if ($dirname =~ s{/[^/]+$}{} && not -d $dirname) { $dirtocreate{$dirname} = 1; } + + # Sanity check, refuse to patch through a symlink + $dirname = $fn; + while (1) { + if (-l $dirname) { + error(_g("diff %s modifies file %s through a symlink: %s"), + $diff, $fn, $dirname); + } + last unless $dirname =~ s{/[^/]+$}{}; + } + if (-e $fn and not -f _) { error(_g("diff `%s' patches something which is not a plain file"), $diff); }