Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string constraint assertions

["Yao, Jiewen" <jiewen.yao@intel.com>]

Hi Vitaly
Before we introduce EDKII version safe string, we did investigate the behavior of below APIs:
1) strcpy - We all know that...
2) strncpy - The problem is that this API cannot guarantee a string is NULL terminated.
3) strlcpy - It is null terminated, but the result is truncated. That data loss might be a problem - https://lwn.net/Articles/507319/ 
4) strcpy_s - Return zero string, if there is violation.

We discussed and decide to follow 4), with a minor update - we just return error without zero the original string.

As such, if you want to use strlcpy, I suggest you had better invent a new API, instead of current StrCpy_S.

Would you please share a real use case on how you use this API verify the untrusted input?
I am very curious on the usage.

Thank you
Yao Jiewen


> -----Original Message-----
> From: vit9696 <vit9696@protonmail.com>
> Sent: Monday, October 21, 2019 4:30 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Marvin.Haeuser@outlook.com
> Subject: Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string
> constraint assertions
> 
> Jiewen,
> 
> Your explanation makes good sense in the context of "This API is NOT design to
> handle untrusted input.", however, I believe this is not how it is should work or
> at least this is not how we would like it to behave.
> 
> In Unix world there are similar hardened interfaces, for example, strlcpy or
> strlcat[1]. These interfaces behave quite similar to what you have in BaseLib
> SafeString, but in the first place they are meant to handle untrusted input. To my
> experience this situation happens no less often (and in fact much more) than the
> need to do extra error checking in trusted code, and I expect EDK2 to follow suit.
> I.e. implement something that can be used not only to check for programmer
> errors within the module, but also perform the validation of external data. After
> all, if these were just the assertions, I would have expected for return value to
> be void not RETURN_STATUS.
> 
> If we consider all the options we need to do either of the following:
> - reimplement most of the functions in the caller code, which is non-trivial,
> increases code size, reduces readability, is more prone to errors, makes
> reviewing more time consuming, etc.
> - reimplement all these functions in a separate library, which solves some of the
> issues, but still increases code size and results in separate interfaces with
> different contracts
> - add a pcd to disable assertions, which will disable the debugging handlers and
> will effectively make these functions behave as runtime checks for untrusted
> data, numerous people me included expect them do.
> 
> Last suggestion makes most sense to me. Therefore, I believe that even in the
> situation we have different opinions on whether this should be on by default, we
> should at least see the benefit for having an option to make this configurable in
> other projects. In this case I can update the patch in the next days to preserve
> the original behaviour and resubmit it as a v2.
> 
> Best wishes,
> Vitaly
> 
> [1] https://linux.die.net/man/3/strlcpy
> 
> > 21 окт. 2019 г., в 11:07, Yao, Jiewen <jiewen.yao@intel.com> написал(а):
> >
> >
> > Hi Vitaly
> > We have discussed the ASSERT usage when we added the first version of code.
> >
> > C11 standard supports runtime violation handler registration.
> > We investigated the behavior of
> > (1) MS Visual Studio - http://msdn.microsoft.com/en-us/library/bb288454.aspx
> > (2) SafeCLib - http://sourceforge.net/projects/safeclib/
> > (3) Slibc - https://code.google.com/p/slibc/
> >
> > The default behavior is:
> > (1) Call Debugger
> > (2) Print error
> > (3) Call abort()
> >
> > As conclusion, we believe it is *caller's responsibility* to make sure the caller
> inputs right parameter, instead of let callee check and return error.
> >
> > In order to catch such error as early as possible, we decide to use ASSERT,
> because it is something never happen. (We still use error handling followed by to
> handle the release build.)
> >
> > This API is NOT design to handle untrusted input. As such, we believe it is
> expected ASSERT behavior.
> >
> >
> > We have other debate, such as if we need ASSERT 2 bytes alignment CHAR16
> process, or if we need ASSERT address alignment for MMIO access.
> > Per our experience, it is much better to let caller guarantee that, instead of
> callee check that. And ASSERT is a good way to catch the issue as early as
> possible.
> >
> >
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly
> >> Cheptsov via Groups.Io
> >> Sent: Monday, October 21, 2019 3:28 PM
> >> To: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> >> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Kinney,
> >> Michael D <michael.d.kinney@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe
> string
> >> constraint assertions
> >>
> >> Liming, Jiewen,
> >>
> >> I am personally fine to resubmit the patch changing the defaults to TRUE, but
> >> does it actually make sense in any other environment but some special
> testing
> >> platform? I cannot imagine any production platform that would need it
> enabled,
> >> the only use case is to perform analysis on the trusted data usage or
> something
> >> similar, as I explained on BZ.
> >>
> >> As for assertions, I would expect that all PCDs we introduce are meant to
> >> control unexpected ASSERT behaviour. I.e. where ASSERTs are potentially
> used
> >> to signalise about issues coming not only from trusted sources (usage
> contract
> >> violation) but also from untrusted sources (external data). Such assertions
> >> cannot be used in production environments, as they may break software, and
> >> thus we have to implement code to disable them.
> >>
> >> Best regards,
> >> Vitaly
> >>
> >>> 21 окт. 2019 г., в 7:28, Yao, Jiewen <jiewen.yao@intel.com> написал(а):
> >>>
> >>>
> >>> Hi Mike
> >>> I remember we have discussed it before.
> >>>
> >>> The general concern is that: how many additional PCD we need introduce,
> to
> >> control different ASSERT in different modules ?
> >>>
> >>> We may want to enable *some* assert in some modules, but disable *some
> >> other* assert.
> >>> E.g. the assert for linkedlist in base lib is another example...
> >>>
> >>> What is your thought?
> >>>
> >>> Thank you
> >>> Yao Jiewen
> >>>
> >>>> -----Original Message-----
> >>>> From: Gao, Liming <liming.gao@intel.com>
> >>>> Sent: Monday, October 21, 2019 11:17 AM
> >>>> To: devel@edk2.groups.io; vit9696@protonmail.com
> >>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> >> <jian.j.wang@intel.com>;
> >>>> Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> >>>> <michael.d.kinney@intel.com>
> >>>> Subject: RE: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe
> >> string
> >>>> constraint assertions
> >>>>
> >>>> Include more people.
> >>>>
> >>>> Basically, to keep the compatible behavior,
> >> PcdAssertOnSafeStringConstraints
> >>>> default value should be TRUE.
> >>>> The different platform can configure it.
> >>>>
> >>>> Thanks
> >>>> Liming
> >>>>> -----Original Message-----
> >>>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> Of
> >>>>> Vitaly Cheptsov via Groups.Io
> >>>>> Sent: Sunday, October 20, 2019 9:06 PM
> >>>>> To: devel@edk2.groups.io
> >>>>> Subject: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe
> >> string
> >>>>> constraint assertions
> >>>>>
> >>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054
> >>>>>
> >>>>> Runtime data checks are not meant to cause debug assertions
> >>>>> unless explicitly needed by some debug code (thus the PCD)
> >>>>> as this breaks debug builds validating data with BaseLib.
> >>>>>
> >>>>> Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com>>
> >>>>> ---
> >>>>> MdePkg/MdePkg.dec                   |  6 ++++++
> >>>>> MdePkg/Library/BaseLib/BaseLib.inf  | 11 ++++++-----
> >>>>> MdePkg/Library/BaseLib/SafeString.c |  4 +++-
> >>>>> MdePkg/MdePkg.uni                   |  6 ++++++
> >>>>> 4 files changed, 21 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> >>>>> index 3fd7d1634c..dda2cdf401 100644
> >>>>> --- a/MdePkg/MdePkg.dec
> >>>>> +++ b/MdePkg/MdePkg.dec
> >>>>> @@ -2221,6 +2221,12 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
> >>>>> # @Prompt Memory Address of GuidedExtractHandler Table.
> >>>>>
> >>>>>
> >> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|0x10000
> >>>>> 00|UINT64|0x30001015
> >>>>>
> >>>>> +  ## Indicates if safe string constraint violation should assert.<BR><BR>
> >>>>> +  #   TRUE  - Safe string constraint violation causes assertion.<BR>
> >>>>> +  #   FALSE - Safe string constraint violation does not cause assertion.<BR>
> >>>>> +  # @Prompt Enable safe string constraint violation assertions.
> >>>>> +
> >>>>>
> >> gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints|FALSE|BOOL
> >>>>> EAN|0x0000002e
> >>>>> +
> >>>>> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic,
> PcdsDynamicEx]
> >>>>> ## This value is used to set the base address of PCI express hierarchy.
> >>>>> # @Prompt PCI Express Base Address.
> >>>>> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
> >>>>> b/MdePkg/Library/BaseLib/BaseLib.inf
> >>>>> index 3586beb0ab..bc98bc6134 100644
> >>>>> --- a/MdePkg/Library/BaseLib/BaseLib.inf
> >>>>> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> >>>>> @@ -390,11 +390,12 @@ [LibraryClasses]
> >>>>> BaseMemoryLib
> >>>>>
> >>>>> [Pcd]
> >>>>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength      ##
> >>>>> SOMETIMES_CONSUMES
> >>>>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength     ##
> >>>>> SOMETIMES_CONSUMES
> >>>>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ##
> >>>>> SOMETIMES_CONSUMES
> >>>>> -
> gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask
> >>>>> ## SOMETIMES_CONSUMES
> >>>>> -  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType       ##
> >>>>> SOMETIMES_CONSUMES
> >>>>> +  gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints       ##
> >>>>> SOMETIMES_CONSUMES
> >>>>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength             ##
> >>>>> SOMETIMES_CONSUMES
> >>>>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength            ##
> >>>>> SOMETIMES_CONSUMES
> >>>>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength
> ##
> >>>>> SOMETIMES_CONSUMES
> >>>>> +
> gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask
> >>>>> ## SOMETIMES_CONSUMES
> >>>>> +  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType              ##
> >>>>> SOMETIMES_CONSUMES
> >>>>>
> >>>>> [FeaturePcd]
> >>>>> gEfiMdePkgTokenSpaceGuid.PcdVerifyNodeInList  ## CONSUMES
> >>>>> diff --git a/MdePkg/Library/BaseLib/SafeString.c
> >>>>> b/MdePkg/Library/BaseLib/SafeString.c
> >>>>> index 7dc03d2caa..56b5e34a8d 100644
> >>>>> --- a/MdePkg/Library/BaseLib/SafeString.c
> >>>>> +++ b/MdePkg/Library/BaseLib/SafeString.c
> >>>>> @@ -14,7 +14,9 @@
> >>>>>
> >>>>> #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status)  \
> >>>>> do { \
> >>>>> -    ASSERT (Expression); \
> >>>>> +    if (PcdGetBool (PcdAssertOnSafeStringConstraints)) { \
> >>>>> +      ASSERT (Expression); \
> >>>>> +    } \
> >>>>>   if (!(Expression)) { \
> >>>>>     return Status; \
> >>>>>   } \
> >>>>> diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
> >>>>> index 5c1fa24065..425b66bb43 100644
> >>>>> --- a/MdePkg/MdePkg.uni
> >>>>> +++ b/MdePkg/MdePkg.uni
> >>>>> @@ -287,6 +287,12 @@
> >>>>>
> >>>>> #string
> >>>>>
> STR_gEfiMdePkgTokenSpaceGuid_PcdGuidedExtractHandlerTableAddress_H
> >>>>> ELP  #language en-US "This value is used to set the available memory
> >> address
> >>>>> to store Guided Extract Handlers. The required memory space is decided
> by
> >>>>> the value of PcdMaximumGuidedExtractHandler."
> >>>>>
> >>>>> +#string
> >>>>>
> STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_PROM
> >>>>> PT  #language en-US "Enable safe string constraint violation assertions"
> >>>>> +
> >>>>> +#string
> >>>>>
> STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_HELP
> >>>>> #language en-US "Indicates if safe string constraint violation should
> >>>>> assert.<BR><BR>\n"
> >>>>> +                                                                                   "TRUE  - Safe string
> >> constraint
> >>>>> violation causes assertion.<BR>\n"
> >>>>> +                                                                                   "FALSE - Safe string
> >> constraint
> >>>>> violation does not cause assertion.<BR>"
> >>>>> +
> >>>>> #string
> >>>>> STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_PROMPT
> >>>>> #language en-US "PCI Express Base Address"
> >>>>>
> >>>>> #string
> STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP
> >>>>> #language en-US "This value is used to set the base address of PCI express
> >>>>> hierarchy."
> >>>>> --
> >>>>> 2.21.0 (Apple Git-122)
> >>>>>
> >>>>>
> >>>>> -=-=-=-=-=-=
> >>>>> Groups.io Links: You receive all messages sent to this group.
> >>>>>
> >>>>> View/Reply Online (#49255):
> >> https://edk2.groups.io/g/devel/message/49255
> >>>>> Mute This Topic: https://groups.io/mt/35943317/1759384
> >>>>> Group Owner: devel+owner@edk2.groups.io
> >>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [liming.gao@intel.com]
> >>>>> -=-=-=-=-=-=
> >>>
> >>
> >>
> >> 
> >