Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

userns: fix id map generating with holes under specific conditions #88

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

Snaipe
Copy link
Member

@Snaipe Snaipe commented Nov 23, 2023

The uid/gid map generation would incorrectly leave holes when the
current ID was greater than 2^16-1 and a combination of other factors.

The gist of it is that bst, assuming the current UID is 93119, and the
following /etc/subuid:

93119:100000:400000

... combined with the following system UID map:

0       1000    1
93119   93119   1
1       100000  93118
93120   193118  406882

... would need to attribute up to 400000 UIDs from the outer 100000 UID.

This attribution is done by mapping the current UID to inner UID 0, then
walking over the set of allotted subuids as defined in the /etc/subuid
file.

However, reading /etc/subuid would produce the following ID map:

0       93119   1
1       100000  400000

That is to say, bst would, upon reading /etc/subuid, insert an implicit
93119:93119:1 entry at the top of the file.

The problem is that the uid map generation would already insert this
implicit entry, and then it would start assigning uids in the allotted
map loaded from /etc/subuid. Normally, the generation code ignores
completely any allotted range in [0, 65535), which means that for UIDs
lower than 2^16, the implicit entry in /etc/subuid is ignored, but in
this scenario, the UID is greater than 2^16, so it does not get ignored.

This means that the uid would get mapped twice: once to UID 0, and once
to uid 1:

0       93119   1
1       93119   1
2       100000  400000

Now comes the crux of the problem: it's not possible to map two
different inner UIDs to the same outer UID, so projecting this map onto
the current uid space causes this map to be generated:

0       93119   1
2       100000  400000

That is to say, UID 1 would be left unmapped.

This PR fixes this bug by ignoring the current id from the allotted
map if encountered. We keep that implicit entry because it is used to
determine whether or not it is acceptable to map some arbitrary ID
ranges within the boundaries of the sub-id map, and the current UID/GID
is allowed to get mapped to any arbitrary inner UID/GID.

This PR also removes the logic that ignores the range [0-65535),
because it was broken, and was really only used to attempt to address
the aforementioned problem.

Additionally, we used to assign to root the entire range if we couldn't assign
more than one UID/GID in the generated maps. This worked, but left an
inconsistency when using --uid-map and --gid-map: even as root, it was
not possible to assign arbitrary IDs when no range was allotted for root
in /etc/subuid and /etc/subgid.

This PR fixes this by instead moving the implicit range allocation
in the subid loading function. That is, if root has no uid range defined
in /etc/subuid, then it gets an implicit [0, 4294967294) range. This
preserves the behaviour of assigning that range during the generation of
the id maps while also allowing root to use arbitrary ranges with the
--[ug]id-map flags if root hasn't been assigned sub[ug]ids already.

The uid/gid map generation would incorrectly leave holes when the
current ID was greater than 2^16-1 and a combination of other factors.

The gist of it is that bst, assuming the current UID is 93119, and the
following /etc/subuid:

    93119:100000:400000

... combined with the following system UID map:

    0       1000    1
    93119   93119   1
    1       100000  93118
    93120   193118  406882

... would need to attribute up to 400000 UIDs from the outer 100000 UID.

This attribution is done by mapping the current UID to inner UID 0, then
walking over the set of allotted subuids as defined in the /etc/subuid
file.

However, reading /etc/subuid would produce the following ID map:

    0       93119   1
    1       100000  400000

That is to say, bst would, upon reading /etc/subuid, insert an implicit
93119:93119:1 entry at the top of the file.

The problem is that the uid map generation would already insert this
implicit entry, and _then_ it would start assigning uids in the allotted
map loaded from /etc/subuid. Normally, the generation code ignores
completely any allotted range in [0, 65535), which means that for UIDs
lower than 2^16, the implicit entry in /etc/subuid is ignored, but in
this scenario, the UID is greater than 2^16, so it does not get ignored.

This means that the uid would get mapped twice: once to UID 0, and once
to uid 1:

    0       93119   1
    1       93119   1
    2       100000  400000

Now comes the crux of the problem: it's not possible to map two
different inner UIDs to the same outer UID, so projecting this map onto
the current uid space causes this map to be generated:

    0       93119   1
    2       100000  400000

That is to say, UID 1 would be left unmapped.

This commit fixes this bug by ignoring the current id from the allotted
map if encountered. We keep that implicit entry because it is used to
determine whether or not it is acceptable to map some arbitrary ID
ranges within the boundaries of the sub-id map, and the current UID/GID
is allowed to get mapped to any arbitrary inner UID/GID.

This commit also removes the logic that ignores the range [0-65535),
because it was broken, and was really only used to attempt to address
the aforementioned problem.
Previously, we assigned to root the entire range if we couldn't assign
more than one UID/GID in the generated maps. This worked, but left an
inconsistency when using --uid-map and --gid-map: even as root, it was
not possible to assign arbitrary IDs when no range was allotted for root
in /etc/subuid and /etc/subgid.

This commit fixes this by instead moving the implicit range allocation
in the subid loading function. That is, if root has no uid range defined
in /etc/subuid, then it gets an implicit [0, 4294967294) range. This
preserves the behaviour of assigning that range during the generation of
the id maps while also allowing root to use arbitrary ranges with the
--[ug]id-map flags _if_ root hasn't been assigned sub[ug]ids already.
@Snaipe Snaipe merged commit 7512a79 into aristanetworks:main Dec 6, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants