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: [PATCH] Re: bug#19479: Package manager vulnerable

[Kelly Dean]

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

Stefan Monnier wrote:
> > 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.

But having security rely on that makes it easier than necessary to accidentally open a window of vulnerability by failing to enforce that constraint. It's a brittle solution.

>> 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.

Ok, but as I explained in my original message, that solution still makes the attacker's job easier than necessary in some cases. Verifying the hash is a more robust solution than verifying the version number, so my patch below verifies the hash.

This is forward compatible. You can install this now and start putting archive-contents with hashes on elpa (and melpa and marmalade), and old clients will simply ignore the hashes and operate as usual.

BTW, one happy side effect of properly fixing this vulnerability is eliminating melpa's incentive to mangle package version numbers (they're mangled apparently to deal with the problem of package maintainers reusing version numbers).

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

Unfortunately, I don't see how to add timestamps to archive-contents without breaking old clients, so the metadata replay vulnerability will have to remain open until you decide how to handle the compatibility problem. My patch here only fixes the package replay vulnerability.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: package-replay-vuln.patch --]
[-- Type: text/x-diff, Size: 3425 bytes --]

--- emacs-24.4/lisp/emacs-lisp/package.el
+++ emacs-24.4/lisp/emacs-lisp/package.el
@@ -314,6 +314,11 @@
 
 (defvar package--default-summary "No description available.")
 
+(defvar package-hashfun 'sha256 "Function for secure hashing.")
+
+(defvar package-acceptable-hashfuns '(sha256)
+  "Past and current `package-hashfun' functions that are still secure.")
+
 (cl-defstruct (package-desc
                ;; Rename the default constructor from `make-package-desc'.
                (:constructor package-desc-create)
@@ -843,6 +848,20 @@
                          (epg-context-result-for context 'verify)))
         good-signatures))))
 
+(defun package--check-size (pkg-desc)
+  (eq (cdr (assoc :size (package-desc-extras pkg-desc)))
+      (pcase (package-desc-kind pkg-desc)
+	(`single (string-bytes (buffer-string)))
+	(`tar (buffer-size)) ; Because insert-file-contents mangled the literal
+	(kind (error "Unknown package kind: %s" kind)))))
+
+(defun package--check-hash (pkg-desc)
+  (let* ((x (cdr (assoc :hash (package-desc-extras pkg-desc))))
+	 (hashfun (car x)) ; Avoid Git's shortsightedness
+	 (hash (cadr x)))
+    (and (memq hashfun package-acceptable-hashfuns)
+	 (string= hash (secure-hash hashfun (current-buffer))))))
+
 (defun package-install-from-archive (pkg-desc)
   "Download and install a tar package."
   (let* ((location (package-archive-base pkg-desc))
@@ -859,6 +878,10 @@
 	    (unless (eq package-check-signature 'allow-unsigned)
 	      (error "Unsigned package: `%s'"
 		     (package-desc-name pkg-desc)))))
+      (unless (package--check-size pkg-desc)
+	(error "File size not correct: %s" (package-desc-name pkg-desc)))
+      (unless (package--check-hash pkg-desc)
+	(error "Failed to verify hash: %s" (package-desc-name pkg-desc)))
       (package-unpack pkg-desc))
     ;; Here the package has been installed successfully, mark it as
     ;; signed if appropriate.
@@ -1172,7 +1195,10 @@
            (package--prepare-dependencies
             (package-read-from-string requires-str)))
        :kind 'single
-       :url homepage))))
+       :url homepage
+       :size (string-bytes (buffer-string))
+       :hash (list package-hashfun
+		   (secure-hash package-hashfun (current-buffer)))))))
 
 (declare-function tar-get-file-descriptor "tar-mode" (file))
 (declare-function tar--extract "tar-mode" (descriptor))
