From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Kelly Dean Newsgroups: gmane.emacs.bugs Subject: bug#19479: [PATCH] Re: bug#19479: Package manager vulnerable Date: Thu, 08 Jan 2015 03:31:01 +0000 Message-ID: References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: ger.gmane.org 1420688040 22112 80.91.229.3 (8 Jan 2015 03:34:00 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 8 Jan 2015 03:34:00 +0000 (UTC) Cc: 19479@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Thu Jan 08 04:33:53 2015 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Y93qR-0004WJ-PC for geb-bug-gnu-emacs@m.gmane.org; Thu, 08 Jan 2015 04:33:12 +0100 Original-Received: from localhost ([::1]:43886 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y93qR-0001LJ-1q for geb-bug-gnu-emacs@m.gmane.org; Wed, 07 Jan 2015 22:33:11 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:54229) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y93qM-0001LC-Qb for bug-gnu-emacs@gnu.org; Wed, 07 Jan 2015 22:33:08 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y93qI-0006zY-JU for bug-gnu-emacs@gnu.org; Wed, 07 Jan 2015 22:33:06 -0500 Original-Received: from debbugs.gnu.org ([140.186.70.43]:58580) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y93qI-0006zU-G6 for bug-gnu-emacs@gnu.org; Wed, 07 Jan 2015 22:33:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.80) (envelope-from ) id 1Y93qI-0005oO-39 for bug-gnu-emacs@gnu.org; Wed, 07 Jan 2015 22:33:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Kelly Dean Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 08 Jan 2015 03:33:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 19479 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: security Original-Received: via spool by 19479-submit@debbugs.gnu.org id=B19479.142068796022306 (code B ref 19479); Thu, 08 Jan 2015 03:33:02 +0000 Original-Received: (at 19479) by debbugs.gnu.org; 8 Jan 2015 03:32:40 +0000 Original-Received: from localhost ([127.0.0.1]:39713 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1Y93pv-0005nh-Gx for submit@debbugs.gnu.org; Wed, 07 Jan 2015 22:32:40 -0500 Original-Received: from relay3-d.mail.gandi.net ([217.70.183.195]:51833) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1Y93ps-0005nW-JX for 19479@debbugs.gnu.org; Wed, 07 Jan 2015 22:32:37 -0500 Original-Received: from mfilter23-d.gandi.net (mfilter23-d.gandi.net [217.70.178.151]) by relay3-d.mail.gandi.net (Postfix) with ESMTP id 9A27CA80AC; Thu, 8 Jan 2015 04:32:35 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter23-d.gandi.net Original-Received: from relay3-d.mail.gandi.net ([217.70.183.195]) by mfilter23-d.gandi.net (mfilter23-d.gandi.net [10.0.15.180]) (amavisd-new, port 10024) with ESMTP id p4Q03FemWhim; Thu, 8 Jan 2015 04:32:34 +0100 (CET) X-Originating-IP: 162.248.99.114 Original-Received: from localhost (114-99-248-162-static.reverse.queryfoundry.net [162.248.99.114]) (Authenticated sender: kelly@prtime.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 9E9F0A80B5; Thu, 8 Jan 2015 04:32:31 +0100 (CET) In-Reply-To: X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.15 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 140.186.70.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:98102 Archived-At: --=-=-= Content-Type: text/plain 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. --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=package-replay-vuln.patch --- 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)))))) --=-=-=--