MdePkg/Include/Library/BaseLib.h | 99 ++++- MdePkg/Library/BaseLib/String.c | 448 +++++++++++++------- OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c | 10 +- 3 files changed, 374 insertions(+), 183 deletions(-)
Repo: https://github.com/lersek/edk2.git Branch: base64_decode_bz1891 Base64Decode() has a number of issues; see - <https://bugzilla.tianocore.org/show_bug.cgi?id=1891> - and the mailing list discussion linked from <https://bugzilla.tianocore.org/show_bug.cgi?id=1891#c6>. In my opinion, rewriting Base64Decode() from scratch, using a different (state machine-based) approach is safer / more robust than attempting to identify and patch up individual problems in the current implementation. The emphasis of the proposed implementation is to reject invalid input; decoding valid input is kind of secondary. (This is the safe approach for all parsers that process untrusted input, in my opinion.) My understanding is that unit tests for Base64Decode() already exist in some repository. While I tested the new implementation through OvmfPkg's EnrollDefaultKeys application -- which makes the sole calls to Base64Decode() in the open source edk2 tree -- I didn't run a unit test suite. Help with that (pointers to the test suite, or actual unit testing) would be highly appreciated. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Marvin Häuser <mhaeuser@outlook.de> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Cc: Zhichao Gao <zhichao.gao@intel.com> Thanks Laszlo Laszlo Ersek (3): MdePkg/BaseLib: re-specify Base64Decode(), and add temporary stub impl MdePkg/BaseLib: rewrite Base64Decode() OvmfPkg/EnrollDefaultKeys: clean up Base64Decode() retval handling MdePkg/Include/Library/BaseLib.h | 99 ++++- MdePkg/Library/BaseLib/String.c | 448 +++++++++++++------- OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c | 10 +- 3 files changed, 374 insertions(+), 183 deletions(-) -- 2.19.1.3.g30247aa5d201 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43159): https://edk2.groups.io/g/devel/message/43159 Mute This Topic: https://groups.io/mt/32284612/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 07/02/19 12:28, Laszlo Ersek wrote: > Repo: https://github.com/lersek/edk2.git > Branch: base64_decode_bz1891 > > Base64Decode() has a number of issues; see > > - <https://bugzilla.tianocore.org/show_bug.cgi?id=1891> > > - and the mailing list discussion linked from > <https://bugzilla.tianocore.org/show_bug.cgi?id=1891#c6>. > > In my opinion, rewriting Base64Decode() from scratch, using a different > (state machine-based) approach is safer / more robust than attempting to > identify and patch up individual problems in the current implementation. > The emphasis of the proposed implementation is to reject invalid input; > decoding valid input is kind of secondary. (This is the safe approach > for all parsers that process untrusted input, in my opinion.) > > My understanding is that unit tests for Base64Decode() already exist in > some repository. While I tested the new implementation through OvmfPkg's > EnrollDefaultKeys application -- which makes the sole calls to > Base64Decode() in the open source edk2 tree -- I didn't run a unit test > suite. Help with that (pointers to the test suite, or actual unit > testing) would be highly appreciated. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Marvin Häuser <mhaeuser@outlook.de> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > > Thanks > Laszlo > > Laszlo Ersek (3): > MdePkg/BaseLib: re-specify Base64Decode(), and add temporary stub impl > MdePkg/BaseLib: rewrite Base64Decode() > OvmfPkg/EnrollDefaultKeys: clean up Base64Decode() retval handling > > MdePkg/Include/Library/BaseLib.h | 99 ++++- > MdePkg/Library/BaseLib/String.c | 448 +++++++++++++------- > OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c | 10 +- > 3 files changed, 374 insertions(+), 183 deletions(-) > Ping. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43468): https://edk2.groups.io/g/devel/message/43468 Mute This Topic: https://groups.io/mt/32284612/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 07/02/19 12:28, Laszlo Ersek wrote: > Repo: https://github.com/lersek/edk2.git > Branch: base64_decode_bz1891 > > Base64Decode() has a number of issues; see > > - <https://bugzilla.tianocore.org/show_bug.cgi?id=1891> > > - and the mailing list discussion linked from > <https://bugzilla.tianocore.org/show_bug.cgi?id=1891#c6>. > > In my opinion, rewriting Base64Decode() from scratch, using a different > (state machine-based) approach is safer / more robust than attempting to > identify and patch up individual problems in the current implementation. > The emphasis of the proposed implementation is to reject invalid input; > decoding valid input is kind of secondary. (This is the safe approach > for all parsers that process untrusted input, in my opinion.) > > My understanding is that unit tests for Base64Decode() already exist in > some repository. While I tested the new implementation through OvmfPkg's > EnrollDefaultKeys application -- which makes the sole calls to > Base64Decode() in the open source edk2 tree -- I didn't run a unit test > suite. Help with that (pointers to the test suite, or actual unit > testing) would be highly appreciated. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Marvin Häuser <mhaeuser@outlook.de> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > > Thanks > Laszlo > > Laszlo Ersek (3): > MdePkg/BaseLib: re-specify Base64Decode(), and add temporary stub impl > MdePkg/BaseLib: rewrite Base64Decode() > OvmfPkg/EnrollDefaultKeys: clean up Base64Decode() retval handling > > MdePkg/Include/Library/BaseLib.h | 99 ++++- > MdePkg/Library/BaseLib/String.c | 448 +++++++++++++------- > OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c | 10 +- > 3 files changed, 374 insertions(+), 183 deletions(-) > Thank you all for the feedback! * Added a paragraph to the commit message of patch#2, as requested by Phil, based on the discussion with Marvin: + The intent is to only strengthen the checks (sanity and input) relative to + the previous implementation, hence the MAX_ADDRESS checks are reinstated. ... + [lersek@redhat.com: add last para to commit msg per talks w/ Marvin & Phil] * Pushed the first two patches in the series as commit range 84a459472075..35e242b698cd; closing <https://bugzilla.tianocore.org/show_bug.cgi?id=1891>. * Filed <https://bugzilla.tianocore.org/show_bug.cgi?id=1980> to track the CCS non-conformance issue, under approach "2a" (i.e., incrementally), as agreed upon with Zhichao. Assigned the BZ to myself; will post a patch soon. * Split off the last patch in the series to a separate TianoCore BZ, namely <https://bugzilla.tianocore.org/show_bug.cgi?id=1981>. Assigned that BZ to myself too. Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43820): https://edk2.groups.io/g/devel/message/43820 Mute This Topic: https://groups.io/mt/32284612/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.