On 02/24/16 18:20, Laszlo Ersek wrote:
> On 02/24/16 18:12, Laszlo Ersek wrote:
>
>>> #4 0x000000007fdf0917 in CRYPTO_free (str=0x0)
>>> at CryptoPkg/Library/OpensslLib/openssl-1.0.2f/crypto/mem.c:442
>>> #5 0x000000007fe20b47 in PKCS7_verify (p7=0x7ee6ff98, certs=0x0,
>>> store=0x7ee62e58, indata=0x7ee62c18, out=0x0,
>>> flags=128)
>>> at
>>> CryptoPkg/Library/OpensslLib/openssl-1.0.2f/crypto/pkcs7/pk7_smime.c:415
>
> These are the key stack frames. "pk7_smime.c:415" is an error handling
> section at the end of PKCS7_verify():
>
> err:
> OPENSSL_free(buf);
> if (tmpin == indata) {
> if (indata)
> BIO_pop(p7bio);
> }
> BIO_free_all(p7bio);
> sk_X509_free(signers);
> return ret;
> }
>
> Now, in the edk2 build, OPENSSL_free() boils down to a FreePool().
> However, *unlike* the free() function of the standard C library,
> FreePool() does *not* handle a NULL argument transparently.
>
> So we should look for jumps to the err label in PKCS7_verify() that
> happen before "buf" is set to anything different from NULL. (It is
> initialized to NULL at the top of the function.)
Okay, I found it. There is no error in your patches, just a minimal omission. :)
The above is actually the *entire error*. There is nothing else.
Namely, verifying the "shim.efi" binary takes *two iterations* of the loop(s)
in IsAllowedByDb()
[SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c].
More precisely, two calls to AuthenticodeVerify().
The first call is supposed to fail. The second call is supposed to succeed.
There is a bug in the free() function, in file
"CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c". As I wrote above,
unlike in standard C, this implementation of free() doesn't handle a NULL
argument transparently -- it is passed to FreePool(), but FreePool() blows up
on a NULL argument, in the assertion that I quoted earlier, in file
"MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c":
VOID
EFIAPI
FreePool (
IN VOID *Buffer
)
{
EFI_STATUS Status;
Status = gBS->FreePool (Buffer);
ASSERT_EFI_ERROR (Status);
}
The difference is that this bug in free() -- in the
"CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c" file -- is *only*
exposed by David's patches. Namely, when the first call to AuthenticodeVerify()
fails internally (as expected), we trigger the assertion before we could get
back out to IsAllowedByDb(), for the second call. This is how:
IsAllowedByDb()
[SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c]
//
// first call in the nested loops, supposed to fail:
//
AuthenticodeVerify()
[CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c]
Pkcs7Verify()
[CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c]
PKCS7_verify()
[CryptoPkg/Library/OpensslLib/openssl-1.0.2f/crypto/pkcs7/pk7_smime.c]
//
// we jump to the "err" label with buf == NULL
//
OPENSSL_free(buf)
...
free()
[CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c]
FreePool()
[MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c]
ASSERT_EFI_ERROR()
//
// second call in the nested loops, supposed to succeed -- never reached
because FreePool() never returns!
//
AuthenticodeVerify()
[CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c]
The bug is exposed by the following backport in David's work:
commit e890487ff1ecfaa27d3f035861fba0c5912b8b4b
Author: Rich Salz <[email protected]>
Date: Fri Sep 4 08:13:19 2015 -0400
RT3955: Reduce some stack usage
Use malloc/free instead of big onstack buffers.
Reviewed-by: Tim Hudson <[email protected]>
(cherry picked from commit 8e704858f21983383be2b77e986f475b51719a1e)
It adds an "OPENSSL_free(buf)" call to the error handling / final part of the
PKCS7_verify() function (among other things). This function call can be reached
with (buf == NULL). Which is fully valid, in standard C, but it exposes the bug
in CryptoPkg's free() implementation.
I will send a trivial patch for CryptoPkg in a moment.
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
