sbsigntool fix (was Re: [PATCH] Fix PE/COFF checksum calculation)

[James Bottomley]

On Sat, 2019-07-27 at 23:09 +0100, Steve McIntyre wrote:
> On Sat, Jul 27, 2019 at 02:40:08PM -0700, James Bottomley wrote:
> > On Sat, 2019-07-27 at 00:23 +0100, Steve McIntyre wrote:
> 
> ...
> 
> > > In the Debian shim-signed package build, we use sbattach to
> > > remove
> > > then re-attach the Microsoft signature on the binary that they
> > > signed
> > > for us, to validate 100% that there are no changes. This code was
> > > even
> > > written by Steve Langasek! See
> > > 
> > >   https://salsa.debian.org/efi-team/shim-signed/blob/master/Makef
> > > ile
> > > 
> > > for what we're doing.
> > > 
> > > We used to use sbsigntool 0.6 and this worked fine. It broke
> > > after we
> > > updated to 0.9.2 in Debian - I could not get the checksums to
> > > match
> > > again, so I worked through the code and found what looked like a
> > > bug. Checking the log on that commit, I also see:
> > > 
> > >     [jejb: add endian to autogen.sh and fix for multi-sign]
> > 
> > OK, this might be it.  Before the addition of multiple signatures,
> > the
> > sigbuf only contained the signature, not the struct
> > _WINH_CERTIFICATE
> > header, which would mean if the checksum is supposed to be updated
> > over
> > the entire certificate table then we'd have to sum the header and
> > the
> > signature separately.  After the multiple certificate support,
> > there's
> > not much difference between cert_table and sigbuf because now they
> > always both contain the header and signature otherwise the code
> > couldn't iterate over multiple signatures (it needs the information
> > from the header to say where the next signature begins).
> > 
> > So I think this means your patch is correct because the header(s)
> > are
> > now included in the sum over sigbuf.
> 
> ACK.
> 
> If you're going to take the patch, could I also give you a friendly
> prod to update the README too? :-)

Well, the README is simple historical information and Canonical hasn't
taken down the historical repository (yes, I suppose).

> Also, for information in case you're interested - the efi list we've
> been including in CC is a new list I created quite recently, to try
> and host some shared discussion and patch reviews for the EFI
> userland
> tools. We have a few people signed up so far (Peter, Ard, etc.).
> Please feel free to join us!
> 
>   https://lists.einval.com/cgi-bin/mailman/listinfo/efi

Sure, although I'll probably just read it over the archive interface.

Regards,

James