[Bug 266101] ucred reference count may overflow

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=266101

--- Comment #1 from Mateusz Guzik <mjg@FreeBSD.org> ---
the refcount API is just a poor man's mitigation, especially for the cred
stuff.

Perf wise, I have to note the counter gets modified *a lot* all while the
struct keeps being accessed. Atomic op on a centralized counter is tons of
memory traffic which does not need to happen, but I don't have numbers handy.

Here are some tidbits:

1. One massive limitation of refcount API, which can't be helped, is that
*underflows* cannot possibly be detected, thus use-after-free remains possible.

2. Even if there were no conditions triggering premature freeing, buggy code
can still have a stale pointer to creds which gets used later.

3. There is nothing being done to prevent overflows from non-cred objects
messing with said creds. instead, memory is allocated with malloc:

cr = malloc(sizeof(*cr), M_CRED, M_WAITOK | M_ZERO);

If one is serious about hardening creds, most of cred management has to be
reworked.

Typical case is that new creds get very temporarily allocated, get modified in
a small manner and assigned. This happens a bunch of times before the process
settles on final state which is then used for a *long time*. Trivial example:
logging in and setgid, setuid etc. calls

Apart from the churn, this also happens to increase memory usage -- you log in
twice, you end up with 2 sets of identical creds which persist.

Thus the general idea would be to maintain a global hash of creds, where you
locally create what creds would look like you and try to find them in said
struct. If that succeds, alloc/free trip got avoided, therwise you alloc to add
them.

I can't stress enough that creds are unmodifiable after creation and *never*
freed. With the proposed scheme ref counting can be literally eliminated(!).

The problem to solve here is making sure a nasty userspace cannot keep creating
creds, without breaking anything. There is probably some bad idioms in place
which result in more allocations than needed.

It may be a hybrid approach will be prudent -- you *do* refcount(9) for all the
transient stuff and "stabilize" after enough traffic.

There are some gaps to fill in the proposal, but the general idea should be
clear.

I can't stress enough how the refcount API fails to address anything most of
things of importance and that addressing them renders it mostly moot.

-- 
You are receiving this mail because:
You are the assignee for the bug.