GNU bug report logs - #19479
Package manager vulnerable

Previous Next

Full log

Message #131 received at 19479 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 19479 <at> debbugs.gnu.org
Subject: Re: bug#19479: Package manager vulnerable
Date: Sun, 6 Sep 2020 16:59:48 -0700

Noam Postavsky <npostavs <at> gmail.com> writes:

> Stefan Kangas <stefan <at> 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).

Indeed, I fixed that.

>> +  (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?

I'm not sure I understand; it should be a function instead of a variable
because there is logic in there to match `(secure-hash-algorithms)'
against `(package-desc-checksums pkg-desc)' and signal an error.

>> +          (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.

Yes, we currently would place the onus on the package archives to not
use those algorithms.  We could choose to filter them out as completely
unacceptable, I think that makes sense.

>> +                   (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.

Thanks, fixed.

>> +(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).

That's true.  Fixed.

Thanks for the review!

Best regards,
Stefan Kangas

Previous Next

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