Re: [edk2] [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for FilePath device path
> -Original Message-
> From: Wang, Jian J
> Sent: Tuesday, February 19, 2019 6:12 PM
> To: Ni, Ray ; edk2-devel@lists.01.org
> Cc: Gao, Liming ; Kinney, Michael D
>
> 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 ; edk2-devel@lists.01.org
> > Cc: Gao, Liming ; Kinney, Michael D
> >
> > Subject: RE: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check
> > for FilePath device path
> >
> >
> >
> > > -Original Message-
> > > From: Wang, Jian J
> > > Sent: Tuesday, February 19, 2019 5:22 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Gao, Liming ; Ni, Ray ;
> > > Kinney, Michael D
> > > 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
> > > Cc: Ray Ni
> > > Cc: Michael D Kinney
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Jian J Wang
> > > ---
> > > 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
> 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [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 ; edk2-devel@lists.01.org
> Cc: Gao, Liming ; Kinney, Michael D
>
> Subject: RE: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for
> FilePath device path
>
>
>
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Tuesday, February 19, 2019 5:22 PM
> > To: edk2-devel@lists.01.org
> > Cc: Gao, Liming ; Ni, Ray ; Kinney,
> > Michael D
> > 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
> > Cc: Ray Ni
> > Cc: Michael D Kinney
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang
> > ---
> > 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for FilePath device path
> -Original Message-
> From: Wang, Jian J
> Sent: Tuesday, February 19, 2019 5:22 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming ; Ni, Ray ; Kinney,
> Michael D
> 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
> Cc: Ray Ni
> Cc: Michael D Kinney
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang
> ---
> 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [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
Cc: Ray Ni
Cc: Michael D Kinney
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
