public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: "Dandan Bi" <dandan.bi@intel.com>,
	"Hao A Wu" <hao.a.wu@intel.com>,
	"Jian J Wang" <jian.j.wang@intel.com>,
	"Liming Gao" <gaoliming@byosoft.com.cn>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: [PATCH v2 RESEND 2/2] MdeModulePkg/Core/Dxe: limit FwVol encapsulation section recursion
Date: Thu, 19 Nov 2020 11:53:40 +0100	[thread overview]
Message-ID: <20201119105340.16225-3-lersek@redhat.com> (raw)
In-Reply-To: <20201119105340.16225-1-lersek@redhat.com>

The DXE Core sets up a protocol notify function in its entry point, for
instances of the Firmware Volume Block2 Protocol:

  DxeMain()           [DxeMain/DxeMain.c]
    FwVolDriverInit() [FwVol/FwVol.c]

Assume that a 3rd party UEFI driver or application installs an FVB
instance, with crafted contents. The notification function runs:

  NotifyFwVolBlock() [FwVol/FwVol.c]

installing an instance of the Firmware Volume 2 Protocol on the handle.

(Alternatively, assume that a 3rd party application calls
gDS->ProcessFirmwareVolume(), which may also produce a Firmware Volume 2
Protocol instance.)

The EFI_FIRMWARE_VOLUME2_PROTOCOL.ReadSection() member performs "a
depth-first, left-to-right search algorithm through all sections found in
the specified file" (quoting the PI spec), as follows:

  FvReadFileSection()   [FwVol/FwVolRead.c]
    GetSection()        [SectionExtraction/CoreSectionExtraction.c]
      FindChildNode()   [SectionExtraction/CoreSectionExtraction.c]
        FindChildNode() // recursive call

FindChildNode() is called recursively for encapsulation sections.

Currently this recursion is not limited. Introduce a new PCD
(fixed-at-build, or patchable-in-module), and make FindChildNode() track
the section nesting depth against that PCD.

Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1743
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MdeModulePkg/MdeModulePkg.dec                                   |  6 ++++
 MdeModulePkg/MdeModulePkg.uni                                   |  6 ++++
 MdeModulePkg/Core/Dxe/DxeMain.inf                               |  1 +
 MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c | 33 ++++++++++++++++++--
 4 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 00075528198d..9b52b3449443 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1529,6 +1529,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
   # @Prompt Enable Capsule On Disk support.
   gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleOnDiskSupport|FALSE|BOOLEAN|0x0000002d
 
