Skip to content

logsrvd_json_log_cb: don't let client info override server-set keys - #594

Open
iefa-m wants to merge 1 commit into
sudo-project:mainfrom
iefa-m:logsrvd-json-reserved-keys
Open

iefa-m wants to merge 1 commit into
sudo-project:mainfrom
iefa-m:logsrvd-json-reserved-keys

Conversation

@iefa-m

@iefa-m iefa-m commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

With log_format = json, store_accept_local() and store_reject_local() pass the client's InfoMessage list to logsrvd_json_log_cb(). That callback writes every key into the event object after format_json() has already written the keys sudo_logsrvd sets itself: uuid, server_time, submit_time, peeraddr, iolog_path, iolog_offset and reason. Nothing checks the client's key names, so a client that sends peeraddr=10.9.9.9, uuid=00000000-... or iolog_path=/var/log/sudo-io/forged in its AcceptMessage gets a second copy of each key in the log. Most JSON readers (Python json, jq, typical SIEM ingest) keep the last value, so the record shows the address, UUID and I/O log path the client chose instead of the ones the server recorded. The real sudo client and sudo_sendlog never send these keys.

The fix skips those reserved keys in the callback and logs a warning. The callback is the only place that writes client keys into the event, and it only runs for accept and reject events. I reproduced it by running sudo_logsrvd (built from main, JSON logfile) with a small protobuf client that sends the keys above. Before the patch the event has two peeraddr/uuid/iolog_path entries and json.load() returns 10.9.9.9. After the patch only the server's values are written and the daemon warns protocol error: reserved key peeraddr. make check passes.

The JSON event log callback copied every InfoMessage key from the
client after format_json() had already written uuid, server_time,
submit_time, peeraddr, iolog_path, iolog_offset and reason.  A client
could send its own peeraddr, uuid or iolog_path and the duplicate key,
which most JSON readers resolve to the last value, replaced the value
recorded by the server.  Skip those keys with a warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant