Improvements to SGX attestation verification #490

bvavala · 2024-05-13T20:22:57Z

This PR adds checks to the attestation verification procedures, as discussed in #195 .
The additions/modifications are as follows:

it makes the sgx debug flag dependent on the PDO_DEBUG_BUILD environment variable. In the cmake file for building the enclave, it use CMAKE_BUILD_TYPE (debug/release) to ensure the the NDEBUG variable is set appropriately. In the swig wrapper, it defines the EDEBUG variable (as defined and used in the SGX SDK) to ensure that the the SDK sets the SGX_DEBUG_FLAG correctly when creating the enclave. These modifications are applied to both the eservice and pservice.
it generates the xml configuration file for the enclave. This is necessary to set the DisableDebug field appropriately. The xml file is generated in the build folder since it is a build artifact.
it updates the enclave registration policy and the transaction processor. The policy now contains the sgx_debug_flag. So now we totally have 3 modes of operation: sim mode (no policy checks), hw mode with enclave debug disabled, hw mode with enclave debug enabled. The TP will enforce the flag check accordingly when it receives enclave registrations. Additionally, it will also check that the enclave runs in 64-bit mode.
it updates the pservice with additional attestation checks before provisioning an enclave. To do this, it extends the metadata header built by the eservice to provide info about the enclave to the pservice. Originally, it contained only mrenclave; now it contains the enclave attributes and relative mask. The pservice always checks the 64-bit flag. Also, it checks the debug flag in the attributes only if this is set in the mask -- if set in the mask, it must be enforced. (Note: the mask is derived from how the SDK manages the DisableDebug field in the configuration file; if 1, it forces the debug flag to 0; if 0, it considers the debug flag irrelevant, and the untrusted host which runs the enclave can set it arbitrarily; in this last case, the debug flag is still verifiable through the attestation, and its value may differ compared to the one appearing in the signed enclave metadata).
it adds consistency checks in cmake files to disallow builds where (sgx_mode=sim, PDO_DEBUG_BUILD=0) and (sgx_mode=hw, cmake_build_type=release, PDO_DEBUG_BUILD=1).

cmickeyb · 2024-05-14T15:18:43Z

As discussed, please take advantage of build/cmake/(SGX|ProjectVariables).cmake. Much of the complexity that you introduce in the build/configuration process is not necessary and inconsistent with the way other artifacts are created.

g2flyer

Seems to work as expected with docker in any combination PDO_DEBUG_BUILD set/not-set, test/sgx_test and ubuntu 22.04/24.04. Code mostly looks good, e.g., logic for TP validation seems fine. However, I'm a bit confused how/whether the check for pservice works (securely).

FYI: the version field in yaml files is obsolete and gives level=warning msg="[...].yaml: version is obsolete" warnings with the latest docker compose plugin. I couldn't immediately figure out from which version on the field is not mandatory one, so not sure if we can already safely remove it. But just a heads up ....

g2flyer · 2024-05-14T01:31:24Z

docker/Makefile

@@ -83,13 +87,15 @@ rebuild_services_sgx : repository
 	$(DOCKER_COMMAND) build $(DOCKER_ARGS) \
 		--build-arg REBUILD=$(TIMESTAMP) \
 		--build-arg PDO_VERSION=$(PDO_VERSION) \
+		--build-arg PDO_DEBUG_BUILD=$(PDO_DEBUG_BUILD) \


Shouldn't we add this for consistency also to the other build targets ? (The docker files for ccf, client & services still have the ARG/ENV. Previously in docker-compose builds we had that it passed via the yamls but with transformation this essentially disapperared, with the reference in test.yaml the last reminder ...)

Probably yes, but... while the current modification is necessary and tested for the services, additional modifications elsewhere require more work which is orthogonal to this PR.

and... with what we've discussed, a future PR is going to remove PDO_DEBUG_BUILD completely. so i would suggest minimal changes right now.

g2flyer · 2024-05-14T01:36:47Z

eservice/Makefile

@@ -16,6 +16,16 @@ SCRIPTDIR ?= $(dir $(abspath $(lastword $(MAKEFILE_LIST))))
 SRCDIR ?= $(abspath $(SCRIPTDIR)/..)
 DSTDIR ?= $(PDO_INSTALL_ROOT)

+# Set options for cmake build


Don't we need the same change also in pservice/Makefile?

Not sure if we "need", but some consistency here is desirable.
Anyway, the changes to the makefile will be moved to the build.sh based on PDO_DEBUG_BUILD.

this should all be moved to build/cmake/SGX.cmake. historically we've gotten in big trouble with inconsistencies in build process and functionality because we keep putting it in the service directories rather than in the common, shared cmake support files. changes to make & cmake files in the service directories should be kept to an absolute minimum.

g2flyer · 2024-05-14T01:58:30Z

