From 6c56843bac68996bd584f733232d1286dd7153bd Mon Sep 17 00:00:00 2001 From: Pierre Le Marre Date: Tue, 26 Sep 2023 17:05:05 +0200 Subject: [PATCH 1/3] Disallow producing NULL character with escape sequences 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. --- doc/message-registry.md | 12 +++++++++++- doc/message-registry.yaml | 5 +++++ src/compose/parser.c | 19 +++++++++++++++---- src/messages-codes.h | 2 ++ src/utils.h | 9 +++++++++ src/xkbcomp/scanner.c | 10 +++++++++- test/compose.c | 12 ++++++++++++ test/data/keymaps/invalid-escape-sequence.xkb | 9 +++++++++ test/filecomp.c | 1 + tools/messages.c | 1 + 10 files changed, 74 insertions(+), 6 deletions(-) create mode 100644 test/data/keymaps/invalid-escape-sequence.xkb diff --git a/doc/message-registry.md b/doc/message-registry.md index a3be5c185..24b2ffc0b 100644 --- a/doc/message-registry.md +++ b/doc/message-registry.md @@ -6,7 +6,7 @@ NOTE: This file has been generated automatically by “update-message-registry.p --> This page lists the warnings and errors generated by xkbcommon. -There are currently 52 entries. +There are currently 53 entries. @todo The documentation of the log messages is a work in progress. @@ -24,6 +24,7 @@ There are currently 52 entries. | [XKB-150] | `wrong-statement-type` | The type of the statement is not allowed in the context | Error | | [XKB-172] | `unsupported-geometry-section` | Geometry sections are not supported | Warning | | [XKB-183] | `cannot-infer-key-type` | Warn if no key type can be inferred | Warning | +| [XKB-193] | `invalid-escape-sequence` | Invalid escape sequence in a string | Warning | | [XKB-195] | `illegal-key-type-preserve-result` | The result of a key type “preserve” entry must be a subset of its input modifiers. | Warning | | [XKB-203] | `invalid-include-statement` | Syntax error in the include statement | Error | | [XKB-206] | `invalid-modmap-entry` | A modmap entry is invalid | Error | @@ -198,6 +199,14 @@ key {[ comma, semicolon, periodcentered, multiply ]};
Summary
Warn if no key type can be inferred
+### XKB-193 – Invalid escape sequence {#XKB-193} + +
+
Since
1.0.0
+
Type
Warning
+
Summary
Invalid escape sequence in a string
+
+ ### XKB-195 – Illegal key type preserve result {#XKB-195}
@@ -624,6 +633,7 @@ The modifiers used in `map` or `preserve` entries should be declared using the e [XKB-150]: @ref XKB-150 [XKB-172]: @ref XKB-172 [XKB-183]: @ref XKB-183 +[XKB-193]: @ref XKB-193 [XKB-195]: @ref XKB-195 [XKB-203]: @ref XKB-203 [XKB-206]: @ref XKB-206 diff --git a/doc/message-registry.yaml b/doc/message-registry.yaml index 880c2f6a4..9f4c45047 100644 --- a/doc/message-registry.yaml +++ b/doc/message-registry.yaml @@ -100,6 +100,11 @@ added: ALWAYS type: warning description: "Warn if no key type can be inferred" +- id: "invalid-escape-sequence" + code: 193 + added: ALWAYS + type: warning + description: "Invalid escape sequence in a string" - id: "illegal-key-type-preserve-result" code: 195 added: ALWAYS diff --git a/src/compose/parser.c b/src/compose/parser.c index e1b81dea2..57be1bd82 100644 --- a/src/compose/parser.c +++ b/src/compose/parser.c @@ -178,13 +178,24 @@ lex(struct scanner *s, union lvalue *val) scanner_buf_append(s, '"'); } else if (scanner_chr(s, 'x') || scanner_chr(s, 'X')) { - if (scanner_hex(s, &o)) + if (scanner_hex(s, &o) && is_valid_char((char) o)) { scanner_buf_append(s, (char) o); - else - scanner_warn(s, "illegal hexadecimal escape sequence in string literal"); + } else { + // [TODO] actually show the sequence + scanner_warn_with_code(s, + XKB_WARNING_INVALID_ESCAPE_SEQUENCE, + "illegal hexadecimal escape sequence in string literal"); + } } else if (scanner_oct(s, &o)) { - scanner_buf_append(s, (char) o); + if (is_valid_char((char) o)) { + scanner_buf_append(s, (char) o); + } else { + // [TODO] actually show the sequence + scanner_warn_with_code(s, + XKB_WARNING_INVALID_ESCAPE_SEQUENCE, + "illegal octal escape sequence in string literal"); + } } else { scanner_warn(s, "unknown escape sequence (%c) in string literal", scanner_peek(s)); diff --git a/src/messages-codes.h b/src/messages-codes.h index 5d8c84c71..af7bb930a 100644 --- a/src/messages-codes.h +++ b/src/messages-codes.h @@ -36,6 +36,8 @@ enum xkb_message_code { XKB_WARNING_UNSUPPORTED_GEOMETRY_SECTION = 172, /** Warn if no key type can be inferred */ XKB_WARNING_CANNOT_INFER_KEY_TYPE = 183, + /** Invalid escape sequence in a string */ + XKB_WARNING_INVALID_ESCAPE_SEQUENCE = 193, /** The result of a key type “preserve” entry must be a subset of its input modifiers. */ XKB_WARNING_ILLEGAL_KEY_TYPE_PRESERVE_RESULT = 195, /** Syntax error in the include statement */ diff --git a/src/utils.h b/src/utils.h index fdf758b73..aa7969c12 100644 --- a/src/utils.h +++ b/src/utils.h @@ -65,6 +65,15 @@ #define STRINGIFY(x) #x #define STRINGIFY2(x) STRINGIFY(x) +/* Check if a character is valid in a string literal */ +static inline bool +is_valid_char(char c) +{ + /* Currently we only check for NULL character, but this could be extended + * in the future to further ASCII control characters. */ + return c != 0; +} + char to_lower(char c); diff --git a/src/xkbcomp/scanner.c b/src/xkbcomp/scanner.c index e9d503ced..800d2d0c1 100644 --- a/src/xkbcomp/scanner.c +++ b/src/xkbcomp/scanner.c @@ -98,7 +98,15 @@ _xkbcommon_lex(YYSTYPE *yylval, struct scanner *s) else if (scanner_chr(s, 'f')) scanner_buf_append(s, '\f'); else if (scanner_chr(s, 'v')) scanner_buf_append(s, '\v'); else if (scanner_chr(s, 'e')) scanner_buf_append(s, '\033'); - else if (scanner_oct(s, &o)) scanner_buf_append(s, (char) o); + else if (scanner_oct(s, &o)) { + if (is_valid_char((char) o)) { + scanner_buf_append(s, (char) o); + } else { + scanner_warn_with_code(s, + XKB_WARNING_INVALID_ESCAPE_SEQUENCE, + "invalid octal escape sequence: \\%o", o); + } + } else { // TODO: display actual sequence! See: scanner_peek(s). // require escaping any potential control character diff --git a/test/compose.c b/test/compose.c index 5e7cba0f3..3d4580503 100644 --- a/test/compose.c +++ b/test/compose.c @@ -684,6 +684,17 @@ test_traverse(struct xkb_context *ctx) xkb_compose_table_unref(table); } +static void +test_escape_sequences(struct xkb_context *ctx) +{ + const char *table_string = " : \"f\\x0o\\0o\" X\n"; + + assert(test_compose_seq_buffer(ctx, table_string, + XKB_KEY_o, XKB_COMPOSE_FEED_ACCEPTED, XKB_COMPOSE_COMPOSING, "", XKB_KEY_NoSymbol, + XKB_KEY_e, XKB_COMPOSE_FEED_ACCEPTED, XKB_COMPOSE_COMPOSED, "foo", XKB_KEY_X, + XKB_KEY_NoSymbol)); +} + int main(int argc, char *argv[]) { @@ -717,6 +728,7 @@ main(int argc, char *argv[]) test_include(ctx); test_override(ctx); test_traverse(ctx); + test_escape_sequences(ctx); xkb_context_unref(ctx); return 0; diff --git a/test/data/keymaps/invalid-escape-sequence.xkb b/test/data/keymaps/invalid-escape-sequence.xkb new file mode 100644 index 000000000..99349ec78 --- /dev/null +++ b/test/data/keymaps/invalid-escape-sequence.xkb @@ -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 + // truncates the string to "evde", while we expect "evdev+aliases(qwerty)". + xkb_keycodes { include "evde\0v+aliases(qwerty)" }; + xkb_types { include "complete" }; + xkb_compat { include "complete" }; + xkb_symbols { include "pc+us" }; +}; diff --git a/test/filecomp.c b/test/filecomp.c index 5fc747778..e73dc4c36 100644 --- a/test/filecomp.c +++ b/test/filecomp.c @@ -48,6 +48,7 @@ main(void) assert(test_file(ctx, "keymaps/quartz.xkb")); assert(test_file(ctx, "keymaps/no-aliases.xkb")); assert(test_file(ctx, "keymaps/modmap-none.xkb")); + assert(test_file(ctx, "keymaps/invalid-escape-sequence.xkb")); assert(!test_file(ctx, "keymaps/divide-by-zero.xkb")); assert(!test_file(ctx, "keymaps/bad.xkb")); diff --git a/tools/messages.c b/tools/messages.c index fc3b4117d..9230dfdbf 100644 --- a/tools/messages.c +++ b/tools/messages.c @@ -48,6 +48,7 @@ static const struct xkb_message_entry xkb_messages[] = { {XKB_ERROR_WRONG_STATEMENT_TYPE, "Wrong statement type"}, {XKB_WARNING_UNSUPPORTED_GEOMETRY_SECTION, "Unsupported geometry section"}, {XKB_WARNING_CANNOT_INFER_KEY_TYPE, "Cannot infer key type"}, + {XKB_WARNING_INVALID_ESCAPE_SEQUENCE, "Invalid escape sequence"}, {XKB_WARNING_ILLEGAL_KEY_TYPE_PRESERVE_RESULT, "Illegal key type preserve result"}, {XKB_ERROR_INVALID_INCLUDE_STATEMENT, "Invalid include statement"}, {XKB_ERROR_INVALID_MODMAP_ENTRY, "Invalid modmap entry"}, From 716f7832129493e357bbf980e299cd2c708db231 Mon Sep 17 00:00:00 2001 From: Pierre Le Marre Date: Tue, 26 Sep 2023 17:05:14 +0200 Subject: [PATCH 2/3] Prevent overflow of octal escape sequences 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. --- src/scanner-utils.h | 9 ++++++++- test/compose.c | 6 +++++- test/data/keymaps/invalid-escape-sequence.xkb | 4 +++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/scanner-utils.h b/src/scanner-utils.h index f4c799eeb..674ecaa0e 100644 --- a/src/scanner-utils.h +++ b/src/scanner-utils.h @@ -188,7 +188,14 @@ scanner_oct(struct scanner *s, uint8_t *out) { int i; for (i = 0, *out = 0; scanner_peek(s) >= '0' && scanner_peek(s) <= '7' && i < 3; i++) - *out = *out * 8 + scanner_next(s) - '0'; + /* Test overflow */ + if (*out < 040) { + *out = *out * 8 + scanner_next(s) - '0'; + } else { + /* Consume valid digit, but mark result as invalid */ + scanner_next(s); + return false; + } return i > 0; } diff --git a/test/compose.c b/test/compose.c index 3d4580503..8c633d704 100644 --- a/test/compose.c +++ b/test/compose.c @@ -687,7 +687,11 @@ test_traverse(struct xkb_context *ctx) static void test_escape_sequences(struct xkb_context *ctx) { - const char *table_string = " : \"f\\x0o\\0o\" X\n"; + /* The following escape sequences should be ignored: + * • \401 overflows + * • \0 and \x0 produce NULL + */ + const char *table_string = " : \"\\401f\\x0o\\0o\" X\n"; assert(test_compose_seq_buffer(ctx, table_string, XKB_KEY_o, XKB_COMPOSE_FEED_ACCEPTED, XKB_COMPOSE_COMPOSING, "", XKB_KEY_NoSymbol, diff --git a/test/data/keymaps/invalid-escape-sequence.xkb b/test/data/keymaps/invalid-escape-sequence.xkb index 99349ec78..5e66f8b50 100644 --- a/test/data/keymaps/invalid-escape-sequence.xkb +++ b/test/data/keymaps/invalid-escape-sequence.xkb @@ -3,7 +3,9 @@ xkb_keymap { // must be ignored. Else it would insert a NULL character and thus // truncates the string to "evde", while we expect "evdev+aliases(qwerty)". xkb_keycodes { include "evde\0v+aliases(qwerty)" }; - xkb_types { include "complete" }; + // The following include statement has two octal escape sequences that + // should be ignored, else they would overflow. + xkb_types { include "com\401ple\777te" }; xkb_compat { include "complete" }; xkb_symbols { include "pc+us" }; }; From ed491a9c19162144614349b76769c4b685cfc847 Mon Sep 17 00:00:00 2001 From: Pierre Le Marre Date: Tue, 26 Sep 2023 17:05:14 +0200 Subject: [PATCH 3/3] Show invalid escape sequences It is easier to debug when the message actually displays the offending escape sequence. --- src/compose/parser.c | 28 ++++++++++++++++------------ src/xkbcomp/scanner.c | 26 +++++++++++++------------- 2 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/compose/parser.c b/src/compose/parser.c index 57be1bd82..5545a33f8 100644 --- a/src/compose/parser.c +++ b/src/compose/parser.c @@ -171,6 +171,7 @@ lex(struct scanner *s, union lvalue *val) while (!scanner_eof(s) && !scanner_eol(s) && scanner_peek(s) != '\"') { if (scanner_chr(s, '\\')) { uint8_t o; + size_t start_pos = s->pos; if (scanner_chr(s, '\\')) { scanner_buf_append(s, '\\'); } @@ -181,24 +182,27 @@ lex(struct scanner *s, union lvalue *val) if (scanner_hex(s, &o) && is_valid_char((char) o)) { scanner_buf_append(s, (char) o); } else { - // [TODO] actually show the sequence scanner_warn_with_code(s, XKB_WARNING_INVALID_ESCAPE_SEQUENCE, - "illegal hexadecimal escape sequence in string literal"); + "illegal hexadecimal escape sequence (%.*s) in string literal", + (int) (s->pos - start_pos + 1), &s->s[start_pos - 1]); } } - else if (scanner_oct(s, &o)) { - if (is_valid_char((char) o)) { - scanner_buf_append(s, (char) o); - } else { - // [TODO] actually show the sequence - scanner_warn_with_code(s, - XKB_WARNING_INVALID_ESCAPE_SEQUENCE, - "illegal octal escape sequence in string literal"); - } + else if (scanner_oct(s, &o) && is_valid_char((char) o)) { + scanner_buf_append(s, (char) o); + } + else if (s->pos > start_pos) { + scanner_warn_with_code(s, + XKB_WARNING_INVALID_ESCAPE_SEQUENCE, + "illegal octal escape sequence (%.*s) in string literal", + (int) (s->pos - start_pos + 1), &s->s[start_pos - 1]); + /* Ignore. */ } else { - scanner_warn(s, "unknown escape sequence (%c) in string literal", scanner_peek(s)); + scanner_warn_with_code(s, + XKB_WARNING_UNKNOWN_CHAR_ESCAPE_SEQUENCE, + "unknown escape sequence (\\%c) in string literal", + scanner_peek(s)); /* Ignore. */ } } else { diff --git a/src/xkbcomp/scanner.c b/src/xkbcomp/scanner.c index 800d2d0c1..57babbb20 100644 --- a/src/xkbcomp/scanner.c +++ b/src/xkbcomp/scanner.c @@ -90,6 +90,7 @@ _xkbcommon_lex(YYSTYPE *yylval, struct scanner *s) while (!scanner_eof(s) && !scanner_eol(s) && scanner_peek(s) != '\"') { if (scanner_chr(s, '\\')) { uint8_t o; + size_t start_pos = s->pos; if (scanner_chr(s, '\\')) scanner_buf_append(s, '\\'); else if (scanner_chr(s, 'n')) scanner_buf_append(s, '\n'); else if (scanner_chr(s, 't')) scanner_buf_append(s, '\t'); @@ -98,20 +99,19 @@ _xkbcommon_lex(YYSTYPE *yylval, struct scanner *s) else if (scanner_chr(s, 'f')) scanner_buf_append(s, '\f'); else if (scanner_chr(s, 'v')) scanner_buf_append(s, '\v'); else if (scanner_chr(s, 'e')) scanner_buf_append(s, '\033'); - else if (scanner_oct(s, &o)) { - if (is_valid_char((char) o)) { - scanner_buf_append(s, (char) o); - } else { - scanner_warn_with_code(s, - XKB_WARNING_INVALID_ESCAPE_SEQUENCE, - "invalid octal escape sequence: \\%o", o); - } - } + else if (scanner_oct(s, &o) && is_valid_char((char) o)) + scanner_buf_append(s, (char) o); + else if (s->pos > start_pos) + scanner_warn_with_code(s, + XKB_WARNING_INVALID_ESCAPE_SEQUENCE, + "invalid octal escape sequence (%.*s) in string literal", + (int) (s->pos - start_pos + 1), &s->s[start_pos - 1]); + /* Ignore. */ else { - // TODO: display actual sequence! See: scanner_peek(s). - // require escaping any potential control character - scanner_warn_with_code(s, XKB_WARNING_UNKNOWN_CHAR_ESCAPE_SEQUENCE, - "unknown escape sequence in string literal"); + scanner_warn_with_code(s, + XKB_WARNING_UNKNOWN_CHAR_ESCAPE_SEQUENCE, + "unknown escape sequence (\\%c) in string literal", + scanner_peek(s)); /* Ignore. */ } } else {