decompression weird behavior

[abazhaniuk]

when decode 3440a02.rom check:
3440a02.zip

in 3440a02.rom.dir/1_180000-7FFFFF_BIOS.bin.dir/FV/01_8C8CE578-8A3D-4F1C-9935-896185C32DD3.dir/146_7D113AA9-6280-48C6-BACE-DFE7668E8307.FV_FREEFORM.dir/
After decompression we have:
3440a02.rom.dir/1_180000-7FFFFF_BIOS.bin.dir/FV/01_8C8CE578-8A3D-4F1C-9935-896185C32DD3.dir/146_7D113AA9-6280-48C6-BACE-DFE7668E8307.FV_FREEFORM.dir/00_S_COMPRESSION.dir/00_S_UNKNOWN_97
But when i run again i got different file:
3440a02.rom.dir/1_180000-7FFFFF_BIOS.bin.dir/FV/01_8C8CE578-8A3D-4F1C-9935-896185C32DD3.dir/146_7D113AA9-6280-48C6-BACE-DFE7668E8307.FV_FREEFORM.dir/00_S_COMPRESSION.dir/00_S_UNKNOWN_8D
Content a bit different as well.
when i try to decompress this file by hand i have different file (consistent file)

[abazhaniuk]

if we take a look at:
https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/0.12.433.B62/utility/efidecompress.c

We can see:

// Try new version first
  r = TianoDecompress(ibuf, isize, obuf, osize, sbuf, ssize);
  if (r != EFI_SUCCESS) {}
  // Try old version
  r = EfiDecompress(ibuf, isize, obuf, osize, sbuf, ssize);
  if (r != EFI_SUCCESS) {}

first check if TianoDecompress can decompress and then EfiDecompress, so in our case we need to fix:

elif CompressionType == 0x01:
data = self.decompress_data( [ EFI, Tiano ], CompressedFileData )

to:

elif CompressionType == 0x01:
data = self.decompress_data( [ Tiano, EFI ], CompressedFileData )

but it will case crashes in library on other UEFI images, like
P11-B2.zip

[abazhaniuk]

with file: sect01_yrfa.zip
we have:
$ ./chipsec_tools/linux/TianoCompress.bin -d sect01_yrfa.gz -o out
Segmentation fault (core dumped)
$

[abazhaniuk]

Details about bug:

1. if you check efidecompress (https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/0.12.433.B62/utility/efidecompress.c), you will find how does decompression work, first it call TianoDecompress and if this decompression algorithms is returning error it check second one: EfiDecompression. Other words it check if one cann't decompress call second one. Check code below.

 // Try new version first
  r = TianoDecompress(ibuf, isize, obuf, osize, sbuf, ssize);
  if (r != EFI_SUCCESS) {
    fprintf(stderr, "%s: TianoDecompress failed with code %d\n",
            progname,
            r);
    // Try old version
    r = EfiDecompress(ibuf, isize, obuf, osize, sbuf, ssize);
    if (r != EFI_SUCCESS) {
      fprintf(stderr, "%s: TianoDecompress failed with code %d\n",
              progname,
              r);
      goto done4;
    }
  }

This is first issue in chipsec, we have first check EfiDecompression and then TianoDecompression.
And there is bioses which work not correctly with this logic, like https://github.com/chipsec/chipsec/files/1235509/3440a02.zip
In this case here two bugs related to this behavior:
1.1) first we need to call TianoDecompression first and then EfiDecompression, like:

- data = self.decompress_data( [ EFI, Tiano ], CompressedFileData )

+ data = self.decompress_data( [ Tiano, EFI ], CompressedFileData )

1.2) EfiDecompression should return error, instead of wrong decompressed file. Also this function contain memory leak, which you can detect by investigation decompressed buffer.

after i apply this fix, i got new bug with bios image: https://github.com/chipsec/chipsec/files/1252866/P11-B2.zip
this is crashing system in integer overflow (next bug, #2 in my list)

2. (when you fix previous) you will find bug when you use TianoDecompression for some bioses (https://github.com/chipsec/chipsec/files/1252866/P11-B2.zip) you will get integer overflow condition (cause crash) in Decode() at chipsec_tools/compression/Tiano/Decompress.c when DataIdx will be large number:

      while ((INT16) (BytesRemain) >= 0) {
        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
        if (Sd->mOutBuf >= Sd->mOrigSize) {
          return ;
        }

and it has condition to check this integer overflow:

      // If this is not the correct decompression algorithm, this is an overflow possibility.
      if (DataIdx > Sd->mOrigSize) {
        return ;
      }

Which is not fixing bug in case of: https://github.com/chipsec/chipsec/files/1252866/P11-B2.zip and file: https://github.com/chipsec/chipsec/files/1253278/sect01_yrfa.zip

I think better fix is:

  UINT16  mPTTable[256];
+ UINT16  mErro;
} SCRATCH_DATA;
...

       // If this is not the correct decompression algorithm, this is an overflow possibility.
      if (DataIdx > Sd->mOrigSize) {
        return ;
 +     mErro = 1;
     }
...
- if (Sd->mBadTableFlag != 0) {
+ if (Sd->mBadTableFlag != 0 || Sd->mErro != 0) {

When we add mErro and check if Decode() set it, then we return EFI_INVALID_PARAMETER

but this fix not correct because it is breaking decompression with Type 2 (LZMA, which is returning error and calling efi decompression for this binary as a fallback algorithm (will provide bios example, if necessary) , check code:

data = self.decompress_data( [ LZMA, Tiano, EFI ] , CompressedFileData )

3. (when you fix previous) You got other bugs in: https://github.com/chipsec/chipsec/blob/master/chipsec_tools/compression/EfiCompressor.c at function Extract
   In this function we don't have checks for Scratch, Destination pointers and reasonable *DstSize number.
   (also may crash app)

4. (last one) use-after-free (or something like this).
   when you apply all previous fixes then at some samples you will get python crash (when it finishing python process) and it seems like it is use-after-free. you can test it with: https://github.com/chipsec/chipsec/files/1252866/P11-B2.zip (also similar bug i say in (with similar fixes) https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/0.12.433.B62/utility/efidecompress.c )
   It may not be a bug, it can be just really bad fix for previous bugs.

[skochinsky]

AFAIK both EFI and Tiano algorithms descend from LZH (used in old Award BIOSes; there was also Phoenix's variation called LZINT), just with different parameters such as number of bits to encode the offsets and so on.
I think it should be possible to create a generalized decompressor that would detect the correct parameters at runtime to achieve successful decompression. In fact, I just found https://github.com/tianocore/edk2/blob/master/BaseTools/Source/C/Common/Decompress.c which implements both in the same file, but you do need to know beforehand which one to call...

[0xfede7c8]

This is fixed already.

[theopolis]

Nice find!