pservice/lib/libpdo_enclave/secret_enclave.cpp

+            "Invalid 64-bit flag: 0");
+
+    // Verify SGX debug flag: check mask and enforce if necessary
+    if(attribute_mask_flags & SGX_FLAGS_DEBUG) //if bit is set, enforce debug flag check


I'm a bit confused, we do we not read the flag from attributes as we do for 64-bit (as as we do in CCF TP)? Where does that mask come from ( i guess from that python script ) and, more importantly, how is it linked to what we verify from IAS on line 641?

Good question, and the answer is in the generate-mrenclave-header.

The mask comes from the metadata, generated at enclave build time, from which we extract the mrenclave provided to the pservice. It is necessary because the sgx-sign disables/enables debug through the mask (which says what flags must be enforced). If the debug flag in the mask is not set, then the debug flag is irrelevant (meaning that the eservice can start the enclave in both modes). The DisableDebug field in the enclave xml file allows to modify the mask accordingly (and if debug is disabled, the debug flag is forced to 0). With this PR the DisableDebug field depends on PDO_DEBUG_BUILD.

The quote and the attestation are instead runtime artifacts which will contain the actual debug flag use to start the enclave. The TP enforces that based on the policy, which this PR modifies by adding the debug flag. Also, in HW mode, the TP can be run the policy debug flag 0/1 based on PDO_DEBUG_BUILD.

Oh, mea culpa, i missed that the test in the if body is conditional (and on the extracted flag). So i see now how (and that) it works. But maybe could still be useful to take some of the text above and as a comment before the attribute_mask test?

cmickeyb · 2024-05-16T22:29:44Z

pservice/lib/libpdo_enclave/CMakeLists.txt

+    TARGET prepare_enclave_xml
+    APPEND
+    PROPERTY ADDITIONAL_CLEAN_FILES ${PROJECT_CONFIG})
+


this needs to be moved to build/cmake/SGX.cmake and used by both eservice and pservice.
see examples for SGX_DEPLOY_FILES and SGX_SIGN_ENCLAVE

You have highlighted the SET_PROPERTY part, but the generation of the xml configuration file has already been moved to the SGX_PREPARE_ENCLAVE_XML function in SGX.cmake -- and it does not seem appropriate to move the custom target and set property into that function as well.

cmickeyb · 2024-05-16T22:32:30Z

pservice/setup.py

@@ -60,6 +60,8 @@
 module_path = 'pdo/pservice/enclave'
 module_src_path = os.path.join(script_dir, module_path)

+debug_flag = os.environ.get('PDO_DEBUG_BUILD', False) in ("1")
+


umm... what? what are you trying to accomplish by this statement? this is way to convoluted.

I have fixed the "False", it should have been "0", and I added a comment to clarify.
It may be convoluted but the retrieval of the environment variables returns a string (we set PDO_DEBUG_BUILD as "0" or "1"), which has to be converted to a bool. I'm open to suggestions.

prakashngit · 2024-05-22T02:42:24Z

ledgers/ccf/transaction_processor/enclave_registry.h

@@ -134,6 +136,7 @@ namespace ccf
      string mrenclave;
      string basename;
      string ias_public_key;
+      string sgx_debug_flag;
    };


any reason why the flag is not a bool ?

prakashngit · 2024-05-22T02:44:38Z

ledgers/ccf/transaction_processor/pdo_tp.cpp

