From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by mx.groups.io with SMTP id smtpd.web12.64379.1597753299671470139 for ; Tue, 18 Aug 2020 05:21:40 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: ispras.ru, ip: 83.149.199.84, mailfrom: cheptsov@ispras.ru) Received: from [127.0.0.1] (unknown [10.10.2.240]) by mail.ispras.ru (Postfix) with ESMTPSA id A4E4A40A204F; Tue, 18 Aug 2020 12:21:36 +0000 (UTC) From: "Vitaly Cheptsov" Message-Id: <35FA0F61-C718-455D-A63C-E1B0E16C2F0A@ispras.ru> Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\)) Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset Date: Tue, 18 Aug 2020 15:21:36 +0300 In-Reply-To: Cc: "Yao, Jiewen" , "devel@edk2.groups.io" , "xiewenyi2@huawei.com" , "Wang, Jian J" , "huangming23@huawei.com" , "songdongkuang@huawei.com" , =?utf-8?Q?Marvin_H=C3=A4user?= To: Laszlo Ersek References: <1597319741-59646-1-git-send-email-xiewenyi2@huawei.com> <1597319741-59646-2-git-send-email-xiewenyi2@huawei.com> <024b1279-609d-fefa-8535-5af072815bf8@huawei.com> <250fc485-8705-88b7-21c9-ecd28132934d@redhat.com> <56d0af8e-39ae-ae3f-1561-a532b697ba5d@redhat.com> X-Mailer: Apple Mail (2.3608.120.23.2.1) X-Groupsio-MsgNum: 64377 Content-Type: multipart/signed; boundary="Apple-Mail=_85F7952A-87F5-4C32-897E-FCB52AC8D93C"; protocol="application/pgp-signature"; micalg=pgp-sha256 --Apple-Mail=_85F7952A-87F5-4C32-897E-FCB52AC8D93C Content-Type: multipart/alternative; boundary="Apple-Mail=_6C23E395-2BC8-4C27-BFBE-26EF85B61453" --Apple-Mail=_6C23E395-2BC8-4C27-BFBE-26EF85B61453 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 As a follow-up to Marvin=E2=80=99s e-mail and in the reference of the = thread. I should add that to make the result more reliable we not only = hope for the reviewer experience and testing, but also write formal = proofs for several code properties. This is the primary reason the C = code has not yet left the facility, despite being mostly written. If you = feel interested, we use an opensource inhouse-written tool, = AstraVer[1][2], which is an extension of Frama-C[3]. Best regards, Vitaly [1] https://arxiv.org/pdf/1809.00626.pdf = [2] https://www.isprasopen.ru/2018/docs/Volkov.pdf = [3] https://frama-c.com > 18 =D0=B0=D0=B2=D0=B3. 2020 =D0=B3., =D0=B2 13:24, Marvin H=C3=A4user = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 >=20 > Good day everyone, >=20 > First off, for your information, I'm sending from my new e-mail = address > from now on. >=20 > Please excuse me, I cannot read your entire thread right now, I will > definitely make sure to catch up as soon as time permits, but I just > wanted to confirm we are indeed working on a reimplementation of the = PE > loader. > It involves correcting several security issues (which will be detailed > as the patches are sent as anything else would be too much work for us > right now), reducing code duplication (how often is the hashing > algorithm duplicated across edk2? :) ) and a more or less experimental > approach to formal verification. We plan to submit it this year, = however > please note that this is a low priority project and is not being = worked > on on a full-time basis. >=20 > Please let us know about your own plans so we do not end up = duplicating > work. >=20 > Best regards, > Marvin >=20 > Am 18.08.2020 um 12:17 schrieb Laszlo Ersek: >> Hi Jiewen, >>=20 >> (+Marvin, +Vitaly) >>=20 >> On 08/18/20 01:23, Yao, Jiewen wrote: >>>> -----Original Message----- >>>> From: devel@edk2.groups.io On Behalf Of = Laszlo >>>> Ersek >>>> Sent: Tuesday, August 18, 2020 12:53 AM >>>> To: Yao, Jiewen ; devel@edk2.groups.io; >>>> xiewenyi2@huawei.com; Wang, Jian J >>>> Cc: huangming23@huawei.com; songdongkuang@huawei.com >>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1] >>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset >>=20 >> [...] >>=20 >>> However, I do think the producer is mandatory for a fix or at least = a >>> security fix. >>> The owner to fix the issue should guarantee the patch is good. >>> The owner shall never rely on the code reviewer to figure out if the >>> patch is good and complete. >>>=20 >>> I have some bad experience that bug owner just wrote a patch and = tried >>> to fix a problem, without any test. >>> And it happened passed code review from someone who does not well >>> understand the problem, but give rb based upon the time pressure. >>> Later, the fix was approved to be useless. >>>=20 >>> In my memory, at least 3 cases were security fix. They are found, = just >>> because they are sensitive, more people took a look later. >>> It was simple. It was one-line change. >>> But it has not test, and it was wrong. >>> "It was ridiculous" -- commented by the people who find the = so-called >>> security fix does not fix the issue. >>=20 >> Just because sloppy/rushed reviews exist, and just because reviewers >> operate under time pressure, we should not automatically reject = security >> fixes that come without a reproducer. >>=20 >> Some organizations do develop reproducers, but they never share them >> publicly (for fear of abuse by others). >>=20 >> But more importantly, in an open development project, a developer = could >> have time and expertise to contribute a fix, but not to create a >> reproducer. >>=20 >> - If we make contributing harder, fewer people will upstream their >> fixes. >>=20 >> - If we make contributing harder, then contributions that do make it = to >> the tree will be of higher quality. >>=20 >> Both statements ring true to me -- so it's a tradeoff. >>=20 >> (By "we", I mean the edk2 community.) >>=20 >>>> Additionally, the exact statement that the bug report does make, >>>> namely >>>>=20 >>>> it's possible to overflow Offset back to 0 causing an endless = loop >>>>=20 >>>> is wrong (as far as I can tell anyway). It is not "OffSet" that can >>>> be overflowed to zero, but the *addend* that is added to OffSet can >>>> be overflowed to zero. Therefore the infinite loop will arise = because >>>> OffSet remains stuck at its present value, and not because OffSet >>>> will be re-set to zero. >>>>=20 >>>> For the reasons, we can only speculate as to what the actual = problem >>>> is, unless Jian decides to join the discussion and clarifies what = he >>>> had in mind originally. >>>=20 >>> [Jiewen] Would you please clarify what do you mean "we" here? >>> If "we" means the bug dispatcher, it is totally OK. The dispatcher >>> just assign the bug. >>> If "we" means the developer assigned to fix the bug, it is NOT OK. = The >>> developer should take the responsibility to understand the problem. >>=20 >> By "we", I mean the edk2 community. >>=20 >>>> We can write a patch based on code analysis. It's possible to >>>> identify integer overflows based on code analysis, and it's = possible >>>> to verify the correctness of fixes by code review. Obviously = testing >>>> is always good, but many times, constructing reproducers for such >>>> issues that were found by code review, is difficult and time >>>> consuming. We can say that we don't fix vulnerabilities without >>>> reproducers, or we can say that we make an effort to fix them even = if >>>> all we have is code analysis (and not a reproducer). >>>=20 >>> [Jiewen] I would say: yes and no. >>> Yes, I agree with you that it might be difficult and time consuming = to >>> construct the reproducer. >>> However, "obviously" is a subject term. Someone may think something = is >>> obvious, but other people does not. >>> We should be clear the responsibility of the patch provider is to >>> provide high quality patch. >>> Having basic unit test is the best way to prove that the fix is = good. >>>=20 >>> I have seen bad cases when I ask for the test for patch, then the >>> answer I got is: "I test the windows boot". >>> But the test - windows boot - has nothing related to the patch. It >>> only proves no regression, but cannot prove the issue described is >>> resolved. >>=20 >> Right. It would be ideal if every patch came with a unit test. But = that >> also means some folks will contribute less. >>=20 >> Consider normal (not security) patches. We require that all function >> return values be checked (unless it really doesn't matter if a = function >> call fails). If a function call fails, we need to roll back the = actions >> taken thus far. Release resources and so on. This is why we have the >> "cascade of error handling labels" pattern. >>=20 >> But, of course, we don't test every possible error path in the code. = So >> what's the solution there: >>=20 >> - reject such patches that carefully construct the error paths, but = do >> not provide unit tests with complete error path coverage? >>=20 >> - say that we don't care about thorough error paths, so let's just = hang, >> or leak resources, whenever something fails? >>=20 >> Personally I prefer the detailed error paths. They need to be written >> and reviewed carefully. And they can be accepted even if they are not >> tested with complete coverage. >>=20 >> Some people think otherwise; they say no untested (untestable) code >> should ever be merged. >>=20 >> Back to security patches -- creating reproducers usually requires a >> setup (tools, expertise, time allocation etc) that is different from = a >> "normal" setup. It may require specialized binary format editors, >> expertise in "penetration testing", and so on. >>=20 >> I mostly know the C language rules that pertain to integer and buffer >> overflows, so I can usually spot their violations in C code, and = propose >> fixes for them too. But I'm not a security researcher, so I don't = write >> exploits as a norm -- I don't even investigate, generally speaking, = the >> potential practical impact of "undefined behavior". When there's a >> buffer overflow or integer overflow in the code, that's the *end* of = the >> story for me, while it's the *start* of the work for a security >> researcher. >>=20 >> When you require reproducers for all security patches, you restrict = the >> potential contributor pool to security researchers. >>=20 >>> Let's think again in this case, if the patch provider does some = basic >>> unit test, he/she may find out the problem by himself/herself. >>> That can save other people's time to review. >>>=20 >>> I don't prefer to move the responsibility from patch provider to the >>> code reviewer to check if the fix is good. >>> Otherwise, the code reviewer may be overwhelmed. >>>=20 >>> We may clarify and document the role and responsibility in EDKII >>> clearly. Once that is ready, we can follow the rule. >>> Before that is ready, in this particular case, I still prefer we = have >>> producer to prove the patch is good. >>=20 >> OK, thanks for explaining. >>=20 >> Given that I'm unable to create such a PE file (from scratch or by >> modifying another one), I won't post the patches stand-alone. >>=20 >>>> So the above paragraph concerns "correctness". Regarding >>>> "completeness", I guarantee you that this patch does not fix *all* >>>> problems related to PE parsing. (See the other BZ tickets.) It does >>>> fix *one* issue with PE parsing. We can say that we try to fix such >>>> issues gradually (give different CVE numbers to different issues, = and >>>> address them one at a time), or we can say that we rewrite PE = parsing >>>> from the ground up. (BTW: I have seriously attempted that in the >>>> past, and I gave up, because the PE format is FUBAR.) >>>=20 >>> [Jiewen] Maybe there is misunderstanding. >>> I do not mean to let patch provider to fix all issue in PE parsing. >>> Just like we cannot file one Bugzilla to fix all issue in EDKII - it >>> is unfair. >>>=20 >>> What I mean is that the patch provider should guarantee the >>> correctness and completeness of the issue in the bug report. >>>=20 >>> One faked bad example of correctness is: >>> A bug report is file to say: the code has overflow class A. >>> The factor is: the code has overflow class A at line X and line = Y. >>> The patch only modified some code at line X, but the overflow >>> class A at line X still exists. >>>=20 >>> One faked bad example of completeness is: >>> A bug report is file to say: the code has overflow class A. >>> The factor is: the code has overflow class A at line X and line = Y. >>> The patch only fixed the overflow class A at line X but not line >>> Y. >>>=20 >>> The patch provider should take responsibility to do that work >>> seriously to find out issue in line X and line Y and fix them. >>> He/she may choose to just fix line X and line Y. Rewrite is whole >>> module is NOT required. >>=20 >> I agree completely. >>=20 >> My point was that we need the bug report to be precise, in the first >> place. If the bug report doesn't clearly identify lines X and Y, we = will >> likely not get the completeness part right. >>=20 >> "Clearly identify" may mean spelling out lines X and Y specifically. = Or >> it may mean defining "class A" sufficiently clearly that someone else >> reading the affected function can find X and Y themselves. >>=20 >>> If I can give some comment, I would think about the provide the fix = in >>> BasePeCoffLib. >>=20 >> =46rom a software design perspective, you are 100% right. >>=20 >> Unfortunately, I can't do it. >>=20 >> That's what I mentioned before -- I had tried rewriting = BasePeCoffLib, >> because in my opinion, BasePeCoffLib is unsalvageable in its current >> form. And I gave up on the rewrite. >>=20 >> Please see the following email. I sent it to some people off-list, on >> 2020-Feb-14: >>=20 >>> There are currently four (4) TianoCore security BZs (1957, 1990, = 1993, >>> 2215), embargoed, that describe various ways in which cunningly >>> crafted PE images can evade Secure Boot verification. >>>=20 >>> [...] >>>=20 >>> Primarily, I just couldn't find my peace with the idea that fixing >>> such PE/COFF parsing mistakes (integer overflows, buffer overflows) >>> *must* be a one-by-one fixing game. I wanted an approach that would >>> fix these *classes* of vulnerabilities, in PE/COFF parsing. >>>=20 >>> So, beginnning of this February I returned to this topic, and spent >>> two days on prototyping / researching a container / interval based >>> approach. Here's one of the commit messages, as a way of explaining: >>>=20 >>> OvmfPkg/DxePeCoffValidatorLib: introduce CONTAINER type and = helper funcs >>>=20 >>> For validating the well-formedness of a PE/COFF file, introduce = the >>> CONTAINER type, and some workhorse functions. (The functions = added in this >>> patch will not be called directly from the code that will = process PE/COFF >>> structures.) >>>=20 >>> The CONTAINER type describes a contiguous non-empty interval in = a PE/COFF >>> file (on-disk representation, or in-memory representation). = Containers can >>> be nested. The data from scalar-sized containers can be read = out, as part >>> of their creation. For on-disk representations of PE/COFF files, = scalar >>> reads are permitted; for in-memory representations, no data = access is >>> permitted (only CONTAINER tracking / nesting). >>>=20 >>> The goals of CONTAINER are the following: >>>=20 >>> - enforce the proper nesting of PE/COFF structures (structure = boundaries >>> must not be crossed by runs of data); >>>=20 >>> - prevent integer overflows and buffer overflows; >>>=20 >>> - prevent zero-size structures; >>>=20 >>> - prevent infinite nesting by requiring proper sub-intervals; >>>=20 >>> - prevent internal PE/COFF pointers from aliasing each other = (unless they >>> point at container and containee structures); >>>=20 >>> - terminate nesting at scalar-sized containers; >>>=20 >>> - assuming an array of pointers is processed in increasing = element order, >>> enforce that the pointed-to objects are located at increasing = offsets >>> too; >>>=20 >>> - assign human-readable names to PE/COFF structures and fields, = for >>> debugging PE/COFF malformations. >>>=20 >>> Because, several of the vulnerabilities exploited cross-directed and >>> aliased internal pointers in PE/COFF files. >>>=20 >>> Two days of delirious spec reading and coding later, and 2000+ lines >>> later, I decided that my idea was unviable. The PE/COFF spec was so >>> incredibly mis-designed and crufty that enforcing the *internal* >>> consistency of all the size fields and the internal pointers would >>> inevitably fall into one of the following categories: >>>=20 >>> - the checks wouldn't be strict enough, and some nasty images would >>> slip through, >>>=20 >>> - the checks would be too strict, and some quirky, but valid, images >>> would be unjustifiedly caught. >>>=20 >>> So I gave up and I've accepted that it remains a whack-a-mole game. >>> [...] >>>=20 >>> (NB: I don't claim that ELF is not similarly brain-damaged.) >>=20 >> So now, I've only considered contributing patches for bug#2215 = because >> the code in question resides in DxeImageVerificationLib, and *not* in >> BasePeCoffLib. I'm not going to touch BasePeCoffLib -- in my opinion, >> BasePeCoffLib is unfixable without a complete rewrite. >>=20 >> I would *like* if BasePeCoffLib were fixable incrementally, but I = just >> don't see how that's possible. >>=20 >> In support of my opinion, please open the following bugzilla ticket: >>=20 >> https://bugzilla.tianocore.org/show_bug.cgi?id=3D2643 >>=20 >> and search the comments (with the browser's in-page search feature, = such >> as Ctrl+F) for the following expression: >>=20 >> new PE loader >>=20 >> I understand exactly what Vitaly and Marvin meant in those comments. = :( >>=20 >> Thanks, >> Laszlo >>=20 --Apple-Mail=_6C23E395-2BC8-4C27-BFBE-26EF85B61453 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 As = a follow-up to Marvin=E2=80=99s e-mail and in the reference of the = thread. I should add that to make the result more reliable we not only = hope for the reviewer experience and testing, but also write formal = proofs for several code properties. This is the primary reason the C = code has not yet left the facility, despite being mostly written. If you = feel interested, we use an opensource inhouse-written tool, = AstraVer[1][2], which is an extension of Frama-C[3].

