Re: [edk2] [PATCH] NX/ASLR/GS/RTCs Security Features

Subject: Re: [edk2] [PATCH] NX/ASLR/GS/RTCs Security Features

From: Eugene Khoruzhenko <Eugene_Khoruzhenko@phoenix.com>

To: "edk2-devel@lists.sourceforge.net" <edk2-devel@lists.sourceforge.net>

Date: 2012-05-22 21:39:08

All right, we can change our PageTable.c code to call gCpu->SetMemoryAttributes if gCpu != NULL, just like this is done in Gcd.c. Note however, that if we only rely on gCpu, the system will remain vulnerable (from the Data Execution Prevention perspective) up until the point the CPU arch protocol is installed. On some platforms there may be quite a few DXE drivers loaded before gCpu becomes available, including drivers from 3rd parties - this weakens the defense we desired to bring with the NX/DEP feature. Also, if a given gCpu->SetMemoryAttributes implementation does not support the EFI_MEMORY_XP attribute (which is currently the case for most silicon packages), the NX/DEP feature will be unavailable until that support is added.

In order to allow setting the memory attributes early, we can call our library instead of gCpu->SetMemoryAttributes when gCpu == NULL. We can also fall back to calling the library if gCpu->SetMemoryAttributes returned EFI_UNSUPPORTED - this will allow us to introduce this feature now, while most of the SetMemoryAttributes implementations do not support EFI_MEMORY_XP. As the SetMemoryAttributes implementations add support for EFI_MEMORY_XP, the core will automatically switch to using those instead of our library.

Would the above changes address your concern?

Regards
ek

-----Original Message-----
From: Andrew Fish [mailto:afish@apple.com]
Sent: Tuesday, May 22, 2012 6:27 AM
To: edk2-devel@lists.sourceforge.net
Cc: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [PATCH] NX/ASLR/GS/RTCs Security Features



Sent from my iPad

On May 21, 2012, at 8:03 PM, Eugene Khoruzhenko  wrote:

> Andrew,
>
> Sure, we'll move the compiler-specific code into a separate library as you suggest. Only can we come up with a better name than BaseBufferOverfowIntrinsicLib, as it is too BufferOverfow specific. The compiler runtime checks (RTCs) handle more than just buffer overflow, so I'd like the name to be along these lines:
>        BaseCompilerLib
>        BaseCompilerRtcLib
>        BaseCompilerIntrinsicLib
>        BaseRtcIntrinsicLib
>        BaseIntrinsicLib
> Maybe there are other suggestions, I just don't like "BufferOverfow" in the name.
>

CompilerRuntimeCheckLib? RTC also means Real Time Clock.

> Let me clarify our page table functionality, I think there is some misunderstanding.
> 1. When I said that paging is architecture-specific rather than CPU package-specific, I meant Intel architecture vs. ARM architecture vs. etc. - not the software architecture vs. implementation terms. Just like the paging code is similar for all Intel CPUs, it is probably similar between ARM CPUs too, so it makes more sense to uplevel it rather than replicate in every CPU package. That's why we did BasePageTableLib in MdePkg; you also mentioned that you like the idea of having a library for paging - here we go.

The PI spec punts things page table to the EFI_CPU_ARCH_PROTOCOL, and I don't think it is possible to co-own the page tables.

>
> 2. We do not use MTRRs in DXE core; I said that in Intel CPU packages MTRRs are used in EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() to deal with cacheability, not paging, and we did not want to mess with Intel silicon code. We only use an MTRR in DxeIplPeim/X64/VirtualMemory.c to enable the NX feature. This code is x64-specific and is used to create page tables and set the long mode, so I think enabling the NX feature belongs there. DxeIpl for ARM is different, and this feature can be enabled in a proper place.
>

So you are confusing implementation with architecture. I can site a counter example of ARM where it is all done in page tables, and I could implement an X64 CPU driver that uses page tables in place of MTRRs. Thus making assumptions has nothing to do with the PI spec, and everything to do with reverse engineering some one else's code. The point of the PI spec is to be able to code to the spec, and I think my suggestion was the closest thing to following the spec. I'm not sure you can cleanly implement NX without updating the PI spec,

