Regression causing invalid .cab files after adding signature

[mcb30]

osslsigncode sign will currently generate an invalid .cab file. This can be observed by using e.g.:

osslsigncode sign -h sha256 -time 1693397653 -certs test.crt -key test.key -in unsigned.cab -out signed.cab

cabextract --test signed.cab

With current master (commit 1fc2c937f25508bba5b8040640502e9342a56d4d) this will produce output such as:

$ cabextract --test signed.cab
signed.cab: WARNING; possible 1792 extra bytes at end of file.
Testing cabinet: signed.cab
  wimboot.i386.efi  failed (checksum error)
  wimboot.x86_64.efi  failed (error in CAB data format)

All done, errors in processing 2 file(s)

rather than the expected

$ cabextract --test signed.cab
signed.cab: WARNING; possible 1792 extra bytes at end of file.
Testing cabinet: signed.cab
  wimboot.i386.efi  OK                         8d864de06286e591eacf5f3dd4b3db69
  wimboot.x86_64.efi  OK                       9d8d96422d645e9e5ca3bbdd5f8bffb4

All done, no errors.

Bisection reveals that the first failing commit is 0f51a06b8f4a99bf9dad785d7f26c75daf5234e3, which performed a substantial amount of refactoring.

Inspection of the output .cab files shows that the first suspicious value is within the CFFOLDER.coffCabStart field. Manually editing the value in this field results in a .cab file that passes the cabextract --test checks (although obviously with an invalid signature, since the content has been edited after signing).

Comparison of cab_add_header() before and after commit 0f51a06b8f4a99bf9dad785d7f26c75daf5234e3 shows that one line of code seems to have been accidentally deleted instead of updated:

-       nfolders = GET_UINT16_LE(indata + 26);
+       nfolders = GET_UINT16_LE(ctx->options->indata + 26);
        while (nfolders) {
-               tmp = GET_UINT32_LE(indata + i);
                tmp += 24;
                PUT_UINT32_LE(tmp, buf);
                BIO_write(hash, buf, 4);
-               BIO_write(hash, indata + i + 4, 4);
+               BIO_write(hash, ctx->options->indata + i + 4, 4);
                i += 8;
               nfolders--;
        }

Note that there is no replacement for the line:

-               tmp = GET_UINT32_LE(indata + i);

This results in the variable tmp effectively containing uninitialised data. (This is not picked up as a compiler error, since the variable tmp is also used earlier in the same function.)

Adding in the expected change:

-               tmp = GET_UINT32_LE(indata + i);
+               tmp = GET_UINT32_LE(ctx->options->indata + i);

results in a valid .cab file that is accepted by cabextract --test (and is byte-for-byte identical to that produced using the commit immediately prior to the commit 0f51a06b8f4a99bf9dad785d7f26c75daf5234e3 that introduced the bug).

[mcb30]

@mtrojnar Thanks for merging so quickly! I need to update the documentation at https://ipxe.org/appnote/etoken to state that osslsigncode version 2.6 cannot be used due to this bug. Is there an expected release date for version 2.7?

[mtrojnar]

version 2.6 cannot be used

You mean "cannot be used with cab files", right? It's not necessarily the most popular of the supported file formats.

Is there an expected release date for version 2.7?

End of September.

[mcb30]

version 2.6 cannot be used

You mean "cannot be used with cab files", right? It's not necessarily the most popular of the supported file formats.

Sorry, I should have been more specific. I meant "cannot be used for signing UEFI Secure Boot submissions" (which have to be submitted as signed .cab files).

Is there an expected release date for version 2.7?

End of September.

That's great, thanks!