[PATCH v2 0/2] Add sanity check for FilePath device path

[PATCH v2 0/2] Add sanity check for FilePath device path

[Jian J Wang]

> v2: fix wrong detection of FilePath device path

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1497

Tests
- Pass specific DevicePathLib test cases
- Boot shell
- Boot to Fedora 26 (Qemu/x64)
- Boot to Ubuntu 18.04 (Qemu/x64)
- Boot to Windows 10 (Qemu/x64)
- Boot to Windows 7 (Qemu/x64)

Jian J Wang (2):
  MdePkg/UefiDevicePathLib: Add sanity check for FilePath device path
  MdePkg/UefiDevicePathLibDevicePathProtocol: Add sanity check for
    FilePath device path

 MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c   | 9 +++++++++
 .../UefiDevicePathLib.c                                  | 9 +++++++++
 2 files changed, 18 insertions(+)

-- 
2.17.1.windows.2

[PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for FilePath device path

[Jian J Wang]

> v2: fix wrong detection of FilePath device path

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1497

Current implementation of IsDevicePathValid() is not enough for type
of MEDIA_FILEPATH_DP, which has NULL-terminated string in the device
path. This patch add a simple NULL character check at Length position.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c b/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
index 5d7635fe3e..dd1bddc1c2 100644
--- a/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
+++ b/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
@@ -95,6 +95,15 @@ IsDevicePathValid (
         return FALSE;
       }
     }
+
+    //
+    // FilePath must be a NULL-terminated string.
+    //
+    if (DevicePathType (DevicePath) == MEDIA_DEVICE_PATH &&
+        DevicePathSubType (DevicePath) == MEDIA_FILEPATH_DP &&
+        *(CHAR16 *)((UINT8 *)DevicePath + NodeLength - 2) != 0) {
+      return FALSE;
+    }
   }
 
   //
-- 
2.17.1.windows.2

[PATCH v2 2/2] MdePkg/UefiDevicePathLibDevicePathProtocol: Add sanity check for FilePath device path

[Jian J Wang]

> v2: fix wrong detection of FilePath device path

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1497

Current implementation of IsDevicePathValid() is not enough for type
of MEDIA_FILEPATH_DP, which has NULL-terminated string in the device
path. This patch add a simple NULL character check at Length position.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 .../UefiDevicePathLib.c                                  | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c b/MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c
index 9a0ee42fd1..c8e78d2373 100644
--- a/MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c
+++ b/MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c
@@ -138,6 +138,15 @@ IsDevicePathValid (
         return FALSE;
       }
     }
+
+    //
+    // FilePath must be a NULL-terminated string.
+    //
+    if (DevicePathType (DevicePath) == MEDIA_DEVICE_PATH &&
+        DevicePathSubType (DevicePath) == MEDIA_FILEPATH_DP &&
+        *(CHAR16 *)((UINT8 *)DevicePath + NodeLength - 2) != 0) {
+      return FALSE;
+    }
   }
 
   //
-- 
2.17.1.windows.2

Re: [PATCH v2 0/2] Add sanity check for FilePath device path

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jian
>J Wang
>Sent: Tuesday, February 19, 2019 5:22 PM
>To: edk2-devel@lists.01.org
>Subject: [edk2] [PATCH v2 0/2] Add sanity check for FilePath device path
>
>> v2: fix wrong detection of FilePath device path
>
>REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1497
>
>Tests
>- Pass specific DevicePathLib test cases
>- Boot shell
>- Boot to Fedora 26 (Qemu/x64)
>- Boot to Ubuntu 18.04 (Qemu/x64)
>- Boot to Windows 10 (Qemu/x64)
>- Boot to Windows 7 (Qemu/x64)
>
>Jian J Wang (2):
>  MdePkg/UefiDevicePathLib: Add sanity check for FilePath device path
>  MdePkg/UefiDevicePathLibDevicePathProtocol: Add sanity check for
>    FilePath device path
>
> MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c   | 9 +++++++++
> .../UefiDevicePathLib.c                                  | 9 +++++++++
> 2 files changed, 18 insertions(+)
>
>--
>2.17.1.windows.2
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for FilePath device path

