Repository navigation
Conversation
3v1n0
force-pushed
the
tty-resolve-once
branch
4 times, most recently
from
September 24, 2026 15:26
374b386 to
acb2916
Compare
Member
Just to clarify: you mean that if we decide to merge this, we can close #1682, right? |
bjorn3
requested changes
Sep 29, 2026
The controlling tty was resolved independently each time it was needed: - To compute PAM_TTY - For the log entry - For session record scope. Each time this meant reading /proc/self/stat and possibly scanning /dev/pts and /dev. Resolve it once when building the sudo context instead, as sudo.ws does in get_user_info(), and pass its device and reported name to all the users, so that they are also guaranteed to agree on the terminal. Closes: trifectatechfoundation#1683
The controlling tty path is resolved before being opened for locking, so in the meantime it may have started referring to another terminal, for example if ours got closed and its pts index was reallocated. Add CurrentTty::open_verified() to open the path and check through the file descriptor that it still refers to the controlling tty device, and use it when locking, falling back to /dev/tty otherwise.
When opening the controlling tty by path, use O_NOCTTY so that it can never become our controlling terminal, and O_NONBLOCK to avoid blocking on serial lines that are waiting for carrier, as we only need it for locking. This matches what sudo.ws does when it opens the tty by path to relabel it in selinux.c.
3v1n0
force-pushed
the
tty-resolve-once
branch
from
October 12, 2026 16:40
acb2916 to
2c32c92
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The controlling tty was resolved independently each time it was needed:
for PAM_TTY, for the log entry and for the session record scope. Each
time this meant reading /proc/self/stat and possibly scanning /dev/pts
and /dev.
Resolve it once when building the sudo context instead, as sudo.ws does
in get_user_info(), and pass its device and reported name to all the
users, so that they are also guaranteed to agree on the terminal.
Also ensures that the opened device is the one we expected to.
Sadly we cannot just pass the fd as PAM_TTY as many modules require it to
be a
/dev/*.Closes: #1683
This is built to be a prerequisite of #1682 (and in fact the second commit includes it) as IMHO it would be cleaner in this order, but if you prefer to just go with the fix we can go with that one and then this can be rebased to be a refactor of that only.