Repository navigation
expand_iolog_path: don't pass escape values to strftime - #577
Merged
Merged
Conversation
The %{user}, %{group}, %{runas_user}, %{runas_group}, %{hostname}
and %{command} values are copied into the path before it is run
through strftime(3), so a '%' in one of them was treated as a
conversion. In sudo_logsrvd these come from the client, and a
submituser of "%D" expands to "10/22/26", re-introducing the '/'
that strlcpy_no_slash() replaced. A literal "%%" in iolog_dir or
iolog_file was also collapsed before strftime(3) saw it.
Double any '%' in substituted values and leave "%%" alone during
expansion, collapsing it afterwards via strftime(3) or by hand.
millert
requested changes
Oct 26, 2026
|
|
||
| /* Shift right from the end, duplicating each '%'. */ | ||
| for (i = len, n = nesc; n > 0; ) { | ||
| i--; |
Collaborator
There was a problem hiding this comment.
While it is not currently possible for the i-- to underflow I'd still prefer a check. Something like:
if (i-- == 0)
debug_return_size_t((size_t)-1);
Contributor
Author
There was a problem hiding this comment.
Added that check in d6e1f43, so escape_percent now returns (size_t)-1 rather than decrementing i past zero. The iolog regress tests still pass.
millert
approved these changes
Oct 27, 2026
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.
expand_iolog_path()copies the%{user},%{group},%{runas_user},%{runas_group},%{hostname}and%{command}values into the path and then, ifiolog_diroriolog_fileuses any strftime escape, passes the whole buffer to strftime(3). A%inside a substituted value is therefore treated as a conversion. In sudo_logsrvd those values come straight from the client's AcceptMessage, so withiolog_dir = /var/log/sudo-io/%{user}/%Ya submituser of%Dlands in/var/log/sudo-io/10/22/26/2026. That puts back the/characters thatstrlcpy_no_slash()just replaced, which the sudoers and sudo_logsrvd.conf manuals promise doesn't happen, and lets the client pick extra directory levels (%nand%tadd control characters too). A literal%%in the configured path had a related problem: it was collapsed before strftime ran, so%%Y_%Ybecame2026_2026.The fix doubles any
%in each substituted value and keeps%%as-is while expanding. strftime then collapses it, or a small loop does when the path has no strftime escapes, so paths without a%in them come out the same as before. The fix sits inexpand_iolog_path()because the sudoers plugin and sudo_logsrvd both use it with their own escape tables. The iolog_path regress data gains cases for a%in a user name and command, a%value with no strftime escapes, and%%next to a strftime escape. The first and third fail before this change, andmake checkpasses after it.