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

["Laszlo Ersek" <lersek@redhat.com>]

On 10/21/19 11:58, Vitaly Cheptsov via Groups.Io wrote:
> Jiewen,
>
> We are aware of all these nuances, and you are right that both of them
> (strlcpy/strcpy_s) have their downsides. Our opinion is that strlcpy
> is more practical, but that is a personal choice, and we do nor care
> much on what is available as long as it is documented.
>
> Our primary issues are not really with string copying, but rather with
> other interfaces. For example:
> - We use AsciiStrToGuid to convert a string to a GUID in the user
>   configuration file, and any intentional or unintentionsl typo there
>   may result in halting the entire app due to length assertion in
>   DEBUG builds.
> - We use AsciiStrCats to append messages to the log buffer as long as
>   they fit. We do not care what happens when they stop to fit, for us
>   that means we will have a cut log and a handled error that some
>   messages did not fit. However, in DEBUG builds that once again
>   results in halts.
>
> I am pretty sure there were more, but the use cases are be pretty
> similar, so you most likely get an idea. We do handle the error where
> necessary, yet we do not expect the code to halt before we can handle
> the error.

(Since I've been CC'd somewhere mid-thread, I'll offer an opinion...)

- For copying strings, I'm with Ulrich Drepper:

  http://www.sourceware.org/ml/libc-alpha/2000-08/msg00061.html

  "Every program which is handling strings has to know how long they
  are."

  Because I share that opinion, I've never considered the "safe string"
  functions extremely helpful.

- For formatting strings, and especially for parsing strings, I tend to
  consider functions that cannot deal, by design, with untrusted input,
  more or less useless.

When I was initially working on OvmfPkg/Library/QemuBootOrderLib, and
had to parse some hex strings (as parts of OpenFirmware device paths
exported by QEMU), I believe I looked for suitable utility functions in
edk2 elsewhere. Ultimately I wrote my own ParseUnitAddressHexList()
function.

A note specific to my use case (= guest firmware): in theory, the host
has complete control over the guest, so one might claim that the guest
should never sanity check input from the host (= the guest should always
blindly trust input from the host). That has never sat well with me,
although I couldn't really tell why; it's just always felt wrong. Later,
my concern has been vindicated in two ways: first, in the ACPI
linker/loader client, those sanity checks helped identify QEMU issues
(not malicious intent for sure, but mistakes nonetheless); and then, we
now have SEV (Secure Encrypted Virtualization), where host software does
not have full control over the guest.

IMO all parser functions (exposed as general utility functions) should
expect malicious input by default. (Another example I've been involved
with: the base64 decoder.)

Thanks
Laszlo