Re: [edk2-devel] Interpretation of specification

["Laszlo Ersek" <lersek@redhat.com>]

(+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.git

(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.git/tree/src/sbvarsign.c?id=0dc3d4b5210dae158651d058f7ac68a9f178ae84#n215

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.html
  https://pubs.opengroup.org/onlinepubs/9699919799/functions/gmtime.html
  https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/time.h.html

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>.)

James, the original thread on edk2-devel is here:

* [edk2-devel] Interpretation of specification
  https://edk2.groups.io/g/devel/message/49402

Thanks,
Laszlo