gramine icon indicating copy to clipboard operation
gramine copied to clipboard

[PAL] Disable attestation flows in child enclaves

Open billionairiam opened this issue 3 years ago • 1 comments

Signed-off-by: Liang Ma [email protected]

Description of the changes

Only the initial process should be possible to attest remotely (otherwise various security properties break) - we need to disable doing this in children.


This change is Reviewable

billionairiam avatar Jul 21 '22 05:07 billionairiam

Reviewed 3 of 3 files at r1, all commit messages. Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @billionairiam)

a discussion (no related file): Generally looks correct.

But now I wonder -- do we really want this restriction? Imagine that we have a Bash script that becomes the first process, and all this Bash script does is to set up some envvars and then calls the real workload. E.g., this is the case for the Redis Docker image.

With this PR, Redis (which is the child enclave) won't be able to perform remote attestation. And asking Bash to perform remote attestation is ridiculous... @mkow @boryspoplawski I think this was the idea from ITL, could you comment on this?

-- commits line 2 at r1: I suggest: [PAL] Disable attestation flows in child enclaves

Documentation/attestation.rst line 25 at r1 (raw file):

In many cases, the user also wants :term:`Secret Provisioning` to transparently
provision secret keys and other sensitive data to the remote TEE.

I would move the added sentence to a separate paragraph and mark it as a note. So like this:

.. note::
   In Gramine, only the initial process can perform remote attestation; child processes
   have no access to remote-attestation flows.

pal/src/host/linux-sgx/host_main.c line 957 at r1 (raw file):

    sgx_target_info_t qe_targetinfo = {0};
    if (enclave->attestation_type != SGX_ATTESTATION_NONE && parent_stream_fd < 0) {
        /* initialize communication with Quoting Enclave only if app requests attestation */

Please append the following in this comment: ... and this is the first process (child processes are forbidden to perform attestation)

Don't forget to re-wrap the comment to 100-chars-per-line limit.

pal/src/host/linux-sgx/pal_main.c line 631 at r1 (raw file):

    }

    enum sgx_attestation_type attestation_type=SGX_ATTESTATION_NONE;

Please add spaces around =

pal/src/host/linux-sgx/pal_main.c line 632 at r1 (raw file):

    enum sgx_attestation_type attestation_type=SGX_ATTESTATION_NONE;
    if(parent_stream_fd < 0){

Please add a comment before ret = parse_attestation_type(...):

/* allow remote attestation only if this is the first process (child processes are forbidden
 * to perform remote attestation) */

Don't forget to re-wrap the comment to 100-chars-per-line limit.

Well, Just now my colleagues discussed these issues carefully, they think that the current implementation is no problem, so there is no need to commit this PR. I will send you a letter to illustrate our discussion details.

billionairiam avatar Jul 21 '22 07:07 billionairiam

I'm currently closing this PR as this requirement doesn't seem to be relevant anymore.

dimakuv avatar Aug 31 '22 08:08 dimakuv