Page MenuHomeFreeBSD

Handle overflow of uid or gid in arguments for chown

Authored by on Apr 17 2018, 6:37 PM.



chown incorrectly allows a uid or gid greater than UID_MAX/GID_MAX respectively.
Using such an argument rolls over to accounts such as root, operator, etc:

dmorris@hvbsdtst:/tmp % ll
-rw-r--r-- 1 dmorris wheel 0 Apr 13 07:52
dmorris@hvbsdtst:/tmp % sudo chown 4294967296;ll
-rw-r--r-- 1 root wheel 0 Apr 13 07:52
dmorris@hvbsdtst:/tmp % sudo chown 4294967298 ; ll
-rw-r--r-- 1 operator wheel 0 Apr 13 07:52

Since sudo is still required for chown usage, this is not a security issue - it is a correctness issue.

Test Plan

Validated overflow values for user and group:
dmorris@hvbsdtst:/tmp % sudo ./chown 4294967296 ; ll
chown: 4294967296: illegal user name
-rw-r--r-- 1 dmorris wheel 0 Apr 13 13:20
dmorris@hvbsdtst:/tmp % sudo ./chown 4294967298 ; ll
chown: 4294967298: illegal user name
-rw-r--r-- 1 dmorris wheel 0 Apr 13 13:20
dmorris@hvbsdtst:/tmp % sudo ./chown dmorris:4294967298 ; ll
chown: 4294967298: illegal group name
-rw-r--r-- 1 dmorris wheel 0 Apr 13 13:20

Diff Detail

rS FreeBSD src repository - subversion
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

68 ↗(On Diff #41588)

Possible whitespace error?

Uploaded file for diff instead of cut/paste that was introducing a whitespace issue.

Next time, please upload a diff with more context. Either use diff -U999999 to generate patches, or the arc command to directly upload changesets to Phab.

255 ↗(On Diff #41596)

This could be relaxed to >=, given a return type of either uid_t or id_t (it may be allowed for id_t to be larger than uid_t, for example).

This revision is now accepted and ready to land.Apr 17 2018, 8:53 PM
247 ↗(On Diff #41596)

Wouldn't this result in a sign-extended return value if the uid == UID_MAX? id_t is signed. Given the static assert, it seems safe to keep the old return type. I don't think this makes a functional difference currently.

256 ↗(On Diff #41596)

Style nit: unneeded parens.

247 ↗(On Diff #41596)

Hm, I was not aware that id_t was signed. That seems unfortunate. It is probably to represent pid_t, but again — it is unclear why that is signed.

I'm not sure about the integer promotion rules and sign extension. On LP64 platforms the unsigned long val should convert to id_t (== int64_t) without sign-extension. On 32-bit FreeBSD platforms, long is 32 bits.

I would be somewhat surprised if conversion from one integer to another integer that can fully represent the range of the former caused sign extension, but I'll admit I am unfamiliar with the exact rules here. This seems to describe arithmetic; I don't know if it applies to what is essentially a cast:

If the type of the operand with signed integer type can represent all of the values of the type of the operand with unsigned integer type, the operand with unsigned integer type is converted to the type of the operand with signed integer type.

(C18, §, "Usual arithmetic conversions")

§ "Signed and unsigned integers" says "When a value with integer type is converted to another integer type other than _Bool , if the value
can be represented by the new type, it is unchanged."

Either way, I would be ok keeping the old return type here.

markj added inline comments.
247 ↗(On Diff #41596)

Oops, I think you're right. I was thinking of the inverted case where we cast from a narrow signed type to a wider unsigned type.

I think I would probably keep the return type as uid_t. Too bad you can't do something like '_Static_assert(typeof(uid_t) == typeof(gid_t))'

247 ↗(On Diff #41596)

pid_t is signed because -1 is used in some cases (e.g. killpg with -1 to kill the group you belong to)

247 ↗(On Diff #41596)

Sure — but it could just as easily use UINT_MAX as sentinel value instead of -1 :-).

247 ↗(On Diff #41596)

It could have, but that ship has kind of sailed (killpg is part of POSIX, etc.)

247 ↗(On Diff #41596)

Sure, understood we can't change it now.

Curiously, Linux' killpg takes int, not pid_t, and our killpg does not document a pgrp=-1 behavior. I am curious what POSIX specifies.

POSIX says:

If pgrp is less than or equal to 1, the behavior of killpg() is undefined.

Oh, I see:

If pgrp is greater than 1, killpg(pgrp, sig) shall be equivalent to kill(-pgrp, sig).

So it's that kill() is required to take a negative first argument, and the type of that argument must be pid_t. Got it. And the value can be any negative process id, not just 1, and of course progress group ids are just pid's. Thanks :-).

Updated per review comments -- did I miss anything?

Anything else that needs to be done?

This revision now requires review to proceed.Aug 24 2018, 2:50 PM
This revision is now accepted and ready to land.Aug 24 2018, 4:07 PM
kib added inline comments.
256 ↗(On Diff #47232)

if (errno != 0 || ....

255 ↗(On Diff #41596)

I am not sure why >= and not ==. UID_MAX is for uid_t not for id_t.

Why not just use sizeof()s ?

256 ↗(On Diff #47232)

(Not introduced in this diff -- only the extra OR case was appended. But it can be fixed at commit.)

255 ↗(On Diff #41596)

I could have been more clear.

When the return type is either uid_t or id_t, this code is correct even if gid_t (or just GID_MAX) is smaller than uid_t (or UID_MAX).

If the opposite is true (gid_t larger than uid_t), then this code is incorrect when the return type is kept as uid_t.

(id_t return type always works, since it is guaranteed to be able to hold a uid, gid, or pid value.)

delphij added inline comments.
257 ↗(On Diff #47232)

Unrelated to the change itself, but shouldn't the 'name"' be replaced by 'id"'?

This revision was automatically updated to reflect the committed changes.