[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#19479: [PATCH] Re: bug#19479: Package manager vulnerable
From: |
Kelly Dean |
Subject: |
bug#19479: [PATCH] Re: bug#19479: Package manager vulnerable |
Date: |
Thu, 08 Jan 2015 03:31:01 +0000 |
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))))))
- bug#19479: Package manager vulnerable, Kelly Dean, 2015/01/01
- bug#19479: Package manager vulnerable, Stefan Monnier, 2015/01/04
- bug#19479: Package manager vulnerable, Kelly Dean, 2015/01/04
- bug#19479: Package manager vulnerable, Stefan Monnier, 2015/01/04
- bug#19479: [PATCH] Re: bug#19479: Package manager vulnerable,
Kelly Dean <=
- bug#19479: [PATCH] Re: bug#19479: Package manager vulnerable, Glenn Morris, 2015/01/07
- bug#19479: Package manager vulnerable, Kelly Dean, 2015/01/08
- bug#19479: Package manager vulnerable, Stefan Monnier, 2015/01/08
- bug#19479: Package manager vulnerable, Kelly Dean, 2015/01/08
- bug#19479: Package manager vulnerable, Stefan Monnier, 2015/01/08
- bug#19479: Copyright issue (was: Re: bug#19479: Package manager vulnerable), Kelly Dean, 2015/01/09
- bug#19479: Copyright issue, Stefan Monnier, 2015/01/09
- bug#19479: Copyright issue, David Kastrup, 2015/01/09
- bug#19479: Copyright issue, Kelly Dean, 2015/01/09
- bug#19479: Copyright issue, Kelly Dean, 2015/01/09