Handle overflow of uid or gid in arguments for chown

[dgmorris_earthlink.net]

Summary

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 foo.bar
-rw-r--r-- 1 dmorris wheel 0 Apr 13 07:52 foo.bar
dmorris@hvbsdtst:/tmp % sudo chown 4294967296 foo.bar;ll foo.bar
-rw-r--r-- 1 root wheel 0 Apr 13 07:52 foo.bar
dmorris@hvbsdtst:/tmp % sudo chown 4294967298 foo.bar ; ll foo.bar
-rw-r--r-- 1 operator wheel 0 Apr 13 07:52 foo.bar

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 foo.bar ; ll foo.bar
chown: 4294967296: illegal user name
-rw-r--r-- 1 dmorris wheel 0 Apr 13 13:20 foo.bar
dmorris@hvbsdtst:/tmp % sudo ./chown 4294967298 foo.bar ; ll foo.bar
chown: 4294967298: illegal user name
-rw-r--r-- 1 dmorris wheel 0 Apr 13 13:20 foo.bar
dmorris@hvbsdtst:/tmp % sudo ./chown dmorris:4294967298 foo.bar ; ll foo.bar
chown: 4294967298: illegal group name
-rw-r--r-- 1 dmorris wheel 0 Apr 13 13:20 foo.bar

[darrick.freebsd_gmail.com]

usr.sbin/chown/chown.c

68 ↗

(On Diff #41588)

Possible whitespace error?

[dgmorris_earthlink.net]

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

[darrick.freebsd_gmail.com]

[cem]

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.

usr.sbin/chown/chown.c

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).

[markj]

usr.sbin/chown/chown.c

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.

[cem]

usr.sbin/chown/chown.c

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, §6.3.1.8, "Usual arithmetic conversions")

§6.3.1.3 "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]

usr.sbin/chown/chown.c

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.

[jhb]

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))'

usr.sbin/chown/chown.c

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)

[cem]

usr.sbin/chown/chown.c

247 ↗

(On Diff #41596)

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

[jhb]

usr.sbin/chown/chown.c

247 ↗

(On Diff #41596)

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

[cem]

usr.sbin/chown/chown.c

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 :-).

[dgmorris_earthlink.net]

Updated per review comments -- did I miss anything?

Anything else that needs to be done?

[kib]

usr.sbin/chown/chown.c

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 ?

[cem]

usr.sbin/chown/chown.c

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]

usr.sbin/chown/chown.c

257 ↗

(On Diff #47232)

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