[PATCH v1] SecurityPkg: Fix assert when setting key from FAT formatted eMMC/SD/USB

[PATCH v1] SecurityPkg: Fix assert when setting key from FAT formatted eMMC/SD/USB

[Roman Bacik]

When secure boot is enabled, if one loads keys from a FAT formatted
eMMC/SD/USB
when trying to provision PK/KEK/DB keys via the menu, an assert in StrLen()
occurs.
This is because the filename starts on odd address, which is not a uint16
aligned
boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
---
 .../SecureBootConfigFileExplorer.c                   | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
index 1b6f88804275..d5338406957c 100644
---
a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
+++
b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
@@ -123,6 +123,8 @@ OpenFileByDevicePath(
   EFI_FILE_PROTOCOL               *Handle1;
   EFI_FILE_PROTOCOL               *Handle2;
   EFI_HANDLE                      DeviceHandle;
+  CHAR16                          *PathName;
+  UINT16                          PathLength;

   if ((FilePath == NULL || FileHandle == NULL)) {
     return EFI_INVALID_PARAMETER;
@@ -173,6 +175,10 @@ OpenFileByDevicePath(
     //
     Handle2  = Handle1;
     Handle1 = NULL;
+    PathLength = ((FILEPATH_DEVICE_PATH*)*FilePath)->Header.Length[0] |
+                 ((FILEPATH_DEVICE_PATH*)*FilePath)->Header.Length[1] << 8;
+    PathName = AllocateZeroPool (PathLength);
+    CopyMem (PathName, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
PathLength);

     //
     // Try to test opening an existing file
@@ -180,7 +186,7 @@ OpenFileByDevicePath(
     Status = Handle2->Open (
                           Handle2,
                           &Handle1,
-                          ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
+                          PathName,
                           OpenMode &~EFI_FILE_MODE_CREATE,
                           0
                          );
@@ -192,7 +198,7 @@ OpenFileByDevicePath(
       Status = Handle2->Open (
                             Handle2,
                             &Handle1,
-                            ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
+                            PathName,
                             OpenMode,
                             Attributes
                            );
@@ -202,6 +208,8 @@ OpenFileByDevicePath(
     //
     Handle2->Close (Handle2);

+    FreePool (PathName);
+
     if (EFI_ERROR(Status)) {
       return (Status);
     }
-- 
2.17.1

Re: [PATCH v1] SecurityPkg: Fix assert when setting key from FAT formatted eMMC/SD/USB

[Laszlo Ersek]

I'm not officially a reviewer for SecurityPkg, so just some light comments:

(1) Can you send out the patch with git-send-email? Currently it looks
like the patch was pasted into a desktop or web mail client, and that
makes it hard to apply the patch (it's wrapped etc).

On 07/10/18 00:11, Roman Bacik wrote:
> When secure boot is enabled, if one loads keys from a FAT formatted
> eMMC/SD/USB
> when trying to provision PK/KEK/DB keys via the menu, an assert in StrLen()
> occurs.
> This is because the filename starts on odd address, which is not a uint16
> aligned
> boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003
> 
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
> ---
>  .../SecureBootConfigFileExplorer.c                   | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> index 1b6f88804275..d5338406957c 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> @@ -123,6 +123,8 @@ OpenFileByDevicePath(
>    EFI_FILE_PROTOCOL               *Handle1;
>    EFI_FILE_PROTOCOL               *Handle2;
>    EFI_HANDLE                      DeviceHandle;
> +  CHAR16                          *PathName;
> +  UINT16                          PathLength;
> 
>    if ((FilePath == NULL || FileHandle == NULL)) {
>      return EFI_INVALID_PARAMETER;
> @@ -173,6 +175,10 @@ OpenFileByDevicePath(
>      //
>      Handle2  = Handle1;
>      Handle1 = NULL;
> +    PathLength = ((FILEPATH_DEVICE_PATH*)*FilePath)->Header.Length[0] |
> +                 ((FILEPATH_DEVICE_PATH*)*FilePath)->Header.Length[1] << 8;

(2) Can we use DevicePathNodeLength() from
"MdePkg/Include/Library/DevicePathLib.h" here? (For that, we should also
switch PathLength to UINTN.)

This module already depends on the DevicePathLib class.

Apologies that I didn't suggest this in the BZ.

> +    PathName = AllocateZeroPool (PathLength);
> +    CopyMem (PathName, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> PathLength);

(3) I think it's not necessary to zero-fill the buffer, we're going to
overwrite it right after the allocation.

There's a convenience function for that: AllocateCopyPool().

(4) The number of bytes is not correct IMO. "PathLength" stands for the
number of bytes in the entire device path node (FILEPATH_DEVICE_PATH),
including Header and PathName. So, for getting the number of bytes in
just PathName, we should subtract the size of Header.

Presently, we over-read the source buffer; it's not causing problems
because PathName is NUL-terminated.

(5) Can you please check whether the allocation succeeds?

If it fails (PathName == NULL), we should return EFI_OUT_OF_RESOURCES.
It seems OK to return this error from the loop body.

> 
>      //
>      // Try to test opening an existing file
> @@ -180,7 +186,7 @@ OpenFileByDevicePath(
>      Status = Handle2->Open (
>                            Handle2,
>                            &Handle1,
> -                          ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> +                          PathName,
>                            OpenMode &~EFI_FILE_MODE_CREATE,
>                            0
>                           );
> @@ -192,7 +198,7 @@ OpenFileByDevicePath(
>        Status = Handle2->Open (
>                              Handle2,
>                              &Handle1,
> -                            ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> +                            PathName,
>                              OpenMode,
>                              Attributes
>                             );
> @@ -202,6 +208,8 @@ OpenFileByDevicePath(
>      //
>      Handle2->Close (Handle2);
> 
> +    FreePool (PathName);
> +
>      if (EFI_ERROR(Status)) {
>        return (Status);
>      }
> 

Right, this logic appears fine to me; no leaks.

Thanks!
Laszlo

Re: [PATCH v1] SecurityPkg: Fix assert when setting key from FAT formatted eMMC/SD/USB

[Laszlo Ersek]

On 07/10/18 02:02, Laszlo Ersek wrote:
> On 07/10/18 00:11, Roman Bacik wrote:

>> +    PathName = AllocateZeroPool (PathLength);
>> +    CopyMem (PathName, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
>> PathLength);
> 
> (3) I think it's not necessary to zero-fill the buffer, we're going to
> overwrite it right after the allocation.
> 
> There's a convenience function for that: AllocateCopyPool().
> 
> (4) The number of bytes is not correct IMO. "PathLength" stands for the
> number of bytes in the entire device path node (FILEPATH_DEVICE_PATH),
> including Header and PathName. So, for getting the number of bytes in
> just PathName, we should subtract the size of Header.
> 
> Presently, we over-read the source buffer; it's not causing problems
> because PathName is NUL-terminated.

Sorry, I was unclear; I meant there were no *observable* problems. The
over-read of the source buffer results in garbage at the end of the
target buffer, which are later not consumed due to the terminating NUL
appearing earlier. Still, we should not over-read the source buffer.

Thanks
Laszlo

Re: [PATCH v1] SecurityPkg: Fix assert when setting key from FAT formatted eMMC/SD/USB

[Roman Bacik]

Laszlo,

Thank you very much for your comments. I will address them and post another
patch.
Regards,

Roman

On Mon, Jul 9, 2018 at 5:24 PM, Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/10/18 02:02, Laszlo Ersek wrote:
> > On 07/10/18 00:11, Roman Bacik wrote:
>
> >> +    PathName = AllocateZeroPool (PathLength);
> >> +    CopyMem (PathName, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> >> PathLength);
> >
> > (3) I think it's not necessary to zero-fill the buffer, we're going to
> > overwrite it right after the allocation.
> >
> > There's a convenience function for that: AllocateCopyPool().
> >
> > (4) The number of bytes is not correct IMO. "PathLength" stands for the
> > number of bytes in the entire device path node (FILEPATH_DEVICE_PATH),
> > including Header and PathName. So, for getting the number of bytes in
> > just PathName, we should subtract the size of Header.
> >
> > Presently, we over-read the source buffer; it's not causing problems
> > because PathName is NUL-terminated.
>
> Sorry, I was unclear; I meant there were no *observable* problems. The
> over-read of the source buffer results in garbage at the end of the
> target buffer, which are later not consumed due to the terminating NUL
> appearing earlier. Still, we should not over-read the source buffer.
>
> Thanks
> Laszlo
>