bug#19479: Package manager vulnerable

[Noam Postavsky <npostavs@gmail.com>]

Stefan Kangas <stefan@marxist.se> writes:

> Subject: [PATCH] Support package checksum verification
>
> This is the first step towards protecting users of package.el against
> metadata replay attacks.

> +(define-error 'bad-checksum "Failed to verify checksum")

Would it be useful to have bad-signature and this one share a parent?
(by the way, I kind of wonder why it's not called
package-bad-signature).

> +  (cl-flet*
> +      ((supported-hashes
> +        (lambda ()

Is this a function (rather than a variable) just so it can be in the
same cl-flet* as do-check?

> +          (or (seq-filter (lambda (h) (memql (car h) (secure-hash-algorithms)))

The list returned by secure-hash-algorithms includes sha1 and md5.  This
is a problem if we're going to rely on signing them.  We should at least
plan to have some way of filtering out some functions.

> +                   (a (cdr hash))
> +                   (b (secure-hash algorithm (current-buffer))))

> +  (when-let ((a (package-desc-size pkg-desc))
> +             (b (string-bytes (buffer-string))))

I risk descending into trivial nitpicking, but I think 'a' and 'b' are
bit too generic.  Something like 'expected' and 'actual' would make it
harder to mix them up.

> +(defmacro run-verify-checksums-test (verify-checksums checksums)
> +  "Run a test for `package-verify-checksums'."

> +(ert-deftest package-test--verify-package-checksums-nil/ignore-invalid ()

I think run-verify-checksums-test should be prefixed with package-test
(whereas the individual test names could be prefixed with just package).