+  ## Maximum permitted encapsulation levels of sections in a firmware volume,
+  #  in the DXE phase. Minimum value is 1. Sections nested more deeply are
+  #  rejected.
+  # @Prompt Maximum permitted FwVol section nesting depth (exclusive).
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth|0x10|UINT32|0x00000030
+
 [PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## This PCD defines the Console output row. The default value is 25 according to UEFI spec.
   #  This PCD could be set to 0 then console output would be at max column and max row.
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
index 40884c57a460..1b347a75f684 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -1160,6 +1160,12 @@
                                                                                            "Note:<BR>"
                                                                                            "If Both Capsule In Ram and Capsule On Disk are provisioned at the same time, the Capsule On Disk will be bypassed."
 
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdFwVolDxeMaxEncapsulationDepth_PROMPT #language en-US "Maximum permitted FwVol section nesting depth (exclusive)."
+
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdFwVolDxeMaxEncapsulationDepth_HELP   #language en-US "Maximum permitted encapsulation levels of sections in a firmware volume,<BR>"
+                                                                                                   "in the DXE phase. Minimum value is 1. Sections nested more deeply are<BR>"
+                                                                                                   "rejected."
+
 #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdCapsuleInRamSupport_PROMPT  #language en-US "Enable Capsule In Ram support"
 
 #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdCapsuleInRamSupport_HELP  #language en-US   "Capsule In Ram is to use memory to deliver the capsules that will be processed after system reset.<BR><BR>"
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 1d4b11dc7318..e4bca895773d 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -185,6 +185,7 @@ [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType                       ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                           ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth           ## CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c b/MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c
index d7f7ef427422..908617d1ca5c 100644
--- a/MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c
+++ b/MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c
@@ -955,6 +955,9 @@ CreateChildNode (
                                  This is an in/out parameter and it is 1-based,
                                  to deal with recursions.
   @param  SectionDefinitionGuid  Guid of section definition
+  @param  Depth                  Nesting depth of encapsulation sections.
+                                 Callers different from FindChildNode() are
+                                 responsible for passing in a zero Depth.
   @param  FoundChild             Output indicating the child node that is found.
   @param  FoundStream            Output indicating which section stream the child
                                  was found in.  If this stream was generated as a
@@ -968,6 +971,9 @@ CreateChildNode (
   @retval EFI_NOT_FOUND          Requested child node does not exist.
   @retval EFI_PROTOCOL_ERROR     a required GUIDED section extraction protocol
                                  does not exist
+  @retval EFI_ABORTED            Recursion aborted because Depth has been
+                                 greater than or equal to
+                                 PcdFwVolDxeMaxEncapsulationDepth.
 
 **/
 EFI_STATUS
@@ -976,6 +982,7 @@ FindChildNode (
   IN     EFI_SECTION_TYPE                           SearchType,
   IN OUT UINTN                                      *SectionInstance,
   IN     EFI_GUID                                   *SectionDefinitionGuid,
+  IN     UINT32                                     Depth,
   OUT    CORE_SECTION_CHILD_NODE                    **FoundChild,
   OUT    CORE_SECTION_STREAM_NODE                   **FoundStream,
   OUT    UINT32                                     *AuthenticationStatus
@@ -990,6 +997,10 @@ FindChildNode (
 
   ASSERT (*SectionInstance > 0);
 
+  if (Depth >= PcdGet32 (PcdFwVolDxeMaxEncapsulationDepth)) {
+    return EFI_ABORTED;
+  }
+
   CurrentChildNode = NULL;
   ErrorStatus = EFI_NOT_FOUND;
 
@@ -1053,6 +1064,7 @@ FindChildNode (
                 SearchType,
                 SectionInstance,
                 SectionDefinitionGuid,
+                Depth + 1,
                 &RecursedChildNode,
                 &RecursedFoundStream,
                 AuthenticationStatus
@@ -1067,9 +1079,17 @@ FindChildNode (
         *FoundStream = RecursedFoundStream;
         return EFI_SUCCESS;
       } else {
+        if (Status == EFI_ABORTED) {
+          //
+          // If the recursive call was aborted due to nesting depth, stop
+          // looking for the requested child node. The skipped subtree could
+          // throw off the instance counting.
+          //
+          return Status;
+        }
         //
-        // If the status is not EFI_SUCCESS, just save the error code and
-        // continue to find the request child node in the rest stream.
+        // Save the error code and continue to find the requested child node in
+        // the rest of the stream.
         //
         ErrorStatus = Status;
       }
@@ -1272,11 +1292,20 @@ GetSection (
                *SectionType,
                &Instance,
                SectionDefinitionGuid,
+               0,                             // encapsulation depth
                &ChildNode,
                &ChildStreamNode,
                &ExtractedAuthenticationStatus
                );
     if (EFI_ERROR (Status)) {
+      if (Status == EFI_ABORTED) {
+        DEBUG ((DEBUG_ERROR, "%a: recursion aborted due to nesting depth\n",
+          __FUNCTION__));
+        //
+        // Map "aborted" to "not found".
+        //
+        Status = EFI_NOT_FOUND;
+      }
       goto GetSection_Done;
     }
 
-- 
2.19.1.3.g30247aa5d201


  parent reply	other threads:[~2020-11-19 10:54 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-19 10:53 [PATCH v2 RESEND 0/2] security fix: unlimited FV recursion, round 2 (DXE Core) Laszlo Ersek
2020-11-19 10:53 ` [PATCH v2 RESEND 1/2] MdeModulePkg/Core/Dxe: assert SectionInstance invariant in FindChildNode() Laszlo Ersek
2020-11-19 10:53 ` Laszlo Ersek [this message]
2020-11-20  5:30 ` 回复: [edk2-devel] [PATCH v2 RESEND 0/2] security fix: unlimited FV recursion, round 2 (DXE Core) gaoliming
2020-11-20 10:57   ` Laszlo Ersek
2020-11-21  1:43   ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201119105340.16225-3-lersek@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox