Cryptlib: Track allocation sizes to fix realloc

[nicholasbishop]

Implementing realloc properly requires knowing the size of the allocation, so that the memory in the old allocation can be copied over to the new allocation. This information isn't provided by UEFI or gnu-efi. Previously the implementation of realloc copied over the new allocation size's number of bytes, which can fail with stricter memory protections.

Fix by expanding each allocation by 8 bytes, and storing the size of the allocation in the first 8 bytes of the allocation. The pointer returned by malloc is then 8 bytes after the actual pool allocation. The realloc and free functions subtract 8 bytes to get the "real" allocation pointer again for passing to the ReallocatePool and FreePool functions. The realloc function reads the original allocation size from the beginning of the allocation so that the correct number of bytes are copied to the new allocation.

Fixes #538

Signed-off-by: Nicholas Bishop nicholasbishop@google.com

[lersek]

AFAICT the shim makefiles pass -std=gnu11, so here you should likely use the flexible array member, which was introduced in C99: data[], rather than the zero-length array GCC extension.

[lersek]

superfluous cast from (void*)

[lersek]

Inserting a space character in a cast expression after (type-name) is a really bad habit of edk2, because the visual separation suggests that cast operators have low precedence (weak binding). In fact, cast operators have one of the strongest precedences in C (one of the tightest bindings).

Furthermore, for consistency's sake with how function calls look (in edk2 and in this patch anyway), a space character should be inserted between the sizeof operator and the (type) that follows it.

[lersek]

Well this is a comment on old code, but anyway. If size_t differs from UINTN, then it's all busted anyway, so the cast is superfluous (both pre- and post-patch).

For example, we expect sizeof to return UINTN, too.

[lersek]

Same comment as above about the superfluous/misleading space after (UINTN), and the missing space after sizeof.

But much more importantly, you are introducing a potential integer overflow here. Assume the caller calculates the size argument to malloc() carefully, so that, even if that argument is ultimately based on external / untrusted data, there is no overflow in the caller. But here you increment that size with a positive integer without pre-checking for UINTN (equiv. size_t) overflow.

If the external / untrusted data source can convince the caller to pass in a large value such as (UINTN)-1 -- again, without any overflow in the caller --, alloc_size here will wrap to sizeof (struct AllocWithSize) - 1, and the AllocatePool() call below will most likely succeed.

Then you get a buffer overflow right after, when you assign alloc->size (assuming the compiler inserts no padding after the size member in the AllocWithSize structure); not to mention the buffer overflow that the caller will perform upon malloc() returning seemingly successfully, by placing up to (UINTN)-1 bytes in alloc->data.

Suggested check:

  UINTN alloc_size;

  if (size > (size_t)-1 - sizeof (struct AllocWithSize)) {
    return NULL;
  }
  alloc_size = size + sizeof (struct AllocWithSize);

Yet another observation is that, for size==0, you will allocate a buffer that can fit just struct AllocWithSize, but no data bytes. That in fact conforms to POSIX:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/malloc.html

and returning the alloc->data flexible array member, automatically cast to a pointer, is also safe per the C99 standard (6.7.2.1 Structure and union specifiers, paragraph 16 -- If this array would have no elements, it behaves as if it had one element but the behavior is undefined if any attempt is made to access that element or to generate a pointer one past it), but it definitely deserves a comment here in the code.

[lersek]

alloc->size is a UINT64 field, but old_size is of type UINTN. Therefore, on 32-bit (IA32), this is a "truncating" conversion (well-defined, yes, but truncating).

Therefore here you should assert that alloc->size <= MAX_UINTN, before the assignment.

For good measure, also assert that alloc->size >= sizeof (struct AllocWithSize)

[lersek]

Well, more of the same overflow and style issues as pointed out for malloc().

[lersek]

same else-after-return issue as with malloc

[lersek]

This is wrong; realloc() may be called with ptr=NULL. In which case it is expected to work like malloc(), for the specified size.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/realloc.html

[lersek]

Two things to consider here.

(1) At this point, we're past the malloc() shortcut (which I pointed out above, for ptr==NULL); that is, we're actually resizing. It's still possible that we're resizing to zero (size==0). In that case, the logic here will preserve a just the "empty" AllocWithSize structure. That actually does conform to POSIX, again:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/realloc.html

and returning the alloc->data flexible array member again conforms to the ISO C99 standard; but again this deserves a comment in the code.

(2) Unfortunately, the other thing to consider is a nasty preexistent bug, inside ReallocatePool().

As of commit 657b248, shim consumes its own fork of gnu-efi (the submodule) at commit 03670e14f263. And at that point, the ReallocatePool function in shim/gnu-efi/lib/misc.c looks like this:

