Skip to content
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.

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

Disallow producing NULL with escape sequences & prevent overflow #374

Merged
merged 3 commits into from
Sep 26, 2023

Conversation

wismill
Copy link
Member

@wismill wismill commented Sep 19, 2023

NULL usually terminates the strings; allowing to produce it via escape sequences may lead to undefined behaviour.

  • Make null escape sequences (e.g. \0 and \x0) invalid
  • Introduce the new message: XKB_WARNING_INVALID_ESCAPE_SEQUENCE.
  • Prevent overflow (e.g. \400, \777)
  • Improve log messages.
  • Add corresponding tests.

Fixes #363

@wismill wismill requested review from whot and bluetech September 19, 2023 16:02
@wismill wismill added bug Indicates an unexpected problem or unintended behavior compose Indicates a need for improvements or additions to Compose handling compile-keymap Indicates a need for improvements or additions to keymap compilation labels Sep 19, 2023
@wismill wismill added this to the 1.6.0 milestone Sep 19, 2023
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Great catches, thanks.

src/utils.h Outdated Show resolved Hide resolved
@@ -0,0 +1,9 @@
xkb_keymap {
// The following include statement has an octal escape sequence that
// must be ignored. Else it would insert a NULL character and thus
Copy link
Member

Choose a reason for hiding this comment

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

NULL -> NUL

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, the character name is NULL. NUL is the ASCII abreviation. I prefer the name, but if you prefer the abbreviation then I suggest we use \NUL instead.

@wismill
Copy link
Member Author

wismill commented Sep 24, 2023

Rebased with review fixes.

@bluetech @whot this PR is only about \NUL character, but I wonder if it make sense to allow the other control characters.

@bluetech
Copy link
Member

this PR is only about \NUL character, but I wonder if it make sense to allow the other control characters.

Is there a reason to prevent it?

@wismill
Copy link
Member Author

wismill commented Sep 24, 2023

Well, I wonder if some strings are exposed and where. They might be exposed by some of the log entries, currently unescaped. They could be exposed by functions like xkb_keymap_layout_get_name and those in xkbregistry, but I would have to investigate this carefully. My point is after discovering the \NUL improper handling, there may be insufficient filtering and/or escaping that the users of the API are unaware of. Also, because there is no explicit encoding, I am not sure how the char* are interpreted. In xkeyboard-config they are all ASCII if I am not mistaken, but there could be a way to craft malicious XKB files. It would not be an issue for xkbcommon itself, but could create problems for its users.

Just thinking out loud here; we should deal with it in a dedicated issue if relevant.

@bluetech
Copy link
Member

there could be a way to craft malicious XKB files

I have thought about this topic before.

My assumption is that xkbcommon, being a huge custom parser written in C in the 90s, is not safe from malicious input. So the question is whether there is a threat model in which xkbcommon is fed untrusted keymaps. In the cases I know and could think of, xkb keymap input comes trusted sources, like a compositor, X server, /usr/share, user's $HOME, xkbcli flag. If any of those are compromised then something has already gone wrong. (Maybe a malicious person posting a "Try my awesome keymap" post on the internet, causing users to download and "execute" it)?

Of course this is no excuse to make xkbcommon full of holes, and we even have a rudimentary fuzzer, but it does make me weigh compatibility more than malicious input.

NULL usually terminates the strings; allowing to produce it via escape
sequences may lead to undefined behaviour.

- Make NULL escape sequences (e.g. `\0` and `\x0`) invalid.
- Add corresponding test.
- Introduce the new message: XKB_WARNING_INVALID_ESCAPE_SEQUENCE.
The octal parser accepts the range `\1..\777`. The result is cast to
`char` which will silently overflow.

This commit prevents overlow and will treat `\400..\777` as invalid
escape sequences.
It is easier to debug when the message actually displays the offending
escape sequence.
@wismill
Copy link
Member Author

wismill commented Sep 26, 2023

@bluetech let’s continue this discussion in private. It is far too serious a topic.

@wismill
Copy link
Member Author

wismill commented Sep 26, 2023

Rebased & improved commit message. Will merge when CI is successful.

@wismill wismill merged commit 9d15c6a into xkbcommon:master Sep 26, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior compile-keymap Indicates a need for improvements or additions to keymap compilation compose Indicates a need for improvements or additions to Compose handling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Octal escape allows \0..\777
2 participants