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

[Kelly Dean]

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.

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