* lp:~mvo/python-apt/recv-key-lp1016643:

  - Only support long (v4) keyids when downloading keys and
    check the keys fingerprint before importing. This avoids
    man-in-the-middle attacks (LP: #1016643)

3 files changed, 97 insertions, 7 deletions

diff --git a/apt/auth.py b/apt/auth.py
index e088ae85..742b5cc2 100644
--- a/apt/auth.py
+++ b/apt/auth.py
@@ -27,6 +27,7 @@
 import atexit
 import os
 import os.path
+import shutil
 import subprocess
 import sys
 import tempfile
@@ -35,6 +36,10 @@ import apt_pkg
 from apt_pkg import gettext as _
 
 
+class AptKeyError(Exception):
+    pass
+
+
 class TrustedKey(object):
 
     """Represents a trusted key."""
@@ -79,7 +84,7 @@ def _call_apt_key_script(*args, **kwargs):
         output, stderr = proc.communicate(content)
 
         if proc.returncode:
-            raise SystemError("The apt-key script failed with return code %s:\n"
+            raise AptKeyError("The apt-key script failed with return code %s:\n"
                               "%s\n"
                               "stdout: %s\n"
                               "stderr: %s" % (proc.returncode, " ".join(cmd),
@@ -99,20 +104,84 @@ def add_key_from_file(filename):
     filename -- the absolute path to the public GnuPG key file
     """
     if not os.path.abspath(filename):
-        raise SystemError("An absolute path is required: %s" % filename)
+        raise AptKeyError("An absolute path is required: %s" % filename)
     if not os.access(filename, os.R_OK):
-        raise SystemError("Key file cannot be accessed: %s" % filename)
+        raise AptKeyError("Key file cannot be accessed: %s" % filename)
     _call_apt_key_script("add", filename)
 
 def add_key_from_keyserver(keyid, keyserver):
     """Import a GnuPG key file to trust repositores signed by it.
 
     Keyword arguments:
-    keyid -- the identifier of the key, e.g. 0x0EB12DSA
+    keyid -- the long keyid (fingerprint) of the key, e.g.
+             A1BD8E9D78F7FE5C3E65D8AF8B48AD6246925553
     keyserver -- the URL or hostname of the key server
     """
-    _call_apt_key_script("adv", "--quiet", "--keyserver", keyserver,
-                         "--recv", keyid)
+    tmp_keyring_dir = tempfile.mkdtemp()
+    try:
+        _add_key_from_keyserver(keyid, keyserver, tmp_keyring_dir)
+    except:
+        raise
+    finally:
+        shutil.rmtree(tmp_keyring_dir)
+
+def _add_key_from_keyserver(keyid, keyserver, tmp_keyring_dir):
+    if len(keyid) < 160/8:
+        raise AptKeyError("Only long keyids (v4, 160bit) are supported")
+    # create a temp keyring dir
+    tmp_secret_keyring = os.path.join(tmp_keyring_dir, "secring.gpg")
+    tmp_keyring = os.path.join(tmp_keyring_dir, "pubring.gpg")
+    # default options for gpg
+    gpg_default_options = [
+        "gpg",
+        "--no-default-keyring", "--no-options",
+        "--homedir", tmp_keyring_dir,
+        ]
+    # download the key to a temp keyring first
+    res = subprocess.call(gpg_default_options + [
+        "--secret-keyring", tmp_secret_keyring,
+        "--keyring", tmp_keyring,
+        "--keyserver", keyserver,
+        "--recv", keyid,
+        ])
+    if res != 0:
+        raise AptKeyError("recv from '%s' failed for '%s'" % (
+            keyserver, keyid))
+    # now export again using the long key id (to ensure that there is
+    # really only this one key in our keyring) and not someone MITM us
+    tmp_export_keyring = os.path.join(tmp_keyring_dir, "export-keyring.gpg")
+    res = subprocess.call(gpg_default_options + [
+        "--keyring", tmp_keyring,
+        "--output", tmp_export_keyring,
+        "--export", keyid,
+        ])
+    if res != 0:
+        raise AptKeyError("export of '%s' failed", keyid)
+    # now verify the fingerprint, this is probably redundant as we
+    # exported by the fingerprint in the previous command but its
+    # still good paranoia
+    output = subprocess.Popen(
+        gpg_default_options + [
+            "--keyring", tmp_export_keyring,
+            "--fingerprint",
+            "--batch",
+            "--with-colons",
+            ],
+            stdout=subprocess.PIPE,
+            universal_newlines=True).communicate()[0]
+    got_fingerprint=None
+    for line in output.splitlines():
+        if line.startswith("fpr:"):
+            got_fingerprint = line.split(":")[9]
+            # stop after the first to ensure no subkey trickery
+            break
+    signing_key_fingerprint = keyid
+    if got_fingerprint != signing_key_fingerprint:
+        raise AptKeyError(
+            "Fingerprints do not match, not importing: '%s' != '%s'" % (
+                signing_key_fingerprint, got_fingerprint))
+    # finally add it
+    add_key_from_file(tmp_export_keyring)
 
 def add_key(content):
     """Import a GnuPG key to trust repositores signed by it.
diff --git a/debian/changelog b/debian/changelog
index b0ca3efd..66da3ea0 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -9,6 +9,10 @@ python-apt (0.8.8) UNRELEASED; urgency=low
     Thanks to "sampo555", LP: #1042916
   * lp:~mvo/python-apt/fix-debfile-crash:
     - fix crash on missing candidates in the multiarch check
+  * lp:~mvo/python-apt/recv-key-lp1016643:
+    - Only support long (v4) keyids when downloading keys and
+      check the keys fingerprint before importing. This avoids
+      man-in-the-middle attacks (LP: #1016643)
 
   [ James Hunt ]
   * python/cache.cc: PkgCacheGetIsMultiArch(): Return calculated
diff --git a/tests/test_auth.py b/tests/test_auth.py
index 99c40db5..4e37b3d3 100644
--- a/tests/test_auth.py
+++ b/tests/test_auth.py
@@ -185,12 +185,29 @@ class TestAuthKeys(unittest.TestCase):
         self.assertEqual(key.keyid, "46925553")
         self.assertEqual(key.date, "2012-04-27")
 
+    def test_add_key_from_keyserver_too_short(self):
+        """Ensure that short keyids are not imported"""
+        with self.assertRaises(apt.auth.AptKeyError):
+            apt.auth.add_key_from_keyserver("46925553", "hkp://localhost:19191")
+
+    def test_add_key_from_server_mitm(self):
+        """Verify that the key fingerprint is verified after download"""
+        self._start_keyserver()
+        self.addCleanup(self._stop_keyserver)
+        with self.assertRaises(apt.auth.AptKeyError) as cm:
+            apt.auth.add_key_from_keyserver(
+                "0101010178F7FE5C3E65D8AF8B48AD6246925553",
+                "hkp://localhost:19191")
+        self.assertTrue(
+            str(cm.exception).startswith("Fingerprints do not match"))
+
     def testAddKeyFromServer(self):
         """Install a GnuPG key from a remote server."""
         self._start_keyserver()
         self.addCleanup(self._stop_keyserver)
 
-        apt.auth.add_key_from_keyserver("46925553", "hkp://localhost:19191")
+        apt.auth.add_key_from_keyserver(
+            "A1BD8E9D78F7FE5C3E65D8AF8B48AD6246925553", "hkp://localhost:19191")
 
         ret = apt.auth.list_keys()
         self.assertEqual(len(ret), 1)