OK. The second point is my personal understanding on embargo process. I might be wrong. Maybe we should let infosec team to clarify the process.
Thank you Yao Jiewen From: Bret Barkelew <bret.barke...@microsoft.com> Sent: Wednesday, August 19, 2020 1:29 AM To: devel@edk2.groups.io; Yao, Jiewen <jiewen....@intel.com>; ler...@redhat.com Cc: xiewen...@huawei.com; Wang, Jian J <jian.j.w...@intel.com>; huangmin...@huawei.com; songdongku...@huawei.com; Marvin Häuser <mhaeu...@outlook.de>; Vitaly Cheptsov <vit9...@protonmail.com> Subject: RE: [edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset Jiewen, I don’t completely agree with your second point. If the underlying issue is already out of embargo, and we’ve just failed to fix it, that’s not a new issue. I would want to see fixes to the fixes fast-tracked (or at least heavily prioritized), rather than re-entering a full embargo period. - Bret From: Yao, Jiewen via groups.io<mailto:jiewen.yao=intel....@groups.io> Sent: Tuesday, August 18, 2020 6:12 AM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; ler...@redhat.com<mailto:ler...@redhat.com> Cc: xiewen...@huawei.com<mailto:xiewen...@huawei.com>; Wang, Jian J<mailto:jian.j.w...@intel.com>; huangmin...@huawei.com<mailto:huangmin...@huawei.com>; songdongku...@huawei.com<mailto:songdongku...@huawei.com>; Marvin Häuser<mailto:mhaeu...@outlook.de>; Vitaly Cheptsov<mailto:vit9...@protonmail.com> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset Hi Laszlo I agree with on your most points. It is all about *return of investment* or *risk control*. Like we cannot pursue 100% security because we will bankrupt ourselves if so. Here I just raise my concern. 1) If we alway allow developers’ low quality patch without test, the overall quality will become lower and lower. Personally I have no confidence to help catch all those issues. You are the role model on code review. But not all people review the code like you. We need both expertise and time for code review. 2) I purposely separate security fix from non security one, because the process is different. The embargo might be 6 months. What if we found the security patch does not fix the problem after embargo? Unlike we send one more patch tomorrow, we need wait for another 6 months... thank you! Yao, Jiewen > 在 2020年8月18日，下午6:17，Laszlo Ersek > <ler...@redhat.com<mailto:ler...@redhat.com>> 写道： > > Hi Jiewen, > > (+Marvin, +Vitaly) > > On 08/18/20 01:23, Yao, Jiewen wrote: >>> -----Original Message----- >>> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >>> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo >>> Ersek >>> Sent: Tuesday, August 18, 2020 12:53 AM >>> To: Yao, Jiewen <jiewen....@intel.com<mailto:jiewen....@intel.com>>; >>> devel@edk2.groups.io<mailto:devel@edk2.groups.io>; >>> xiewen...@huawei.com<mailto:xiewen...@huawei.com>; Wang, Jian J >>> <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>> >>> Cc: huangmin...@huawei.com<mailto:huangmin...@huawei.com>; >>> songdongku...@huawei.com<mailto:songdongku...@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. > > From 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2643&data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cada52c41a3db41968b7408d843785e58%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637333531565237004&sdata=C59ZmrHEONxeui4lkIo1RFTTLUOEuU8OQXUbWoS9JWc%3D&reserved=0 > > 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 > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64411): https://edk2.groups.io/g/devel/message/64411 Mute This Topic: https://groups.io/mt/76165658/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
