Protected env #1667

mkow · 2020-07-10T18:48:08Z

Description of the changes

This is similar to #1562, but for environment variables. It implements a way to pass environment variables from a file, with intention to use it in conjunction with either trusted or protected files.

Fixes #508.

How to test this PR?

Two new tests were added for the new functionality.

This change is

dimakuv

Reviewed 18 of 18 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)

Documentation/manifest-syntax.rst, line 90 at r1 (raw file):

By default, environment variables from the host will *not* be passed to the app.
This can be overriden by adding ``loader.insecure__use_host_env = 1``, but most

typo: overridden

Documentation/manifest-syntax.rst, line 90 at r1 (raw file):

By default, environment variables from the host will *not* be passed to the app.
This can be overriden by adding ``loader.insecure__use_host_env = 1``, but most

The insecure__use_host_env manifest option is a bit hidden in this description. Could you move it to a list, like for command-line args, so it is more visible to readers of this document?

Documentation/manifest-syntax.rst, line 109 at r1 (raw file):

environment, which can be generated using :file:`Tools/argv_serializer`. This
option is intended to point to either a trusted file or a protected file. The
former allows to securely hardcode environments (in more flexible way than

in more -> in a more

LibOS/shim/test/regression/test_libos.py, line 80 at r1 (raw file):

            manifest = self.get_manifest('env_from_file')
            stdout, _ = self.run_binary([manifest], env=host_envs)
            self.assertIn('# of envs: %d\n' % (len(envs) + len(manifest_envs)), stdout)

Please also add self.assertNotIn(... host_envs)

Pal/src/db_main.c, line 75 at r1 (raw file):

    ssize_t cfgsize_envs = get_config_entries_size(store, "loader.env");
    if (cfgsize_envs < 0)

Why not <= 0? I guess it's impossible to return 0 in this function, but just for sanity.

Pal/src/db_main.c, line 87 at r1 (raw file):

    }

    setenvs = __alloca(sizeof(struct setenv) * setenvs_cnt);

Why alloca() instead of malloc()? A bit dangerous to allocate on stack.

Actually, I guess it's fine. Given that these are loader.env entries from the manifest, they should be small and won't overflow the main-thread stack.

Pal/src/db_main.c, line 113 at r1 (raw file):

    /* TODO: This code appears to rely on the memory buffer being zero-
     * initialized, so we use calloc here to get zeroed memory. We should
     * audit this code to verify that it's correct. */

I suggest to remove this TODO comment.

Pal/src/db_main.c, line 117 at r1 (raw file):

    if (nenvs)
        memcpy(new_envp, *envpp, sizeof(**envpp) * nenvs);

Remove trailing whitespace

Pal/src/db_main.c, line 143 at r1 (raw file):

            *ptr = e;
        }
    }

This whole code is contrived, but it seems to do its job. I understand that refactoring it is not in scope of this PR.

Pal/src/db_main.c, line 479 at r1 (raw file):

    // TODO: Envs from file should be able to override ones from the manifest, but current
    // code makes this hard to implement.

Current code uses an opposite approach: envvars from the manifest override the ones from the file. Is that bad? I think it sounds reasonable. Why this TODO then? Do you want to ideally flip the current logic?

Pal/src/host/Linux-SGX/db_main.c, line 112 at r1 (raw file):

{
    if (len > 7 && key[0] == 'l' && key[1] == 'o' && key[2] == 'a' && key[3] == 'd' &&
        key[4] == 'e' && key[5] == 'r' && key[6] == '.')

Cute!

Pal/src/host/Linux-SGX/sgx-driver, line 1 at r1 (raw file):

Subproject commit fd435f02ff526fd4e473de03797b0bbab95fb748

Why is this here?

mkow

Reviewable status: 15 of 17 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

Documentation/manifest-syntax.rst, line 90 at r1 (raw file):