Skip to content

json_append_string: compute hex escape from the unsigned byte - #559

Merged
millert merged 1 commit into
sudo-project:mainfrom
iefa-m:json-escape-unsigned-byte
Oct 8, 2026
Merged

millert merged 1 commit into
sudo-project:mainfrom
iefa-m:json-escape-unsigned-byte

Conversation

@iefa-m

@iefa-m iefa-m commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

json_append_string escapes control characters as \u00XX and takes the two hex digits from hex[ch >> 4] and hex[ch & 0x0f], where ch is a plain char. The iscntrl() test above it is cast to unsigned char, so bytes with the high bit set do reach that code wherever the ctype table marks the C1 range as control characters, which macOS does in the default en_US.UTF-8 locale and glibc does in the ISO-8859-x locales. For one of those bytes ch is negative, ch >> 4 is negative too, and hex[-8] reads eight bytes before the array. The byte it picks up becomes the first hex digit, so a command, argument, user name or environment value containing 0x85 writes an adjacent stack byte into log.json instead of '8'. cvtsudoers -f json over a sudoers file holding such a byte reproduces it (ubsan reports "index -8 out of bounds for type 'const char[17]'", asan a stack-buffer-underflow in json_append_string), and sudo_logsrvd reaches the same code with strings supplied by a client. Taking both nibbles from an unsigned copy of the byte fixes it, which is what escape() in lbuf.c already does; this is the only place in the tree with the pattern. The json regress test now walks every byte value and compares the escape it gets against iscntrl().

@millert millert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for adding a test too.

@millert
millert merged commit b2e6269 into sudo-project:main Oct 8, 2026
5 checks passed
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.

2 participants