Best regards,
Vitaly

[1] https://arxiv.org/pdf/1809.00626.pdf
[2] https://www.isprasopen.ru/2018/docs/Volkov.pdf
[3] https://frama-c.com

18 = =D0=B0=D0=B2=D0=B3. 2020 =D0=B3., =D0=B2 13:24, Marvin H=C3=A4user = <mhaeuser@posteo.de> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0= =D0=BB(=D0=B0):


Good day everyone,

First off, for your information, I'm sending = from my new e-mail address
from now on.

Please excuse me, I cannot read your entire thread right now, = I will
definitely make sure to catch up as soon as time = permits, but I just
wanted to confirm we are indeed = working on a reimplementation of the PE
loader.
It involves correcting several security issues (which will be = detailed
as the patches are sent as anything else would be = too much work for us
right now), reducing code duplication = (how often is the hashing
algorithm duplicated across = edk2? :) ) and a more or less experimental
approach to = formal verification. We plan to submit it this year, however
please note that this is a low priority project and is not = being worked
on on a full-time basis.

Please let us know about your own plans so we do not end up = duplicating
work.

Best = regards,
Marvin

Am 18.08.2020 = um 12:17 schrieb Laszlo Ersek:
Hi Jiewen,

(+Marvin, +Vitaly)

On 08/18/20 01:23, Yao, Jiewen wrote:
-----Original Message-----
From: devel@edk2.groups.io = <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Tuesday, August 18, 2020 12:53 = AM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
xiewenyi2@huawei.com; = Wang, Jian J <jian.j.wang@intel.com>
Cc: huangming23@huawei.com; songdongkuang@huawei.com
Subject: Re: = [edk2-devel] [PATCH EDK2 v2 1/1]
SecurityPkg/DxeImageVerificationLib:Enhanced verification of = Offset

[...]

However, = I do think the producer is mandatory for a fix or at least a
security fix.
The owner to fix the issue should = guarantee the patch is good.
The owner shall never rely on = the code reviewer to figure out if the
patch is good and = complete.

I have some bad experience that = bug owner just wrote a patch and tried
to fix a problem, = without any test.
And it happened passed code review from = someone who does not well
understand the problem, but give = rb based upon the time pressure.
Later, the fix was = approved to be useless.

In my memory, at = least 3 cases were security fix. They are found, just
because they are sensitive, more people took a look later.
    It was simple. It was one-line = change.
   But it has not test, and it was = wrong.
"It was ridiculous" -- commented by the people who = find the so-called
security fix does not fix the issue.

Just because sloppy/rushed = reviews exist, and just because reviewers
operate under = time pressure, we should not automatically reject security
fixes that come without a reproducer.

Some organizations do develop reproducers, but they never = share them
publicly (for fear of abuse by others).

But more importantly, in an open development = project, a developer could
have time and expertise to = contribute a fix, but not to create a
reproducer.

- If we make contributing harder, fewer people = will upstream their
  fixes.

- If we make contributing harder, then contributions that do = make it to
  the tree will be of higher = quality.

Both statements ring true to me -- = so it's a tradeoff.

(By "we", I mean the = edk2 community.)

