GNU bug report logs - #19479
Package manager vulnerable

Previous Next

Full log

View this message in rfc822 format

From: Kelly Dean <kelly <at> prtime.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 19479 <at> debbugs.gnu.org
Subject: bug#19479: [PATCH] Re: bug#19479: Package manager vulnerable
Date: Thu, 08 Jan 2015 03:31:01 +0000

[Message part 1 (text/plain, inline)]

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.

[package-replay-vuln.patch (text/x-diff, inline)]

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.