[LibOS] Assign UID and GID to stat fields in chroot

[dzygann]

Set the UID and GID in the chroot function to the one defined in the manifest
by using the loader.uid and loader.gid options. Otherwise, a check fails
from the wrapped application, because the UID/GID is set to 0 and the application
expects another one.

This topic has been discussed in the issue 2632

Signed-off-by: Denis Zygann [email protected]

---

This change is 

[dimakuv]

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

---

-- commits, line 2 at r1:
in chroot -> in chroot FS (we will fix it during final rebase)

---

-- commits, line 4 at r1:
chroot function -> chroot_istat() function (we will fix it during final rebase)

one -> ones (we will fix it during final rebase)

---

-- commits, line 6 at r1:
from the wrapped application -> for some applications (we will fix it during final rebase)

---

LibOS/shim/src/fs/chroot/fs.c, line 343 at r1 (raw file):

}

static int chroot_istat(struct shim_inode* inode, struct stat* buf) {

Is this the only place where we have struct stat buf? @pwmarcz Are there any other places where we should do the same? Probably also in tmpfs?

---

LibOS/shim/test/regression/uid_gid.c, line 7 at r1 (raw file):

#include <sys/types.h>
#include <unistd.h>

Please remove one empty line.

---

LibOS/shim/test/regression/uid_gid.c, line 10 at r1 (raw file):

int main(int argc, char** argv) {
    int r;

r -> ret (we prefer to use ret word for return values)

---

LibOS/shim/test/regression/uid_gid.c, line 12 at r1 (raw file):

    int r;
    struct stat buffer;
    struct stat* buf = &buffer;

Having two variables is an overkill. You can simply have:

struct stat buf;

...

r = stat(argv[0], &buf);

---

LibOS/shim/test/regression/uid_gid.c, line 28 at r1 (raw file):

    }

    // check stat uid and gid

This comment is useless -- it is easy to understand from the below code what you're checking. Please remove the comment.

---

LibOS/shim/test/regression/uid_gid.c, line 30 at r1 (raw file):

    // check stat uid and gid
    r = stat(argv[0], buf);
    if (r != 0) {

Errors are always negative, so we prefer to write if (r < 0). Please fix.

---

LibOS/shim/test/regression/uid_gid.c, line 31 at r1 (raw file):

    r = stat(argv[0], buf);
    if (r != 0) {
        errx(EXIT_FAILURE, "Something unexpected went wrong in stat function. Error code: %d", r);

You can use err() instead of errx(), then you don't need to explicitly add the error code. Also read man errx.

So it will be simply:

    err(EXIT_FAILURE, "stat failed");

---

LibOS/shim/test/regression/uid_gid.c, line 35 at r1 (raw file):

    if (buf->st_uid != 1338) {
        errx(EXIT_FAILURE, "UID is not equal to the value in the manifest");

Please make it more explicit that this is the result of stat():

UID is not equal... -> UID from stat() is not equal...

---

LibOS/shim/test/regression/uid_gid.c, line 39 at r1 (raw file):

    if (buf->st_gid != 1337) {
        errx(EXIT_FAILURE, "GID is not equal to the value in the manifest");

Please make it more explicit that this is the result of stat():

GID is not equal... -> GID from stat() is not equal...

[pwmarcz]

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

a discussion (no related file):
Please also add this to documentation: the description for loader.uid/gid should mention that these settings also influence owner and group of files in chroot and tmpfs filesystems.

---

---

-- commits, line 6 at r1:
Is this about Postgres specifically? If so, I think you should mention it in the commit message ("a check fails from the wrapped application" is not helpful because it depends on what application it is).

---

LibOS/shim/src/fs/chroot/fs.c, line 343 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is this the only place where we have struct stat buf? @pwmarcz Are there any other places where we should do the same? Probably also in tmpfs?

Yes, please add this also to tmpfs.

(Pseudo-filesystems should keep uid/gid at 0: we could refine that for process directories in /proc/, but I don't think we should. And other filesystems (pipe, socket) have marginal support for stat anyway, so let's not get into that now).

[dzygann]

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

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Please also add this to documentation: the description for loader.uid/gid should mention that these settings also influence owner and group of files in chroot and tmpfs filesystems.

Can you review the additional text in the documentation, please?

---

---

-- commits, line 6 at r1:

his about Postgres sp
I think this check could also be done by other applications, but I also think that we can give Postgres as an example in the description or at least that we have experienced the check by the Postgres application.

---

LibOS/shim/src/fs/chroot/fs.c, line 343 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Yes, please add this also to tmpfs.

(Pseudo-filesystems should keep uid/gid at 0: we could refine that for process directories in /proc/, but I don't think we should. And other filesystems (pipe, socket) have marginal support for stat anyway, so let's not get into that now).

Done.

---

LibOS/shim/src/fs/tmpfs/fs.c, line 167 at r2 (raw file):

    buf->st_gid   = cur_thread->gid;
    ret = 0;

How can I test this one in Gramine?

---

LibOS/shim/test/regression/uid_gid.c, line 7 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove one empty line.

Done.

---

LibOS/shim/test/regression/uid_gid.c, line 10 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

r -> ret (we prefer to use ret word for return values)

Done.

---

LibOS/shim/test/regression/uid_gid.c, line 12 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Having two variables is an overkill. You can simply have:

struct stat buf;

...

r = stat(argv[0], &buf);

Done.

---

LibOS/shim/test/regression/uid_gid.c, line 28 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This comment is useless -- it is easy to understand from the below code what you're checking. Please remove the comment.

Done.

---

LibOS/shim/test/regression/uid_gid.c, line 30 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Errors are always negative, so we prefer to write if (r < 0). Please fix.

Done.

---

LibOS/shim/test/regression/uid_gid.c, line 31 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You can use err() instead of errx(), then you don't need to explicitly add the error code. Also read man errx.

So it will be simply:

    err(EXIT_FAILURE, "stat failed");

Done.
If I understood it correctly theerr appends an error message and errx doesn't.

---

LibOS/shim/test/regression/uid_gid.c, line 35 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please make it more explicit that this is the result of stat():

UID is not equal... -> UID from stat() is not equal...

Done.

---

LibOS/shim/test/regression/uid_gid.c, line 39 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please make it more explicit that this is the result of stat():

GID is not equal... -> GID from stat() is not equal...

Done.

[dimakuv]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dzygann and @pwmarcz)

---

Documentation/manifest-syntax.rst, line 211 at r2 (raw file):

user/group ID. It must be non-negative. By default Gramine emulates the
user/group ID and effective user/group ID as the root user (uid = gid = 0).
The user/group ID affects also the stat members for ``tmpfs`` and ``chroot``.

Maybe The user and group IDs are also reflected in the information about "tmpfs" and "chroot" files returned via stat().

---

LibOS/shim/src/fs/tmpfs/fs.c, line 167 at r2 (raw file):

Previously, dzygann (Denis Zygann) wrote…

How can I test this one in Gramine?

In your test uid_gid.c, you can open any file under /mnt/tmpfs (for example, open("/mnt/tmpfs/file1", O_RDWR | O_CREAT, S_IRUSR | S_IWUSR)). After you opened this file, you can do stat("/mnt/tmpfs/file1", &buf) and perform the same check as for the argv[0] case.

Note that this created file (/mnt/tmpfs/file1) is a temporary file and is only created inside enclave memory. So there will be no corresponding file on your host system, don't be surprised.

---

LibOS/shim/test/regression/uid_gid.c, line 31 at r1 (raw file):

Previously, dzygann (Denis Zygann) wrote…

Done.
If I understood it correctly theerr appends an error message and errx doesn't.

Yes, exactly.

[pwmarcz]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @dzygann)

a discussion (no related file):

Previously, dzygann (Denis Zygann) wrote…

Can you review the additional text in the documentation, please?

Done, thanks. I replied inline.

---

---

-- commits, line 6 at r1:

Previously, dzygann (Denis Zygann) wrote…

his about Postgres sp
I think this check could also be done by other applications, but I also think that we can give Postgres as an example in the description or at least that we have experienced the check by the Postgres application.

Yes. There might be other applications, but since Postgres is the one you're trying to fix, I think it should be mentioned (for example, someone might want to test it later after refactoring). Let's do it during final rebase.

---

Documentation/manifest-syntax.rst, line 211 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Maybe The user and group IDs are also reflected in the information about "tmpfs" and "chroot" files returned via stat().

Yes, I don't think "stat members" is understandable.

+1 to @dimakuv's suggestion (with monospace formatting for tmpfs, chroot and stat(), same as in the current version).

---

LibOS/shim/test/regression/uid_gid.c, line 26 at r2 (raw file):

    }

    ret = stat(argv[0], &buf);

Apart from tmpfs, can you also check a file that is supposed to still have UID and GID equal to 0? For example /proc/meminfo.

I don't want someone to come later and change the returned UID/GID globally, a check like this should remind them of this case.

[dzygann]

Reviewable status: 2 of 5 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @dzygann, and @pwmarcz)

---

Documentation/manifest-syntax.rst, line 211 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Yes, I don't think "stat members" is understandable.

+1 to @dimakuv's suggestion (with monospace formatting for tmpfs, chroot and stat(), same as in the current version).

Done.

---

LibOS/shim/src/fs/tmpfs/fs.c, line 167 at r2 (raw file):

r test uid_gid.c, you can open any file under /mnt/tmpfs (for example, open("/mnt/tmpfs/file1", O_RDWR | O_CREAT, S_IRUSR | S_IWUSR)). After you opened this file, you can do stat("/mnt/tmpfs/
I don't have the directory /mnt/tmpfs. I did this under the /tmp. The test was fine. I hope it is okay?

---

LibOS/shim/test/regression/uid_gid.c, line 26 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Apart from tmpfs, can you also check a file that is supposed to still have UID and GID equal to 0? For example /proc/meminfo.

I don't want someone to come later and change the returned UID/GID globally, a check like this should remind them of this case.

I tried to create a test, which points to the /proc/meminfofile. Unfortunately, I get every time a Permission denied. The test was equal to the open(/tmp/file1, ...) and I also add the /proc directory as mount and add the /proc/meminfo file to the allowed files. Still I get a permission denied, when I run the test. Do you have an idea what I'm doing wrong?

[dimakuv]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dzygann and @pwmarcz)

---

LibOS/shim/src/fs/tmpfs/fs.c, line 167 at r2 (raw file):

Previously, dzygann (Denis Zygann) wrote…

r test uid_gid.c, you can open any file under /mnt/tmpfs (for example, open("/mnt/tmpfs/file1", O_RDWR | O_CREAT, S_IRUSR | S_IWUSR)). After you opened this file, you can do stat("/mnt/tmpfs/
I don't have the directory /mnt/tmpfs. I did this under the /tmp. The test was fine. I hope it is okay?

No, this is wrong. What you've done is to forward the host-level /tmp to the Gramine-visible /tmp. And since you used chroot mount point, this will not test fs/tmpfs/fs.c logic -- this logic is only for tmpfs type of mount points.

You don't need to have /mnt/tmpfs directory on your host machine. This directory will be visible only in Gramine. See my other comment.

---

LibOS/shim/test/regression/uid_gid.c, line 26 at r2 (raw file):

Previously, dzygann (Denis Zygann) wrote…

I tried to create a test, which points to the /proc/meminfofile. Unfortunately, I get every time a Permission denied. The test was equal to the open(/tmp/file1, ...) and I also add the /proc directory as mount and add the /proc/meminfo file to the allowed files. Still I get a permission denied, when I run the test. Do you have an idea what I'm doing wrong?

You must not add /proc as a mount point in the manifest file. And you must not add /proc/meminfo to SGX allowed files.

All files under /proc are generated dynamically inside Gramine, and they are not the same as the host files. Procfs (/proc) is a pseudo-file system, which means that it doesn't have actual files but rather generates content on demand.

So just think of /proc/meminfo file as always-there inside Gramine. No changes to the manifest file are required.

---

LibOS/shim/test/regression/uid_gid.c, line 40 at r3 (raw file):

    }

    char file[] = "/tmp/file1";

Please call it at least tmpfile. The file name is too generic.

---

LibOS/shim/test/regression/uid_gid.c, line 41 at r3 (raw file):

    char file[] = "/tmp/file1";
    ret = open(file, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);

You forgot to close(ret) afterwards.

Also, please don't use ret here but use instead fd.

---

LibOS/shim/test/regression/uid_gid.c, line 57 at r3 (raw file):

    if (buf.st_gid != 1337) {
        errx(EXIT_FAILURE, "GID from tmpfs stat() is not equal to the value in the manifest");
    }

Can you move these three statements into a separate helper function, with stat and buf.st_uid check and buf.st_gid check? Then you won't have duplication for chroot files, tmpfs files, and /proc files.

---

LibOS/shim/test/regression/uid_gid.manifest.template, line 15 at r3 (raw file):

fs.mount.tmp.type = "chroot"
fs.mount.tmp.path = "/tmp"
fs.mount.tmp.uri = "file:/tmp"

This is wrong. You're simply forwarding /tmp from the host to inside Gramine.

You should do like this: https://github.com/gramineproject/gramine/blob/master/LibOS/shim/test/regression/manifest.template#L25

Also, please read https://gramine.readthedocs.io/en/latest/manifest-syntax.html#fs-mount-points -- about tmpfs mount points.

---

LibOS/shim/test/regression/uid_gid.manifest.template, line 27 at r3 (raw file):

sgx.allowed_files = [
  "file:/tmp/file1",
]

You should remove this.

[pwmarcz]

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

[mkow]

There was no activity here for half a year, I'm assuming @dzygann is not working on this anymore (and this PR would require non-trivial amount of changes to be merged).