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

[Laszlo Ersek <lersek@redhat.com>]

Hi Roman,

On 07/11/18 00:51, rbacik@gmail.com wrote:
> From: Roman Bacik <roman.bacik@broadcom.com>
> 
> 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>
> ---
>  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

Thank you for sending a well-formed patch.

I notice that you sent this email from <rbacik@gmail.com>, which is not
the same as the Signed-off-by line. I realize you posted from
<rbacik@gmail.com> for technical reasons, and it should be no problem.

However, I *think* in such cases we usually request the following:

- Using your broadcom.com email address, please respond to this patch
(not my present email, but your original git posting), keeping full
context, and just repeat your Signed-off-by line (referencing the
broadcom address).

I'm CC'ing Jordan and Ard for confirmation -- I believe this is what
we've done in the past, in cases when submitters had to post their work
from private addresses due to company email issues.

Technical comments below:

> diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> index 1b6f88804275..19b13a5569a6 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;
> +  UINTN                           PathLength;
>  
>    if ((FilePath == NULL || FileHandle == NULL)) {
>      return EFI_INVALID_PARAMETER;
> @@ -173,6 +175,11 @@ OpenFileByDevicePath(
>      //
>      Handle2  = Handle1;
>      Handle1 = NULL;
> +    PathLength = DevicePathNodeLength(*FilePath) - sizeof(EFI_DEVICE_PATH_PROTOCOL);
> +    PathName = AllocateCopyPool(PathLength, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName);

(1) On both lines above, space characters are missing after:
DevicePathNodeLength, sizeof, and AllocateCopyPool. (Edk2 coding style.)
I think we can fix this up for you when we push the patch. (I'm willing
to help with that, but we need SecurityPkg maintainer review first.)


> +    if (PathName == NULL) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }

(2) I have now reviewed the original state of the function more
carefully, and, while the above "return" branch introduces a leak
*path*, it does not introduce a leak that doesn't already exist!

In fact, the original function has multiple issues:

- If the OpenVolume() call fails, "FileHandle" is set to NULL. That's
useless; the intent is obviously to set (*FileHandle) to NULL.

- At the top of the "while" loop body, "Handle1" stands for an open
EFI_FILE_PROTOCOL. If the device path type check at the top of the loop
body returns EFI_INVALID_PARAMETER, then it (a) performs the same
useless assignment to "FileHandle" as described above, and (b) fails to
close "Handle1". This is why I say that the above EFI_OUT_OF_RESOURCES
branch introduces no new leak, just a new path to the existent leak.

- The OpenFileByDevicePath() function is duplicated in the following
modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and
"MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c". With the
implication that the alignment issue you found affects all three drivers!


Roman, I realize this could be more than what you signed up for; so
please pick one:

(2a) you could submit a patch series:

* Write a patch that sets (*FilePath) to NULL right after the
(FileHandle==NULL) check, in preparation for failure, and removes all
the bogus FileHandle=NULL assignments.

* Write another patch that plugs the leak when the device path type
check fails -- introduce a "CloseHandle1" label at the end of the
function, and jump to it when the devpath type check fails, so that we
close "Handle1". This patch should also invert the meanings of Handle2
and Handle1 -- the reassignment to Handle1 should only occur *after* we
successfully open Handle2. "Handle1" should *always* remain suitable for
closing through the "CloseHandle1" error path.

* Include your current patch, for fixing the alignment issue.

* Write another patch that moves the OpenFileByDevicePath() function to
UefiLib in MdePkg -- under the name EfiOpenFileByDevicePath() -- from
SecureBootConfigDxe.

* write two more patches, namely for TlsAuthConfigDxe and RamDiskDxe, in
order to consume EfiOpenFileByDevicePath() from UefiLib. Both of those
modules already depend on UefiLib.

(2b) Alternatively:

* we can report a new TianoCore BZ about the issues I list above,

* we can commit this patch of yours as-is, just additionally reference
the *new* BZ in the commit message, as "further known issues",

* I can work on the rest of the issues.


If you pick (2b), then I can
- file the new BZ,
- update the commit message for you,
- update the patch for you, as described in (1),
- ACK this patch (as updated above),
- push the patch (if SecurityPkg maintainers agree),
- take on the new BZ as well.

Thanks!
Laszlo

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