[Ni, Ray]

> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Tuesday, February 19, 2019 5:22 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Subject: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for
> FilePath device path
> 
> > v2: fix wrong detection of FilePath device path
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1497
> 
> Current implementation of IsDevicePathValid() is not enough for type of
> MEDIA_FILEPATH_DP, which has NULL-terminated string in the device path.
> This patch add a simple NULL character check at Length position.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> b/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> index 5d7635fe3e..dd1bddc1c2 100644
> --- a/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> +++ b/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> @@ -95,6 +95,15 @@ IsDevicePathValid (
>          return FALSE;
>        }
>      }
> +
> +    //
> +    // FilePath must be a NULL-terminated string.
> +    //
> +    if (DevicePathType (DevicePath) == MEDIA_DEVICE_PATH &&
> +        DevicePathSubType (DevicePath) == MEDIA_FILEPATH_DP &&
> +        *(CHAR16 *)((UINT8 *)DevicePath + NodeLength - 2) != 0) {
Can we assume the device path node buffer contains exact the null terminated string?
What if the buffer contains some padding bytes after null terminated string?

To handle this case, can we convert the DevicePath to FILEPATH_DEVICE_PATH and use logic similar as below?

FILEPATH_DEVICE_PATH *FilePath;
FilePath = (FILEPATH_DEVICE_PATH *) DevicePath;
MaxSize = (NodeLength - sizeof (EFI_DEVICE_PATH_PROTOCOL)) / sizeof (CHAR16);
If (StrnLenS (FilePath->PathName, MaxSize) == MaxSize) {
  Return FALSE;
}



> +      return FALSE;
> +    }
>    }
> 
>    //
> --
> 2.17.1.windows.2

Re: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for FilePath device path

[Wang, Jian J]

Hi Ray,

> -----Original Message-----
> From: Ni, Ray
> Sent: Tuesday, February 19, 2019 5:34 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for
> FilePath device path
> 
> 
> 
> > -----Original Message-----
> > From: Wang, Jian J <jian.j.wang@intel.com>
> > Sent: Tuesday, February 19, 2019 5:22 PM
> > To: edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Kinney,
> > Michael D <michael.d.kinney@intel.com>
> > Subject: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for
> > FilePath device path
> >
> > > v2: fix wrong detection of FilePath device path
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1497
> >
> > Current implementation of IsDevicePathValid() is not enough for type of
> > MEDIA_FILEPATH_DP, which has NULL-terminated string in the device path.
> > This patch add a simple NULL character check at Length position.
> >
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> > b/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> > index 5d7635fe3e..dd1bddc1c2 100644
> > --- a/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> > +++ b/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> > @@ -95,6 +95,15 @@ IsDevicePathValid (
> >          return FALSE;
> >        }
> >      }
> > +
> > +    //
> > +    // FilePath must be a NULL-terminated string.
> > +    //
> > +    if (DevicePathType (DevicePath) == MEDIA_DEVICE_PATH &&
> > +        DevicePathSubType (DevicePath) == MEDIA_FILEPATH_DP &&
> > +        *(CHAR16 *)((UINT8 *)DevicePath + NodeLength - 2) != 0) {
> Can we assume the device path node buffer contains exact the null terminated
> string?
> What if the buffer contains some padding bytes after null terminated string?
> 
> To handle this case, can we convert the DevicePath to FILEPATH_DEVICE_PATH
> and use logic similar as below?
> 
> FILEPATH_DEVICE_PATH *FilePath;
> FilePath = (FILEPATH_DEVICE_PATH *) DevicePath;
> MaxSize = (NodeLength - sizeof (EFI_DEVICE_PATH_PROTOCOL)) / sizeof
> (CHAR16);
> If (StrnLenS (FilePath->PathName, MaxSize) == MaxSize) {
>   Return FALSE;
> }
> 

I did think about your implementation and I agree it might be the best way
from compatibility perspective. But I have three considerations:

1. It's not a good programming habit to pass unwanted data around and should not
     be encouraged to do so.
2. It might leave potential security hole (maybe I'm a little too cautious)
3. The UEFI spec has following statement on this type of device path (ch10.3.6.4).

" A NULL-terminated Path string including directory and file names. The length of
this string n can be determined by subtracting 4 from the Length entry."

But I'm still open to your suggestion, if all agree that compatibility is more important.

Regards,
Jian

> 
> 
> > +      return FALSE;
> > +    }
> >    }
> >
> >    //
> > --
> > 2.17.1.windows.2

