From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Kangas Newsgroups: gmane.emacs.bugs Subject: bug#19479: Package manager vulnerable Date: Sun, 6 Sep 2020 16:59:48 -0700 Message-ID: References: <87k11pnap2.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="31071"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 19479@debbugs.gnu.org To: Noam Postavsky Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Sep 07 02:00:12 2020 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kF4Zw-0007zt-0d for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 07 Sep 2020 02:00:12 +0200 Original-Received: from localhost ([::1]:40546 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kF4Zu-0008TZ-RE for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 06 Sep 2020 20:00:10 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:44916) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kF4Zn-0008Sw-53 for bug-gnu-emacs@gnu.org; Sun, 06 Sep 2020 20:00:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:35896) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kF4Zm-0003lt-Pe for bug-gnu-emacs@gnu.org; Sun, 06 Sep 2020 20:00:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1kF4Zm-0000Y6-OV for bug-gnu-emacs@gnu.org; Sun, 06 Sep 2020 20:00:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Kangas Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 07 Sep 2020 00:00: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.15994368012079 (code B ref 19479); Mon, 07 Sep 2020 00:00:02 +0000 Original-Received: (at 19479) by debbugs.gnu.org; 7 Sep 2020 00:00:01 +0000 Original-Received: from localhost ([127.0.0.1]:47442 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kF4Zk-0000XS-So for submit@debbugs.gnu.org; Sun, 06 Sep 2020 20:00:01 -0400 Original-Received: from mail-ej1-f51.google.com ([209.85.218.51]:41185) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kF4Zf-0000Wh-Dy for 19479@debbugs.gnu.org; Sun, 06 Sep 2020 19:59:55 -0400 Original-Received: by mail-ej1-f51.google.com with SMTP id lo4so15844971ejb.8 for <19479@debbugs.gnu.org>; Sun, 06 Sep 2020 16:59:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:in-reply-to:references:mime-version:date :message-id:subject:to:cc; bh=yB/bcYPkgXxAmF9r4TBAuyOBGvRVcw39BszSYHcCRXk=; b=o6oXuf9KtTQthF7WMl5+JqAow6XqIiF57jgfk4QtTTLhnxnExrM3Xn+dfGrg5VFXj3 iZRJqEH/KWN1vqt0l+kTNMLvQwp/dfIKXfXq+XMfUwytI8RsMAGJtGRcu1HPC/rmPg7d 5Xu2KPUIlFRE+vt0misQGkfVqyaCiGI8GVqHB54sXkEWTTvlRaAOXnYNmOLtUssTQdh4 M2N1E8zj90c8P/jj6cELtKEb109A4xm0vhiC+GJ0d5sPSSy3lzOW5Ea9nnCu9EIYW9si Q0kVo/JvjZmVpGPILFoW9EGM1NAiwHhShqukD8cyhbXQBcAThKTp0aws+5ukM8m0vy32 1FGg== X-Gm-Message-State: AOAM531JkL9sPO+dkwkTyyukBImujr0HTDRT3W5c6MPRtYKrZ6Zzyh4a RAc7oJHAFYFlJlGyIeM/1Xdr70m6nJSK23Xyktc= X-Google-Smtp-Source: ABdhPJwgXKiuaxSRDCCXdWBoV5Qh4dT2mjM1lrnF4hzs/imCspNbNGBsJHb1Lr2+tUmonluZxpTgVUoAwp2O+Xmj2vg= X-Received: by 2002:a17:906:bb0e:: with SMTP id jz14mr19290279ejb.525.1599436789578; Sun, 06 Sep 2020 16:59:49 -0700 (PDT) Original-Received: from 753933720722 named unknown by gmailapi.google.com with HTTPREST; Sun, 6 Sep 2020 16:59:48 -0700 In-Reply-To: <87k11pnap2.fsf@gmail.com> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list 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-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:187403 Archived-At: Noam Postavsky writes: > Stefan Kangas 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