scratch/package-security c7664ac 3/3: Check timestamp in package archive file

[Stefan Kangas]

branch: scratch/package-security
commit c7664ac5b73ecbce70d48656c1388fbe728b801a
Author: Stefan Kangas <stefan@marxist.se>
Commit: Stefan Kangas <stefan@marxist.se>

    Check timestamp in package archive file
    
    This is the second step towards protecting users of package.el against
    metadata replay attacks.  (Bug#19479)
    
    * lisp/emacs-lisp/package.el (package-check-timestamp): New defcustom.
    (bad-timestamp): New error.
    (package--parse-timestamp-from-buffer)
    (package--compare-archive-timestamps)
    (package--check-archive-timestamp): New defuns.
    (package--download-one-archive): Check timestamp of the
    'archive-contents' file using above functions.  It is only checked if
    it exists, which makes this change backwards compatible.
    
    * lisp/calendar/iso8601.el (iso8601-parse): Add autoload cookie.
    
    * test/lisp/emacs-lisp/package-tests.el
    (package-tests--parse-timestamp-from-buffer)
    (package-test-verify-package-timestamp)
    (package-test-check-archive-timestamp): New tests.
    
    * test/lisp/emacs-lisp/package-resources/archives/older/archive-contents:
    * test/lisp/emacs-lisp/package-resources/archives/newer/archive-contents:
    New files.
---
 lisp/calendar/iso8601.el                           |  1 +
 lisp/emacs-lisp/package.el                         | 76 ++++++++++++++++++++++
 .../archives/newer/archive-contents                |  1 +
 .../archives/older/archive-contents                |  1 +
 test/lisp/emacs-lisp/package-tests.el              | 55 ++++++++++++++++
 5 files changed, 134 insertions(+)

diff --git a/lisp/calendar/iso8601.el b/lisp/calendar/iso8601.el
index 906c29b..7a6f14a 100644
--- a/lisp/calendar/iso8601.el
+++ b/lisp/calendar/iso8601.el
@@ -114,6 +114,7 @@
          iso8601--duration-week-match
          iso8601--duration-combined-match)))
 
+;;;###autoload
 (defun iso8601-parse (string &optional form)
   "Parse an ISO 8601 date/time string and return a `decode-time' structure.
 
diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 5c1cbd0..1f322bd 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -360,6 +360,12 @@ should normally not be used since it will decrease 
security."
   :risky t
   :version "28.1")
 