Re: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for FilePath device path

[Ni, Ray]

> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Tuesday, February 19, 2019 6:12 PM
> To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for
> FilePath device path
> 
> Hi Ray,
> 
> > -----Original Message-----
> > From: Ni, Ray
> > Sent: Tuesday, February 19, 2019 5:34 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Subject: RE: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check
> > for FilePath device path
> >
> >
> >
> > > -----Original Message-----
> > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > Sent: Tuesday, February 19, 2019 5:22 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Gao, Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > > Kinney, Michael D <michael.d.kinney@intel.com>
> > > Subject: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check
> > > for FilePath device path
> > >
> > > > v2: fix wrong detection of FilePath device path
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1497
> > >
> > > Current implementation of IsDevicePathValid() is not enough for type
> > > of MEDIA_FILEPATH_DP, which has NULL-terminated string in the device
> path.
> > > This patch add a simple NULL character check at Length position.
> > >
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > > ---
> > >  MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c | 9
> > > +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> > > b/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> > > index 5d7635fe3e..dd1bddc1c2 100644
> > > --- a/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> > > +++ b/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> > > @@ -95,6 +95,15 @@ IsDevicePathValid (
> > >          return FALSE;
> > >        }
> > >      }
> > > +
> > > +    //
> > > +    // FilePath must be a NULL-terminated string.
> > > +    //
> > > +    if (DevicePathType (DevicePath) == MEDIA_DEVICE_PATH &&
> > > +        DevicePathSubType (DevicePath) == MEDIA_FILEPATH_DP &&
> > > +        *(CHAR16 *)((UINT8 *)DevicePath + NodeLength - 2) != 0) {
> > Can we assume the device path node buffer contains exact the null
> > terminated string?
> > What if the buffer contains some padding bytes after null terminated string?
> >
> > To handle this case, can we convert the DevicePath to
> > FILEPATH_DEVICE_PATH and use logic similar as below?
> >
> > FILEPATH_DEVICE_PATH *FilePath;
> > FilePath = (FILEPATH_DEVICE_PATH *) DevicePath; MaxSize = (NodeLength
> > - sizeof (EFI_DEVICE_PATH_PROTOCOL)) / sizeof (CHAR16); If (StrnLenS
> > (FilePath->PathName, MaxSize) == MaxSize) {
> >   Return FALSE;
> > }
> >
> 
> I did think about your implementation and I agree it might be the best way from
> compatibility perspective. But I have three considerations:
> 
> 1. It's not a good programming habit to pass unwanted data around and should
> not
>      be encouraged to do so.
> 2. It might leave potential security hole (maybe I'm a little too cautious) 3. The
> UEFI spec has following statement on this type of device path (ch10.3.6.4).
> 
> " A NULL-terminated Path string including directory and file names. The length
> of this string n can be determined by subtracting 4 from the Length entry."
> 

With such statement, I am ok to your changes.

Reviewed-by: Ray Ni <ray.ni@intel.com>

> But I'm still open to your suggestion, if all agree that compatibility is more
> important.
> 
> Regards,
> Jian
> 
> >
> >
> > > +      return FALSE;
> > > +    }
> > >    }
> > >
> > >    //
> > > --
> > > 2.17.1.windows.2