Additionally, the exact = statement that the bug report does make,
namely

  it's possible to overflow Offset = back to 0 causing an endless loop

is wrong = (as far as I can tell anyway). It is not "OffSet" that can
be overflowed to zero, but the *addend* that is added to = OffSet can
be overflowed to zero. Therefore the infinite = loop will arise because
OffSet remains stuck at its = present value, and not because OffSet
will be re-set to = zero.

For the reasons, we can only = speculate as to what the actual problem
is, unless Jian = decides to join the discussion and clarifies what he
had = in mind originally.

[Jiewen] = Would you please clarify what do you mean "we" here?
If = "we" means the bug dispatcher, it is totally OK. The dispatcher
just assign the bug.
If "we" means the = developer assigned to fix the bug, it is NOT OK. The
developer should take the responsibility to understand the = problem.

By "we", I mean the = edk2 community.

We can write a patch = based on code analysis. It's possible to
identify integer = overflows based on code analysis, and it's possible
to = verify the correctness of fixes by code review. Obviously testing
is always good, but many times, constructing reproducers for = such
issues that were found by code review, is difficult = and time
consuming. We can say that we don't fix = vulnerabilities without
reproducers, or we can say that we = make an effort to fix them even if
all we have is code = analysis (and not a reproducer).

[Jiewen] I would say: yes and no.
Yes, I agree = with you that it might be difficult and time consuming to
construct the reproducer.
However, "obviously" = is a subject term. Someone may think something is
obvious, = but other people does not.
We should be clear the = responsibility of the patch provider is to
provide high = quality patch.
Having basic unit test is the best way to = prove that the fix is good.

