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>

1 files changed, 3 insertions, 3 deletions

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),
         },
     };