Isolation: Use an appropriate type for storing uid/gids

Andrei reported an issue on arm64 where he was seeing the following
error message when running the tests

  2024/01/17 18:32:31.109 [error] 54904#54904 "gidmap" field has an entry with "size": 1, but for unprivileged unit it must be 1.

This error message is guarded by the following if statement

  if (nxt_slow_path(m.size > 1)

Turns out size was indeed > 1, in this case it was 289356276058554369,
m.size is defined as a nxt_int_t, which on arm64 is actually 8 bytes,
but was being printed as a signed int (4 bytes) and by chance/undefined
behaviour comes out as 1.

But why is size so big? In this case it should have just been 1 with a
config of

  'gidmap': [{'container': 0, 'host': os.getegid(), 'size': 1}],

This is due to nxt_int_t being 64bits on arm64 but using a conf type of
NXT_CONF_MAP_INT which means in nxt_conf_map_object() we would do (using
our m.size variable as an example)

  ptr = nxt_pointer_to(data, map[i].offset);
  ...
  ptr->i = num;

Where ptr is a union pointer and is now pointing at our m.size

Next we set m.size to the value of num (which is 1 in this case), via
ptr->i where i is a member of that union of type int.

So here we are setting a 64bit memory location (nxt_int_t on arm64)
through a 32bit (int) union alias, this means we are only setting the
lower half (4) of the bytes.

Whatever happens to be in the upper 4 bytes will remain, giving us our
exceptionally large value.

This is demonstrated by this program

  #include <stdio.h>
  #include <stdint.h>

  int main(void)
  {
          int64_t num = -1; /* All 1's in two's complement */
          union {
                  int32_t i32;
                  int64_t i64;
          } *ptr;

          ptr = (void *)&num;

          ptr->i32 = 1;
          printf("num : %lu / %ld\n", num, num);
          ptr->i64 = 1;
          printf("num : %ld\n", num);

          return 0;
  }
  $ make union-32-64-issue
  cc     union-32-64-issue.c   -o union-32-64-issue
  $ ./union-32-64-issue
  num : 18446744069414584321 / -4294967295
  num : 1

However that is not the only issue, because the members of
nxt_clone_map_entry_t were specified as nxt_int_t's on the likes of
x86_64 this would be a 32bit signed integer. However uid/gids on Linux
at least are defined as unsigned integers, so a nxt_int_t would not be
big enough to hold all potential values.

We could make the nxt_uint_t's but then we're back to the above union
aliasing problem.

We could just set the memory for these variables to 0 and that would
work, however that's really just papering over the problem.

The right thing is to use a large enough sized type to store these
things, hence the previously introduced nxt_cred_t. This is an int64_t
which is plenty large enough.

So we switch the nxt_clone_map_entry_t structure members over to
nxt_cred_t's and use NXT_CONF_MAP_INT64 as the conf type, which then
uses the right sized union member in nxt_conf_map_object() to set these
variables.

Reported-by: Andrei Zeliankou <zelenkov@nginx.com>
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
This commit is contained in:
Andrew Clayton
2024-01-24 18:01:49 +00:00
parent 9919b50aec
commit f7c9d3a8b3
3 changed files with 9 additions and 9 deletions

View File

@@ -143,7 +143,7 @@ nxt_clone_credential_map_set(nxt_task_t *task, const char* mapfile, pid_t pid,
end = mapinfo + len; end = mapinfo + len;
for (i = 0; i < map->size; i++) { for (i = 0; i < map->size; i++) {
p = nxt_sprintf(p, end, "%d %d %d", map->map[i].container, p = nxt_sprintf(p, end, "%L %L %L", map->map[i].container,
map->map[i].host, map->map[i].size); map->map[i].host, map->map[i].size);
if (nxt_slow_path(p == end)) { if (nxt_slow_path(p == end)) {
@@ -332,7 +332,7 @@ nxt_clone_vldt_credential_gidmap(nxt_task_t *task,
if (nxt_slow_path((nxt_gid_t) m.host != nxt_egid)) { if (nxt_slow_path((nxt_gid_t) m.host != nxt_egid)) {
nxt_log(task, NXT_LOG_ERR, "\"gidmap\" field has an entry for " nxt_log(task, NXT_LOG_ERR, "\"gidmap\" field has an entry for "
"host gid %d but unprivileged unit can only map itself " "host gid %L but unprivileged unit can only map itself "
"(gid %d) into child namespaces.", m.host, nxt_egid); "(gid %d) into child namespaces.", m.host, nxt_egid);
return NXT_ERROR; return NXT_ERROR;
@@ -340,7 +340,7 @@ nxt_clone_vldt_credential_gidmap(nxt_task_t *task,
if (nxt_slow_path(m.size > 1)) { if (nxt_slow_path(m.size > 1)) {
nxt_log(task, NXT_LOG_ERR, "\"gidmap\" field has an entry with " nxt_log(task, NXT_LOG_ERR, "\"gidmap\" field has an entry with "
"\"size\": %d, but for unprivileged unit it must be 1.", "\"size\": %L, but for unprivileged unit it must be 1.",
m.size); m.size);
return NXT_ERROR; return NXT_ERROR;

View File

@@ -12,9 +12,9 @@
typedef int64_t nxt_cred_t; typedef int64_t nxt_cred_t;
typedef struct { typedef struct {
nxt_int_t container; nxt_cred_t container;
nxt_int_t host; nxt_cred_t host;
nxt_int_t size; nxt_cred_t size;
} nxt_clone_map_entry_t; } nxt_clone_map_entry_t;
typedef struct { typedef struct {

View File

@@ -326,19 +326,19 @@ nxt_isolation_credential_map(nxt_task_t *task, nxt_mp_t *mp,
static nxt_conf_map_t nxt_clone_map_entry_conf[] = { static nxt_conf_map_t nxt_clone_map_entry_conf[] = {
{ {
nxt_string("container"), nxt_string("container"),
NXT_CONF_MAP_INT, NXT_CONF_MAP_INT64,
offsetof(nxt_clone_map_entry_t, container), offsetof(nxt_clone_map_entry_t, container),
}, },
{ {
nxt_string("host"), nxt_string("host"),
NXT_CONF_MAP_INT, NXT_CONF_MAP_INT64,
offsetof(nxt_clone_map_entry_t, host), offsetof(nxt_clone_map_entry_t, host),
}, },
{ {
nxt_string("size"), nxt_string("size"),
NXT_CONF_MAP_INT, NXT_CONF_MAP_INT64,
offsetof(nxt_clone_map_entry_t, size), offsetof(nxt_clone_map_entry_t, size),
}, },
}; };