Bug 233578

Summary: Unprivileged local user can prevent other users logging in by locking utx.active
Product: Base System Reporter: Davin McCall <davmac>
Component: binAssignee: freebsd-bugs (Nobody) <bugs>
Status: Open ---    
Severity: Affects Many People CC: cem, crest, ed, emaste, jilles, kib, secteam, sigsys
Priority: Normal Keywords: needs-qa, patch, security
Version: CURRENTFlags: koobs: maintainer-feedback? (secteam)
koobs: mfc-stable12?
koobs: mfc-stable11?
Hardware: Any   
OS: Any   
URL: https://davmac.wordpress.com/2019/05/04/bad-utmp-implementations-in-glibc-and-freebsd/
Attachments:
Description Flags
Proposed patch (untested) none

Description Davin McCall 2018-11-27 20:03:48 UTC 
The utx.active database (/var/run/utx.active) maintains a list of currently logged-in users; it needs to be updated when a user logs in or out. This file is world-readable (which allows "who" to list logged-in users without requiring suid root).

Since updating the file requires locking it, and this is done via open with O_EXLOCK, it is possible for a user to indefinitely postpone updates to the file by locking the file themselves. Program below can be used to do this (does not require root privileges). While this program is running it will be impossible for any other user (including root) to log in to the system.

The problematic locking code is in pututxline.c, function futx_open(), here:

https://github.com/freebsd/freebsd/blob/master/lib/libc/gen/pututxline.c#L46

The example program is as follows:

--- begin ---
#include <fcntl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>

int main(int argc, char **argv)
{
    open("/var/run/utx.active", O_EXLOCK | O_RDONLY);
    sleep(100);
}
--- end ---

This program runs for 100 seconds during which no other logins will be possible (and logouts will also stall).

In terms of solution, I would recommend either:
(a) making the file not world-readable and making "who" and any other relevant programs setgid to a group with permission to read the file, or
(b) changing the locking mechanism implemented in pututxline.c, so that it locks a separate file which is not world readable and uses that lock to control access to the utx.active file.

Note that GNU libc has a similar issue, but uses an fcntl-based lock with a timeout of 10 seconds. This means that logins can not be completely disabled by the user, but they can prevent the utmp (equivalent to utx.active) database from being updated. I do not recommend this approach.
Comment 1 Conrad Meyer freebsd_committer freebsd_triage 2018-11-27 20:31:25 UTC 
(a) sort of breaks the libc getutxent APIs for ordinary users.

Happily, the getutxent APIs do not try to use locking to get a consistent snapshot of the file, so (b) should fix the problem just fine.

https://github.com/freebsd/freebsd/blob/master/lib/libc/gen/getutxent.c
Comment 2 Conrad Meyer freebsd_committer freebsd_triage 2018-11-28 04:09:46 UTC 
Created attachment 199629 [details]
Proposed patch (untested)
Comment 3 Kubilay Kocak freebsd_committer freebsd_triage 2019-05-05 04:50:50 UTC 
CC recent committers (and original author) to lib/libc/gen/pututxline.c who may be able to review attachment 199629 [details] by cem@
Comment 4 Ed Schouten freebsd_committer freebsd_triage 2019-05-05 12:37:02 UTC 
Hi there,

Instead of going down this road, what are your thoughts on the following?

- Tossing out the use of O_EXLOCK entirely and leave the file writing as it is now,
- Using a single lock file acquisition in pututxline() to serialise write access all data files in one go.

This likely makes the code a bit simpler/lighter, while also improving the sequential consistency guarantees between the data files.
Comment 5 Conrad Meyer freebsd_committer freebsd_triage 2019-05-06 00:35:02 UTC 
(In reply to Ed Schouten from comment #4)
Sure, that approach seems unobjectionable to me.
Comment 6 crest 2019-05-06 12:24:55 UTC 
Is there any reason to keep this "service" in a flat file? Would it be acceptable to turn it into a directory with one file per user or even require writes to go through a daemon serializing writes and using atomic updates (rename and fsync a temp file)=
Comment 7 Ed Schouten freebsd_committer freebsd_triage 2019-05-06 12:35:37 UTC 
(In reply to crest from comment #6)

Sure, we could! The file format/backend used by this API can now be changed to work any way we want. When I implemented utmpx for FreeBSD, the goal was to first see if we could eliminate any direct access to underlying storage, which has now been realised.

That said, to solve this specific issue, there is no need to do anything that drastic.
Comment 8 Konstantin Belousov freebsd_committer freebsd_triage 2019-05-18 18:48:21 UTC 
(In reply to Ed Schouten from comment #7)
On FreeBSD, we guarantee that reader never see torn writes, assuming writer always write single record using one atomic write(2) syscall, and similarly reader uses single atomic read(2) syscall to get the record.

My guess is that if you get rid of stdio(3) use with its buffers, then you can drop O_EXCLOCK and the issue disappears.
Comment 9 Ed Schouten freebsd_committer freebsd_triage 2019-05-19 01:42:39 UTC 
No, that's not the case. The code does more than simply write a record into the file at a certain offset. It also reads entries to determine what the offset is at which the record should be placed.

It's insufficient to replace this code by something that doesn't use file locking. There may be race conditions in which you end up overwriting recently created login session entries.