+
+            // post assignment checks
+            if(expected_sgx_measurements.sgx_debug_flag.length() != 1)
+                return ccf::make_error(


again puzzled as to why the flag is not a bool? and if even it is a string, why would we check its length ? we have expected flag and actual flag, they need to match, regardless of length.

prakashngit · 2024-05-22T02:49:25Z

ledgers/ccf/transaction_processor/pdo_tp.cpp

+                    return ccf::make_error(HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidInput,
+                        "Enclave attestation report verification Failed. Enclave debug flag " + flag + 
+                        " does not match policy " + expected_sgx_measurements.sgx_debug_flag);
+


More of a philosophical question: Why would we check attestation if it is sgx debug mode ON? What is the security model?

Because that's the case when you want to test/debug PDO in HW-mode, and you can have legitimate attestations in that case.

prakashngit · 2024-05-22T02:53:17Z

.github/workflows/ci.yaml

@@ -23,6 +23,8 @@ jobs:
              if: "!contains(github.event.commits[0].message, '[debug]')"
              env:
                  PDO_INTERPRETER: ${{ matrix.interpreter }}
+                  PDO_DEBUG_BUILD: 1
+                  PDO_LOG_LEVEL: warning


Where are we defining what PDO_DEBUG_BUILD means? (especially for SIM mode). Given that even with DEBUG_BUILD, we permit log level that is not necessarily DEBUG.

PDO_DEBUG_BUILD is unrelated to the logging levels. It determines compilation flags (-g vs -O2) and adds some ocalls that enable enclave debugging and performance testing.

prakashngit · 2024-05-22T02:54:59Z

build/cmake/SGX.cmake

+
+    IF (${PDO_DEBUG_BUILD} STREQUAL "0")
+        MESSAGE(FATAL_ERROR "SGX_MODE=SIM does not accept PDO_DEBUG_BUILD=0")
+    ENDIF()


goes back to my earlier question about definition of PDO_DEBUG_BUILD: what does "SGX_MODE=SIM does not accept PDO_DEBUG_BUILD=0" mean ?

It means that PDO_DEBUG_BUILD must be 1 in SIM mode. As SIM mode is by definition not a production mode, it assumed that debug must be turned on.

Given that we are trying to hide PDO_DEBUG_BUILD behind CMAKE_BUILD_TYPE wouldn't it be good to also mention the latter as we do below for abort if build_type=release and PDO_DEBUG_BUILD=1?

mergify · 2024-05-23T21:40:23Z

⚠️ The sha of the head commit of this PR conflicts with #492. Mergify cannot evaluate rules on this PR. ⚠️

mergify · 2024-05-24T20:02:15Z

⚠️ The sha of the head commit of this PR conflicts with #492. Mergify cannot evaluate rules on this PR. ⚠️

bvavala · 2024-05-24T23:21:18Z

@cmickeyb I put together the contributions, cleaned up the commit log and tested.

cmickeyb · 2024-05-28T14:28:41Z

@cmickeyb I put together the contributions, cleaned up the commit log and tested.

this doesn't seem to include any of the cmake/makefile fixes i sent you?

bvavala · 2024-05-28T15:26:19Z

@cmickeyb I put together the contributions, cleaned up the commit log and tested.

this doesn't seem to include any of the cmake/makefile fixes i sent you?

I have merged them in the 1st and 4th commits -- those coauthored with you.

g2flyer · 2024-05-28T17:48:11Z

Haven't looked yet at code and just restarted testing but one thing i noticed is that our defaults do not build anymore with make -C docker: We have SGX_MODE=SIM and PDO_DEBUG_BUILD=0 as defaults yet they are mutually exclusive and result in a build error. I suggest we change PDO_DEBUG_BUILD=1 as default (as i guess we don't really want to set HW mode as default ...). Given that SGX_MODE has now also more intertwined with build and tests, i think we should also revisit the decision to codify SGX_MODE=HW in the make (sgx_..) targets rather than choosing the right execution based on the settings ...

cmickeyb · 2024-05-28T19:07:11Z

Haven't looked yet at code and just restarted testing but one thing i noticed is that our defaults do not build anymore with make -C docker: We have SGX_MODE=SIM and PDO_DEBUG_BUILD=0 as defaults yet they are mutually exclusive and result in a build error. I suggest we change PDO_DEBUG_BUILD=1 as default (as i guess we don't really want to set HW mode as default ...). Given that SGX_MODE has now also more intertwined with build and tests, i think we should also revisit the decision to codify SGX_MODE=HW in the make (sgx_..) targets rather than choosing the right execution based on the settings ...

The only target we have that changes based on HW mode is the services and it already has an explicit target.

If PDO_DEBUG_BUILD is not set or set to 0, the enclave is built with SGX_DEBUG_FLAG set to 0, and signed with the DisableDebug flag set to 1. So this commit adds one more step in the enclave cmake build to create the xml configuration file accordingly. Co-authored-by: Mic Bowman <mic.bowman@intel.com> Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>

This commit adds the sgx debug flag to the TP policy, dependent on PDO_DEBUG_BUILD. Inside the TP, it adds the 64-bit flag check, and it checks that that debug flag matches the one in registered TP policy. Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>

This normalizes the attestation verification checks with the TP. Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>

Co-authored-by: Mic Bowman <mic.bowman@intel.com> Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>

bvavala · 2024-05-28T21:04:51Z

Haven't looked yet at code and just restarted testing but one thing i noticed is that our defaults do not build anymore with make -C docker: We have SGX_MODE=SIM and PDO_DEBUG_BUILD=0 as defaults yet they are mutually exclusive and result in a build error. I suggest we change PDO_DEBUG_BUILD=1 as default (as i guess we don't really want to set HW mode as default ...). Given that SGX_MODE has now also more intertwined with build and tests, i think we should also revisit the decision to codify SGX_MODE=HW in the make (sgx_..) targets rather than choosing the right execution based on the settings ...

@g2flyer

I have changed the PDO_DEBUG_BUILD default to 1 in the last commit. This fixes the inconsistency of the default build.

Also, I have fixed the NDEBUG/EDEBUG in setup.py. This fixes the HW-mode build with debug disabled.

This increases consistency with the SGX_MODE, which is set to SIM by default. Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>