> 3. We do not sync the AP page tables with BSP in DXE core, this is done by CPU package when it enables MP and starts APs. APs get their CR3 copied from BSP, so they all point to the same *existing* page tables.
>

I've seen code do things different from this. Aren't there cases where the architecture requires TLB flushes on certain page table updates? Some architectures are less cache coherent than X64 and can require cache flushes. And again you seem to be architecting to some ones implementation and not the specs. If you look you will see that MTRRs require syncing to APs and that is why it is done in the CPU driver and not the DXE core.

Aircraft door is closing ......

> 4. Let me clarify how we setup NX. CoreInitializePageTables called from DxeMain.c does not necessarily initialize the page tables from scratch. The details for what initialization of page tables actually means is left to the specific library implementation. For example, for the BasePageTableLibIA32E implementation, if there are already existing page tables, the initialization code reconfigures those existing page tables to set page properties according to the current memory map. There are three primary steps for this implementation of the library:
>        a. Support for features is determined.
>        b. The memory map is scanned for a region that describes the currently executing code, and NX is then set on the entire memory map excluding the region that is currently executing.
>        c. The memory map is scanned a second time and NX is cleared on all pages known to contain code.
>
> Note that the library does support full generation of page tables from scratch, but this is an optional feature. Page tables are actually initially created in DxeIpl, not in DXE core.
>
> 5. I think there are a few issues with using EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() in DXE core to set EFI_MEMORY_XP attributes. Since the CPU DXE drivers are loaded by the Dispatcher later in the DXE phase, we cannot use CPU protocols in DXE core, unless they were created in PEI. Also, to reduce the security risk, we wanted to set NX on all pages (except the existing code, of course) as early as possible, e.g. right after memory services are initialized, certainly before the Dispatcher starts loading the other DXE images.
>
> On some of your other comments:
>
>> I think what make the most sense is the following:
>> 1) Page tables initialized in PEI.
> EK: no problem, in Intel architecture it is done in DxeIpl, which is technically PEI. On ARM it can be done even earlier.
>> 2) DXE Core updates existing page tables with NX bits
> EK: this is exactly what we do, please see #4 above.
>> 3) when EFI_CPU_ARCH_PROTOCOL is added DXE Core moves over to that.
> EK: I think this is too late for NX, please see #5 above.
>
>> The CPU AP would need to inherit the BSP page tables.
> EK: this is exactly what currently happens in CPU packages, please see #3 above.
>
>> PS We can set page zero to generate a page fault, so having two chunks of code initializing page tables generates issues for us.
> EK: as mentioned earlier, this should not be the case. We want a centralized page table handler, which works together with memory manager - this is what we tried to achieve. We thought about optionally removing page 0 from the table to catch NULL pointer references - this can be useful for debugging, just as your next suggestion; we can add this later.
>
>> PPS It would also be interesting to write protect freed/unallocated memory on a debug build.
> EK: we have prerequisites for this and thought about adding it as an optional feature too. This can certainly be added after we put the base paging functionality in place.
>
> Regards
> ek
>
> -----Original Message-----
> From: Andrew Fish [mailto:afish@apple.com]
> Sent: Monday, May 21, 2012 10:44 AM
> To: edk2-devel@lists.sourceforge.net
> Cc: edk2-devel@lists.sourceforge.net
> Subject: Re: [edk2] [PATCH] NX/ASLR/GS/RTCs Security Features
>
>
>
> Sent from my iPad
>
> On May 20, 2012, at 10:39 PM, Eugene Khoruzhenko  wrote:
>
>> Andrew, thanks for your suggestions and the actual code fragment.
>>
>> I agree, support for different architectures and compilers adds a good deal of complexity. We've been thinking to start with ia32/x64 implementations and then add the other platforms. Also, per Jordan's recommendation, we'll split the code into smaller independent pieces - that should simplify things a lot.
>>
>> The reason we created BaseBinSecurityLib is to abstract the common functions like AslrRandomNumber, however then we added the GS/RTCs code, which is MSVC-specific. While compiler-specific code should be organized better (separate libraries or sub-classes of BaseBinSecurityLib), routines like ReportStackFault and ReportBufferOverflow should be in one common place, correct?
>
> Yes I'd like to see the compiler generated intrinsics in a separate BaseBufferOverfowIntrinsicLib, that supports multiple compilers. So it would look like
> https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf.
>>
>> We looked at EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), but it deals with cacheability of the whole memory regions (through MTRRs), not pages.
> This is implementation and not architecture. One could argue this was more a lazy port of IA32 to x64, as opposed to the right long term x64 solution. So assuming MTRR is hard coding a design choice into the DXE core, BUT the DXE Core should just follow PI.
>
>> We did not want to mix these two things; plus, it would require all CPU packages to be reworked to support paging. Since paging implementation is very similar for all x86 CPUs I think it is architecture-specific, rather than CPU-specific - hence should go to the core.
>
> I think is more complex as what if the AP page tables need to be synced with the BSP page tables? DXE core is the wrong place for that.
>
>> So we can have BasePageTableLibArm for ARM and Null for none. It looks that CPU packages currently do not implement the EFI_MEMORY_XP attribute and cannot be whacking our page table; once they do, they would have to read the existing table and gracefully make the required region attribute modifications, rather than whacking the whole table. Or even better, CPU packages would need to use our BasePageTableLib API, rather than reimplement the page handling code.
>>
>> If DxeMain is the wrong place to initialize paging on ARM then ARM implementation of CoreInitializePageTables should be different - do not setup the tables, but do whatever is appropriate (or nothing). Then the code that sets up paging on ARM would have to be updated in a similar manner as we do in BasePageTableLib - all pages should be initially set to NX, then as pages are allocated to load executable modules, the NX attribute on these pages are cleared. If we only set NX on some region then the non-NX region may remain vulnerable for attacks. We would have to look into ARM implementation more deeply to select the right approach and ensure its security. On x86 though, setting up page tables right after CoreInitializeMemoryServices seems like the right thing to do.
>>
>
> Sorry I don't understand that statement, given you can't be in x64 mode without page tables. Also how does the DXE core run if all memory is set to NX?
>
> Seems to me the right thing to do is update DXE Core to use EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() to set EFI_MEMORY_XP as part of the startup of the memory manager. I like the idea of adding the library for paging, as the DXE IPL and DXE CPU AP both need to setup page tables.
>
> I think what make the most sense is the following:
> 1) Page tables initialized in PEI.
> 2) DXE Core updates existing page tables with NX bits
> 3) when EFI_CPU_ARCH_PROTOCOL is added DXE Core moves over to that.
>
> The CPU AP would need to inherit the BSP page tables.
>
> PS We can set page zero to generate a page fault, so having two chunks of code initializing page tables generates issues for us.
>
> PPS It would also be interesting to write protect freed/unallocated memory on a debug build.
>
>> Regards
>> ek
>>
>> -----Original Message-----
>> From: Andrew Fish [mailto:afish@apple.com]
>> Sent: Friday, May 18, 2012 3:16 PM
>> To: edk2-devel@lists.sourceforge.net
>> Subject: Re: [edk2] [PATCH] NX/ASLR/GS/RTCs Security Features
>>
>> If we move to a generic StackCheckLib I think the attached code is how to implement for the GCC/clang -fstack-protector-all compiler flag.
>>
>> Andrew Fish
>>
>> Code in this email is
>> Contributed-under: TianoCore Contribution Agreement 1.0
>>
>> /** @file
>> Base Stack Check library for GCC/clang.
>>
>> Use -fstack-protector-all compiler flag to make the compiler insert the
>> __stack_chk_guard "canary" value into the stack and check the value prior
>> to exiting the function. If the "canary" is overwritten __stack_chk_fail()
>> is called. This is GCC specific code.
>>
>> Copyright (c) 2012, Apple Inc. All rights reserved.
>> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD License >> which accompanies this distribution. The full text of the license may be found at >> http://opensource.org/licenses/bsd-license.php. >> >> THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> >> **/ >> >> #include >> #include >> #include >> #include >> >> void >> __stack_chk_fail ( >> void >> ); >> >> >> /// "canary" value that is inserted by the compiler into the stack frame. >> void *__stack_chk_guard = (void)0x0AFF; >> >> // If ASLR was enabled we could use >> //void (*__stack_chk_guard)(void) = __stack_chk_fail; >> >> /** >> Error path for compiler generated stack "canary" value check code. If the >> stack canary has been overwritten this function gets called on exit of the >> function. >> **/ >> void >> __stack_chk_fail ( >> void >> ) >> { >> DEBUG ((DEBUG_ERROR, "STACK FAULT: Buffer Overflow in function %a.\n", __builtin_return_address(0))); >> >> // >> // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings even if >> // BaseDebugLibNull is in use. >> // >> if ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) { >> CpuBreakpoint (); >> } else if ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) { >> CpuDeadLoop (); >> } >> } >> >> >> On May 17, 2012, at 8:26 AM, Jordan Justen wrote: >> >>> Given that there are several features, it seems like this should be >>> broken into quite a few smaller changes. (For instance, maybe it make >>> sense for there to be 10+ changes here?) >>> >>> It looks as though .S assembly is missing in many places. >>> >>> For the new libraries, should there be NULL versions? >>> >>> I assume blamo.txt was not meant to be included. :) >>> >>> -Jordan >>> >>> On Wed, May 16, 2012 at 7:34 PM, Eugene Khoruzhenko >>> wrote: >>>> Dear EDK2 MdeModulePkg maintainer and community, >>>> >>>> Please find the attached patch for the NX/ASLR/GS/RTCs features. Adding these features provides blanket security protection for latent vulnerabilities. >>>> >>>> The NX feature uses page tables and DXE memory management to mark pages containing data (or that do not contain code) as No Execute, causing a page fault if there is any attempt to execute code from those pages. This is to prevent code that exploits buffer overruns from including the code to be executed directly in the buffer overrun; for example, NX prevents code on the stack from being executed. This feature is implemented as a pair of libraries under MdePkg, one of which is BasePageTableLib stub library, and the other is a full implementation of the page table library for IA32E - BasePageTableLibIA32E. Integration involves changes to DxeCore and DxeIplPeim, as well as a bunch of changes to platform and silicon code to enable NXP in AP processors and SMM (not explicitly included with this patch). >>>> >>>> The ASLR feature causes PE images that are loaded to RAM to be loaded at randomized addresses. The intent is to prevent code that exploits stack buffer overruns from being able to use return oriented code from exploiting code loaded at known or fixed locations. This feature is implemented as a library that provides a randomization function called BaseBinSecurityLib. >>>> Integration involves changes to PeiCore, DxeIplPeim, DxeCore and SmmCore. >>>> >>>> GS and RTCs are to support VS2010 build with /GS and /RTCs switches enabled. >>>> Note that the /GS switch is only secure when ASLR is enabled, as we leverage ASLR's randomizing of the address of loaded code to automatically initialize the security cookie. Rather than setting the security cookie randomly in the program entrypoint code, we let PE loader set the security cookie value to the address of an arbitrarily selected function within BaseBinSecurityLib, and that address is random as a side effect of ASLR. This way, we don't have to link the full randomization code into every single driver or application. >>>> >>>> Regards, >>>> Eugene Khoruzhenko >>>> Principal Software Architect >>>> Phoenix Technologies Ltd. >>>> (425) 443-3883 >>>> >>>> >>> >>> ------------------------------------------------------------------------------ >>> Live Security Virtual Conference >>> Exclusive live event will cover all the ways today's security and >>> threat landscape has changed and how IT managers can respond. Discussions >>> will include endpoint security, mobile security and the latest in malware >>> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/edk2-devel >> >> >> ------------------------------------------------------------------------------ >> Live Security Virtual Conference >> Exclusive live event will cover all the ways today's security and >> threat landscape has changed and how IT managers can respond. Discussions >> will include endpoint security, mobile security and the latest in malware >> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel >> >> ------------------------------------------------------------------------------ >> Live Security Virtual Conference >> Exclusive live event will cover all the ways today's security and >> threat landscape has changed and how IT managers can respond. Discussions >> will include endpoint security, mobile security and the latest in malware >> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and > threat landscape has changed and how IT managers can respond. Discussions > will include endpoint security, mobile security and the latest in malware > threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and > threat landscape has changed and how IT managers can respond. Discussions > will include endpoint security, mobile security and the latest in malware > threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel