Re: [edk2-devel] Interpretation of specification

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Wed, 2020-01-08 at 12:24 +0100, Laszlo Ersek wrote:
> (+James)
> 
> On 01/07/20 19:13, Eugene Khoruzhenko wrote:
> > I think I may have found the problem. I can write the
> > file_name.signed created by your scripts in NT32 emulated
> > environment and in EDKII on Minnow board that I build myself.
> > However, I cannot write the file_name.signed on a commercial
> > device. I can write the same authenticate variable with the same
> > Name/GUID and same cert/key on a device when I create the payload
> > in a UEFI Shell app. So the only difference is creating the signed
> > payload by sbvarsign in Ubuntu vs doing it in UEFI. I compared both
> > the working and non-working payloads and the main difference I see
> > is in the timestamp. For some reason sbvarsign writes the Year as
> > 0x0078 (120) vs the UEFI app writing 0x07e4 (2020). The
> > month/day/hour/min seems to be OK, but the year is really off in
> > the sbvarsign's payload. I cannot prove it, but I think the
> > commercial firmware may be having a sanity check for the
> > timestamp date/time, e.g. compare with the device manufacture date.
> > Since sbvarsign does not allow setting a timestamp separately, I
> > cannot force it to create a payload with the correct year.
> 
> That looks like a bug in sbvarsign. In UEFI-2.8, the EFI_TIME
> structure is defined under 8.3 "Time Services" / GetTime(). The Year
> field is given like this:
> 
>   UINT16 Year; // 1900 - 9999
> 
> The comment indicates the valid range. It does not indicate that the
> value stored should be relative to the year 1900 (which is what
> sbvarsign appears to assume; 2020-1900=120).
> 
> The UEFI application likely uses GetTime(), for populating
> "EFI_VARIABLE_AUTHENTICATION_2.TimeStamp" with the current time
> (which is prescribed in 8.2.2 "Using the
> EFI_VARIABLE_AUTHENTICATION_2 descriptor").
> 
> After GetTime() returns successfully, the application may still need
> to manually ensure that "Pad1, Nanosecond, TimeZone, Daylight and
> Pad2" are zeroed. This could involve timezone translation (to the
> required UTC), which in some cases could even imply a change to the
> Year field. But that doesn't change the fact that GetTime() initally
> places an absolute year number in the Year field, and not one
> relative to 1900.
> 
> Let me see the "sbsigntool" source code:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.gi
> t
> 
> (Repository URL via
> <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=920013>.)
> 
> Yup, here's the bug (at current master, that is, commit 0dc3d4b5210d,
> "Fix PE/COFF checksum calculation", 2019-07-27):
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.gi
> t/tree/src/sbvarsign.c?id=0dc3d4b5210dae158651d058f7ac68a9f178ae84#n2
> 15
> 
> Quote:
> 
>    199  static int set_timestamp(EFI_TIME *timestamp)
>    200  {
>    201          struct tm *tm;
>    202          time_t t;
>    203
>    204          time(&t);
>    205
>    206          tm = gmtime(&t);
>    207          if (!tm) {
>    208                  perror("gmtime");
>    209                  return -1;
>    210          }
>    211
>    212          /* copy to our EFI-specific time structure. Other
> fields (Nanosecond,
>    213           * TimeZone, Daylight and Pad) are defined to be zero
> */
>    214          memset(timestamp, 0, sizeof(*timestamp));
>    215          timestamp->Year = tm->tm_year;
>    216          timestamp->Month = tm->tm_mon;
>    217          timestamp->Day = tm->tm_mday;
>    218          timestamp->Hour = tm->tm_hour;
>    219          timestamp->Minute = tm->tm_min;
>    220          timestamp->Second = tm->tm_sec;
>    221
>    222          return 0;
>    223  }
> 
> Please refer to POSIX for time(), gmtime(), and "struct tm":
> 
>   https://pubs.opengroup.org/onlinepubs/9699919799/functions/time.htm
> l
>   https://pubs.opengroup.org/onlinepubs/9699919799/functions/gmtime.h
> tml
>   https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/time.h.ht
> ml
> 
> Quoting the last reference:
> 
> > The <time.h> header shall declare the tm structure, which shall
> > include at least the following members:
> > 
> > int    tm_sec   Seconds [0,60].
> > int    tm_min   Minutes [0,59].
> > int    tm_hour  Hour [0,23].
> > int    tm_mday  Day of month [1,31].
> > int    tm_mon   Month of year [0,11].
> > int    tm_year  Years since 1900.
> > int    tm_wday  Day of week [0,6] (Sunday =0).
> > int    tm_yday  Day of year [0,365].
> > int    tm_isdst Daylight Savings flag.
> 
> That is, field "tm_year" is relative to 1900.
> 
> The bug is therefore in set_timestamp(), on line 215. It should be
> 
>   timestamp->Year = 1900 + tm->tm_year;
> 
> The bug goes back to commit 953b00481f39 ("sbvarsign: First cut of a
> variable-signing tool", 2012-08-02).
> 
> (Otherwise, gmtime() is a good choice, because it gives us UTC at
> once, which is what EFI_VARIABLE_AUTHENTICATION_2 requires, per UEFI
> spec ("GMT").)
> 
> I don't know where sbsigntools development occurs (mailing list, bug
> tracker, ...). For now, I'm CC'ing James. (The git repo lives in his
> space on <git.kernel.org>.)

Heh, that's a sore point: since Jeremy Kerr left ubuntu, no-one except
me has been maintaining it; Jeremy owns the original tree on the Ubuntu
servers but can't update it any more.

I use groups.io for the openssl_tpm2_engine development list, so I
suppose I can set one up for sbsigntools ... OK, done.  If you want to
you can package the above up as a patch and send it there and I'll
apply it.

James

> James, the original thread on edk2-devel is here:
> 
> * [edk2-devel] Interpretation of specification
>   https://edk2.groups.io/g/devel/message/49402
> 
> Thanks,
> Laszlo
>