Skip to content

userdb: Reference-count DBusUserInfo, DBusGroupInfo

Simon McVittie requested to merge smcv/dbus:issue305 into master

  • userdb: Make lookups return a const pointer

    This makes it more obvious that the returned pointer points to a struct owned by the userdb, which must not be freed or have its contents modified, and is only valid to dereference until the next modification to the userdb's underlying hash tables (which in practice means until the lock is released, because after that we have no guarantees about what might be going on in another thread).

  • userdb: Reference-count DBusUserInfo, DBusGroupInfo

    Previously, the hash table indexed by uid (or gid) took ownership of the single reference to the heap-allocated struct, and the hash table indexed by username (or group name) had a borrowed pointer to the same struct that exists in the other hash table.

    However, this can break down if you have two or more distinct usernames that share a numeric identifier. This is generally a bad idea, because the user-space model in such situations does not match the kernel-space reality, and in particular there is no effective kernel-level security boundary between such users, but it is sometimes done anyway.

    In this case, when the second username is looked up in the userdb, it overwrites (replaces) the entry in the hash table that is indexed by uid, freeing the DBusUserInfo. This results in both the key and the value in the hash table that is indexed by username becoming dangling pointers (use-after-free), leading to undefined behaviour, which is certainly not what we want to see when doing access control.

    An equivalent situation can occur with groups, in the rare case where a numeric group ID has two names (although I have not heard of this being done in practice).

    Solve this by reference-counting the data structure. There are up to three references in practice: one held temporarily while the lookup function is populating and storing it, one held by the hash table that is indexed by uid, and one held by the hash table that is indexed by name.

    Closes: #305 (closed)

/cc @pwithnall @thiago @daniel.onaca @poettering

This is potentially a security issue and is already public, so I would appreciate a timely review.

It's potentially a security issue because an attacker might be able to influence our access patterns in the userdb (I can elaborate on this in private email if you want), and use-after-free/undefined behaviour in the vicinity of authentication and authorization is always bad news.

Mitigation: the only systems on which this can happen could be argued to be misconfigured (more than one name for the same uid/gid).

See also #305 (closed) for further discussion of systems where a uid/gid has more than one name, and please comment there (or preferably, open another merge request) if you believe there is anything else that needs to be done for such systems.

Merge request reports