bug#19479: Package manager vulnerable

[Stefan Kangas]

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

0001-Support-package-checksum-verification.patch
Description: Text Data