@@ -1184,7 +1210,10 @@
   (let* ((dir-name (file-name-directory
                     (tar-header-name (car tar-parse-info))))
          (desc-file (package--description-file dir-name))
-         (tar-desc (tar-get-file-descriptor (concat dir-name desc-file))))
+         (tar-desc (tar-get-file-descriptor (concat dir-name desc-file)))
+	 (size (buffer-size tar-data-buffer))
+	 (hash (list package-hashfun
+		     (secure-hash package-hashfun tar-data-buffer))))
     (unless tar-desc
       (error "No package descriptor file found"))
     (with-current-buffer (tar--extract tar-desc)
@@ -1196,7 +1225,8 @@
                       (error "Can't find define-package in %s"
                              (tar-header-name tar-desc))
                     (apply #'package-desc-from-define
-                           (append (cdr pkg-def-parsed))))))
+                           (append (cdr pkg-def-parsed)
+				   (list :size size :hash hash))))))
             (setf (package-desc-kind pkg-desc) 'tar)
             pkg-desc)
         (kill-buffer (current-buffer))))))

bug#19536: [PATCH] package-upload-buffer-internal fails for tar files

[Kelly Dean]

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

It creates a file with a ⌜.tar⌝ extension that isn't a valid tar file.

Since elpa.gnu.org _does_ have valid tar files, I guess somebody wrote a script to work around this bug by overwriting the invalid tar files with the originals.

I'm submitting a patch for this since it affects validation of my patch for bug #19479, and I'm submitting the latter patch because it fixes a security vulnerability.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: package-upload-bug.patch --]
[-- Type: text/x-diff, Size: 446 bytes --]

--- emacs-24.4/lisp/emacs-lisp/package-x.el
+++ emacs-24.4/lisp/emacs-lisp/package-x.el
@@ -243,7 +243,7 @@
 	        	     (concat (symbol-name pkg-name) "-readme.txt")
 	        	     package-archive-upload-base)))
 
-	    (set-buffer pkg-buffer)
+	    (set-buffer (if (eq file-type 'tar) tar-data-buffer pkg-buffer))
 	    (write-region (point-min) (point-max)
 			  (expand-file-name
 			   (format "%s-%s.%s" pkg-name pkg-version extension)

bug#19479: [PATCH] Re: bug#19479: Package manager vulnerable

[Glenn Morris]

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.

(Ref: http://debbugs.gnu.org/14492#19)

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#19536: [PATCH] package-upload-buffer-internal fails for tar files

[Stefan Monnier]

> It creates a file with a ⌜.tar⌝ extension that isn't a valid tar file.

IOW, it's been broken for all these years?

> Since elpa.gnu.org _does_ have valid tar files, I guess somebody
> wrote a script to work around this bug by overwriting the invalid tar files
> with the originals.

We don't use this code.  The tarballs are built directly on elpa.gnu.org
from elpa/admin/* scripts.

> I'm submitting a patch for this since it affects validation of my patch for
> bug #19479, and I'm submitting the latter patch because it
> fixes a security vulnerability.

If this function's been broken for so long and nobody reported it, I'd
rather just remove it.


        Stefan

bug#19536: [PATCH] package-upload-buffer-internal fails for tar files

[Kelly Dean]

Stefan Monnier wrote:
> We don't use this code.  The tarballs are built directly on elpa.gnu.org
> from elpa/admin/* scripts.

What do those scripts do, that package-x doesn't?

Well, besides ‟actually work”. ;-)

> If this function's been broken for so long and nobody reported it, I'd
> rather just remove it.

What, you mean the admins don't want to use Emacs as a scripting engine?!

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: Copyright issue (was: Re: bug#19479: Package manager vulnerable)

[Kelly Dean]

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

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.
>
> I didn't mean "a trivial document" in the sense "any trivial document",
> but rather "the particular trivial document that everybody else signed".

The FSF doesn't have just one document for contributors; it has multiples, three of which I linked to in my previous message, and at least two more that are for assignment instead of disclaimer (one for only past contributions, and one for past and future contributions).

More than two years ago, I asked the copyright clerk to send me a disclaimer form to sign. He refused. This is the _only_ reason that the FSF doesn't already have a disclaimer on file for me.

If I sign an assignment document (i.e. saying that I own intellectual property for my work and that I'm assigning that ownership to the FSF), then I would just be committing perjury, because I don't own PD works. Nothing I sign can remove anything from the public domain.

Again, please don't conflate two separate issues:
0. The FSF is refusing new PD code in Emacs. (I would be happy to learn that I'm mistaken about this.)
1. My code is PD. (In case the FSF disputes this fact, I'm attaching a signed document to establish it.)

Because the clerk refused to send me anything to sign that would establish #1 to the FSF's satisfaction, today I printed, signed, and scanned the attached document based on the disclaimer forms the FSF has published, to make it abundantly clear that my work is PD and that the FSF is free to use my work with no legal restrictions whatsoever.

I'm also CCing it to assign@gnu.org, even though at this point I assume the clerk will come up with some excuse to reject it.

If the clerk feels this doesn't make #1 clear enough, then please tell me what needs to change. Even better, send me the exact disclaimer form you want me to sign, which I asked for in the first place.

I repeat: nothing I sign can remove anything from the public domain. So nothing I sign can assign to the FSF ownership of my work; if assignment is what the FSF insists on, then it's asking for the impossible.

The attached document is to establish #1 to the FSF's satisfaction. The FSF alone has the ability to solve #0; it has nothing to do with me.

Here's the text of the attached document:

This document is derived from the following sources:
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.program

I, Kelly Dean, American citizen and resident, hereby disclaim all patent, copyright, and all other forms of intellectual property ownership of and interest in all of my patches, software manuals, software programs, source code, documentation, revisions thereof, and all other works, past, present, and future, that I sent or will send to the emacs-devel@gnu.org or bug-gnu-emacs@gnu.org mailing lists, to 19479@debbugs.gnu.org, to any other mailing list or email address at gnu.org or any subdomain thereof, or to any developer or maintainer of GNU Emacs or any other GNU software, from my previous (no longer active) email address of kellydeanch@yahoo.com, my current email address of kelly@prtime.org, or any other email address.

I affirm that I have no other proprietary interest that would undermine this release, and will do nothing to undermine it in the future. I represent that all of the aforementioned works are my own and not a copy of someone else's work, except where sources are cited. Patches include citations and partial copies of the works to which the patches apply.

I created all of the works exclusively on my own time. They are not works made for hire, and there's no educational institution, employer, or any other organization or person who owns them. I do not have any agreement with any person or organization saying he or it owns programs I write, and I did not have any such agreement when I created any of the aforementioned works.

All of the works are permanently and irrevocably in the public domain.

Kelly Dean
kelly@prtime.org
January 8, 2015


[-- Attachment #2: gnu-disclaimer.pdf --]
[-- Type: application/pdf, Size: 29848 bytes --]

bug#19479: Copyright issue

[Stefan Monnier]

> 1. My code is PD. (In case the FSF disputes this fact, I'm attaching
>    a signed document to establish it.)

It can't be PD.  You're simply confused about it.  It will only be PD 75
years after your death (or something like that).  Until then, all you
can do is sign paperworks, and currently for Emacs contributions we
require this paperwork to be a copyright assignment rather than
a disclaimer.


        Stefan

bug#19479: Copyright issue

[David Kastrup]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> 1. My code is PD. (In case the FSF disputes this fact, I'm attaching
>>    a signed document to establish it.)
>
> It can't be PD.  You're simply confused about it.  It will only be PD
> 75 years after your death (or something like that).

If I remember correctly, if he is living in the U.S.A. and registers a
specific work with the U.S. copyright office as being released by him
into the public domain, then the work will indeed be in the public
domain within the U.S.A.  We need to bother with more than the U.S.A.,
however, and one can only register specific works which means it is not
possible to register them before they are even created.

-- 
David Kastrup

bug#19479: Copyright issue

[Kelly Dean]

Stefan Monnier wrote:
>> 1. My code is PD. (In case the FSF disputes this fact, I'm attaching
>>    a signed document to establish it.)
>
> It can't be PD.  You're simply confused about it.  It will only be PD 75
> years after your death (or something like that).  Until then, all you
> can do is sign paperworks, and currently for Emacs contributions we
> require this paperwork to be a copyright assignment rather than
> a disclaimer.

GNU's own website says it can be PD. The documents at the three links I sent you start with:
⌜I'd like to ask you to sign a disclaimer for the manual, thus putting it in the public domain.⌝
⌜I'd like to ask you to sign a disclaimer for the program, thus putting it in the public domain.⌝
⌜I'd like to ask you to sign a disclaimer for your changes, thus putting them in the public domain.⌝

Notice the ⌜thus putting them in the public domain⌝.

Also, do you claim that SQLite is not PD? The author, Richard Hipp, says it's PD, and the many millions of users of SQLite, including many major companies with lots of copyright lawyers, accept the legal fact that it's PD. And Richard Hipp is not dead.

Also, do you claim that feedmail.el is not PD? The first lines of it are:
;;; feedmail.el --- assist other email packages to massage outgoing messages
;;; This file is in the public domain.

;; This file is part of GNU Emacs.

bug#19479: Copyright issue

[Glenn Morris]

I must say, that it was not my impression that disclaimers were not
accaptable for Emacs. Only that the FSF does not offer a "past and
future" option for disclaimers like it does for assignments, so a new
disclaimer would have to be completed for every new change. I thought
this was not worth bothering with, so I advised you not to send more patches.

But I certainly don't know, I just go with whatever assign@gnu says.

I don't see much point discussing this on emacs-devel. None of us are
lawyers so our opinions are pretty irrelevant. We need to wait and see
what assign@gnu says.

bug#19479: Copyright issue

[Glenn Morris]

Glenn Morris wrote:

> I must say, that it was not my impression that disclaimers were not
> accaptable for Emacs. Only that the FSF does not offer a "past and
> future" option for disclaimers like it does for assignments, so a new
> disclaimer would have to be completed for every new change. I thought
> this was not worth bothering with, so I advised you not to send more patches.

PS but yes, for a non-trivial security issue like 19479 it did seem
worth it to me, so I was on the verge of saying, would you be willing to
complete a disclaimer for this change. But then Stefan said disclaimers
were not viable, so I didn't bother to say it.

bug#19479: (on-topic) Re: bug#19479: Package manager vulnerable

[Kelly Dean]

Back on topic...

I found a good way to add timestamps to prevent metadata replay (the other vulnerability), and to further harden the package manager's security, but of course I'll wait until we hear from the clerk before trying to implement it.

The reason I said there's a compatibility problem for timestamps is that archive-contents is a list consisting just of a version number followed by a bunch of package records; the list's format isn't extensible (though the package record format is extensible). There's no way to insert a timestamp without changing the list's format (and thus, the version number), but if you do that, then old clients can't understand archive-contents anymore.

Even worse, old clients become stuck because they store the new-format (incompatible) file before checking the version number, then barf on it and refuse to accept even an old-format (compatible) file to replace it until you manually delete the stored one.

I see four possible solutions:
0. Have a flag day, on which all the elpas switch to the new format, and on or before which everybody must upgrade to Emacs 25 or his package manager will stop working.
1. Have the server check the User-Agent header, and send the old-format file if it's ⌜URL/Emacs⌝, and the new-format if it's ⌜URL/Emacs-25⌝ or later.
2. Use a different URL for the new-format file.
3. Keep the old format, and put the timestamp in a different file.

#0 obviously isn't an option.
I advise against #1, for reasons which everybody here already knows.
#2 would work, but is inelegant, since you would still have to retain the old-format file for the sake of old clients, and it's inefficient, since new clients would have to periodically re-download the entire file (fairly big, in Melpa's case) even if nothing but the timestamp changed (and clients have to demand fresh timestamps in order to prevent metadata replay attacks).

#3 looks like the best solution. The timestamp file includes the timestamp and the hash of archive-contents. Sign the timestamp file for the sake of new clients.

Old clients would ignore the timestamp file. If archive-contents is unchanged, then new clients would only have to periodically re-download the timestamp file and signature--the minimal amount of data necessary. They'd see that the current hash of archive-contents matches the version they already have stored. IOW, to whoever made archive-contents inextensible: thank you! You've forced the right solution to timestamping. ;-)

Combined with my previous patch, this leaves the timestamp-file's signature as the only one that's necessary to secure the entire archive (packages and metadata, including timestamp) and prevent both package and metadata replay attacks. IMHO, this simplicity makes it practical to insist that all elpas provide this signature, so Emacs 25 could enforce it by default.

Optionally continue signing archive-contents for the sake of 24.4 clients, but since 25 won't need that signature, nothing before 24.4 is capable of checking it, 24.4 doesn't enforce it by default, Melpa doesn't even provide it IIUC (GNU Elpa does), and 24.4 is vulnerable to package and metadata replay anyway, you might as well not. The kind of people who have changed package-check-signature to t will upgrade to 25 anyway.

bug#19479: Disclaimer is now on file at FSF

[Kelly Dean]

The FSF has accepted my disclaimer and added me to their records. You can install my patches if you find them satisfactory on technical grounds.

bug#19536: package-upload-buffer-internal fails for tar files

[Kelly Dean]

Fixed in trunk.

bug#19479: Emacs package manager vulnerable to replay attacks

[Kelly Dean]

Note, I'm not implementing the metadata-replay fix, because it's unlikely my patch would be accepted, so somebody else will need to do it. See my January 11th message to bug #19479 for a description of how to do it.

bug#19479: Disclaimer is now on file at FSF

[Glenn Morris]

So, I don't want to get into this discussion, but I've always assumed
that disclaimers do not/cannot apply to future changes. I asked
assign@gnu.org, and they confirmed "Disclaimers are not valid for future
contributions".

I mention this because AFAICS you are sending new patches.

Your disclaimer is dated 2015-1-8. AFAICS we cannot apply anything after
that. Someone should also check the several patches from you that have
been applied recently to make sure they originated before this date.

Sorry, I don't have time/inclination to discuss special cases.
Maybe you want to take it up with rms and/or assign@gnu.

bug#19479: Disclaimer is now on file at FSF

[Kelly Dean]

> So, I don't want to get into this discussion,

And yet you chose to dig it back up, even after everybody else was satisfied that it was resolved more than a month ago. The copyright clerk's exact words on January 20th were, ‟We've accepted the public domain disclaimer and added you to our records”, not ‟we've accepted part of the disclaimer, but rejected another part”.

The disclaimer covers future changes, and everybody's comments about that part had already been CCed to the clerk, and his answer was, ‟We've accepted the public domain disclaimer”.

> but I've always assumed
> that disclaimers do not/cannot apply to future changes. I asked
> assign@gnu.org, and they confirmed "Disclaimers are not valid for future
> contributions".

Good luck finding a copyright judge anywhere in America who would agree with your absurd claim that my work since January 8th is not in the public domain, despite my signed statement that it is.

Or if you admit it is PD, I'm sure you can dream up some rationalization of why PD code isn't allowed in Emacs, and then try to remove it all, which is a lot more than just my code.

Either way, I'm done trying to work on Emacs. This B.S. isn't worth my time.

bug#19479: Disclaimer is now on file at FSF

[Glenn Morris]

Kelly Dean wrote:

>> So, I don't want to get into this discussion,
>
> And yet you chose to dig it back up, even after everybody else was
> satisfied that it was resolved more than a month ago.

I've been largely on a break from Emacs. I always thought there was
something strange here, and I just happened to get motivated enough now
to ask assign@gnu for clarification when I saw patches were still arriving.

> The copyright clerk's exact words on January 20th were, ‟We've
> accepted the public domain disclaimer and added you to our records",
> not ‟we've accepted part of the disclaimer, but rejected another
> part".

I specifically mentioned you by name in the question I asked assign@gnu,
and the reply I got (one day ago) was, in totality:

   Disclaimers are not valid for future contributions. Thanks for checking in.

Like I said, you can take it up with them if you disagree.
I'be glad to be corrected, but it all seems pretty clear to me.

I am not trying to be the bad guy and I am not out to get you.
I applied several patches from you and would have been happy to apply more.
I am just trying to ensure Emacs follows the FSF's procedures,
which seem pretty clear to me.


Vibhav Pant wrote:

> Well, what about
> http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future?
> This is the form to request documents for assigning past and future
> works, which according to you isn't possible.

That's not what I said.
I said: I am told *disclaimers* cannot apply to future changes.

You will note that there are separate documents for *assigning* past
changes (request-assign.changes), and past-and-future changes
(request-assign.future). But for *disclaimers* there is only
request-disclaim.changes. There is no request-disclaim.future.
If you read

http://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-disclaim.changes

it quite clearly states that it only applies to past, finished changes.

I am not a lawyer (AFAIK, neither is anyone else on this list) and
have no interest in discussing why these things are different.
They just are.

I said the first time we went through this that it was my
understanding that disclaimers worked this way. I said it again here:
https://lists.gnu.org/archive/html/emacs-devel/2015-01/msg00198.html

I have now had this confirmed by assign@gnu.


If you (the generic you) want to contribute to Emacs, there is a
well-defined, simple procedure that hundreds of people have followed
with no problem.

If you don't want to follow the procedure, then fine, that's your
prerogative. Then you can't contribute.

But please don't start arguing with us about what the procedure is, or
should be, or what you think a judge might say, or why you need to be an
exception. We don't set the rules here at Emacs, and it just isn't a
productive use of anyone's time.

bug#19479: Disclaimer is now on file at FSF

[Eli Zaretskii]

unblock 24655 by 19479
thanks

> From: Glenn Morris <rgm@gnu.org>
> Date: Wed, 25 Feb 2015 16:09:57 -0500
> Cc: 19479@debbugs.gnu.org, emacs-devel@gnu.org
> 
> I am not a lawyer (AFAIK, neither is anyone else on this list) and
> have no interest in discussing why these things are different.
> They just are.
> 
> I said the first time we went through this that it was my
> understanding that disclaimers worked this way. I said it again here:
> https://lists.gnu.org/archive/html/emacs-devel/2015-01/msg00198.html
> 
> I have now had this confirmed by assign@gnu.
> 
> 
> If you (the generic you) want to contribute to Emacs, there is a
> well-defined, simple procedure that hundreds of people have followed
> with no problem.
> 
> If you don't want to follow the procedure, then fine, that's your
> prerogative. Then you can't contribute.
> 
> But please don't start arguing with us about what the procedure is, or
> should be, or what you think a judge might say, or why you need to be an
> exception. We don't set the rules here at Emacs, and it just isn't a
> productive use of anyone's time.

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.

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.

bug#19479: Package manager vulnerable to replay attacks

[Stefan Kangas]

I have just pushed the branch scratch/package-security with proper
support for timestamps, as discussed below.  More details are in the
commit messages and the proposed documentation changes.  Once this is
merged, I hope to work on adding support for this to both GNU ELPA and
NonGNU ELPA.

I would like to merge this change to the master branch.  Is it
sufficient to ask for reviews and comments here first, or is there
anything else I should do in addition?

Any comments and feedback on all this is of course more than welcome.
Please also see my previous message about this change below.

Stefan Kangas <stefan@marxist.se> writes:

> 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.

bug#19479: Package manager vulnerable to replay attacks

[Stefan Monnier]

> I have just pushed the branch scratch/package-security with proper
> support for timestamps, as discussed below.  More details are in the
> commit messages and the proposed documentation changes.  Once this is
> merged, I hope to work on adding support for this to both GNU ELPA and
> NonGNU ELPA.

Do we need this hash-checksum, really?

AFAICT, I think if we want to avoid replay attacks we need indeed
a monotone "counter" (e.g. a timestamp) on the `archive-contents` and
then a way to verify that the tarballs are what they claim to be.

We can already verify that they are what they claim to be since the
tarball includes the version number inside the `<pkg>-pkg.el` file.

So, I think all we need is to verify the contents of `<pkg>-pkg.el`
after unpacking a tarball, to make sure it is indeed the package and
version its name claimed to be.  This check would be welcome in any case
to detect packaging errors.


        Stefan

bug#19479: Package manager vulnerable to replay attacks

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Do we need this hash-checksum, really?

AFAIK, a cryptographic hash is the generally accepted solution for
securely verifying the contents of a file.  That is, when you worry
about that file having been tampered with by a hostile actor.

> AFAICT, I think if we want to avoid replay attacks we need indeed
> a monotone "counter" (e.g. a timestamp) on the `archive-contents` and
> then a way to verify that the tarballs are what they claim to be.

We also need to sign `archive-contents', of course.  But otherwise
correct: we need to know that the metadata is not out-of-date, and we
need to have a (secure) mapping from the package metadata to individual
packages.

> We can already verify that they are what they claim to be since the
> tarball includes the version number inside the `<pkg>-pkg.el` file.
>
> So, I think all we need is to verify the contents of `<pkg>-pkg.el`
> after unpacking a tarball, to make sure it is indeed the package and
> version its name claimed to be.  This check would be welcome in any case
> to detect packaging errors.

I think the question here is: how do we securely map from the (signed)
package metadata to an individual package?

While not perfect, with a secure hash function, collisions are hard
enough to find that we for our purposes (like the rest of the world) can
happily not worry about it.  In comparison, it is immeasurably easier to
fake a version number than having to defeat a hash function.  I think
this is a significant drawback of what you propose.

We would need to add in a number of assumptions (e.g. packages are
individually signed, we never accidentally sign an old package with a
newer version number, etc.), to gain more confidence in a simple version
number check.

But even then the security it provides will not be as strong, and much
more brittle; there are just more moving parts where things could go
wrong.  And I'm not sure what we would gain.  Importantly, I don't think
it would simplify our implementation in Emacs (or GNU/NonGNU ELPA)
significantly.

But we could of course also check that the version number is correct.
That sounds like a useful sanity check to do.

Thanks for taking a look at this!

PS. Note that if we add a checksum, there will no longer be any need to
    sign individual packages for future versions of Emacs.  We would
    then only need to sign the metadata.

bug#19479: Package manager vulnerable to replay attacks

[Stefan Monnier]

> While not perfect, with a secure hash function, collisions are hard
> enough to find that we for our purposes (like the rest of the world) can
> happily not worry about it.  In comparison, it is immeasurably easier to
> fake a version number than having to defeat a hash function.  I think
> this is a significant drawback of what you propose.

I'm not sure what you mean by it being easier: since the tarballs are
cryptographically signed, just like `archive-contents`, if
`archive-contents` points to `foo-42.1.tar` and that tarball has
a correct signature and its content says that it is indeed the package
`foo-42.1`, then I'm not sure which easy attack would be applicable.

AFAICT you'd have to find some old signed tarball which we accidentally
built with an incorrect content?  But presumably if we ever mess up
a tarball like that (which is indeed possible), we'd then want to be
careful not to "reuse" that version number, no?

> We would need to add in a number of assumptions (e.g. packages are
> individually signed,

Which they already are.

> we never accidentally sign an old package with a newer version number,
> etc.),

That's indeed the case as well.

> to gain more confidence in a simple version
> number check.

I think it's comparable: the main failings wold require some error on
our side in how we build and sign packages, and in most such cases
I think we'd end up with holes with either scheme.

> But we could of course also check that the version number is correct.
> That sounds like a useful sanity check to do.

Exactly.

> PS. Note that if we add a checksum, there will no longer be any need to
>     sign individual packages for future versions of Emacs.  We would
>     then only need to sign the metadata.

I think we'd want to keep the signatures anyway, e.g. they can still be
checked manually for old tarballs which aren't listed in
`archive-contents` any more.  And more generally they allow
authenticating the origin of a package without having to look it up in
`archive-contents`.


        Stefan

bug#19479: Package manager vulnerable to replay attacks

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> While not perfect, with a secure hash function, collisions are hard
>> enough to find that we for our purposes (like the rest of the world) can
>> happily not worry about it.  In comparison, it is immeasurably easier to
>> fake a version number than having to defeat a hash function.  I think
>> this is a significant drawback of what you propose.
>
> I'm not sure what you mean by it being easier: since the tarballs are
> cryptographically signed, just like `archive-contents`, if
> `archive-contents` points to `foo-42.1.tar` and that tarball has
> a correct signature and its content says that it is indeed the package
> `foo-42.1`, then I'm not sure which easy attack would be applicable.

I guess it's in the nature of attacks that we generally don't know or
think about them in advance.  This is precisely why, when it comes to
security, it IMO a good idea to use battle-tested and generally accepted
solutions.

> AFAICT you'd have to find some old signed tarball which we accidentally
> built with an incorrect content?  But presumably if we ever mess up
> a tarball like that (which is indeed possible), we'd then want to be
> careful not to "reuse" that version number, no?

I'm not sure we can assume that we would always detect when we mess up.
But yes, this sounds like one possible attack vector.

> I think it's comparable: the main failings wold require some error on
> our side in how we build and sign packages, and in most such cases
> I think we'd end up with holes with either scheme.

Agreed, we could make mistakes in implementing either scheme.

FWIW, I think that with the version number idea, there are more places
where we could make important mistakes, since more code would be
involved.  There are also more assumptions that we need to ensure hold
true under all circumstances.  (But see below.)

>> But we could of course also check that the version number is correct.
>> That sounds like a useful sanity check to do.
>
> Exactly.

How about adding this check in addition to the checksum check?  Having
two separate checks together should surely bring more confidence than
either of them would separately.  That sounds like good "defense in
depth" thinking to me.

> I think we'd want to keep the signatures anyway, e.g. they can still be
> checked manually for old tarballs which aren't listed in
> `archive-contents` any more.  And more generally they allow
> authenticating the origin of a package without having to look it up in
> `archive-contents`.

Valid points.  Let's keep them indefinitely.

bug#19479: Package manager vulnerable to replay attacks

[Stefan Monnier]

> How about adding this check in addition to the checksum check?

I think we should add this check in any case, yes.

> Having two separate checks together should surely bring more
> confidence than either of them would separately.  That sounds like
> good "defense in depth" thinking to me.

I'm not sure the added hash is needed, but it seems reasonably harmless.

>> I think we'd want to keep the signatures anyway, e.g. they can still be
>> checked manually for old tarballs which aren't listed in
>> `archive-contents` any more.  And more generally they allow
>> authenticating the origin of a package without having to look it up in
>> `archive-contents`.
> Valid points.  Let's keep them indefinitely.

Especially since some people seem interested to add commands to
`package.el` to programatically install old packages.


        Stefan

bug#19479: Package manager vulnerable to replay attacks

[Jean Louis]

* Stefan Kangas <stefan@marxist.se> [2020-11-26 05:07]:
> PS. Note that if we add a checksum, there will no longer be any need to
>     sign individual packages for future versions of Emacs.  We would
>     then only need to sign the metadata.

I do not know internals as I did not see yet signed package. But if
signed package fetched from GNU ELPA then such is verified against
official key on user's computer, right?

Now take in account that signed packages will be distributed through
mirrors and mirrors already exist.

If archive-contents or meta data is signed and can be technically used
by mirror, that would be fine. If archive-contents need to be changed
or mirror wants to mirror only specific packages then package need to
be signed.