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

[James Bottomley]

On Sat, 2019-07-27 at 00:23 +0100, Steve McIntyre wrote:
> Hi James!
> 
> [ Adding Steve to CC for info too ]
> 
> Thanks for getting back to me.
> 
> On Fri, Jul 26, 2019 at 03:23:57PM -0700, James Bottomley wrote:
> > On Thu, 2019-06-13 at 16:57 +0100, Steve McIntyre wrote:
> > > On Thu, Jun 13, 2019 at 08:54:21AM -0700, James Bottomley wrote:
> > > > On Thu, 2019-06-13 at 13:53 +0100, Steve McIntyre wrote:
> > > > > Sharing with others too. No idea if James is having mail
> > > > > problems
> > > > > or
> > > > > something...
> > > > 
> > > > Sorry, no, sbsigntool has been somewhat low on my list of
> > > > things to
> > > > look after for a while (I was hoping after engine support it
> > > > would
> > > > just
> > > > be complete).
> > > > 
> > > > Let me actually dust off the git tree and have a look at the
> > > > problem.
> > > 
> > > ACK, thanks!
> > 
> > OK, I looked at this but there's no way of checking or validating
> > the
> > change: the PECOFF checksum value is defined helpfully as 'whatever
> > comes out of IMAGHELP.DLL'.  For EFI binaries this means it could
> > be
> > any random number and we never check it so originally we did just
> > that
> > (or more accurately assigned it to zero).  Steve Langasek later
> > patched
> > the tools to an algorithm he claimed he'd verified with
> > IMAGHELP.DLL,
> > in this commit:
> > 
> > commit be1f3d8350c6d86fa5fd36bd22c94bf86e106dbb
> > Author: Steve Langasek <steve.langasek at canonical.com>
> > Date:   Wed Jan 27 11:06:02 2016 -0800
> > 
> >    Update the PE checksum field using the somewhat-underdocumented
> >    algorithm, so that we match the Microsoft implementation in our
> >    signature generation.
> > 
> > You're saying what he did was wrong and thus shouldn't have matched
> > the
> > Microsoft tools.  How have you verified this?
> 
> I have *not* directly run anything against IMAGHELP.DLL, but what
> prompted me to work on this was output from Microsoft themselves.
> 
> 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/Makefile
> 
> 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.

James


> and I'm thinking this maybe looks like a bug in the multi-sign
> implementation. Can you (plural) shed some light on that maybe?
> 
> To explain for Steve's benefit too, my change at
> 
>   https://salsa.debian.org/efi-team/sbsigntool/blob/master/debian/pat
> ches/fix_checksum_calc.patch
> 
> made things interoperate again. We've since had another set of
> MS-signed binaries come back, and the checksums also all worked on
> that with no more changes. I think it's fair to say things are
> working
> as they should with my change included.