Enclave Properties #160
Enclave Properties #160
Conversation
…soft/openenclave into feature.enclave-properties
…soft/openenclave into feature.enclave-properties
host/CMakeLists.txt
Outdated
| @@ -14,7 +14,7 @@ set(PLATFORM_SRC | |||
| linux/entersim.S | |||
| linux/exception.c | |||
| linux/hostthread.c | |||
| sign.c | |||
| linux/signsgx.c | |||
There was a problem hiding this comment.
Can we use sgxsign.c for consistency with existing files like sgxmeasure.c?
include/openenclave/bits/sgxtypes.h
Outdated
| #define SGX_ATTRIBUTES_DEFAULT_XFRM 0x0000000000000007 | ||
|
|
||
| /* | ||
| **============================================================================== | ||
| ** | ||
| ** OE_SGXSigStruct | ||
| ** SGX_Attributes: |
There was a problem hiding this comment.
Duplicate of lines 71-82?
There was a problem hiding this comment.
Resolved. Thanks for catching that.
| #define SGX_SIGSTRUCT_ATTRIBUTEMASK_FLAGS 0Xfffffffffffffffb | ||
|
|
||
| /* SGX_SigStruct.xfrm */ | ||
| #define SGX_SIGSTRUCT_ATTRIBUTEMASK_XFRM 0x0000000000000000 |
There was a problem hiding this comment.
Nonblocking: Based on the conversation with Intel, this is probably wrong (this is a permissive mask, so my original comment was incorrect) but we can wait on Intel to give us a definitive response and fix it then.
There was a problem hiding this comment.
Okay, we can address later. By the way, the original mask probably came from dumping Intel signatures.
tests/props/host/host.c
Outdated
| OE_TEST(config->padding == 0); | ||
| OE_TEST(config->attributes == attributes); | ||
|
|
||
| /* Initailize a zero-filled sigstruct */ |
| return (x > 0 && x < OE_MAX_UINT64) ? true : false; | ||
| } | ||
|
|
||
| OE_INLINE bool OE_ValidAttributes(uint64_t x) |
There was a problem hiding this comment.
Per our naming discussion, should we tag these as OE_SGX_* since all these checks are SGX specific?
There was a problem hiding this comment.
Resolved as OE_SGXValidAttributes for now (you can rename during your merge depending on what we decide about OE_SGX_ vs OE_SGX.
tools/oesign/main.c
Outdated
| props.settings.attributes |= OE_SGX_FLAGS_DEBUG; | ||
| const char* fieldName; | ||
|
|
||
| if (OE_ValidateEnclaveProperties_SGX(&props, &fieldName) != OE_OK) |
There was a problem hiding this comment.
Nice, thanks for implementing the merge and validation. To make sure we're on the same page, the expected experience is we never provide defaults and the author is expected to provide a fully valid set of parameters through any combination of macro and signing, correct?
Can you also file an issue to track adding more verbose help/instructions to this tool? (like oegen)
There was a problem hiding this comment.
Resolved. We were defaulting the ProductID and SecurityVersion so I fixed that and added these to every .conf file.
host/windows/aesm.c
Outdated
| @@ -285,7 +285,7 @@ OE_Result AESMGetLaunchToken( | |||
| modulus, /* public_key */ | |||
| OE_KEY_SIZE, /* public_key_size */ | |||
| (PUINT8)attributes, /* se_attributes */ | |||
| sizeof(OE_SGXAttributes), /* se_attributes_size */ | |||
| sizeof(SGX_Attributes), /* se_attributes_size */ | |||
There was a problem hiding this comment.
Nit: restore indent for comment
host/enclave.h
Outdated
|
|
||
| // This structure is copied from the enclave properties section during | ||
| // enclave loading. | ||
| OE_EnclaveProperties_SGX properties; |
There was a problem hiding this comment.
This bothers me more than it probably should. I get that it's a quick way to solve the test question, but it's a relatively large structure that really doesn't serve any purpose for the lifetime of the app after the enclave is created. It's also SGX-specific, which means we'll likely bloat the OE_Enclave struct in the future to accommodate each variant (even SGX2 properties, for example) or we will need to make this some kind of union or obfuscated type.
I would prioritize getting this set of changes in, but could you file an issue to track revisiting a better way to handle this?
There was a problem hiding this comment.
Resolved. Yeah, it feels like a hack. I removed it and added a function to load the enclave properties in the test case.
host/create.c
Outdated
| OE_Result result = OE_UNEXPECTED; | ||
| uint8_t* p = sectionData; | ||
| uint8_t* end = p + sectionSize; | ||
| size_t r = end - p; |
There was a problem hiding this comment.
Nit: prefer meaningful names (e.g. bytesRemaining) ... also, you could just assign sectionSize here instead of doing math.
host/create.c
Outdated
| const void* data; | ||
| size_t size; | ||
|
|
||
| if (properties) |
There was a problem hiding this comment.
Okay, we might want to do a scrub to ensure that we apply this principle consistently. I wasn't aware that's the desire in this codebase. Can we track this in an issue and also communicate the expectation more broadly?
mikbras> Resolved. Issue #191: Verify that all function output variables are initialized on function exit
tools/oesign/main.c
Outdated
| } | ||
| } | ||
| /* Debug option is present */ | ||
| if (options->debug != OE_MAX_UINT8) |
There was a problem hiding this comment.
Didn't catch this earlier, but we set the debug flag even if options->debug is 0.
Since _CheckForMissingOptions() doesn't require that this is set, why not just simplify and make this nominally a flag value? i.e. define debug as a bool that defaults to false, and in _LoadConfigFile(), only set debug to true if the parsed value is 1.
|
Merging enclave properties feature into master |
This PR introduces enclave properties per the OE API surface design (please see that document for details as well as issue #5). This design introduces the .oeinfo section that contains one or more initialized enclave property structures which are injected into the image by:
- The OE_SET_ENCLAVE_SGX macro, or
- The OESIGN tool
There is one deviation from the original design: unsigned debug images may be loaded as designed, but the loader must auto-sign them with a test key since SGX will not accept unsigned images.
Enclave properties supersede the OE_SignatureSection structure that now resides in .oesig.