* [PATCH 1/1] SecurityPkg: Fix incorrect return value in documentation @ 2020-02-04 22:26 Philippe Mathieu-Daudé 2020-02-04 23:28 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 3+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-04 22:26 UTC (permalink / raw) To: devel; +Cc: Philippe Mathieu-Daude, Jiewen Yao, Jian J Wang, Chao Zhang The DxeTpmMeasureBootHandler and DxeTpm2MeasureBootHandler handlers are SECURITY2_FILE_AUTHENTICATION_HANDLER prototype. This prototype can not return EFI_INVALID_PARAMETER. The prototype documentation states it returns EFI_ACCESS_DENIED if: "The file specified by File and FileBuffer did not authenticate, and the platform policy dictates that the DXE Foundation may not use File." Note in practice both functions return EFI_SECURITY_VIOLATION: "The file specified by DevicePath and FileBuffer did not authenticate, and the platform policy dictates that the file should be placed in the untrusted state. The image has been added to the file execution table." Noticed while reviewing commit 6d57592740cdd0b6868baeef7929d6e6fef. Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Chao Zhang <chao.b.zhang@intel.com> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com> --- .../Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c | 2 +- SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c index 04b9b0d7fbf3..0c07f65c6835 100644 --- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c +++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c @@ -384,7 +384,7 @@ Finish: and other exception operations. The File parameter allows for possible logging within the SAP of the driver. - If File is NULL, then EFI_INVALID_PARAMETER is returned. + If File is NULL, then EFI_ACCESS_DENIED is returned. If the file specified by File with an authentication status specified by AuthenticationStatus is safe for the DXE Core to use, then EFI_SUCCESS is returned. diff --git a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c index 1f2eed29a1df..0dccbb66eb26 100644 --- a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c +++ b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c @@ -678,7 +678,7 @@ Finish: and other exception operations. The File parameter allows for possible logging within the SAP of the driver. - If File is NULL, then EFI_INVALID_PARAMETER is returned. + If File is NULL, then EFI_ACCESS_DENIED is returned. If the file specified by File with an authentication status specified by AuthenticationStatus is safe for the DXE Core to use, then EFI_SUCCESS is returned. -- 2.21.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] SecurityPkg: Fix incorrect return value in documentation 2020-02-04 22:26 [PATCH 1/1] SecurityPkg: Fix incorrect return value in documentation Philippe Mathieu-Daudé @ 2020-02-04 23:28 ` Laszlo Ersek 2020-02-05 14:18 ` Wang, Jian J 0 siblings, 1 reply; 3+ messages in thread From: Laszlo Ersek @ 2020-02-04 23:28 UTC (permalink / raw) To: devel, philmd; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang Hi Phil, On 02/04/20 23:26, Philippe Mathieu-Daudé wrote: > The DxeTpmMeasureBootHandler and DxeTpm2MeasureBootHandler handlers > are SECURITY2_FILE_AUTHENTICATION_HANDLER prototype. This prototype > can not return EFI_INVALID_PARAMETER. > > The prototype documentation states it returns EFI_ACCESS_DENIED if: > > "The file specified by File and FileBuffer did not authenticate, > and the platform policy dictates that the DXE Foundation may not > use File." > > Note in practice both functions return EFI_SECURITY_VIOLATION: > > "The file specified by DevicePath and FileBuffer did not > authenticate, and the platform policy dictates that the file > should be placed in the untrusted state. The image has been > added to the file execution table." > > Noticed while reviewing commit 6d57592740cdd0b6868baeef7929d6e6fef. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Chao Zhang <chao.b.zhang@intel.com> > Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com> > --- > .../Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c | 2 +- > SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > index 04b9b0d7fbf3..0c07f65c6835 100644 > --- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > +++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > @@ -384,7 +384,7 @@ Finish: > and other exception operations. The File parameter allows for possible logging > within the SAP of the driver. > > - If File is NULL, then EFI_INVALID_PARAMETER is returned. > + If File is NULL, then EFI_ACCESS_DENIED is returned. > > If the file specified by File with an authentication status specified by > AuthenticationStatus is safe for the DXE Core to use, then EFI_SUCCESS is returned. I've tried seeing where this code actually returns EFI_INVALID_PARAMETER, but I can't find it. If File is NULL in DxeTpm2MeasureBootHandler(), then first we seem to do: OrigDevicePathNode = DuplicateDevicePath (File); which I believe will produce a NULL. Then we seem to pass this NULL around a little bit, so I think there's a fair chance of crashing there. I wonder if this code should be fixed, to check "File" in the first place, and then return EFI_ACCESS_DENIED. There are also some other places in the function that could apparently return a wide array of error codes -- FvbProtocol->GetPhysicalAddress() (EFI_UNSUPPORTED?), PeCoffLoaderGetImageInfo(), etc. I do agree this patch is an improvement, however; at least it says what *should* be returned. So with that in mind: Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo > diff --git a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > index 1f2eed29a1df..0dccbb66eb26 100644 > --- a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > +++ b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > @@ -678,7 +678,7 @@ Finish: > and other exception operations. The File parameter allows for possible logging > within the SAP of the driver. > > - If File is NULL, then EFI_INVALID_PARAMETER is returned. > + If File is NULL, then EFI_ACCESS_DENIED is returned. > > If the file specified by File with an authentication status specified by > AuthenticationStatus is safe for the DXE Core to use, then EFI_SUCCESS is returned. > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] SecurityPkg: Fix incorrect return value in documentation 2020-02-04 23:28 ` [edk2-devel] " Laszlo Ersek @ 2020-02-05 14:18 ` Wang, Jian J 0 siblings, 0 replies; 3+ messages in thread From: Wang, Jian J @ 2020-02-05 14:18 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io, philmd@redhat.com Cc: Yao, Jiewen, Zhang, Chao B Hi, I agree with Laszlo. The updated comment still doesn't match the code. I'd suggest to fix the code as well as comment to make sure it confirms to the prototype. Regards, Jian > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Wednesday, February 05, 2020 7:28 AM > To: devel@edk2.groups.io; philmd@redhat.com > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; > Zhang, Chao B <chao.b.zhang@intel.com> > Subject: Re: [edk2-devel] [PATCH 1/1] SecurityPkg: Fix incorrect return value in > documentation > > Hi Phil, > > On 02/04/20 23:26, Philippe Mathieu-Daudé wrote: > > The DxeTpmMeasureBootHandler and DxeTpm2MeasureBootHandler handlers > > are SECURITY2_FILE_AUTHENTICATION_HANDLER prototype. This prototype > > can not return EFI_INVALID_PARAMETER. > > > > The prototype documentation states it returns EFI_ACCESS_DENIED if: > > > > "The file specified by File and FileBuffer did not authenticate, > > and the platform policy dictates that the DXE Foundation may not > > use File." > > > > Note in practice both functions return EFI_SECURITY_VIOLATION: > > > > "The file specified by DevicePath and FileBuffer did not > > authenticate, and the platform policy dictates that the file > > should be placed in the untrusted state. The image has been > > added to the file execution table." > > > > Noticed while reviewing commit 6d57592740cdd0b6868baeef7929d6e6fef. > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > Cc: Chao Zhang <chao.b.zhang@intel.com> > > Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com> > > --- > > .../Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c | 2 +- > > SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c | 2 > +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git > a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > > index 04b9b0d7fbf3..0c07f65c6835 100644 > > --- > a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > > +++ > b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c > > @@ -384,7 +384,7 @@ Finish: > > and other exception operations. The File parameter allows for possible > logging > > within the SAP of the driver. > > > > - If File is NULL, then EFI_INVALID_PARAMETER is returned. > > + If File is NULL, then EFI_ACCESS_DENIED is returned. > > > > If the file specified by File with an authentication status specified by > > AuthenticationStatus is safe for the DXE Core to use, then EFI_SUCCESS is > returned. > > I've tried seeing where this code actually returns > EFI_INVALID_PARAMETER, but I can't find it. If File is NULL in > DxeTpm2MeasureBootHandler(), then first we seem to do: > > OrigDevicePathNode = DuplicateDevicePath (File); > > which I believe will produce a NULL. Then we seem to pass this NULL > around a little bit, so I think there's a fair chance of crashing there. > > I wonder if this code should be fixed, to check "File" in the first > place, and then return EFI_ACCESS_DENIED. > > There are also some other places in the function that could apparently > return a wide array of error codes -- FvbProtocol->GetPhysicalAddress() > (EFI_UNSUPPORTED?), PeCoffLoaderGetImageInfo(), etc. > > I do agree this patch is an improvement, however; at least it says what > *should* be returned. So with that in mind: > > Acked-by: Laszlo Ersek <lersek@redhat.com> > > Thanks > Laszlo > > > diff --git > a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > > index 1f2eed29a1df..0dccbb66eb26 100644 > > --- a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > > +++ > b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c > > @@ -678,7 +678,7 @@ Finish: > > and other exception operations. The File parameter allows for possible > logging > > within the SAP of the driver. > > > > - If File is NULL, then EFI_INVALID_PARAMETER is returned. > > + If File is NULL, then EFI_ACCESS_DENIED is returned. > > > > If the file specified by File with an authentication status specified by > > AuthenticationStatus is safe for the DXE Core to use, then EFI_SUCCESS is > returned. > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-05 14:19 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-04 22:26 [PATCH 1/1] SecurityPkg: Fix incorrect return value in documentation Philippe Mathieu-Daudé 2020-02-04 23:28 ` [edk2-devel] " Laszlo Ersek 2020-02-05 14:18 ` Wang, Jian J
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox