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.