Skip to content

lib/utmp.c: prepare_utmp(): Fix buffer overrun when 'line' isn't "tty*" #1297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
alejandro-colomar wants to merge 1 commit into from
Closed

lib/utmp.c: prepare_utmp(): Fix buffer overrun when 'line' isn't "tty*" #1297

alejandro-colomar wants to merge 1 commit into from

Conversation

@alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Jul 16, 2025 •
edited
Loading 

Sometimes, line may be a string of length less than 3, in which case the
+3 was jumping somewhere after the terminating null byte.  This is
probably not easy to exploit, as we only read at most 4 bytes, but this
was theoretically quite bad.

Reported-by: @Karlson2k

@alejandro-colomar alejandro-colomar changed the title lib/utmp.c: prepare_utmp(): Fix bug when 'line' isn't "/dev/tty*" lib/utmp.c: prepare_utmp(): Fix bug when 'line' isn't "tty*" Jul 16, 2025
@alejandro-colomar alejandro-colomar force-pushed the ut_id branch 2 times, most recently from f0c6249 to e65c252 Compare July 16, 2025 21:07
@alejandro-colomar alejandro-colomar changed the title lib/utmp.c: prepare_utmp(): Fix bug when 'line' isn't "tty*" lib/utmp.c: prepare_utmp(): Fix buffer overrun when 'line' isn't "tty*" Jul 16, 2025
@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Jul 16, 2025

@Karlson2k Do you have a system where this bug is reproducible?

@alejandro-colomar
lib/utmp.c: prepare_utmp(): Fix buffer overrun when 'line' isn't "tty*"
92ccb50 
Sometimes, line may be a string of length less than 3, in which case the
+3 was jumping somewhere after the terminating null byte.  This is
probably not easy to exploit, as we only read at most 4 bytes, but this
was theoretically quite bad.

Fixes: 7fb1063 (2009-04-22; "* libmisc/utmp.c: The ut argument of prepare_utmp() might be NULL.      ut_id needs to be forged in that case.")
Reported-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@Karlson2k
Copy link
Contributor

Karlson2k commented Jul 17, 2025

@Karlson2k Do you have a system where this bug is reproducible?

No.
It is a hypothetical possibility.
However, it is not very hard to modify other software to use shorter strings, but this assumes that user already has full control on the system.

It would be nice to pick my patch for this issue.

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Jul 17, 2025

@Karlson2k Do you have a system where this bug is reproducible?

No. It is a hypothetical possibility. However, it is not very hard to modify other software to use shorter strings, but this assumes that user already has full control on the system.

It would be nice to pick my patch for this issue.

If you write it with ?:, I'd pick your patch. I'm just opposed to being excessively verbose where ?: will just work.

Also, please open a separate PR for the second patch. We can only merge via github, which doesn't allow picking.

@Karlson2k
Copy link
Contributor

Karlson2k commented Jul 17, 2025

If you write it with ?:, I'd pick your patch. I'm just opposed to being excessively verbose where ?: will just work.

Also, please open a separate PR for the second patch. We can only merge via github, which doesn't allow picking.

Done.
Regarding merging: manual cherry picking locally + manual PR closing always works. 😉

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Jul 17, 2025

If you write it with ?:, I'd pick your patch. I'm just opposed to being excessively verbose where ?: will just work.
Also, please open a separate PR for the second patch. We can only merge via github, which doesn't allow picking.

Done. Regarding merging: manual cherry picking locally + manual PR closing always works. 😉

Nope; we can't push manually to master. The branch is protected. (We could change that, but we'd need to agree on that.)

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Jul 17, 2025 •
edited
Loading

Merged @Karlson2k 's equivalent patch in #1301 .

@alejandro-colomar alejandro-colomar deleted the ut_id branch July 18, 2025 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hallyn hallyn Awaiting requested review from hallyn

@ikerexxe ikerexxe Awaiting requested review from ikerexxe

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alejandro-colomar @Karlson2k