uupdate: CVE-2014-1833: symlink directory traversal

Debian Bug report logs - #737160
uupdate: CVE-2014-1833: symlink directory traversal

Toggle useless messages

Message #3 received at submit@bugs.debian.org (full text, mbox, reply):

From: Jakub Wilk <jwilk@debian.org>

To: Debian Bug Tracking System <submit@bugs.debian.org>

Subject: [uupdate] symlink directory traversal

Date: Thu, 30 Jan 2014 21:06:38 +0100

[Message part 1 (text/plain, inline)]

Package: devscripts
Version: 2.14.1
Tags: security

A malicious .orig.tar file can trick uupdate into patching files outside 
the source package directory. Proof of concept:

$ apt-get source -qq chewmail
gpgv: Signature made Tue Aug 15 08:10:17 2006 CEST using DSA key ID 16D970C6
gpgv: Can't check signature: public key not found
dpkg-source: warning: failed to verify signature on ./chewmail_1.2-1.dsc
dpkg-source: info: extracting chewmail in chewmail-1.2
dpkg-source: info: unpacking chewmail_1.2.orig.tar.gz
dpkg-source: info: applying chewmail_1.2-1.diff.gz

$ cd chewmail-1.2/

$ ls /tmp/*
ls: cannot access /tmp/*: No such file or directory

$ uupdate -v2 /path/to/chewmail-2.tar.gz
New Release will be 2-1.
Symlinking to pristine source from chewmail_2.orig.tar.gz...
-- Untarring the new sourcecode archive /path/to/chewmail-2.tar.gz
Success!  The diffs from version 1.2-1 worked fine.
Remember: Your current directory is the OLD sourcearchive!
Do a "cd ../chewmail-2" to see the new package

$ ls /tmp/*
/tmp/changelog  /tmp/compat  /tmp/control  /tmp/copyright  /tmp/rules

-- 
Jakub Wilk

[chewmail-2.tar.gz (application/octet-stream, attachment)]

Message #10 received at 737160@bugs.debian.org (full text, mbox, reply):

From: James McCoy <jamessan@debian.org>

To: Jakub Wilk <jwilk@debian.org>, 737160@bugs.debian.org

Subject: Re: Bug#737160: [uupdate] symlink directory traversal

Date: Fri, 21 Feb 2014 22:53:34 -0500

[Message part 1 (text/plain, inline)]

On Thu, Jan 30, 2014 at 09:06:38PM +0100, Jakub Wilk wrote:
> A malicious .orig.tar file can trick uupdate into patching files
> outside the source package directory. Proof of concept:

Thanks for the report and PoC.

Looking into it some, below is my understanding of the issue and
concerns on fixing it.

First, this is only a problem for 1.0 format source packages, since
unpacking a 3.0 format's diff tarball will replace a, potentially
malicious, symlink in upstream's source with the corresponding directory
in the diff tarball.

With it constrained to 1.0 format, the problem exists for any file the
diff.gz is adding (or possibly, but much less likely, modifying) where
one of the directories in the path is a symlink pointing outside of the
upstream source tree.

We basically need to add the following just inside the if on line 730:

    for link in $(find -type l); do
        resolved="$(readlink -f "$link")"
        if ! expr "$resolved" : "$(pwd)" >/dev/null; then
            complain loudly
        fi
    done

The problem with the above is that it's not robust in the face of paths
which contain whitespace.  That means, at best, some paths aren't
properly detected and therefore are still subject to original issue.

If someone more familiar with the inrticacies of handling this sort of
scenario in (ba)sh has an idea on how to properly implement this, I'm
all ears.  Otherwise, I'm tempted to rewrite the whole thing in Perl,
but I'd rather taking the time to do that.

Cheers,
-- 
James
GPG Key: 4096R/331BA3DB 2011-12-05 James McCoy <jamessan@debian.org>

[signature.asc (application/pgp-signature, inline)]

Message #13 received at 737160@bugs.debian.org (full text, mbox, reply):

From: Jakub Wilk <jwilk@debian.org>

To: 737160@bugs.debian.org

Subject: Re: Bug#737160: [uupdate] symlink directory traversal

Date: Sun, 23 Feb 2014 12:11:04 +0100

* James McCoy <jamessan@debian.org>, 2014-02-21, 22:53:
>>A malicious .orig.tar file can trick uupdate into patching files 
>>outside the source package directory. Proof of concept:
>
>Thanks for the report and PoC.
>
>Looking into it some, below is my understanding of the issue and 
>concerns on fixing it.
>
>First, this is only a problem for 1.0 format source packages, since 
>unpacking a 3.0 format's diff tarball will replace a, potentially 
>malicious, symlink in upstream's source with the corresponding 
>directory in the diff tarball.

Indeed.

>With it constrained to 1.0 format, the problem exists for any file the 
>diff.gz is adding (or possibly, but much less likely, modifying) where 
>one of the directories in the path is a symlink pointing outside of the 
>upstream source tree.

That's right.

>We basically need to add the following just inside the if on line 730:
>
>    for link in $(find -type l); do
>        resolved="$(readlink -f "$link")"
>        if ! expr "$resolved" : "$(pwd)" >/dev/null; then
>            complain loudly
>        fi
>    done
>
>The problem with the above is that it's not robust in the face of paths 
>which contain whitespace.  That means, at best, some paths aren't 
>properly detected and therefore are still subject to original issue.

More (minor) problems I see:
- Special characters in the regexp are not escaped. At least "." could 
be legitimately part of cwd.
- TOCTOU problem: what the symlink resolves to can change between the 
readlink call and unpacking. This is because the symlink could point to 
another symlink, owned by malicious local user.
- The check probably shouldn't complain about symlinks that weren't 
going to be patched anyway.

>If someone more familiar with the inrticacies of handling this sort of 
>scenario in (ba)sh has an idea on how to properly implement this, I'm 
>all ears.

Perhaps a more viable way would be to construct a temporary new source 
package, and let dpkg-source deal with all the corner cases of unpacking 
it?

Or maybe patch(1) should (have an option to) avoid following directory 
symlinks.

-- 
Jakub Wilk

Message #18 received at 737160@bugs.debian.org (full text, mbox, reply):

From: Jakub Wilk <jwilk@debian.org>

To: 737160@bugs.debian.org

Subject: Re: Bug#737160: [uupdate] symlink directory traversal

Date: Mon, 28 Apr 2014 13:47:06 +0200

* Jakub Wilk <jwilk@debian.org>, 2014-02-23, 12:11:
>Perhaps a more viable way would be to construct a temporary new source 
>package, and let dpkg-source deal with all the corner cases of 
>unpacking it?

Now I realized that this won't work, because dpkg-source insist that 
patches apply without fuzz.

So here's a different strategy, similar to what tar(1) implements to 
defend against symlink attacks:

1) Unpack .orig.tar.
2) Delete all symlinks (and maybe also other non-regular files).
3) Apply the diff.
4) Restore all the files deleted in step 2.

-- 
Jakub Wilk

Message #23 received at 737160-submitter@bugs.debian.org (full text, mbox, reply):

From: James McCoy <jamessan@debian.org>

To: 737160-submitter@bugs.debian.org

Subject: Bug#737160 marked as pending

Date: Sat, 11 Oct 2014 04:18:59 +0000

tag 737160 pending
thanks

Hello,

Bug #737160 reported by you has been fixed in the Git repository. You can
see the changelog below, and you can check the diff of the fix at:

    http://git.debian.org/?p=collab-maint/devscripts.git;a=commitdiff;h=0fef671

---
commit 0fef671831e667a2abfe459d47589d6ea4eee32b
Author: James McCoy <jamessan@debian.org>
Date:   Sat Oct 11 00:19:55 2014 -0400

    uupdate: Avoid patching through symlinks for 1.0 source format
    
    Closes: #737160
    Closes: CVE-2014-1833
    Signed-off-by: James McCoy <jamessan@debian.org>

diff --git a/debian/changelog b/debian/changelog
index 74d70f2..2cfbacb 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -9,6 +9,12 @@ devscripts (2.14.8) UNRELEASED; urgency=medium
       with a web page containing a <meta refresh=...> redirect to the actual
       file, causing uscan to save the web page rather than the file.  (Closes:
       #764367)
+  * uupdate: When updating a 1.0 source format package, remove any symlinks in
+    the new upstream source before applying the Debian diff, restoring the
+    symlinks after.  This prevents patch from following the symlinks, which
+    may point to targets outside of the source tree, when applying the diff.
+    Thanks to Jakub Wilk for the discovery and suggested fix.
+    (Closes: #737160, CVE-2014-1833)
 
   [ Ron Lee ]
   * cowpoke: Add --sign and --upload command line overrides.

Message #28 received at 737160-close@bugs.debian.org (full text, mbox, reply):

From: James McCoy <jamessan@debian.org>

To: 737160-close@bugs.debian.org

Subject: Bug#737160: fixed in devscripts 2.14.8

Date: Sat, 11 Oct 2014 04:33:42 +0000

Source: devscripts
Source-Version: 2.14.8

We believe that the bug you reported is fixed in the latest version of
devscripts, which is due to be installed in the Debian FTP archive.

A summary of the changes between this version and the previous one is
attached.

Thank you for reporting the bug, which will now be closed.  If you
have further comments please address them to 737160@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.

Debian distribution maintenance software
pp.
James McCoy <jamessan@debian.org> (supplier of updated devscripts package)

(This message was generated automatically at their request; if you
believe that there is a problem with it please contact the archive
administrators by mailing ftpmaster@ftp-master.debian.org)


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Format: 1.8
Date: Sat, 11 Oct 2014 00:22:34 -0400
Source: devscripts
Binary: devscripts
Architecture: source amd64
Version: 2.14.8
Distribution: unstable
Urgency: medium
Maintainer: James McCoy <jamessan@debian.org>
Changed-By: James McCoy <jamessan@debian.org>
Description:
 devscripts - scripts to make the life of a Debian Package maintainer easier
Closes: 737160 764367
Changes:
 devscripts (2.14.8) unstable; urgency=medium
 .
   [ James McCoy ]
   * uscan:
     + Ensure $keyring is defined before trying to use it when checking whether
       the upstream keyring exists.
     + Strip the Referer header when using qa.debian.org's Sourceforge
       redirector.  When there's a foreign Referer header, Sourceforge responds
       with a web page containing a <meta refresh=...> redirect to the actual
       file, causing uscan to save the web page rather than the file.  (Closes:
       #764367)
   * uupdate: When updating a 1.0 source format package, remove any symlinks in
     the new upstream source before applying the Debian diff, restoring the
     symlinks after.  This prevents patch from following the symlinks, which
     may point to targets outside of the source tree, when applying the diff.
     Thanks to Jakub Wilk for the discovery and suggested fix.
     (Closes: #737160, CVE-2014-1833)
 .
   [ Ron Lee ]
   * cowpoke: Add --sign and --upload command line overrides.
Checksums-Sha1:
 a852ec4483abcb9d9a2d3d3d46ba323fb6d4562b 2236 devscripts_2.14.8.dsc
 d722277952c69996a2e7ab7e14ac324ecb25edb0 607240 devscripts_2.14.8.tar.xz
 b52e75dcaf05d316e4d3e509e223f900ea2ff6ce 898224 devscripts_2.14.8_amd64.deb
Checksums-Sha256:
 f234b68b85c98c3e7305800056833b238e7a936049dec598fa03821975d141ff 2236 devscripts_2.14.8.dsc
 4e188e60bfe9597d9a7e5162cb45ea88336914fb60d102ded31e90fde67dc407 607240 devscripts_2.14.8.tar.xz
 e454706b8d7fc12bdb6d5f190392c6e5f0e063198ec90ae14f9819ef480e78f0 898224 devscripts_2.14.8_amd64.deb
Files:
 b65ec1cb215442045b7adf5a33c5dd57 2236 devel optional devscripts_2.14.8.dsc
 446fa259995c261a2d329ed1270660f6 607240 devel optional devscripts_2.14.8.tar.xz
 c889ed63630fa0f66a360055dd2d353d 898224 devel optional devscripts_2.14.8_amd64.deb

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQJ8BAEBCgBmBQJUOLHKXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXQ5MUJGQkY0RDY5NTZCRDVERjdCNzJEMjNE
RkU2OTFBRTMzMUJBM0RCAAoJEN/mka4zG6PbNJYP/11T7d31T9na8sAWoS0C9zbs
kHPGtG85QI/N60uJ4ENcCTvHkNqNmd/z3VyA8bGePMTzEUXmu//y8sXn+utwfSqb
ZHcwLR9UGfQ+SRdNMKI5B1dz4aZm6iw/dpgRIK/qkhAcz1Tm9dRQV01bXhpj2rNQ
/YMpYF+NdDjrB6JMxsz7k3ZhNLFg4z6sTYRroSMYpZXrhlyxIcF0BxYkkCsA5cR7
5E5kt76wvOYnM7cSWgBO5L7subwBNqaVBS5Zr0IJqTyeU1xx1Xbk+ac80BSOASyI
X/pDroHAPCGnklHoSi0RFheBMWVwnZM3Qk/eVrsbS7m8y8IpefZ9YuTV+s+wJISO
dKUelWMhE8tDKOzqBH5KzqVrxkhExj4+/mk9G8eHTNOX1985XhftYUHwI+gmwDrN
VoQ3NU70FUdJ+lFPym52pPZo5A9qMzbQgamTLUxu3i8u7zvptWQSllMNh4ell8MA
ANlSD4bXMC8PFUZw7ki/kclqdBlTuamtdMyaiIQnl2F5haGbMnnaVr/lhaF2lwRs
vBG/L7rwOd67FgbyQEdvnJ6hhHY38zozb83KOwF+5PRal0TDOdIW+o0dnjTmFXTp
FflCqiCC5XHcJxAHrE+lVQ8Hc8Vk/J42U5l+Sq4iJ9QoRMEl3CW0KKLvmEcpg6BR
OzM2he0L286IQX6zCpsa
=8hR3
-----END PGP SIGNATURE-----

Send a report that this bug log contains spam.