I have seen bad = cases when I ask for the test for patch, then the
answer I = got is: "I test the windows boot".
But the test - windows = boot - has nothing related to the patch. It
only proves no = regression, but cannot prove the issue described is
resolved.

Right. It = would be ideal if every patch came with a unit test. But that
also means some folks will contribute less.

Consider normal (not security) patches. We require that all = function
return values be checked (unless it really = doesn't matter if a function
call fails). If a function = call fails, we need to roll back the actions
taken thus = far. Release resources and so on. This is why we have the
"cascade of error handling labels" pattern.

But, of course, we don't test every possible error path in = the code. So
what's the solution there:

- reject such patches that carefully construct the error = paths, but do
  not provide unit tests with = complete error path coverage?

- say that we = don't care about thorough error paths, so let's just hang,
=   or leak resources, whenever something fails?
Personally I prefer the detailed error paths. They need to = be written
and reviewed carefully. And they can be = accepted even if they are not
tested with complete = coverage.

Some people think otherwise; they = say no untested (untestable) code
should ever be = merged.

Back to security patches -- = creating reproducers usually requires a
setup (tools, = expertise, time allocation etc) that is different from a
"normal" setup. It may require specialized binary format = editors,
expertise in "penetration testing", and so on.

I mostly know the C language rules that = pertain to integer and buffer
overflows, so I can usually = spot their violations in C code, and propose
fixes for = them too. But I'm not a security researcher, so I don't write
exploits as a norm -- I don't even investigate, generally = speaking, the
potential practical impact of "undefined = behavior". When there's a
buffer overflow or integer = overflow in the code, that's the *end* of the
story for = me, while it's the *start* of the work for a security
researcher.

When you require = reproducers for all security patches, you restrict the
potential contributor pool to security researchers.

Let's = think again in this case, if the patch provider does some basic
unit test, he/she may find out the problem by = himself/herself.
That can save other people's time to = review.

I don't prefer to move the = responsibility from patch provider to the
code reviewer to = check if the fix is good.
Otherwise, the code reviewer may = be overwhelmed.

We may clarify and document = the role and responsibility in EDKII
clearly. Once that is = ready, we can follow the rule.
Before that is ready, in = this particular case, I still prefer we have
producer to = prove the patch is good.

OK, = thanks for explaining.

Given that I'm = unable to create such a PE file (from scratch or by
modifying another one), I won't post the patches = stand-alone.

So the above paragraph = concerns "correctness". Regarding
"completeness", I = guarantee you that this patch does not fix *all*
problems = related to PE parsing. (See the other BZ tickets.) It does
fix *one* issue with PE parsing. We can say that we try to = fix such
issues gradually (give different CVE numbers to = different issues, and
address them one at a time), or we = can say that we rewrite PE parsing
from the ground up. = (BTW: I have seriously attempted that in the
past, and I = gave up, because the PE format is FUBAR.)

[Jiewen] Maybe there is misunderstanding.
I do = not mean to let patch provider to fix all issue in PE parsing.
Just like we cannot file one Bugzilla to fix all issue in = EDKII - it
is unfair.

What I = mean is that the patch provider should guarantee the
correctness and completeness of the issue in the bug = report.

One faked bad example of = correctness is:
    A bug report is = file to say: the code has overflow class A.
=     The factor is: the code has overflow class A at = line X and line Y.
    The patch only = modified some code at line X, but the overflow
=     class A at line X still exists.

One faked bad example of completeness is:
=     A bug report is file to say: the code has = overflow class A.
    The factor is: = the code has overflow class A at line X and line Y.
=     The patch only fixed the overflow class A at = line X but not line
    Y.

The patch provider should take responsibility = to do that work
seriously to find out issue in line X and = line Y and fix them.
He/she may choose to just fix line X = and line Y. Rewrite is whole
module is NOT required.

I agree completely.

My point was that we need the bug report to be = precise, in the first
place. If the bug report doesn't = clearly identify lines X and Y, we will
likely not get the = completeness part right.

"Clearly identify" = may mean spelling out lines X and Y specifically. Or
it = may mean defining "class A" sufficiently clearly that someone else
reading the affected function can find X and Y themselves.

If I can = give some comment, I would think about the provide the fix in
BasePeCoffLib.

= =46rom a software design perspective, you are 100% right.

Unfortunately, I can't do it.

That's what I mentioned before -- I had tried rewriting = BasePeCoffLib,
because in my opinion, BasePeCoffLib is = unsalvageable in its current
form. And I gave up on the = rewrite.

Please see the following email. I = sent it to some people off-list, on
2020-Feb-14:

There are = currently four (4) TianoCore security BZs (1957, 1990, 1993,
2215), embargoed, that describe various ways in which = cunningly
crafted PE images can evade Secure Boot = verification.

[...]

Primarily, I just couldn't find my peace with the idea that = fixing
such PE/COFF parsing mistakes (integer overflows, = buffer overflows)
*must* be a one-by-one fixing game. I = wanted an approach that would
fix these *classes* of = vulnerabilities, in PE/COFF parsing.

So, = beginnning of this February I returned to this topic, and spent
two days on prototyping / researching a container / interval = based
approach. Here's one of the commit messages, as a = way of explaining:

=     OvmfPkg/DxePeCoffValidatorLib: introduce = CONTAINER type and helper funcs

=     For validating the well-formedness of a PE/COFF = file, introduce the
    CONTAINER = type, and some workhorse functions. (The functions added in this
    patch will not be called directly = from the code that will process PE/COFF
=     structures.)

=     The CONTAINER type describes a contiguous = non-empty interval in a PE/COFF
=     file (on-disk representation, or in-memory = representation). Containers can
=     be nested. The data from scalar-sized containers = can be read out, as part
    of their = creation. For on-disk representations of PE/COFF files, scalar
    reads are permitted; for in-memory = representations, no data access is
=     permitted (only CONTAINER tracking / = nesting).

    The = goals of CONTAINER are the following:

=     - enforce the proper nesting of PE/COFF = structures (structure boundaries
=       must not be crossed by runs of = data);

    - prevent = integer overflows and buffer overflows;

