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 <vit9...@protonmail.com> > Sent: Monday, October 21, 2019 4:30 PM > To: Yao, Jiewen <jiewen....@intel.com> > Cc: devel@edk2.groups.io; Gao, Liming <liming....@intel.com>; Wang, Jian J > <jian.j.w...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; > Laszlo Ersek <ler...@redhat.com>; marvin.haeu...@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....@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....@intel.com>; Gao, Liming > <liming....@intel.com> > >> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.w...@intel.com>; Kinney, > >> Michael D <michael.d.kin...@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....@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....@intel.com> > >>>> Sent: Monday, October 21, 2019 11:17 AM > >>>> To: devel@edk2.groups.io; vit9...@protonmail.com > >>>> Cc: Yao, Jiewen <jiewen....@intel.com>; Wang, Jian J > >> <jian.j.w...@intel.com>; > >>>> Gao, Liming <liming....@intel.com>; Kinney, Michael D > >>>> <michael.d.kin...@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 <vit9...@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+ow...@edk2.groups.io > >>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub > [liming....@intel.com] > >>>>> -=-=-=-=-=-= > >>> > >> > >> > >> > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49283): https://edk2.groups.io/g/devel/message/49283 Mute This Topic: https://groups.io/mt/35943317/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