+(defcustom package-check-timestamp t
+  "Non-nil means to verify the package archive timestamp."
+  :type 'boolean
+  :risky t
+  :version "28.1")
+
 (defcustom package-check-signature 'allow-unsigned
   "Non-nil means to check package signatures when installing.
 More specifically the value can be:
@@ -449,6 +455,7 @@ synchronously."
 (define-error 'bad-size "Package size mismatch" 'package-error)
 (define-error 'bad-signature "Failed to verify signature" 'package-error)
 (define-error 'bad-checksum "Failed to verify checksum" 'package-error)
+(define-error 'bad-timestamp "Failed to verify timestamp" 'package-error)
 
 
 ;;; `package-desc' object definition
@@ -1812,6 +1819,74 @@ Once it's empty, run 
`package--post-download-archives-hook'."
     (message "Package refresh done")
     (run-hooks 'package--post-download-archives-hook)))
 
+(defun package--parse-timestamp-from-buffer (name)
+  "Return \"archive-contents\" timestamp for archive named NAME.
+This function assumes that the current buffer contains the
+\"archive-contents\" file.  If there is no valid timestamp, return nil.
+
+A valid timestamp looks like:
+
+;; Last-Updated: <TIMESTAMP>
+
+Where <TIMESTAMP> is a valid ISO-8601 (RFC 3339) date.  If there
+is such a line but <TIMESTAMP> is invalid, show a warning and
+return nil."
+  (save-excursion
+    (goto-char (point-min))
+    (when (re-search-forward "^;; Last-Updated: *\\(.+?\\) *$" nil t)
+      (condition-case-unless-debug nil
+          (encode-time (iso8601-parse (match-string 1)))
+        (lwarn '(package timestamp)
+               (list (format "Malformed timestamp for archive `%s': `%s'"
+                             name (match-string 1))))))))
+
+(defun package--compare-archive-timestamps (old new name)
+  "Signal error unless NEW timestamp is more recent than OLD."
+  ;; If timestamp is missing on cached (old) file, do nothing here.
+  ;; This package archive recently introduced support for timestamps.
+  ;; We will require a timestamp for that archive in future updates.
+  (when old
+    ;; On the other hand, if we did find a timestamp in the cached
+    ;; file, but the new file is missing one, something is wrong.
+    (unless new
+      (signal 'bad-timestamp
+              (list (format-message
+                     "New archive contents for `%s' missing timestamp, 
refusing to proceed"
+                     name))))
+    ;; Found timestamps in both files.  Compare them.
+    (if (time-less-p new old)
+        (signal 'bad-timestamp
+                (list (format-message
+                       "New archive contents for `%s' older than cached, 
refusing to proceed"
+                       name)))
+      (when (time-less-p (current-time) new)
+        (lwarn '(package timestamp) :warning
+               (format-message
+                "Archive `%s' data is in the future"
+                name)))
+      ;; Test passed, return nil.
+      nil)))
+
+(defun package--check-archive-timestamp (name)
+  "Verify timestamp of \"archive-contents\" file for archive NAME.
+Compare the archive timestamp of a previously downloaded
+\"archive-contents\" file to the timestamp in the current buffer.
+Signal error if the old timestamp is more recent than the new one.
+
+Do nothing if `package-check-timestamp' is nil, if there is no
+previously downloaded file, or if such a file exists but doesn't
+contain a timestamp."
+  (let ((old-file (expand-file-name
+                   (concat "archives/" name "/archive-contents")
+                   package-user-dir)))
+    (when (and package-check-timestamp
+               (file-readable-p old-file))
+      (let ((old (with-temp-buffer
+                   (insert-file-contents old-file)
+                   (package--parse-timestamp-from-buffer name)))
+            (new (package--parse-timestamp-from-buffer name)))
+        (package--compare-archive-timestamps old new name)))))
+
 (defun package--download-one-archive (archive file &optional async)
   "Retrieve an archive file FILE from ARCHIVE, and cache it.
 ARCHIVE should be a cons cell of the form (NAME . LOCATION),
@@ -1825,6 +1900,7 @@ similar to an entry in `package-alist'.  Save the cached 
copy to
            (content (buffer-string))
            (dir (expand-file-name (concat "archives/" name) package-user-dir))
            (local-file (expand-file-name file dir)))
+      (package--check-archive-timestamp name)
       (when (listp (read content))
         (make-directory dir t)
         (if (or (not (package-check-signature))
diff --git 
a/test/lisp/emacs-lisp/package-resources/archives/newer/archive-contents 
b/test/lisp/emacs-lisp/package-resources/archives/newer/archive-contents
new file mode 100644
index 0000000..59a7997
--- /dev/null
+++ b/test/lisp/emacs-lisp/package-resources/archives/newer/archive-contents
@@ -0,0 +1 @@
+;; Last-Updated: 2020-06-01T00:00:00.000Z
diff --git 
a/test/lisp/emacs-lisp/package-resources/archives/older/archive-contents 
b/test/lisp/emacs-lisp/package-resources/archives/older/archive-contents
new file mode 100644
index 0000000..193a6b5
--- /dev/null
+++ b/test/lisp/emacs-lisp/package-resources/archives/older/archive-contents
@@ -0,0 +1 @@
+;; Last-Updated: 2019-01-01T00:00:00.000Z
diff --git a/test/lisp/emacs-lisp/package-tests.el 
b/test/lisp/emacs-lisp/package-tests.el
index f4b7528..e6fb70c 100644
--- a/test/lisp/emacs-lisp/package-tests.el
+++ b/test/lisp/emacs-lisp/package-tests.el
@@ -854,6 +854,61 @@ If the rest succeed, just ignore the unsupported one."
       (insert "7")
       (should-error (package--verify-package-size pkg-desc)))))
 
+(ert-deftest package-tests--parse-timestamp-from-buffer ()
+  (with-temp-buffer
+    (insert ";; Last-Updated: 2020-05-01T15:43:35.000Z\n(foo bar baz)")
+    (should (equal (package--parse-timestamp-from-buffer "foo")
+                   '(24236 17319)))))
+
+(defun package-tests-parse-timestamp (text)
+  (with-temp-buffer
+     (insert text)
+     (package--parse-timestamp-from-buffer "foo")))
+
+(ert-deftest package-test-verify-package-timestamp ()
+  (let ((a (package-tests-parse-timestamp
+            ";; Last-Updated: 2020-05-01T15:43:35.000Z\n"))
+        (b (package-tests-parse-timestamp
+            ";; Last-Updated: 2020-06-01T15:43:35.000Z\n"))
+        (c (package-tests-parse-timestamp
+            ";; Last-Updated: 2020-07-01T15:43:35.000Z\n")))
+    ;; Return nil on success.
+    (should-not (package--compare-archive-timestamps nil b "foo"))
+    (should-not (package--compare-archive-timestamps a b "foo"))
+    (should-not (package--compare-archive-timestamps a c "foo"))
+    (should-not (package--compare-archive-timestamps b c "foo"))
+    ;; Signal error.
+    (should-error (package--compare-archive-timestamps b a "foo"))
+    (should-error (package--compare-archive-timestamps c a "foo"))
+    (should-error (package--compare-archive-timestamps c b "foo"))
+    (should-error (package--compare-archive-timestamps a nil "foo"))))
+
+(ert-deftest package-test-verify-package-timestamp ()
+  (let ((a (package-tests-parse-timestamp
+            ";; Last-Updated: 2020-05-01T15:43:35.000Z\n"))
+        (b (package-tests-parse-timestamp
+            ";; Last-Updated: 2020-06-01T15:43:35.000Z\n"))
+        (c (package-tests-parse-timestamp
+            ";; Last-Updated: 2020-07-01T15:43:35.000Z\n")))
+    ;; Return nil on success.
+    (should-not (package--compare-archive-timestamps nil b "foo"))
+    (should-not (package--compare-archive-timestamps a b "foo"))
+    (should-not (package--compare-archive-timestamps a c "foo"))
+    (should-not (package--compare-archive-timestamps b c "foo"))
+    ;; Signal error.
+    (should-error (package--compare-archive-timestamps b a "foo"))
+    (should-error (package--compare-archive-timestamps c a "foo"))
+    (should-error (package--compare-archive-timestamps c b "foo"))
+    (should-error (package--compare-archive-timestamps a nil "foo"))))
+
+(ert-deftest package-test-check-archive-timestamp ()
+  (let ((package-user-dir package-test-data-dir))
+    (with-temp-buffer
+      (insert ";; Last-Updated: 2020-01-01T00:00:00.000Z\n")
+      (should-not (package--check-archive-timestamp "older"))
+      (should-not (package--check-archive-timestamp "missing"))
+      (should-error (package--check-archive-timestamp "newer")))))
+
 
 ;;; Tests for package-x features.