VOID *
ReallocatePool (
    IN VOID                 *OldPool,
    IN UINTN                OldSize,
    IN UINTN                NewSize
    )
{
    VOID                    *NewPool;

    NewPool = NULL;
    if (NewSize) {
        NewPool = AllocatePool (NewSize);
    }

    if (OldPool) {
        if (NewPool) {
            CopyMem (NewPool, OldPool, OldSize < NewSize ? OldSize : NewSize);
        }
    
        FreePool (OldPool);
    }
    
    return NewPool;
}

You do realize that this ReallocatePool() implementation frees OldPool even in case it attempts to, but fails, to allocate NewPool, right? That is, even in case the caller does not intend to use ReallocatePool() in a FreePool() role, but in an actual resizing role. Note that the releasing of OldPool only depends on OldPool being non-NULL on entry.

So, from our realloc() function, we're certainly calling ReallocatePool() with:

• a nonzero old_size (see the assertion I point out above),
• a nonzero new_size,
• and a non-NULL "old pool" pointer (because we're going to handle ptr==NULL on its own separate path, earlier).

That means this ReallocatePool() execution, internal to our realloc() implementation, is an actual resizing operation. And if the new area cannot be allocated, we're going to trash the existent area nevertheless. We're going to propagate NULL out to the caller of realloc(), and they will think that, while realloc() surely failed, they could continue accessing the old (original) allocation. They're going to think that because that's what POSIX guarantees -- but this implementation of ReallocatePool() violates.

In other words, ReallocatePool() itself is buggy.

... It's so very demoralizing that no matter where I look in shim, I run into errors. fallback.c is full of issues (#382), and I've just briefly looked at lib/simple_file.c, and immediately spotted a leak in simple_file_open_by_handle() (namely that of root). There's basically no healthy code to build fixes upon; one doesn't even know where to start contributing patches (only for them to be ignored nearly forever). Oh well.

[nicholasbishop]

Thanks for your detailed review comments. I ended up closing this PR as discussed below. The new PR does not call ReallocatePool anymore.

It's so very demoralizing that no matter where I look in shim, I run into errors.

Well, we just have to fix things one step at a time :) I know it can be frustrating to find errors, but taking a step back I don't think it's fair to stay there's no healthy code to build fixes on. Just because there are additional errors that need to be fixed doesn't mean it's not worth it to go ahead and fix some bugs.

[lersek]

Thanks for the comment. In general I agree with that attitude. It's just that having to fix (say) three bugs as a dependency chain for the one bug you actually want to fix, is frustrating enough in itself. On top, not seeing any maintainer feedback for a good while in the discussion phase, does not evoke a lot of trust that your work, which has just grown unexpectedly in scope, will not be in vain. "One step at a time" is definitely the way to go, but once it grows into "one series at a time", maintainer overload becomes an even worse obstacle, and a strong discouragement. It makes me doubt my effort is worthwhile. But yes, in general you are right.

[lersek]

It looks like github lost a review comment of mine here.

Here I pointed out that this should be formatted as follows -- no else after return please:

  if (alloc) {
    alloc->size = alloc_size;
    return alloc->data;
  }
  return NULL;

[dennis-tseng99]

I merged malloc(), realloc() and free() from https://github.com/tianocore/edk2/blob/master/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c into shim in my local site, and did a testing roughly. It seems working well. Maybe we should align these 3 functions to EDK2 codes.

Here is my understanding about realloc() in the EDK2 codes:

[OldPoolHdr]

<--- CRYPTMEM_OVERHEAD ---><---- OldSize ------>
+-------------------------+--------------------+
|     PoolHdr             | real ptr   ......  |             
+-------------------------+--------------------+
^                         ^
OldPoolHdr               ptr

OldPoolHdr = (CRYPTMEM_HEAD *)ptr - 1;
OldPoolHdr will be freed by FreePool()

[NewPoolHdr]

<-------------------------- NewSize ---------------------------------->
<--- CRYPTMEM_OVERHEAD ---><------------------ size ------------------>
+-------------------------+-------------------------------------------+
|        NewPoolHdr       |             empty space                   |             
+-------------------------+-------------------------------------------+
^                         ^
NewPoolHdr                NewPoolHdr + 1 <--- return vaule

Here is the orginal EDK2 committed patch:

Author: Long Qin <qin.long@intel.com>
Date:   Wed Nov 1 16:10:04 2017 +0800

    CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper
    
    There is one long-standing problem in CRT realloc wrapper, which will
    cause the obvious buffer overflow issue when re-allocating one bigger
    memory block:
        void *realloc (void *ptr, size_t size)
        {
          //
          // BUG: hardcode OldSize == size! We have no any knowledge about
          // memory size of original pointer ptr.
          //
          return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
        }
    This patch introduces one extra header to record the memory buffer size
    information when allocating memory block from malloc routine, and re-wrap
    the realloc() and free() routines to remove this BUG.
    
    Cc: Laszlo Ersek <lersek@redhat.com>
    Cc: Ting Ye <ting.ye@intel.com>
    Cc: Jian J Wang <jian.j.wang@intel.com>
    Contributed-under: TianoCore Contribution Agreement 1.0
    Signed-off-by: Qin Long <qin.long@intel.com>
    Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
    Validated-by: Jian J Wang <jian.j.wang@intel.com>

diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c b/CryptoPkg/
Library/BaseCryptLib/SysCall/BaseMemAllocation.c
index f390e0d449..19c071e2bf 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
@@ -16,6 +16,18 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS
 OR IMPLIED.
 #include <CrtLibSupport.h>
 #include <Library/MemoryAllocationLib.h>
 
+//
+// Extra header to record the memory buffer size from malloc routine.
+//
+#define CRYPTMEM_HEAD_SIGNATURE    SIGNATURE_32('c','m','h','d')
+typedef struct {
+  UINT32    Signature;
+  UINT32    Reserved;
+  UINTN     Size;
+} CRYPTMEM_HEAD;
+
+#define CRYPTMEM_OVERHEAD      sizeof(CRYPTMEM_HEAD)
+  
 //
 // -- Memory-Allocation Routines --
 //
@@ -23,27 +35,84 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRES
S OR IMPLIED.
 /* Allocates memory blocks */
 void *malloc (size_t size)
 {
-  return AllocatePool ((UINTN) size);
+  CRYPTMEM_HEAD  *PoolHdr;
+  UINTN          NewSize;
+  VOID           *Data;
+
+  //
+  // Adjust the size by the buffer header overhead
+  //
+  NewSize = (UINTN)(size) + CRYPTMEM_OVERHEAD;
+
+  Data  = AllocatePool (NewSize);
+  if (Data != NULL) {
+    PoolHdr = (CRYPTMEM_HEAD *)Data;
+    //
+    // Record the memory brief information
+    //
+    PoolHdr->Signature = CRYPTMEM_HEAD_SIGNATURE;
+    PoolHdr->Size      = size;
+
+    return (VOID *)(PoolHdr + 1);
+  } else {
+    //
+    // The buffer allocation failed.
+    //
+    return NULL;
+  }
 }
 
 /* Reallocate memory blocks */
 void *realloc (void *ptr, size_t size)
 {
-  //
-  // BUG: hardcode OldSize == size! We have no any knowledge about
-  // memory size of original pointer ptr.
-  //
-  return ReallocatePool ((UINTN) size, (UINTN) size, ptr);
+  CRYPTMEM_HEAD  *OldPoolHdr;
+  CRYPTMEM_HEAD  *NewPoolHdr;
+  UINTN          OldSize;
+  UINTN          NewSize;
+  VOID           *Data;
+
+  NewSize = (UINTN)size + CRYPTMEM_OVERHEAD;
+  Data = AllocatePool (NewSize);
+  if (Data != NULL) {
+    NewPoolHdr = (CRYPTMEM_HEAD *)Data;
+    NewPoolHdr->Signature = CRYPTMEM_HEAD_SIGNATURE;
+    NewPoolHdr->Size      = size;
+    if (ptr != NULL) {
+      //
+      // Retrieve the original size from the buffer header.
+      //
+      OldPoolHdr = (CRYPTMEM_HEAD *)ptr - 1;
+      ASSERT (OldPoolHdr->Signature == CRYPTMEM_HEAD_SIGNATURE);
+      OldSize = OldPoolHdr->Size;
+
+      //
+      // Duplicate the buffer content.
+      //
+      CopyMem ((VOID *)(NewPoolHdr + 1), ptr, MIN (OldSize, size));
+      FreePool ((VOID *)OldPoolHdr);
+    }
+
+    return (VOID *)(NewPoolHdr + 1);
+  } else {
+    //
+    // The buffer allocation failed.
+    //
+    return NULL;
+  }
 }
 
 /* De-allocates or frees a memory block */
 void free (void *ptr)
 {
+  CRYPTMEM_HEAD  *PoolHdr;
+
   //
   // In Standard C, free() handles a null pointer argument transparently. This
   // is not true of FreePool() below, so protect it.
   //
   if (ptr != NULL) {
-    FreePool (ptr);
+    PoolHdr = (CRYPTMEM_HEAD *)ptr - 1;
+    ASSERT (PoolHdr->Signature == CRYPTMEM_HEAD_SIGNATURE);
+    FreePool (PoolHdr);
   }
 }```

[nicholasbishop]

Thanks @dennis-tseng99, good find. Importing those changes from edk2 is probably the better path here, so I'll close this PR.