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>

3 files changed, 9 insertions, 9 deletions

diff --git a/src/nxt_clone.c b/src/nxt_clone.c
index 305f4261..e78a7822 100644
--- a/src/nxt_clone.c
+++ b/src/nxt_clone.c
@@ -143,7 +143,7 @@ nxt_clone_credential_map_set(nxt_task_t *task, const char* mapfile, pid_t pid,
         end = mapinfo + len;
 
         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);
 
             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)) {
             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);
 
             return NXT_ERROR;
@@ -340,7 +340,7 @@ nxt_clone_vldt_credential_gidmap(nxt_task_t *task,
 
         if (nxt_slow_path(m.size > 1)) {
             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);
 
             return NXT_ERROR;
diff --git a/src/nxt_clone.h b/src/nxt_clone.h
index 1677dc77..bf28322f 100644
--- a/src/nxt_clone.h
+++ b/src/nxt_clone.h
@@ -12,9 +12,9 @@
 typedef int64_t                 nxt_cred_t;
 
 typedef struct {
-    nxt_int_t                   container;
-    nxt_int_t                   host;
-    nxt_int_t                   size;
+    nxt_cred_t                  container;
+    nxt_cred_t                  host;
+    nxt_cred_t                  size;
 } nxt_clone_map_entry_t;
 
 typedef struct {
diff --git a/src/nxt_isolation.c b/src/nxt_isolation.c
index cfa494a8..ed5e0d76 100644
--- a/src/nxt_isolation.c
+++ b/src/nxt_isolation.c
@@ -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[] = {
         {
             nxt_string("container"),
-            NXT_CONF_MAP_INT,
+            NXT_CONF_MAP_INT64,
             offsetof(nxt_clone_map_entry_t, container),
         },
 
         {
             nxt_string("host"),
-            NXT_CONF_MAP_INT,
+            NXT_CONF_MAP_INT64,
             offsetof(nxt_clone_map_entry_t, host),
         },
 
         {
             nxt_string("size"),
-            NXT_CONF_MAP_INT,
+            NXT_CONF_MAP_INT64,
             offsetof(nxt_clone_map_entry_t, size),
         },
     };