usermod: refuse invalid uidmaps during --add-sub{u,g}ids

tych0 commented 6 months ago

It is slightly confusing to allow adding these only to later refuse them.

Here is a (lightly tested :) patch to also refuse them when adding.

alejandro-colomar commented 6 months ago

I've assigned myself because I want to investigate this in the future, regarding https://github.com/shadow-maint/shadow/pull/893.

hallyn commented 6 months ago

I've assigned myself because I want to investigate this in the future, regarding #893.

I thought you might :)

And generally speaking I'm a bit torn on this - on the one hand it's good to catch bad values as early as possible, on the other hand the kernel protects itself already, and we don't want to hardcode limitations which might be removed in the future. So that's just to say I'll want to discuss any other improvements in this area :)

Thanks!

alejandro-colomar commented 6 months ago

I've assigned myself because I want to investigate this in the future, regarding #893.

I thought you might :)

=)

And generally speaking I'm a bit torn on this - on the one hand it's good to catch bad values as early as possible, on the other hand the kernel protects itself already, and we don't want to hardcode limitations which might be removed in the future. So that's just to say I'll want to discuss any other improvements in this area :)

Agree. I don't really like this patch either, although it was very helpful, to understand the purpose of that function. So, thanks for the patch, @tych0 ! :)

When we add liba2i functions, we can do something like:

if (str2i(id_t, &num, str) == -1)
    goto err;

That would do the appropriate range checks for the type id_t, without hard-coding what id_t actually is.

Thanks!

Have a lovely day! Alex

tych0 commented 6 months ago

Maybe it would look better to put all this "uid range check" logic in one place, instead of trying to duplicate it across functions? Or even better: fork and try to create a ns, see if the kernel allows it? Then it is future proof.

hallyn commented 6 months ago

Certainly yes to commonizing the code in one place.

But I think just checking /proc/self/uid_map to look for the available range is probably safer than actually trying the mapping. All of it, however, becomes a tradeoff between the fragility of the new tests and the risk of not doing the tests at all.

alejandro-colomar commented 6 months ago

Maybe it would look better to put all this "uid range check" logic in one place, instead of trying to duplicate it across functions?

That's more or less my plan. The code base is full of range checks after strtol(3) calls. What I did is create a library that parses numbers into arbitrary types, such as uid_t, or id_t, implicitly performing the range checks for the type.

So, just by changing this code to use id_t instead of unsigned long, and calling these functions of mine, the code would be doing the range checks in a centralized place.

See for example:

BTW, looking at those, I see we already have a get_uid() function. I'll probably use that.

https://www.alejandro-colomar.es/src/alx/alx/lib/liba2i.git/

shadow-maint / shadow

usermod: refuse invalid uidmaps during --add-sub{u,g}ids #951