Bug 233578 - Unprivileged local user can prevent other users logging in by locking utx.active
Summary: Unprivileged local user can prevent other users logging in by locking utx.active
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: Normal Affects Many People
Assignee: freebsd-bugs mailing list
URL: https://davmac.wordpress.com/2019/05/...
Keywords: needs-qa, patch, security
Depends on:
Blocks:
 
Reported: 2018-11-27 20:03 UTC by Davin McCall
Modified: 2019-05-19 01:42 UTC (History)
8 users (show)

See Also:
koobs: maintainer-feedback? (secteam)
koobs: mfc-stable11?
koobs: mfc-stable12?


Attachments
Proposed patch (untested) (3.45 KB, patch)
2018-11-28 04:09 UTC, Conrad Meyer
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 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 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 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 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 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 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 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.