Re: Patch attestation RFC + proof of concept

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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@xxxxxxxxxx

I suppose you have in mind WKD instead of the now-DoS-broken key
> $ 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@xxxxxxxxxx
>   (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@xxxxxxxxxx> (pgp:8972F4DFDC6DC026)
>   Attestation-verified: Konstantin Ryabitsev <konstantin@xxxxxxxxxxxxxxxxxxx>

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?


[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux