Skip to content

Commit

Permalink
xkbcomp: Add stricter bounds for keycodes and levels
Browse files Browse the repository at this point in the history
Our current implementation uses continuous arrays indexed by keycodes
and levels. This is simple and good enough for realistic keymaps.
However, they are allowed to have big values that will lead to either
memory exhaustion or a waste of memory (sparse arrays).

Added the much stricter upper bounds `0xfff` for keycodes[^1] and 2048
for levels[^2], which should still be plenty enough and provides stronger
memory security.

[^1]: Current max keycode is 0x2ff in Linux.
[^2]: Should be big enough to satisfy automatically generated keymaps.
  • Loading branch information
wismill committed Jan 29, 2025
1 parent 307ce5a commit 502e9e5
Show file tree
Hide file tree
Showing 5 changed files with 85 additions and 3 deletions.
4 changes: 3 additions & 1 deletion src/darray.h
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,12 @@ typedef darray (unsigned long) darray_ulong;
((arr).alloc = (arr).size) * sizeof(*(arr).item)); \
} while (0)

#define darray_max_alloc(itemSize) (UINT_MAX / (itemSize))

static inline unsigned
darray_next_alloc(unsigned alloc, unsigned need, unsigned itemSize)
{
assert(need < UINT_MAX / itemSize / 2); /* Overflow. */
assert(need < darray_max_alloc(itemSize) / 2); /* Overflow. */
if (alloc == 0)
alloc = 4;
while (alloc < need)
Expand Down
30 changes: 30 additions & 0 deletions src/keymap.h
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,36 @@ struct xkb_mod_set {
xkb_mod_mask_t explicit_vmods;
};

/*
* Our current implementation with continuous arrays does not allow efficient
* mapping of keycodes. Allowing the API max valid keycode XKB_KEYCODE_MAX could
* result in memory exhaustion or memory waste (sparse arrays) with huge enough
* valid values. Let’s be more conservative for now, based on the existing Linux
* keycodes.
*/
#define XKB_KEYCODE_MAX_IMPL 0xfff
static_assert(XKB_KEYCODE_MAX_IMPL < XKB_KEYCODE_MAX,
"Valid keycodes");
static_assert(XKB_KEYCODE_MAX_IMPL < darray_max_alloc(sizeof(xkb_atom_t)),
"Max keycodes names");
static_assert(XKB_KEYCODE_MAX_IMPL < darray_max_alloc(sizeof(struct xkb_key)),
"Max keymap keys");

/*
* Our current implementation with continuous arrays does not allow efficient
* mapping of levels. Allowing the max valid level UINT32_MAX could result in
* memory exhaustion or memory waste (sparse arrays) with huge enough valid
* values. Let’s be more conservative for now. This value should be big enough
* to satisfy automatically generated keymaps.
*/
#define XKB_LEVEL_MAX_IMPL 2048
static_assert(XKB_LEVEL_MAX_IMPL < XKB_LEVEL_INVALID,
"Valid levels");
static_assert(XKB_LEVEL_MAX_IMPL < darray_max_alloc(sizeof(xkb_atom_t)),
"Max key types level names");
static_assert(XKB_LEVEL_MAX_IMPL < darray_max_alloc(sizeof(struct xkb_level)),
"Max keys levels");

/* Common keyboard description structure */
struct xkb_keymap {
struct xkb_context *ctx;
Expand Down
5 changes: 3 additions & 2 deletions src/xkbcomp/expr.c
Original file line number Diff line number Diff line change
Expand Up @@ -445,9 +445,10 @@ ExprResolveLevel(struct xkb_context *ctx, const ExprDef *expr,
if (!ok)
return false;

if (result < 1) {
if (result < 1 || result > XKB_LEVEL_MAX_IMPL) {
log_err(ctx, XKB_ERROR_UNSUPPORTED_SHIFT_LEVEL,
"Shift level %d is out of range\n", result);
"Shift level %d is out of range (1..%u)\n",
result, XKB_LEVEL_MAX_IMPL);
return false;
}

Expand Down
8 changes: 8 additions & 0 deletions src/xkbcomp/keycodes.c
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,14 @@ AddKeyName(KeyNamesInfo *info, xkb_keycode_t kc, xkb_atom_t name,

report = report && ((same_file && verbosity > 0) || verbosity > 7);

/* Check keycode is not too huge for our continuous array */
if (kc > XKB_KEYCODE_MAX_IMPL) {
log_err(info->ctx, XKB_LOG_MESSAGE_NO_ID,
"Keycode too big: must be < %#"PRIx32", got %#"PRIx32"; "
"Key ignored\n", XKB_KEYCODE_MAX_IMPL, kc);
return false;
}

if (kc >= darray_size(info->key_names))
darray_resize0(info->key_names, kc + 1);

Expand Down
41 changes: 41 additions & 0 deletions test/buffercomp.c
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,46 @@ test_recursive(struct xkb_context *ctx)
}
}

/* Test some limits related to allocations */
static void
test_alloc_limits(struct xkb_context *ctx)
{
const char * const keymaps[] = {
/* Keycodes */
"xkb_keymap {\n"
/* Valid keycode value, but we should not handle it
* with our *continuous* array! */
" xkb_keycodes { <> = 0xfffffffe; };\n"
" xkb_types { };\n"
" xkb_compat { };\n"
" xkb_symbols { key <> {[a]}; };\n"
"};",
/* Key types */
"xkb_keymap {\n"
" xkb_keycodes { };\n"
" xkb_types {\n"
" type \"X\" { map[none] = 0xfffffffe; };\n" /* Invalid level index */
" };\n"
" xkb_compat { };\n"
" xkb_symbols { };\n"
"};",
"xkb_keymap {\n"
" xkb_keycodes { };\n"
" xkb_types {\n"
" type \"X\" {levelname[0xfffffffe]=\"x\";};\n" /* Invalid level index */
" };\n"
" xkb_compat { };\n"
" xkb_symbols { };\n"
"};"
};
for (unsigned int k = 0; k < ARRAY_SIZE(keymaps); k++) {
fprintf(stderr, "------\n*** %s: #%u ***\n", __func__, k);
const struct xkb_keymap *keymap =
test_compile_buffer(ctx, keymaps[k], strlen(keymaps[k]));
assert(!keymap);
}
}

/* Test various multi-{keysym,action} syntaxes */
static void
test_multi_keysyms_actions(struct xkb_context *ctx)
Expand Down Expand Up @@ -421,6 +461,7 @@ main(int argc, char *argv[])
test_encodings(ctx);
test_component_syntax_error(ctx);
test_recursive(ctx);
test_alloc_limits(ctx);
test_multi_keysyms_actions(ctx);
test_invalid_symbols_fields(ctx);
test_prebuilt_keymap_roundtrip(ctx);
Expand Down

0 comments on commit 502e9e5

Please sign in to comment.