=     - prevent zero-size structures;

    - prevent infinite nesting by = requiring proper sub-intervals;

=     - prevent internal PE/COFF pointers from = aliasing each other (unless they
=       point at container and containee = structures);

    - = terminate nesting at scalar-sized containers;

    - assuming an array of pointers is = processed in increasing element order,
=       enforce that the pointed-to objects = are located at increasing offsets
=       too;

=     - assign human-readable names to PE/COFF = structures and fields, for
=       debugging PE/COFF malformations.

Because, several of the vulnerabilities = exploited cross-directed and
aliased internal pointers in = PE/COFF files.

Two days of delirious spec = reading and coding later, and 2000+ lines
later, I decided = that my idea was unviable. The PE/COFF spec was so
incredibly mis-designed and crufty that enforcing the = *internal*
consistency of all the size fields and the = internal pointers would
inevitably fall into one of the = following categories:

- the checks wouldn't = be strict enough, and some nasty images would
=   slip through,

- the checks = would be too strict, and some quirky, but valid, images
=   would be unjustifiedly caught.

So= I gave up and I've accepted that it remains a whack-a-mole game.
[...]

(NB: I don't claim that = ELF is not similarly brain-damaged.)

So now, I've only considered contributing patches for = bug#2215 because
the code in question resides in = DxeImageVerificationLib, and *not* in
BasePeCoffLib. I'm = not going to touch BasePeCoffLib -- in my opinion,
BasePeCoffLib is unfixable without a complete rewrite.

I would *like* if BasePeCoffLib were fixable = incrementally, but I just
don't see how that's = possible.

In support of my opinion, please = open the following bugzilla ticket:

=   https://bugzilla.tianocore.org/show_bug.cgi?id=3D2643

and search the comments (with the browser's = in-page search feature, such
as Ctrl+F) for the following = expression:

  new PE loader

I understand exactly what Vitaly and Marvin = meant in those comments. :(

Thanks,
Laszlo


= --Apple-Mail=_6C23E395-2BC8-4C27-BFBE-26EF85B61453-- --Apple-Mail=_85F7952A-87F5-4C32-897E-FCB52AC8D93C Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEsLABAI5Y5VbvBdmpL8K2O86Eyz4FAl87x9AACgkQL8K2O86E yz4fDw//Yq8Ry0FQOCgUP+aasFNIEGgTP/C1/DZzGzBxS4SYDj0DUZHbbj9yz45m GIbmAhNKXF6DEGFzEGLTqwt2WZfyEI7NnG4q8wyruf8a97cB2n9oIZhU6kVtraG8 YMk4acuL6FOyarWZjxjzwNSNASv5NUR/bLP0Yt654aEcVXHWFrWFH8xJXZHA4u13 CbSzr3EqNa/m5WmlhB012y7ROwCzlhXFx5QOKa65vx9IrZRX0LJfvJP+xisuhUxO B0yqZayiLPirBLLQmlb5+UPGBUY/YZpbEnCMjdsP9tLCxsDFzQm+y4VIGGN9Z4l+ fI7ew3g726mxJJ0YKiu3x+uKvUZvnzc71RzF4zJYAk7DwoKYyyotmAsOegzubh8H Kxe3vEgxoxtLFYkJsCy/d7yY2QtxxlTC06INDiDMYrwFKHi8ziyHtlqyIOAp3DaF z45Je4S1Pqwlq2EeAzV6Slh3sSRgJ87HnvjnGWEzB0IvaqTwR9uU1Qd+jHvKVRwh sPjjuxiKNQrOY0xWOhwHyGlkH9WiC2SuizG3NEJA7B3reDmgtUBmqFXMRGXf+XQ/ zru06tmPtef6d+6OGg74YS/oD7ZMi5Qmns/k5KwcmPkHuD+JWqf0zEYqM/LZeU4O llBgoqzGa8bWg0ru8n7RPphRO6WMYUOfUq5bDr6y4aHmm2Tr8WY= =E9YT -----END PGP SIGNATURE----- --Apple-Mail=_85F7952A-87F5-4C32-897E-FCB52AC8D93C--