Workflows Archive on lore.kernel.org
 help / color / Atom feed
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: workflows@vger.kernel.org
Cc: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
Subject: Re: Patch attestation RFC + proof of concept
Date: Thu, 27 Feb 2020 12:11:44 +0800
Message-ID: <20200227041144.GA36493@zx2c4.com> (raw)
In-Reply-To: <20200226172502.q3fl67ealxsonfgp@chatter.i7.local>

Hi Konstantin,

A few comments on the proposal, inline below:

On Wed, Feb 26, 2020 at 12:25:02PM -0500, Konstantin Ryabitsev wrote:

> 4. gpg --locate-keys kees@kernel.org

I suppose you have in mind WKD instead of the now-DoS-broken key
servers?
 
> $ egrep '^(Author|Email|Subject)' i | sha256sum
> 2a02abe02216f626105622aee2f26ab10c155b6442e23441d90fc5fe4071b86e  -

What if there are multiple such lines? What if there's
Subject-justkidding? That grep doesn't use a colon in it. Are there
other malleability issues to account for here?

> To calculate the "p" hash, we first need to remove any surrounding junk 
> that isn't just the patch itself.

This has to be done extremely carefully, and it's probably worth reading
the source of git apply and gnu patch and such to find any edge cases.
Again, potential malleability issues which could be exploitable if not
done carefully.

> We then take the first 8 characters of the i-m-p hash to create the
> attestation-ID: 2a02abe0-215cf3f1-2acb5798.

It's now easy to forge collisions and complicate the lookup process by
forcing clients to do lots of trial hashing and lore fetching. Remember:
anybody can submit to the signatures list. If you want an attestation ID
that uses each of the three hashes, hash the hashes together instead of
truncating and concatenating them.


Shouldn't this step:

> - the developer executes "git-send-email"

be after these steps:

> - the developer runs "attest-patches -a *.patches"
> - the developer sends attestation.eml to signatures@kernel.org
>   (or the tool auto-POSTs it to the submission URL, as mentioned)

? Otherwise there's a small race window.


> - if attestation passes, get-lore-mbox adds two trailers to each patch: 
>   "Attestation-by:" and "Attestation-verified:". In our example case 
>   those are:
>   Attestation-by: Kees Cook <kees@kernel.org> (pgp:8972F4DFDC6DC026)
>   Attestation-verified: Konstantin Ryabitsev <konstantin@linuxfoundation.org>

What do these lines actually add that is useful? The Signed-off-by is
already a non-cryptographic trail of which maintainers patches flow
through. These two add another, also non-cryptographic, summary. If
they're forgible, then what's the point? Seems like clutter. If
anything, these signatures help maintainers verify that the sender
actually sent it before they merge it into their trees. But after that,
those commit messages could contain anything, including bogus
"Attestation-by" lines. It seems like it's basically just tinfoil virtue
signaling at that point? Ostensibly the maintainers should also be
reading the patches for content anyway...

> Okay, what do you all think? I believe this scheme has the following 
> merits:
> Downsides:
> 
> - we aren't solving the problem of delegated trust, which will continue 
>   to be the hardest part behind any distributed development effort
> 

Another odd quirk worth considering: vanilla patches aren't tied to a
specific commit base, so there could be a "replay attack" where an
attacker resends an old patch that still applies without issue, but
means a different thing in the present state of the tree. For example,
Alice sends patch P in November 2019. Bob discovers it causes a remotely
exploitable vulnerability in December 2019 and submits a revert patch.
The seasons change a few times, and it's now March 2025, maintainers
have changed a bit, but the code is still mostly the same. Eve resubmits
P which has Alice's name on it. Signature verifies. Doom ensues. Real
git pgp signing involves a signature over the whole object, which
contains the hash of the parent, which avoids this issue.

General reflection: this is solving a odd "problem". As mentioned above,
maintainers should be reading commits as they come in on the mailing
list. There might be a practical argument to be made, though, that
sometimes they trust things on face value, even emailed commits, and so
this gives some extra assurance. I also wonder if this adds even more
overhead to what hipsters are already decrying as an overly clunky
contribution process. Even more generally, is the lack of signatures on
email patches a real problem we're facing?

Regards,
Jason

  parent reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-26 17:25 Konstantin Ryabitsev
2020-02-26 17:50 ` Kees Cook
2020-02-26 18:47   ` Konstantin Ryabitsev
2020-02-26 20:11 ` Jason Gunthorpe
2020-02-26 20:42   ` Konstantin Ryabitsev
2020-02-26 21:04     ` Jason Gunthorpe
2020-02-26 21:18       ` Konstantin Ryabitsev
2020-02-27  1:23         ` Jason Gunthorpe
2020-02-27  4:11 ` Jason A. Donenfeld [this message]
2020-02-27 10:05   ` Geert Uytterhoeven
2020-02-27 13:30     ` Jason A. Donenfeld
2020-02-27 14:29   ` Konstantin Ryabitsev
2020-02-28  1:57     ` Jason A. Donenfeld
2020-02-28  2:30       ` Jason A. Donenfeld
2020-02-28 18:33         ` Konstantin Ryabitsev
2020-02-28 17:54       ` Konstantin Ryabitsev
2020-03-06 16:53       ` Geert Uytterhoeven

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200227041144.GA36493@zx2c4.com \
    --to=jason@zx2c4.com \
    --cc=konstantin@linuxfoundation.org \
    --cc=workflows@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Workflows Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/workflows/0 workflows/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 workflows workflows/ https://lore.kernel.org/workflows \
		workflows@vger.kernel.org
	public-inbox-index workflows

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.workflows


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git