g2flyer

Ok, now docker build works with defaults, sgx_test works for both variants of (explicit or implicit) PDO_DEBUG_BUILD. test and default make targets fail when setting explicitly PDO_DEBUG_BUILD=0 but at least error message is reasonably clear. Ideally, though, we would also document the meaning of PDO_DEBUG_BUILD=0, e.g., in docs/environment.md, and maybe also mention in docker/README.md that that setting results in failures for default make target (but does work when using sgx_test.)

g2flyer · 2024-05-29T00:35:09Z

build/cmake/SGX.cmake

+
+    IF (${PDO_DEBUG_BUILD} STREQUAL "0")
+        MESSAGE(FATAL_ERROR "SGX_MODE=SIM does not accept PDO_DEBUG_BUILD=0")
+    ENDIF()


Given that we are trying to hide PDO_DEBUG_BUILD behind CMAKE_BUILD_TYPE wouldn't it be good to also mention the latter as we do below for abort if build_type=release and PDO_DEBUG_BUILD=1?

cmickeyb · 2024-05-29T16:56:24Z

Very nice update, @bvavala. Thank you!!!!!

bvavala changed the title ~~Improvement to SGX attestation verification~~ Improvements to SGX attestation verification May 13, 2024

bvavala requested review from g2flyer and cmickeyb May 13, 2024 20:23

bvavala assigned cmickeyb May 13, 2024

bvavala force-pushed the bruno.240509.fix-sgx-debug branch from 3f336e4 to 198b149 Compare May 13, 2024 20:24

cmickeyb requested a review from prakashngit May 13, 2024 22:06

bvavala marked this pull request as draft May 14, 2024 15:21

g2flyer reviewed May 14, 2024

View reviewed changes

bvavala force-pushed the bruno.240509.fix-sgx-debug branch from 198b149 to 35a5aa9 Compare May 16, 2024 00:07

bvavala marked this pull request as ready for review May 16, 2024 00:13

cmickeyb reviewed May 16, 2024

View reviewed changes

bvavala force-pushed the bruno.240509.fix-sgx-debug branch from 35a5aa9 to c13eee8 Compare May 18, 2024 00:48

prakashngit reviewed May 22, 2024

View reviewed changes

bvavala force-pushed the bruno.240509.fix-sgx-debug branch from f80725f to 17de98d Compare May 24, 2024 23:03

bvavala marked this pull request as draft May 24, 2024 23:07

bvavala marked this pull request as ready for review May 24, 2024 23:21

prakashngit approved these changes May 25, 2024

View reviewed changes

cmickeyb approved these changes May 28, 2024

View reviewed changes

bvavala requested a review from g2flyer May 28, 2024 16:45

bvavala and others added 2 commits May 28, 2024 19:41

bvavala and others added 2 commits May 28, 2024 19:44

Add sgx flag checks to pservice.

36f7dde

This normalizes the attestation verification checks with the TP. Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>

Port eservice build updates to pservice.

1b9dae1

Co-authored-by: Mic Bowman <mic.bowman@intel.com> Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>

cmickeyb mentioned this pull request May 28, 2024

Improved cmake for generating the configuration file #492

Closed

bvavala force-pushed the bruno.240509.fix-sgx-debug branch from be6783d to 29c155e Compare May 28, 2024 20:59

Change the default value of PDO_DEBUG_BUILD to 1.

252e9df

This increases consistency with the SGX_MODE, which is set to SIM by default. Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>

bvavala force-pushed the bruno.240509.fix-sgx-debug branch from 29c155e to 252e9df Compare May 28, 2024 21:36

g2flyer approved these changes May 29, 2024

View reviewed changes

cmickeyb merged commit 37ab11b into hyperledger-labs:main May 29, 2024
4 checks passed

cmickeyb deleted the bruno.240509.fix-sgx-debug branch May 29, 2024 16:56

bvavala mentioned this pull request Jun 10, 2024

SGX attestation verification #195

Closed

Improvements to SGX attestation verification #490

Improvements to SGX attestation verification #490

Conversation

bvavala commented May 13, 2024 • edited

cmickeyb commented May 14, 2024

g2flyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmickeyb May 15, 2024 • edited

Choose a reason for hiding this comment

g2flyer May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmickeyb May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented May 23, 2024

mergify bot commented May 24, 2024

bvavala commented May 24, 2024

cmickeyb commented May 28, 2024

bvavala commented May 28, 2024

g2flyer commented May 28, 2024 • edited

cmickeyb commented May 28, 2024

bvavala commented May 28, 2024

g2flyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmickeyb commented May 29, 2024

bvavala commented May 13, 2024 •

edited

cmickeyb May 15, 2024 •

edited

g2flyer May 14, 2024 •

edited

cmickeyb May 16, 2024 •

edited

g2flyer commented May 28, 2024 •

edited