bug#19479: Package manager vulnerable

bug#19479: Package manager vulnerable

[Kelly Dean]

Ivan Shmakov requested that I send this message to the bug list.

For details, see my message with subject ⌜Emacs package manager vulnerable to replay attacks⌝ to emacs-devel on 30 Dec 2014:
https://lists.gnu.org/archive/html/emacs-devel/2014-12/msg02319.html

Executive summary to fix the vulnerabilities:

0. Include a hash and length of each package's content in the package's record in archive-contents, rather than only including the package name and version number in that file as Emacs currently does. Barf if a package hash doesn't verify, regardless of whether any signatures verify.
(Length technically not necessary, but still generally useful, e.g. if there's a length mismatch then you know there's a content mismatch and you don't have to bother checking the hash.)

Stop distributing elpa-key signatures of packages, since they're superfluous if you have package hashes in archive-contents and have elpa-key signatures of archive-contents, and you already have the latter.

1. Include a timestamp of archive-contents in that file itself (so that the signature in archive-contents.sig depends on the timestamp, so that the timestamp can't be forged), and have Emacs ignore any new archive-contents that's older than the latest valid one that Emacs has already seen or is older than some specified limit. One thing I forgot to mention in my original message: have Emacs signal a warning if it ever sees an archive-contents dated in the future, which indicates misconfiguration of the client or server (or of course, some kind of mischief).

Optional alternative timestamp handling, as Ivan pointed out that Debian does (at least sometimes): Instead of expiring archive-contents after some limit configured in Emacs, put an explicit expiration date in it. Personally, I don't like server-supplied expiration dates, kind of for a similar reason that RMS doesn't like server-supplied Javascript, or maybe just because I have too many irritating memories of expired SSL certs.

Ivan suggested maybe filing those as separate bug reports, but it's pointless to fix either of them unless both are fixed, so it makes more sense to include them together.

One more feature: include in each version of archive-contents a hash (and length) of the previous version of that file. This isn't necessary for preventing any of the vulnerabilities above, but it's easy insurance that slightly mitigates the disaster if the metadata signing key is compromised. It's pointless unless both the above problems are fixed, so it makes sense to put it here.

BTW, check whether Emacs is vulnerable to endless-data attack. (I haven't.) If it is, then the length field mentioned above (which is a good idea in any case) will assist in early detection of this attack. This belongs here because... well no it doesn't, but I don't want to file a separate bug report for it because the report would be bogus if it turns out Emacs isn't vulnerable, and I've already filled my bogusness quota for the week.

bug#19479: Package manager vulnerable

[Stefan Monnier]

> For details, see my message with subject ⌜Emacs package manager vulnerable
> to replay attacks⌝ to emacs-devel on 30 Dec 2014:
> https://lists.gnu.org/archive/html/emacs-devel/2014-12/msg02319.html

AFAICT, this vulnerability also applies to the way GNU packages are
distributed in ftp.gnu.org (i.e. as a tarball plus a .sig file).

Is that right?

> Executive summary to fix the vulnerabilities:

Another way to attack the problem is to include the file name along with
its content in "the thing that gets signed".
I.e. the signature shouldn't apply to the output of "cat <foo>" but to
the output of "echo <foo>; cat <foo>".

This way an attacker can't take <vulnerable>.tar along with
<vulnerable>.tar.sig and send them off as <safe>.tar along with
<safe>.tar.sig.


        Stefan

bug#19479: Package manager vulnerable

[Kelly Dean]

Stefan Monnier wrote:
> AFAICT, this vulnerability also applies to the way GNU packages are
> distributed in ftp.gnu.org (i.e. as a tarball plus a .sig file).
>
> Is that right?

Yes, because there are no hashes in, or signatures on, http://ftp.gnu.org/find.txt.gz or http://ftp.gnu.org/ls-lrRt.txt.gz

They used to do it right; see
http://ftp.gnu.org/before-2003-08-01.md5sums.asc

But it looks like they stopped.

Having to redo a huge monolithic metadata file whenever any data file changes is inefficient; it's more efficient for the metadata for each directory to just have the hash of each file in the directory and the hash of the metadata of each subdirectory, like Git does. But either way will prevent package replay attacks.

>> Executive summary to fix the vulnerabilities:
>
> Another way to attack the problem is to include the file name along with
> its content in "the thing that gets signed".
> I.e. the signature shouldn't apply to the output of "cat <foo>" but to
> the output of "echo <foo>; cat <foo>".
>
> This way an attacker can't take <vulnerable>.tar along with
> <vulnerable>.tar.sig and send them off as <safe>.tar along with
> <safe>.tar.sig.

If filenames include version numbers and the version numbers are never reused, then your solution does prevent package replay attacks. Since Emacs packages already include a Version header (and the package name), you could actually do your proposed verification using that header, without changing the way signatures are currently made, which is a solution I addressed in my original emacs-devel message.

But having a list of hashes of all the packages (and even better, chaining together all the versions of that list) makes changes to any package more conspicuous, which makes the attacker's job harder, as I explained. And if you do that, then the elpa key no longer needs to sign individual packages at all. Git, Fossil, and Debian's apt-get use hash lists, and Git and Fossil also chain together the lists, so there's good precedence. Both are simple to do for Emacs: in the archive-contents file, include the hash of each package and the hash of the previous version of archive-contents.

But remember, none of the above prevents metadata replay attacks. If the user himself is specifying the metadata (e.g. you manually request Emacs 24.4 because you know that's the latest version), then verification to prevent metadata replay attacks isn't the computer's job. But when the user just says to update some package(s) to the latest version, without specifying the version, then it is the computer's job. For this, put a timestamp of the archive-contents file into the file itself.

bug#19479: Package manager vulnerable

[Stefan Monnier]

> If filenames include version numbers and the version numbers are never
> reused,

The ELPA system in general does not enforce that.  But the GNU ELPA
scripts do, and other ELPA servers work in a way that should generally
make sure this is also the case.

> then your solution does prevent package replay attacks. Since Emacs
> packages already include a Version header (and the package name), you could
> actually do your proposed verification using that header, without changing
> the way signatures are currently made, which is a solution I addressed in my
> original emacs-devel message.

Indeed, I realized this just after I sent my message.
So we can fix this problem simply by changing package.el so as to check
that the name&version of the downloaded file match the name&version
contained therein.
Patch welcome.

> But remember, none of the above prevents metadata replay attacks. If the
> user himself is specifying the metadata (e.g. you manually request Emacs
> 24.4 because you know that's the latest version), then verification to
> prevent metadata replay attacks isn't the computer's job. But when the user
> just says to update some package(s) to the latest version, without
> specifying the version, then it is the computer's job. For this,
> put a timestamp of the archive-contents file into the file itself.

Agreed.  It should be fairly easy to add a timestamp in there without
causing any backward incompatibility.


        Stefan

bug#19479: Package manager vulnerable

[Kelly Dean]

Richard Stallman wrote:
> What do we need to do on ftp.gnu.org to avoid these dangers?

It depends on what you expect the user's responsibility to be.

If you expect him to know the latest version number of a package (without relying on the gnu.org webserver to find out, in case it's compromised), and you expect him to manually verify that his download is the latest version (in addition to verifying the signature, of course), and you give him the ability to do this by always including both the name and the version number in your packages (so far as I'm aware, you already do) and never re-using version numbers (I think you're ok here too), then you have no problem, so there's nothing you need to do.

Otherwise, the problems and solution are the same as for package distribution systems in general, as detailed at
https://www.cs.arizona.edu/stork/packagemanagersecurity/attacks-on-package-managers.html
https://www.cs.arizona.edu/stork/packagemanagersecurity/otherattacks.html

bug#19479: Package manager vulnerable

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > If you expect him to know the latest version number of a package
  > (without relying on the gnu.org webserver to find out, in case
  > it's compromised),

It is normal for users to find the latest version based on gnu.org.
So we don't expect that.

   > and you expect him to manually verify that his download is the
   > latest version (in addition to verifying the signature, of
   > course),

The file name has the version in it.

So it seems we have a problem to fix.  Would you like to help
us fix it?

-- 
Dr Richard Stallman
President, Free Software Foundation
51 Franklin St
Boston MA 02110
USA
www.fsf.org  www.gnu.org
Skype: No way! That's nonfree (freedom-denying) software.
  Use Ekiga or an ordinary phone call.

bug#19479: Package manager vulnerable

[Kelly Dean]

Glenn Morris wrote:
> I appreciate the spirit of wanting to provide a patch, but unless you
> have changed your position on the Emacs copyright assignment, I don't
> see that this patch can be used by Emacs.

I did do what you requested: submit a bug report, but not a patch. But this isn't just a bug; it's a security vulnerability, and Stefan invited me to submit a patch to fix it. So then I did.

Regarding the copyright issue, please don't conflate two separate issues like your copyright clerk tried to.

The first issue is: does the FSF want any more public domain code in Emacs than is already there? The answer is ‟no”, as explained by Donald R Robertson III, your copyright clerk, on February 19, 2013. When explaining why the FSF wouldn't accept my PD code, he wrote, ‟It really is more beneficial for our enforcement efforts if we get the work assigned instead of 'disclaimed'. We will only accept a disclaimer instead of an assignment in particular circumstances.”

Of course, he's right; PD code isn't useful for your enforcement efforts, but it's absurd to say it's an issue for my patches, which even including this latest one, amount to no more than a few parts per million of the Emacs code base. Obviously it doesn't hurt your efforts; no copyright judge is going to care if Emacs has a few lines of Hamlet or any other PD information in it. The judge will let you sue people for GPL violations just the same.

Anyway, the first issue is clear: new PD code is unwelcome in Emacs. Emacs is your project, not mine, so regardless of how silly I think your exclusion of PD code is, I abided (and still abide) by your wishes. I submitted this patch because Stefan invited me to. Maybe Stefan just forgot that you asked me not to submit any more patches, but I assumed he invited this patch because a security vulnerability counted as a ‟particular circumstance” that your copyright clerk mentioned.

The second issue is: is my code in the public domain? The answer is ‟yes”; the author of SQLite says that's PD, and it is, the author of Qmail says that's PD, and it is, and I'm simply doing the same thing they are. My code is in the public domain. If you want, I can PGP-sign and publish on my website a statement that my patches are PD, even though that's more than the authors of SQLite and Qmail deemed necessary for their code.

Your clerk wrote, ‟placing a work in the public domain is difficult/may not be possible”. But that's obviously false, as proven by his statement that you do (sometimes) accept disclaimers, and as proven by the general legal acceptance of other people's statements that their work is PD, including highly respected authors such as Richard Hipp.

It's clear that the second issue is not an issue, especially in the United States, which is where I am, and the only purpose served by the FSF bringing it up is clouding the first issue, which is the only real issue.

I recommend not rejecting a patch to fix a security vulnerability just for the sake of keeping 29 lines of new PD code out of Emacs. If it really is too much PD code, then I recommend deleting feedmail.el (PD) to compensate.

bug#19479: Package manager vulnerable

[Kelly Dean]

BTW, Stefan mentioned (see bug #19536) that you don't use package-x for elpa.gnu.org, and instead use some other scripts, so it just occurred to me that you might not immediately notice that my patch not only verifies hashes, but also generates them, so there's nothing extra you need to do.

Just use package-upload-file from package-x.el, and it will automatically add the appropriate entry (including hash) for the package to the archive-contents file.

Apply the fix for bug #19536 if you want package-upload-file to correctly add tar files to the archive's package directory. (It already correctly adds single-file packages.)

GNU elpa, Melpa, and Marmalade can start using the new archive-contents now. Old clients will still work fine, and simply ignore the hashes. New clients will verify them.

bug#19479: Package manager vulnerable

[Stefan Monnier]

> of PD code is, I abided (and still abide) by your wishes. I submitted this
> patch because Stefan invited me to. Maybe Stefan just forgot that you asked
> me not to submit any more patches,

Indeed, that's the case.  You're one of the very rare oddballs who can't
be bothered to sign a trivial document to get this out of the way, but
for the life of me, I can't remember the names of the handful of
oddballs, so I keep repeating this error.

> but I assumed he invited this patch because a security vulnerability
> counted as a ‟particular circumstance” that your copyright
> clerk mentioned.

Emacs is full of vulnerabilities and has barely started using encryption
technology to try and eliminate some of them, so no, it's definitely not
"special" in this sense.  And in any case the "special"ness usually
doesn't refer to the usefulness of the code but rather to the fact that
it'd be difficult to get this code some other way (i.e. it's both
important/useful code and it'd take a while to rewrite it).


        Stefan

bug#19479: Package manager vulnerable

[Kelly Dean]

Stefan Monnier wrote:
> You're one of the very rare oddballs who can't
> be bothered to sign a trivial document to get this out of the way

That's not true. I offered to sign a document saying my work is PD.

The following say that's an option:
http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/disclaim.manual
http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/disclaim.changes.manual
http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/disclaim.program

The copyright clerk declined my offer.

bug#19479: Package manager vulnerable

[Stefan Monnier]

>> You're one of the very rare oddballs who can't
>> be bothered to sign a trivial document to get this out of the way
> That's not true. I offered to sign a document saying my work is PD.

I didn't mean "a trivial document" in the sense "any trivial document",
but rather "the particular trivial document that everybody else signed".


        Stefan

bug#19479: Package manager vulnerable

[Glenn Morris]

[Dropping emacs-devel, since that seems unlikely to be productive given
the lack of context.]

Eli Zaretskii wrote:

> Two and a half years later, with no one complaining about this, it
> doesn't sound right for this issue to block the release of Emacs 26.1.

The context here was security vulnerabilities in the package manager.
Personally I'm uneasy with saying "we've ignored this for X years so
let's continue to ignore it.". But I don't have anything substantive to
add.

bug#19479: Package manager vulnerable

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 4472 bytes --]

Kelly Dean <kelly@prtime.org> writes:

> Ivan Shmakov requested that I send this message to the bug list.
>
> For details, see my message with subject ⌜Emacs package manager vulnerable to replay attacks⌝ to emacs-devel on 30 Dec 2014:
> https://lists.gnu.org/archive/html/emacs-devel/2014-12/msg02319.html
>
> Executive summary to fix the vulnerabilities:
>
> 0. Include a hash and length of each package's content in the package's record
> in archive-contents, rather than only including the package name and version
> number in that file as Emacs currently does. Barf if a package hash doesn't
> verify, regardless of whether any signatures verify.
> (Length technically not necessary, but still generally useful, e.g. if
> there's a length mismatch then you know there's a content mismatch and
> you don't have to bother checking the hash.)

I have implemented the first part of the protection against metadata
replay attacks in the attached patch: support for checksum (or hash)
verification.  This change is backwards-compatible; the new fields can
be added to "archive-contents" file without impacting old clients.
I've not yet updated documentation, NEWS, etc. but will get to that
next.

I introduce a new user option `package-verify-checksums' that controls
this new behaviour.  The default is 'allow-missing', which only
carries out this check if there are checksums in "archive-contents",
and does nothing otherwise.  In itself, this does nothing to protect
against metadata replay attacks (but might protect against data
corruption).  You need to set `package-verify-checksums' to t, and
implement timestamping (discussed below).

I still suggest to stick with this default for Emacs 27.1, or at least
until common package archives can catch up.  Once this is implemented
in GNU ELPA and MELPA, it makes more sense to move to a stricter
default.  Otherwise, the transition will be very bumpy.  I therefore
suggest to discuss stricter defaults later.

(BTW, I didn't bother fixing the package-x.el code for this patch,
since it seems like it's not that widely used.  It will work as
before, but lack support for adding the checksums automatically.)

> 1. Include a timestamp of archive-contents in that file itself (so that the
> signature in archive-contents.sig depends on the timestamp, so that the
> timestamp can't be forged), and have Emacs ignore any new archive-contents
> that's older than the latest valid one that Emacs has already seen or is older
> than some specified limit. One thing I forgot to mention in my original message:
> have Emacs signal a warning if it ever sees an archive-contents dated in the
> future, which indicates misconfiguration of the client or server (or of course,
> some kind of mischief).

To protect against metadata replay attacks, it is correct that we need
timestamps too.  I haven't done that in this first patch, but I hope
to do it in a following patch.  I wanted to get this first part done
before I started working on that.

My current best idea for how to do it is one which AFAICT haven't been
raised in this thread before: to add a comment with an RFC3339
timestamp to the top of the "archive-contents" file:

    ;; Last-Updated: 2019-10-01T15:32:55.000Z

This will be ignored by older versions of Emacs, since package.el uses
(read (current-buffer)) to read this file.  New versions will have
an easy time parsing this header, caching the value, and refusing to
update the package cache if the timestamp is older than one we have
already seen.  With that, we would have implemented protection
against metadata replay attacks.

I think it would be highly beneficial if this could go in before Emacs
27, not least so that package archives can start implementing support
for this.

Comments on all this are obviously more than welcome.

Best regards,
Stefan Kangas

PS. Note that the original thread ended up highly off-topic discussing
copyright issues, because one potential contributor refused to sign
the standard copyright assignment.  The eventual outcome was that we
could not use a patch written by that person.  I have therefore
deliberately not looked at that persons patch in order to avoid any
copyright issues.  I have implemented this from scratch based solely
on the below link, and the discussion in this thread:
https://www2.cs.arizona.edu/stork/packagemanagersecurity/attacks-on-package-managers.html

[-- Attachment #2: 0001-Support-package-checksum-verification.patch --]
[-- Type: text/x-patch, Size: 29457 bytes --]

From 543029f4d3dcc4e0401263c93e01fe9319395708 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Fri, 4 Oct 2019 10:36:14 +0200
Subject: [PATCH] Support package checksum verification

This is the first step towards protecting users of package.el against
metadata replay attacks.

* lisp/emacs-lisp/package.el (package-verify-checksums): New
defcustom.
(package-desc, package--ac-desc)
(package--add-to-archive-contents, package-install-from-archive): New
fields 'size' and 'checksums'.
(package-desc-filename): New function.
(package-process-define-package): Doc fix.

(bad-checksum): New error type.
(package--verify-package-checksum)
(package--verify-package-size): New function to verify that the
checksum and size of a package corresponds to the checksum and size
data in the "archive-contents" file on the package archive.
(package--show-verify-checksum-error): New function to show
details of an error on checksum verification.  (Bug#19479)

* lisp/emacs-lisp/package-x.el (package-upload-buffer-internal):
Update to use above new fields 'size' and 'checksums'.

* test/lisp/emacs-lisp/package-tests.el (package-test-refresh-contents)
(package-test-install-single-from-archive): Update tests.
(with-install-using-checksum): New macro.
(package-test-install-with-checksum/single-valid)
(package-test-install-with-checksum/single-invalid)
(package-test-install-with-checksum/tar-valid)
(package-test-install-with-checksum/tar-invalid): New tests for
installing packages with checksums.
(package-test-verification-text)
(package-tests-valid-md5-checksum)
(package-tests-valid-sha256-checksum)
(package-tests-valid-sha512-checksum): New variables.
(run-verify-checksums-test): New macro.
(package-test--verify-package-checksums-nil/ignore-invalid)
(package-test--verify-package-checksums-allow-missing)
(package-test--verify-package-checksums-allow-missing/missing)
(package-test--verify-package-checksums-allow-missing/ignore-unsupported)
(package-test--verify-package-checksums-t)
(package-test--verify-package-checksums-t/invalid-fails)
(package-test--verify-package-checksums-t/missing-fails)
(package-test--verify-package-checksums-all)
(package-test--verify-package-checksums-all/invalid-fails)
(package-test--verify-package-checksums-all/missing-fails)
(package-test--verify-package-checksums-all/no-supported-hash-fails)
(package-test--verify-package-checksums-all/ignore-unsupported)
(package-test--verify-package-size): New tests for the checksum
support.

* test/lisp/emacs-lisp/package-resources/archive-contents:
* test/lisp/emacs-lisp/package-resources/checksum-invalid-1.0.el:
* test/lisp/emacs-lisp/package-resources/checksum-valid-123.el:
* test/lisp/emacs-lisp/package-resources/checksum-valid-tar-0.99.tar:
* test/lisp/emacs-lisp/package-resources/checksum-valid-tar-0.99.tar:
New test data files.
---
 lisp/emacs-lisp/package-x.el                  |   4 +-
 lisp/emacs-lisp/package.el                    | 159 ++++++++++++++--
 .../package-resources/archive-contents        |  26 ++-
 .../package-resources/checksum-invalid-1.0.el |  17 ++
 .../checksum-invalid-tar-0.1.tar              | Bin 0 -> 10240 bytes
 .../package-resources/checksum-valid-123.el   |  17 ++
 .../checksum-valid-tar-0.99.tar               | Bin 0 -> 10240 bytes
 test/lisp/emacs-lisp/package-tests.el         | 177 +++++++++++++++++-
 8 files changed, 378 insertions(+), 22 deletions(-)
 create mode 100644 test/lisp/emacs-lisp/package-resources/checksum-invalid-1.0.el
 create mode 100644 test/lisp/emacs-lisp/package-resources/checksum-invalid-tar-0.1.tar
 create mode 100644 test/lisp/emacs-lisp/package-resources/checksum-valid-123.el
 create mode 100644 test/lisp/emacs-lisp/package-resources/checksum-valid-tar-0.99.tar

diff --git a/lisp/emacs-lisp/package-x.el b/lisp/emacs-lisp/package-x.el
index 2815be3fe6..56373c14c4 100644
--- a/lisp/emacs-lisp/package-x.el
+++ b/lisp/emacs-lisp/package-x.el
@@ -219,7 +219,9 @@ package-upload-buffer-internal
 	  (let ((contents (or (package--archive-contents-from-url archive-url)
 			      (package--archive-contents-from-file)))
 		(new-desc (package-make-ac-desc
-                           split-version requires desc file-type extras)))
+                           split-version requires desc file-type extras
+                           ;; FIXME: Use better values than nil nil.
+                           nil nil)))
 	    (if (> (car contents) package-archive-version)
 		(error "Unrecognized archive version %d" (car contents)))
 	    (let ((elt (assq pkg-name (cdr contents))))
diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 188f398a56..6795e17ac3 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -335,6 +335,30 @@ package-gnupghome-dir
   :risky t
   :version "26.1")
 
+(defcustom package-verify-checksums 'allow-missing
+  "Non-nil means to verify the checksum of a package before installing it.
+
+This can be one of:
+- nil: Ignore checksums.
+- `allow-missing': Same as t if a checksum exists, but install a
+  package even if there is no checksum.
+- t: Require a valid checksum; refuse to install package if the
+  checksum is missing or invalid.  Verifies one checksum.
+- `all': Same as t, but verify all available (and supported)
+  checksums.
+
+The package checksums are automatically fetched from package
+archives with the package data on `package-refresh-contents'.
+
+Note that setting this to nil is intended for debugging, and
+should normally not be used since it will decrease security."
+  :type '(choice (const nil :tag "Never")
+                 (const allow-missing :tag "Allow missing")
+                 (const t :tag "Require valid checksum")
+                 (const t :tag "Require valid checksum, and check all"))
+  :risky t
+  :version "27.1")
+
 (defcustom package-check-signature 'allow-unsigned
   "Non-nil means to check package signatures when installing.
 More specifically the value can be:
@@ -429,6 +453,8 @@ package--default-summary
                                  requirements)))
                  (kind (plist-get rest-plist :kind))
                  (archive (plist-get rest-plist :archive))
+                 (checksums (plist-get rest-plist :checksums))
+                 (size (plist-get rest-plist :size))
                  (extras (let (alist)
                            (while rest-plist
                              (unless (memq (car rest-plist) '(:kind :archive))
@@ -466,6 +492,13 @@ package--default-summary
 
 `extras' Optional alist of additional keyword-value pairs.
 
+`size'  Size of the package in bytes.
+
+`checksums' Checksums for the package file.  Alist of ((ALGORITHM
+        . CHECKSUM)) where ALGORITHM is a symbol specifying a
+        `secure-hash' algorithm, and CHECKSUM is a string
+        containing the checksum.
+
 `signed' Flag to indicate that the package is signed by provider."
   name
   version
@@ -475,7 +508,9 @@ package--default-summary
   archive
   dir
   extras
-  signed)
+  signed
+  size
+  checksums)
 
 (defun package--from-builtin (bi-desc)
   "Create a `package-desc' object from BI-DESC.
@@ -538,6 +573,13 @@ package-desc-suffix
     ('dir "")
     (kind (error "Unknown package kind: %s" kind))))
 
+(defun package-desc-filename (pkg-desc)
+  "Return file-name of package-desc object PKG-DESC.
+This is the concatenation of `package-desc-full-name' and
+`package-desc-suffix'."
+  (concat (package-desc-full-name pkg-desc)
+          (package-desc-suffix pkg-desc)))
+
 (defun package-desc--keywords (pkg-desc)
   "Return keywords of package-desc object PKG-DESC.
 These keywords come from the foo-pkg.el file, and in general
@@ -603,11 +645,11 @@ package-activated-list
 (defun package-process-define-package (exp)
   "Process define-package expression EXP and push it to `package-alist'.
 EXP should be a form read from a foo-pkg.el file.
-Convert EXP into a `package-desc' object using the
-`package-desc-from-define' constructor before pushing it to
-`package-alist'.
-If there already exists a package by that name in
-`package-alist', replace that definition with the new one."
+
+Convert EXP into a `package-desc' object, then push it to
+`package-alist'.  If there already exists a package by the same
+name in `package-alist', add the object to the list of packages,
+and sort the entries by version."
   (when (eq (car-safe exp) 'define-package)
     (let* ((new-pkg-desc (apply #'package-desc-from-define (cdr exp)))
            (name (package-desc-name new-pkg-desc))
@@ -1310,6 +1352,81 @@ package--with-response-buffer-1
                    url))
           (insert-file-contents-literally url)))))
 
+(define-error 'bad-checksum "Failed to verify checksum")
+
+(defun package--show-verify-checksum-error (pkg-desc details)
+  "Show error on failed checksum verification of PKG-DESC with DETAILS.
+Error is displayed in a new buffer named \"*Error*\"."
+  (with-output-to-temp-buffer "*Error*"
+    (with-current-buffer standard-output
+      (insert (format "Failed to verify checksum of package `%s':\n\n"
+                      (package-desc-name pkg-desc)))
+      (insert details))))
+
+(defun package--verify-package-checksum (pkg-desc)
+  "Verify checksums of `package-desc' object PKG-DESC.
+This assumes that the we are in a buffer containing package.
+
+The value of `package-verify-checksums' decides what this
+function does:
+- nil: Do nothing.
+- 'allow-missing: Verify checksum if it exists, otherwise do
+  nothing.
+- t: Verify that there is at least one valid checksum.
+- 'all': Like t, but check all (supported) checksums in turn.
+
+Signal an error of type `bad-checksum' if the verification."
+  (cl-flet*
+      ((supported-hashes
+        (lambda ()
+          (or (seq-filter (lambda (h) (memql (car h) (secure-hash-algorithms)))
+                          (package-desc-checksums pkg-desc))
+              ;; Failed; signal error.
+              (package--show-verify-checksum-error
+               pkg-desc
+               (concat
+                (if (package-desc-checksums pkg-desc)
+                    (concat
+                     "No supported checksums found\n\n"
+                     (format-message "Package archive had: %s\n"
+                                     (package-desc-checksums pkg-desc))
+                     (format-message "Emacs supports: %s\n"
+                                     (secure-hash-algorithms)))
+                  "Package archive had no checksums for this package\n")))
+              (signal 'bad-checksum "no supported checksums found"))))
+       (do-check
+        (lambda (&optional all)
+          (dolist (hash (seq-take (supported-hashes)
+                                  (if all most-positive-fixnum 1)))
+            (let* ((algorithm (car hash))
+                   (a (cdr hash))
+                   (b (secure-hash algorithm (current-buffer))))
+              (if (equal a b) t
+                ;; Failed; signal error.
+                (package--show-verify-checksum-error
+                 pkg-desc
+                 (concat
+                  (format-message "\nChecksum mismatch (%s)\n\n" algorithm)
+                  (format-message "Expected: %s\n" a)
+                  (format-message "Result: %s\n" b)))
+                (signal 'bad-checksum (list "checksum mismatch" a b))))))))
+    (pcase package-verify-checksums
+      ('nil nil)
+      ('allow-missing (when (package-desc-checksums pkg-desc) (do-check)))
+      ('t (do-check))
+      ('all (do-check 'all))
+      (_ (user-error "Value of `package-verify-checksums' is invalid: `%s'"
+                     package-verify-checksums)))))
+
+(defun package--verify-package-size (pkg-desc)
+  "Verify package size of `package-desc' object PKG-DESC.
+This assumes that the we are in a buffer containing package."
+  (when-let ((a (package-desc-size pkg-desc))
+             (b (string-bytes (buffer-string))))
+    (unless (equal a b)
+      (error "Mismatch in size in package `%s'.  Size was %s, but expected %s."
+             (package-desc-name pkg-desc) b a))))
+
 (define-error 'bad-signature "Failed to verify signature")
 
 (defun package--check-signature-content (content string &optional sig-file)
@@ -1437,14 +1554,19 @@ package--add-to-compatibility-table
                 (version-list-< table-version version))
         (puthash name version package--compatibility-table)))))
 
-;; Package descriptor objects used inside the "archive-contents" file.
-;; Changing this defstruct implies changing the format of the
-;; "archive-contents" files.
 (cl-defstruct (package--ac-desc
-               (:constructor package-make-ac-desc (version reqs summary kind extras))
+               (:constructor
+                package-make-ac-desc (version reqs summary kind extras size checksums))
                (:copier nil)
                (:type vector))
-  version reqs summary kind extras)
+  "Package descriptor object used inside the \"archive-contents\" file.
+Changing this defstruct implies changing the format of the
+\"archive-contents\" files.
+
+This is mainly used in `package--add-to-archive-contents' to make
+the code that parses the \"archive-contents\" file more
+readable."
+  version reqs summary kind extras size checksums)
 
 (defun package--append-to-alist (pkg-desc alist)
   "Append an entry for PKG-DESC to the start of ALIST and return it.
@@ -1482,10 +1604,14 @@ package--add-to-archive-contents
            :summary (package--ac-desc-summary (cdr package))
            :kind (package--ac-desc-kind (cdr package))
            :archive archive
+           ;; Older "archive-contents" files might not have the
+           ;; below elements.
            :extras (and (> (length (cdr package)) 4)
-                        ;; Older archive-contents files have only 4
-                        ;; elements here.
-                        (package--ac-desc-extras (cdr package)))))
+                        (package--ac-desc-extras (cdr package)))
+           :size (and (> (length (cdr package)) 5)
+                      (package--ac-desc-size (cdr package)))
+           :checksums (and (> (length (cdr package)) 6)
+                           (package--ac-desc-checksums (cdr package)))))
          (pinned-to-archive (assoc name package-pinned-packages)))
     ;; Skip entirely if pinned to another archive.
     (when (not (and pinned-to-archive
@@ -1956,9 +2082,10 @@ package-install-from-archive
   (when (eq (package-desc-kind pkg-desc) 'dir)
     (error "Can't install directory package from archive"))
   (let* ((location (package-archive-base pkg-desc))
-         (file (concat (package-desc-full-name pkg-desc)
-                       (package-desc-suffix pkg-desc))))
+         (file (package-desc-filename pkg-desc)))
     (package--with-response-buffer location :file file
+      (package--verify-package-size pkg-desc)
+      (package--verify-package-checksum pkg-desc)
       (if (or (not (package-check-signature))
               (member (package-desc-archive pkg-desc)
                       package-unsigned-archives))
diff --git a/test/lisp/emacs-lisp/package-resources/archive-contents b/test/lisp/emacs-lisp/package-resources/archive-contents
index e2f92304f8..b41287ae89 100644
--- a/test/lisp/emacs-lisp/package-resources/archive-contents
+++ b/test/lisp/emacs-lisp/package-resources/archive-contents
@@ -14,4 +14,28 @@
  (multi-file .
              [(0 2 3)
               nil "Example of a multi-file tar package" tar
-              ((:url . "http://puddles.li"))]))
+              ((:url . "http://puddles.li"))])
+ (checksum-valid  .
+                  [(123)
+                    nil "A single-file package with a valid checksum." single
+                    nil
+                    343
+                    ((sha512 . "6270d64d63c90ef541c3386c40acb7716865499c506465f2ff9b408f05c6612fb0c58f5d83b60af9d7f8d972796ee270d0bc6ca8a17bd0412cc249dede6f7359"))])
+ (checksum-valid-tar .
+                     [(0 99)
+                     nil "A multi-file package with a valid checksum." tar
+                     nil
+                     10240
+                     ((sha512 . "2be7c37a16db32a2b08fc917ed5f4241814e2665bda1bd15328c2e5a842e45b81f6f31274697248ffaabf8010796685acb3342c5920af53ddd1e75d7fd764bd1"))])
+ (checksum-invalid .
+                   [(1 0)
+                    nil "A single-file package with an invalid checksum." single
+                    nil
+                    365
+                    ((sha512 . "not-a-valid-checksum"))])
+ (checksum-invalid-tar .
+                       [(0 1)
+                        nil "A multi-file package with an invalid checksum." tar
+                        nil
+                        10240
+                        ((sha512 . "not-a-valid-checksum"))]))
diff --git a/test/lisp/emacs-lisp/package-resources/checksum-invalid-1.0.el b/test/lisp/emacs-lisp/package-resources/checksum-invalid-1.0.el
new file mode 100644
index 0000000000..3b8b07a4b8
--- /dev/null
+++ b/test/lisp/emacs-lisp/package-resources/checksum-invalid-1.0.el
@@ -0,0 +1,17 @@
+;;; invalid-checksum.el --- A package with an invalid checksum in archive-contents
+
+;; Version: 1.0
+
+;;; Commentary:
+
+;; This package has an invalid checksum in archive-contents and is
+;; just used to verify that package.el refuses to install.
+
+;;; Code:
+
+(defun p-equal-to-np-p ()
+  (error "FIXME"))
+
+(provide 'invalid-checksum)
+
+;;; invalid-checksum.el ends here
diff --git a/test/lisp/emacs-lisp/package-resources/checksum-invalid-tar-0.1.tar b/test/lisp/emacs-lisp/package-resources/checksum-invalid-tar-0.1.tar
new file mode 100644
index 0000000000000000000000000000000000000000..f153eb366b1264c293327c08c507876943572533
GIT binary patch
literal 10240
zcmeIwv2KGf5C&k+d5YUQL2a<ZGi2`zoWOvEV=8PS<?U;v3>}Ipq70S#-{Rry!~TBS
z(Y8}uuZ0UY_O2@uFNG}CyLes6T#YdzFRC%}`?|HZ5~?=Zn5I@|Eu=D)R)WmyuCPC8
zjrqkyB2F9zj=LLw>+c@?+l_WF|DPJA_0PO!3;3*at~>dwx_RWUO|2>+D`grfNIvti
zqi6nk{vV^Ib`Hsg6lv}$jV{tBw-XPRy4l9?mgveU*`+*P62);|eMdbzPwW@V-JLkm
z%`UFLyD;PddEn!xDo;n#z<vlo00Izz00bZa0SG_<0uX=z1Rwwb2tWV=5P$##AOHaf
TKmY;|fB*y_009U<;DEpvqoiQ<

literal 0
HcmV?d00001

diff --git a/test/lisp/emacs-lisp/package-resources/checksum-valid-123.el b/test/lisp/emacs-lisp/package-resources/checksum-valid-123.el
new file mode 100644
index 0000000000..9611fd8c87
--- /dev/null
+++ b/test/lisp/emacs-lisp/package-resources/checksum-valid-123.el
@@ -0,0 +1,17 @@
+;;; valid-checksum.el --- A package with an valid checksum in archive-contents
+
+;; Version: 123
+
+;;; Commentary:
+
+;; This package has an valid checksum in archive-contents and is
+;; used to verify that package.el installs it.
+
+;;; Code:
+
+(defun p-equal-to-np-p ()
+  (error "FIXME"))
+
+(provide 'valid-checksum)
+
+;;; valid-checksum.el ends here
diff --git a/test/lisp/emacs-lisp/package-resources/checksum-valid-tar-0.99.tar b/test/lisp/emacs-lisp/package-resources/checksum-valid-tar-0.99.tar
new file mode 100644
index 0000000000000000000000000000000000000000..e468754bb94d96a047df7a5c5926eb1d065e363f
GIT binary patch
literal 10240
zcmeHLU60y25aoG)#V9WYX#)vMsLEDX;_ihOX+IFKZTHb616Z5bxwZr4-`{cg=w1O^
zwc3?h)e#~P+cV>HX3iMm1;&rM$owTsdEy(U{Gk5sU8C}XS3uX>D5}scd>aK%?{>&u
zmGB~JMNvC!h3!fZMnM=<AbP<VPMRt-?HQn=ADNgleRAIS#!oK%wFlx8{EvFwe{Om&
z6T2n2-D*AMeU}Gzh`5gS{8JuS{@d+l1%j7x|Gbqyod3y!YoHf{DO2cAr9ce|S&|{l
zcuH?lfmt9NCJN*%eq?j3pFNMT8~ue5IHYx>|A)`(nEvEGY>hut|IOf{wXgq+uvrP3
zAvFwF|33-$4=uaAygX9c#5MT7552D}%Si;}j0EB^CBU}MtqPo-k)<n{uN9v!3{<ab
z&<w&R^c5nE<;<)|U!X4wkubxhIER6V>*cE9Mn^hFJXoWjP-$Aw0edhh7nGt^suL+!
z&XlnMez?7dUdCd*F}nY1)oo^j(Ayw7u$BeHOpwkcTpPuwg+bs3m`EBVcbQE1Y;9fB
zGl~~C3TRMe+Iq5bXw82>fr_OtET$=s;hM)NGy<cT>=V7f1g51OyW%tu$Z1@`a<fmk
z*!kERJO`<4FjZ!3<O*bN0jyEDYe*S|wOe{*=ifG%02gKG3z;K*AZxU}6;D%`A`}{D
zMS5=i5E}4#F!^|QKY{M;1AOj|M%~-V!zE2N3rVB6#EmIV*}-X-#I0h&tSNG9;ifmb
z`bbN<e-Ew)lLv|)`h&@BZ#;(n-3Yoc?2aabtNZKj2!{8g;oZ1b2N)Aa1cWv447#=-
zWs*`ULBn{uW&42`)f8Krz=AE2n2nJyDKbi1%E=v~r|nv=ER|wjZt_Vo0Ssm!q&Wvo
z@x5OdlXn`8)oN9ri=r;oyg}Ss-gV=z5`S9-S%!jYW8L|du9Sr~3o=G5l&QN-B=4}S
zyj@N0?IlQ-stHjfAoua#f~CHZzR3L?s<ktKDk??5c>Tebo1Ryz(d=BwB~K9E96F%k
z({+y`(Lni#uC4!&lK!5P50m3)|IZ)iuh##AX3*aEe>x0o`ak{tbLaop&nYV^T%f=0
z;&4=sJllCa@cwrk$cof(zm&2k$AFD|mVYMf+qnmVrzBFHLZ#qs6*MoBBt>0MxmR;a
y^ZG<P4-Nr`fJ49`;1F;KI0PI54grUNL%<>65O4@M1RMem0f&G?z#;HQA@Cm&a2WFd

literal 0
HcmV?d00001

diff --git a/test/lisp/emacs-lisp/package-tests.el b/test/lisp/emacs-lisp/package-tests.el
index f450fd27c2..823b68b234 100644
--- a/test/lisp/emacs-lisp/package-tests.el
+++ b/test/lisp/emacs-lisp/package-tests.el
@@ -43,6 +43,9 @@
 
 (setq package-menu-async nil)
 
+;; Silence byte-compiler.
+(defvar epg-config--program-alist)
+
 (defvar package-test-user-dir nil
   "Directory to use for installing packages during testing.")
 
@@ -296,14 +299,15 @@ package-test-refresh-contents
   (with-package-test ()
     (package-initialize)
     (package-refresh-contents)
-    (should (eq 4 (length package-archive-contents)))))
+    (should (eq 8 (length package-archive-contents)))))
 
 (ert-deftest package-test-install-single-from-archive ()
   "Install a single package from a package archive."
   (with-package-test ()
     (package-initialize)
     (package-refresh-contents)
-    (package-install 'simple-single)))
+    (package-install 'simple-single)
+    (should (package-installed-p 'simple-single))))
 
 (ert-deftest package-test-install-prioritized ()
   "Install a lower version from a higher-prioritized archive."
@@ -557,6 +561,167 @@ package-test-signed
 		"Status: Installed in ['`‘]signed-good-1.0/['’]."
 		nil t))))))
 
+\f
+;;; Tests for package checksum verification.
+
+(defmacro with-install-using-checksum (ok fail package &rest body)
+  "Test installing PACKAGE while setting `package-verify-checksums'."
+  (declare (indent 2))
+  `(progn
+     (dolist (opt ,ok)
+       (let ((package-verify-checksums opt))
+         (with-package-test ()
+           (package-initialize)
+           (package-refresh-contents)
+           (package-install ,package)
+           (package-installed-p ,package))))
+     (dolist (opt ,fail)
+       (let ((package-verify-checksums opt))
+         (should-error
+          (with-package-test ()
+            (package-initialize)
+            (package-refresh-contents)
+            (package-install ,package))
+          :type 'bad-checksum)))))
+
+(ert-deftest package-test-install-with-checksum/single-valid ()
+  "Install a single package with valid checksum."
+  (with-install-using-checksum '(nil allow-missing t all) '() 'checksum-valid))
+
+(ert-deftest package-test-install-with-checksum/single-invalid ()
+  "Install a tar package with invalid checksum."
+  (with-install-using-checksum '(nil) '(allow-missing t all) 'checksum-invalid))
+
+(ert-deftest package-test-install-with-checksum/tar-valid ()
+  "Install a tar package with valid checksum."
+  (with-install-using-checksum '(nil allow-missing t all) '() 'checksum-valid-tar))
+
+(ert-deftest package-test-install-with-checksum/tar-invalid ()
+  "Install a tar package with invalid checksum."
+  (with-install-using-checksum '(nil) '(allow-missing t all) 'checksum-invalid-tar))
+
+(defconst package-test-verification-text
+  "Example text for testing checksum verification.")
+(defconst package-tests-valid-md5-checksum
+  ;; (secure-hash 'md5 package-test-verification-text)
+  "abe6375809e532f081b808b3aa052dfb")
+(defconst package-tests-valid-sha256-checksum
+  ;; (secure-hash 'sha256 package-test-verification-text)
+  "6875aa4523e45ddef627b4edf1296f1d7dd0c22ddd6a6584f0228215d25eefcd")
+(defconst package-tests-valid-sha512-checksum
+  ;; (secure-hash 'sha512 package-test-verification-text)
+  (concat "bdc631f9e675b1ea34570f0a4bb44568dc5cecac905eea737f5f451bc52fd0c6"
+          "81b0d8b3dc2a942b9950fbe9096ebdf517668245c9b5a7bbdea8487a8f9cdce6"))
+
+(defmacro run-verify-checksums-test (verify-checksums checksums)
+  "Run a test for `package-verify-checksums'."
+  (declare (indent 1))
+  `(with-temp-buffer
+     (insert package-test-verification-text)
+     (let ((package-verify-checksums ,verify-checksums)
+           (pkg (package-desc-create :name 'foobar
+                              :version '(1 0)
+                              :summary "Just a package with checksum."
+                              :kind 'single
+                              :checksums ,checksums)))
+       (package--verify-package-checksum pkg))))
+
+(ert-deftest package-test--verify-package-checksums-nil/ignore-invalid ()
+  "Ignore all checksums even when invalid."
+  (run-verify-checksums-test nil
+    '((sha512 . "invalid")
+      (invalid . "invalid"))))
+
+(ert-deftest package-test--verify-package-checksums-nil/ignore-empty ()
+  "Ignore all checksums even when empty."
+  (run-verify-checksums-test nil
+    nil))
+
+(ert-deftest package-test--verify-package-checksums-allow-missing ()
+  "Verify checksums (allow-missing) -- verify if available."
+  (run-verify-checksums-test 'allow-missing
+    `((sha512 . ,package-tests-valid-sha512-checksum))))
+
+(ert-deftest package-test--verify-package-checksums-allow-missing/missing ()
+  "Verify checksums (allow-missing) -- allow missing."
+  (run-verify-checksums-test 'allow-missing
+    nil))
+
+(ert-deftest package-test--verify-package-checksums-allow-missing/ignore-unsupported ()
+  "Verify checksums (t) -- ignore unsupported algorithm."
+  (run-verify-checksums-test 'allow-missing
+    `((ignore . "not supported")
+      (sha512 . ,package-tests-valid-sha512-checksum))))
+
+(ert-deftest package-test--verify-package-checksums-t ()
+  "Verify checksums (t) -- succeed when valid."
+  (run-verify-checksums-test t
+    `((sha512 . ,package-tests-valid-sha512-checksum))))
+
+(ert-deftest package-test--verify-package-checksums-t/invalid-fails ()
+  "Verify checksums (t) -- fail on invalid."
+  (should-error
+   (run-verify-checksums-test t
+     '((sha512 . "invalid")))
+   :type 'bad-checksum))
+
+(ert-deftest package-test--verify-package-checksums-t/missing-fails ()
+  "Verify checksums (t) -- fail on missing."
+  (should-error
+   (run-verify-checksums-test t
+     nil)
+   :type 'bad-checksum))
+
+(ert-deftest package-test--verify-package-checksums-t/ignore-unsupported ()
+  "Verify checksums (t) -- ignore unsupported algorithm."
+  (run-verify-checksums-test t
+    `((ignore . "not supported")
+      (sha512 . ,package-tests-valid-sha512-checksum))))
+
+(ert-deftest package-test--verify-package-checksums-all ()
+  "Verify checksums (all) -- succeed on valid."
+  (run-verify-checksums-test 'all
+    `((md5    . ,package-tests-valid-md5-checksum)
+      (sha256 . ,package-tests-valid-sha256-checksum)
+      (sha512 . ,package-tests-valid-sha512-checksum))))
+
+(ert-deftest package-test--verify-package-checksums-all/invalid-fails ()
+  "Verify checksums (all) -- fail if one checksum is invalid."
+  (should-error
+   (run-verify-checksums-test 'all
+     `((md5    . ,package-tests-valid-md5-checksum)
+       (sha256 . "invalid")
+       (sha512 . ,package-tests-valid-sha512-checksum)))
+   :type 'bad-checksum))
+
+(ert-deftest package-test--verify-package-checksums-all/missing-fails ()
+  "Verify checksums (all) -- fail on missing checksums."
+  (should-error
+   (run-verify-checksums-test 'all
+     nil)
+   :type 'bad-checksum))
+
+(ert-deftest package-test--verify-package-checksums-all/no-supported-hash-fails ()
+  "Verify checksums (all) -- fail if we have no supported hash."
+  (should-error
+   (run-verify-checksums-test 'all
+     '((unsupported . "invalid")))
+   :type 'bad-checksum))
+
+(ert-deftest package-test--verify-package-checksums-all/ignore-unsupported ()
+  "Verify checksums (all) -- succed if one hash algorithm is unsupported.
+If the rest succeed, just ignore the unsupported one."
+  (run-verify-checksums-test 'all
+    `((md5    . ,package-tests-valid-md5-checksum)
+      (sha256 . ,package-tests-valid-sha256-checksum)
+      (sha512 . ,package-tests-valid-sha512-checksum)
+      (ignore . "not supported"))))
+
+(ert-deftest package-test--verify-package-size ()
+  (with-temp-buffer
+    (let ((len (1+ (abs (random 1000)))))
+      (insert (make-string len ?#))
+      (package--verify-package-size (package-desc-create :size len)))))
 
 \f
 ;;; Tests for package-x features.
@@ -570,7 +735,9 @@ package-x-test--single-archive-entry-1-3
                               'single
                               '((:authors ("J. R. Hacker" . "jrh@example.com"))
                                 (:maintainer "J. R. Hacker" . "jrh@example.com")
-                                (:url . "http://doodles.au"))))
+                                (:url . "http://doodles.au"))
+                              nil
+                              nil))
   "Expected contents of the archive entry from the \"simple-single\" package.")
 
 (defvar package-x-test--single-archive-entry-1-4
@@ -579,7 +746,9 @@ package-x-test--single-archive-entry-1-4
                               "A single-file package with no dependencies"
                               'single
                               '((:authors ("J. R. Hacker" . "jrh@example.com"))
-                                (:maintainer "J. R. Hacker" . "jrh@example.com"))))
+                                (:maintainer "J. R. Hacker" . "jrh@example.com"))
+                              nil
+                              nil))
   "Expected contents of the archive entry from the updated \"simple-single\" package.")
 
 (ert-deftest package-x-test-upload-buffer ()
-- 
2.20.1

bug#19479: Package manager vulnerable

[Noam Postavsky]

Stefan Kangas <stefan@marxist.se> writes:

> Subject: [PATCH] Support package checksum verification
>
> This is the first step towards protecting users of package.el against
> metadata replay attacks.

> +(define-error 'bad-checksum "Failed to verify checksum")

Would it be useful to have bad-signature and this one share a parent?
(by the way, I kind of wonder why it's not called
package-bad-signature).

> +  (cl-flet*
> +      ((supported-hashes
> +        (lambda ()

Is this a function (rather than a variable) just so it can be in the
same cl-flet* as do-check?

> +          (or (seq-filter (lambda (h) (memql (car h) (secure-hash-algorithms)))

The list returned by secure-hash-algorithms includes sha1 and md5.  This
is a problem if we're going to rely on signing them.  We should at least
plan to have some way of filtering out some functions.

> +                   (a (cdr hash))
> +                   (b (secure-hash algorithm (current-buffer))))

> +  (when-let ((a (package-desc-size pkg-desc))
> +             (b (string-bytes (buffer-string))))

I risk descending into trivial nitpicking, but I think 'a' and 'b' are
bit too generic.  Something like 'expected' and 'actual' would make it
harder to mix them up.

> +(defmacro run-verify-checksums-test (verify-checksums checksums)
> +  "Run a test for `package-verify-checksums'."

> +(ert-deftest package-test--verify-package-checksums-nil/ignore-invalid ()

I think run-verify-checksums-test should be prefixed with package-test
(whereas the individual test names could be prefixed with just package).

bug#19479: Package manager vulnerable

[Stefan Kangas]

Noam Postavsky <npostavs@gmail.com> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> Subject: [PATCH] Support package checksum verification
>>
>> This is the first step towards protecting users of package.el against
>> metadata replay attacks.
>
>> +(define-error 'bad-checksum "Failed to verify checksum")
>
> Would it be useful to have bad-signature and this one share a parent?
> (by the way, I kind of wonder why it's not called
> package-bad-signature).

Indeed, I fixed that.

>> +  (cl-flet*
>> +      ((supported-hashes
>> +        (lambda ()
>
> Is this a function (rather than a variable) just so it can be in the
> same cl-flet* as do-check?

I'm not sure I understand; it should be a function instead of a variable
because there is logic in there to match `(secure-hash-algorithms)'
against `(package-desc-checksums pkg-desc)' and signal an error.

>> +          (or (seq-filter (lambda (h) (memql (car h) (secure-hash-algorithms)))
>
> The list returned by secure-hash-algorithms includes sha1 and md5.  This
> is a problem if we're going to rely on signing them.  We should at least
> plan to have some way of filtering out some functions.

Yes, we currently would place the onus on the package archives to not
use those algorithms.  We could choose to filter them out as completely
unacceptable, I think that makes sense.

>> +                   (a (cdr hash))
>> +                   (b (secure-hash algorithm (current-buffer))))
>
>> +  (when-let ((a (package-desc-size pkg-desc))
>> +             (b (string-bytes (buffer-string))))
>
> I risk descending into trivial nitpicking, but I think 'a' and 'b' are
> bit too generic.  Something like 'expected' and 'actual' would make it
> harder to mix them up.

Thanks, fixed.

>> +(defmacro run-verify-checksums-test (verify-checksums checksums)
>> +  "Run a test for `package-verify-checksums'."
>
>> +(ert-deftest package-test--verify-package-checksums-nil/ignore-invalid ()
>
> I think run-verify-checksums-test should be prefixed with package-test
> (whereas the individual test names could be prefixed with just package).

That's true.  Fixed.

Thanks for the review!

Best regards,
Stefan Kangas

bug#19479: Package manager vulnerable

[Noam Postavsky]

Stefan Kangas <stefan@marxist.se> writes:

>> Is this a function (rather than a variable) just so it can be in the
>> same cl-flet* as do-check?
>
> I'm not sure I understand; it should be a function instead of a variable
> because there is logic in there to match `(secure-hash-algorithms)'
> against `(package-desc-checksums pkg-desc)' and signal an error.

Ah, I think had forgotten about/was confused by cl-flet's (FUNC (lambda
ARGLIST ...)) syntax when I wrote that.  Although I suppose you could
make it a plain variable by moving it inside do-check's lambda (not sure
if that's an improvement)?

bug#19479: Package manager vulnerable

[Stefan Kangas]

Kelly Dean <kelly@prtime.org> writes:

> Stop distributing elpa-key signatures of packages, since they're
> superfluous if you have package hashes in archive-contents and have
> elpa-key signatures of archive-contents, and you already have the
> latter.

I disagree with this part.

We should continue signing packages _at least_ until such a time that
there is likely to be zero users left who are using an Emacs version
without support for checking package hashes.

> Optional alternative timestamp handling, as Ivan pointed out that
> Debian does (at least sometimes): Instead of expiring archive-contents
> after some limit configured in Emacs, put an explicit expiration date
> in it. Personally, I don't like server-supplied expiration dates, kind
> of for a similar reason that RMS doesn't like server-supplied
> Javascript, or maybe just because I have too many irritating memories
> of expired SSL certs.

Is there any reason not to support both?  Package archives could decide
if they want to use this functionality or not, as could users.

> One more feature: include in each version of archive-contents a hash (and
> length) of the previous version of that file. This isn't necessary for
> preventing any of the vulnerabilities above, but it's easy insurance that
> slightly mitigates the disaster if the metadata signing key is compromised. It's
> pointless unless both the above problems are fixed, so it makes sense to put it
> here.

Does anyone understand how this would improve security in our case?
AFAIU, it can help with APT since they support distributing package
metadata in several files.  ELPA uses only one file, so I'm not sure it
would make much of a difference?

bug#19479: Package manager vulnerable

[Stefan Kangas]

Noam Postavsky <npostavs@gmail.com> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>>> Is this a function (rather than a variable) just so it can be in the
>>> same cl-flet* as do-check?
>>
>> I'm not sure I understand; it should be a function instead of a variable
>> because there is logic in there to match `(secure-hash-algorithms)'
>> against `(package-desc-checksums pkg-desc)' and signal an error.
>
> Ah, I think had forgotten about/was confused by cl-flet's (FUNC (lambda
> ARGLIST ...)) syntax when I wrote that.  Although I suppose you could
> make it a plain variable by moving it inside do-check's lambda (not sure
> if that's an improvement)?

Sure, you could do that.  I guess it's mostly down to style, but I
personally feel like that change would make the code a little bit harder
to read here.

bug#19479: Package manager vulnerable

[Noam Postavsky]

Stefan Kangas <stefan@marxist.se> writes:
>
>> One more feature: include in each version of archive-contents a hash
[...]
> Does anyone understand how this would improve security in our case?
> AFAIU, it can help with APT since they support distributing package
> metadata in several files.  ELPA uses only one file, so I'm not sure it
> would make much of a difference?

Not entirely, but there's a bit more detail on the emacs-devel thread
linked from the OP:

    One final feature that isn't necessary for preventing any of the 
    vulnerabilities above, but still is helpful to make the historical record even 
    more clear, is to include in each version of archive-contents a hash (and 
    length) of the previous version of that file. This further constrains an 
    attacker who has compromised the elpa key; he can still launch attacks, but 
    it's harder to keep the attacks secret for very long, since he's forced to 
    cause a fork in what's supposed to be a linear hash chain.

I think the idea is that if the attacker has the signing key and sends
out a bad version of archive-contents, it will be revealed as soon as
the victim gets a "good" version, since its previous-version hash won't
match.  Except that only works if the user can expect to get all
versions of archive-contents, so maybe I've missed something.

bug#19479: Package manager vulnerable

[Stefan Kangas]

Noam Postavsky <npostavs@gmail.com> writes:

> I think the idea is that if the attacker has the signing key and sends
> out a bad version of archive-contents, it will be revealed as soon as
> the victim gets a "good" version, since its previous-version hash won't
> match.

Yes, this is what I understood to be the case as well.

> Except that only works if the user can expect to get all versions of
> archive-contents, so maybe I've missed something.

Exactly my point.  So we can't rely on it to bail out if the hashes
don't match up, I think.