From f7c9d3a8b3dbe083007e73c8c7b7e50094bf3763 Mon Sep 17 00:00:00 2001 From: Andrew Clayton Date: Wed, 24 Jan 2024 18:01:49 +0000 Subject: 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 #include int main(void) { int64_t num = -1; /* All 1's in two's complement */ union { int32_t i32; int64_t i64; } *ptr; ptr = (void *)# 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 Reviewed-by: Zhidao Hong Signed-off-by: Andrew Clayton --- src/nxt_clone.c | 6 +++--- src/nxt_clone.h | 6 +++--- src/nxt_isolation.c | 6 +++--- 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), }, }; -- cgit