GetComponentByName returns an allocated string, so let's free that if we
fail somewhere.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit 18f91b950e)
GetCountedString did a check for the whole string to be within the
request buffer but not for the initial 2 bytes that contain the length
field. A swapped client could send a malformed request to trigger a
swaps() on those bytes, writing into random memory.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit 11beef0b7f)
Unlike other elements of the keymap, this pointer was freed but not
reset. On a subsequent XkbGetKbdByName request, the server may access
already freed memory.
CVE-2022-4283, ZDI-CAN-19530
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Acked-by: Olivier Fourdan <ofourdan@redhat.com>
(cherry picked from commit ccdd431cd8)
No validation of the various fields on that report were done, so a
malicious client could send a short request that claims it had N
sections, or rows, or keys, and the server would process the request for
N sections, running out of bounds of the actual request data.
Fix this by adding size checks to ensure our data is valid.
ZDI-CAN 16062, CVE-2022-2319.
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit 6907b6ea2b)
XKB often uses a FooCheck and Foo function pair, the former is supposed
to check all values in the request and error out on BadLength,
BadValue, etc. The latter is then called once we're confident the values
are good (they may still fail on an individual device, but that's a
different topic).
In the case of XkbSetDeviceInfo, those functions were incorrectly
named, with XkbSetDeviceInfo ending up as the checker function and
XkbSetDeviceInfoCheck as the setter function. As a result, the setter
function was called before the checker function, accessing request
data and modifying device state before we ensured that the data is
valid.
In particular, the setter function relied on values being already
byte-swapped. This in turn could lead to potential OOB memory access.
Fix this by correctly naming the functions and moving the length checks
over to the checker function. These were added in 87c64fc5b0 to the
wrong function, probably due to the incorrect naming.
Fixes ZDI-CAN 16070, CVE-2022-2320.
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Introduced in c06e27b2f6
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit dd8caf39e9)
Most similar loops here use a pointer that advances with each loop
iteration, let's do the same here for consistency.
No functional changes.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
(cherry picked from commit f1070c01d6)
As the comment says:
"symsPerKey/mapWidths must be filled regardless of client-side flags"
so we always have to call CheckKeyTypes which will notably fill mapWidths
and nTypes. That is needed for CheckKeySyms to work since it checks the
width. Without it, any request with XkbKeySymsMask but not
XkbKeyTypesMask will fail because of the missing width information, for
instance this:
XkbDescPtr xkb;
if (!(xkb = XkbGetMap (dpy, XkbKeyTypesMask|XkbKeySymsMask, XkbUseCoreKbd))) {
fprintf (stderr, "ERROR getting map\n");
exit(1);
}
XFlush (dpy);
XSync (dpy, False);
XkbMapChangesRec changes = { .changed = 0 };
int oneGroupType[XkbNumKbdGroups] = { XkbOneLevelIndex };
if (XkbChangeTypesOfKey(xkb, keycode, 1, XkbGroup1Mask, oneGroupType, &changes)) {
fprintf(stderr, "ERROR changing type of key\n");
exit(1);
}
XkbKeySymEntry(xkb,keycode,0,0) = keysym;
if (!XkbChangeMap(dpy,xkb,&changes)) {
fprintf(stderr, "ERROR changing map\n");
exit(1);
}
XkbFreeKeyboard (xkb, 0, TRUE);
XFlush (dpy);
XSync (dpy, False);
This had being going under the radar since about ever until commit
de940e06f8 ("xkb: fix key type index check
in _XkbSetMapChecks") fixed checking the values of kt_index, which was
previously erroneously ignoring errors and ignoring all other checks, just
because nTypes was not set, precisely because CheckKeyTypes was not called.
Note: yes, CheckKeyTypes is meant to be callable without XkbKeyTypesMask, it
does properly check for that and just fills nTypes and mapWidths in that
case.
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
(cherry picked from commit 0217cc6e0c)
The previous if/else condition resulted in us always setting the key
type count to the current number of key types. Split this up correctly.
Regression introduced in de940e06f8Fixes#1249
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit be16bd8543)
Commit 446ff2d317 added checks to
prevalidate the size of incoming SetMap requests.
That commit checks for the XkbSetMapResizeTypes flag to be set before
allowing key types data to be processed.
key types data can be changed or even just sent wholesale unchanged
without the number of key types changing, however. The check for
XkbSetMapResizeTypes rejects those legitimate requests. In particular,
XkbChangeMap never sets XkbSetMapResizeTypes and so always fails now
any time XkbKeyTypesMask is in the changed mask.
This commit drops the check for XkbSetMapResizeTypes in flags when
prevalidating the request length.
(cherry picked from commit 8b7f4d3259)
I get this:
In function ‘TryCopyStr’,
inlined from ‘CopyISOLockArgs’ at ../xkb/xkbtext.c:875:9:
../xkb/xkbtext.c:720:13: warning: ‘tbuf’ may be used uninitialized [-Wmaybe-uninitialized]
720 | strcat(to, from);
| ^~~~~~~~~~~~~~~~
../xkb/xkbtext.c: In function ‘CopyISOLockArgs’:
<built-in>: note: by argument 1 of type ‘const char *’ to ‘__builtin_strlen’ declared here
../xkb/xkbtext.c:871:10: note: ‘tbuf’ declared here
871 | char tbuf[64];
| ^~~~
Just initialize tbuf so it definitely works.
xkb.c: In function ‘ProcXkbSetMap’:
xkb.c:2747:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
2747 | DeviceIntPtr master = GetMaster(dev, MASTER_KEYBOARD);
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Rework the function to use a single snprintf call instead of a mix of
strcpy/strcats. This now also appends a trailing slash where needed so we
don't rely on the build system to set this for us.
Also, since /tmp/ is the fallback and we never check if everything succeeded,
assert if we can't use /tmp/. This will never be triggered anyway, the only
caller to OutputDirectory() uses sizeof(PATH_MAX-sized array).
Follow-up from 6c51818a0f
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
The xkb spec defines that the 7th element of the DeviceValuator key
action description is "valuator 2 value".
This error most likely was accidentally introduced as a copy-paste error
in edeb033f29.
Signed-off-by: Povilas Kanapickas <povilas@radix.lt>
Calling strlen on char[4] that does not need to contain '\0' is wrong and X
server may end up running into uninitialized memory.
In addition GCC 8 is clever enough that it knows that strlen on char[4] can
return 0, 1, 2, 3 or cause undefined behavior. With this knowledge it can
optimize away the min(..., 4). In reality it can cause the memcpy to be called
with bigger size than 4 and overflow the destination buffer.
Fixes: 83913de25d (xkb: Silence some compiler warnings)
Closes: https://gitlab.freedesktop.org/xorg/xserver/-/issues/288
Signed-off-by: Matt Turner <mattst88@gmail.com>
Avoid out of bounds memory accesses on too short request.
ZDI-CAN 11572 / CVE-2020-14360
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Signed-off-by: Matthieu Herrb <matthieu@herrb.eu>
ZDI-CAN 11389 / CVE-2020-25712
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Signed-off-by: Matthieu Herrb <matthieu@herrb.eu>
Resolves warning from Oracle Parfait static analyser:
Error: Uninitialised memory
Uninitialised memory variable [uninitialised-mem-var] (CWE 457):
Possible access to uninitialised memory referenced by variable 'mask'
at line 721 of xkb/XKBMisc.c in function 'XkbUpdateKeyTypeVirtualMods'.
Path in callee avoiding write at line 720
mask allocated at line 718
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
CVE-2020-14361 ZDI-CAN 11573
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Signed-off-by: Matthieu Herrb <matthieu@herrb.eu>
CVE-2020-14345 / ZDI 11428
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Signed-off-by: Matthieu Herrb <matthieu@herrb.eu>
Most (but not all) of these were found by using
codespell --builtin clear,rare,usage,informal,code,names
but not everything reported by that was fixed.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
This code block was moved from a function that returns 0 for failure to a
function that returns 0 for Success in commit
649293f6b6. Change the return value to
BadValue to match the other checks in _XkbSetMapChecks.
Set nTypes to xkb->map->num_types when XkbKeyTypesMask is not set, to
allow requests with the XkbKeyTypesMask flag unset in stuff->present to
succeed.
Fixes a potential heap smash when client->swapped is true, because the
remainder of the request will not be swapped after "return 0", but
_XkbSetMap will be called anyway (because 0 is Success).
Signed-off-by: Peter Harris <pharris@opentext.com>
The server swaps part of the request in _XkbSetMapChecks instead of
SProcXkbSetMap (presumably because walking the XkbSetMap request is hard,
and we don't want to maintain another copy of that code).
Swap the first time _XkbSetMapChecks is called, not the second time.
Signed-off-by: Peter Harris <pharris@opentext.com>
I can't think of a good reason why this would need to be deferred to the
work queue. When we get to this point we're never in the middle of
request processing, so we can't corrupt the event/reply stream.
Signed-off-by: Adam Jackson <ajax@redhat.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Of the form:
../xkb/XKBGAlloc.c: In function ‘SrvXkbAddGeomKeyAlias’:
../xkb/XKBGAlloc.c:591:13: warning: ‘strncpy’ specified bound 4 equals destination size [-Wstringop-truncation]
strncpy(alias->real, realStr, XkbKeyNameLength);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This is intentional; the code that reads from these fields never reads
more than 4 bytes anyway. Rephrase things in terms of memcpy so that's
clear. Obviously this is awful but in XKB awful is par.
Signed-off-by: Adam Jackson <ajax@redhat.com>
Acked-by: Keith Packard <keithp@keithp.com>
This fixes some “Conditional jump depends on uninitialized value(s)”
errors spotted by valgrind.
Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
I don't know how many times I've had a broken server due to a bad
directory to xkbcomp, and only finding the whole path has shown me
where I went wrong.
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Roundhouse kick replacing the various (sizeof(foo)/sizeof(foo[0])) with
the ARRAY_SIZE macro from dix.h when possible. A semantic patch for
coccinelle has been used first. Additionally, a few macros have been
inlined as they had only one or two users.
Signed-off-by: Daniel Martin <consume.noise@gmail.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
XKB stores some stuff in the ClientRec that, style-wise, should probably
be in a client private. vMinor tracks the client's idea of the XKB
minor version, but is never read, we can just nuke it. vMajor is only
used for a bug-compat workaround for X11R6.0-vintage clients. We're
only using though (1<<4) for xkbClientFlags in the protocol, so we can
pack that field down to a u8 and store the bug-compat flag there.
Signed-off-by: Adam Jackson <ajax@redhat.com>
Reviewed-by: Julien Cristau <jcristau@debian.org>
Generating strings for XKB data used a single shared static buffer,
which offered several opportunities for errors. Use a ring of
resizable buffers instead, to avoid problems when strings end up
longer than anticipated.
Reviewed-by: Michal Srb <msrb@suse.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Julien Cristau <jcristau@debian.org>
XkbStringText escapes non-printable characters using octal numbers. Such escape
sequence would be at most 5 characters long ("\0123"), so it reserves 5 bytes
in the buffer. Due to char->unsigned int conversion, it would print much longer
string for negative numbers.
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Julien Cristau <jcristau@debian.org>
This is a work in progress that builds Xvfb, Xephyr, Xwayland, Xnest,
and Xdmx so far. The outline of Xquartz/Xwin support is in tree, but
hasn't been built yet. The unit tests are also not done.
The intent is to build this as a complete replacement for the
autotools system, then eventually replace autotools. meson is faster
to generate the build, faster to run the bulid, shorter to write the
build files in, and less error-prone than autotools.
v2: Fix indentation nits, move version declaration to project(), use
existing meson_options for version-config.h's vendor name/web.
Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
The build defines these, so having the defaults is just a way for the
build system's configuration to get out of sync with the code.
v2: Drop #ifndefs around the other two defines.
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Keith Packard <keithp@keithp.com>
Declared as unsigned short in xkb/xkbAccessX.c.
Signed-off-by: Matt Turner <mattst88@gmail.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
The X Server never generates a global config.h, and instead all these
paths are including dix-config.h or xorg-config.h.
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Eric Anholt <eric@anholt.net>
This touches everything that ends up in the Xorg binary; the big missing
part is GLX since that's all generated code. Cuts about 14k from the
binary on amd64.
Signed-off-by: Adam Jackson <ajax@redhat.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
RedirectKey() action had been broken by commit 2e6190.
A dropped check caused over-intense autorepeat of keysyms enriched
with the action.
Previous to this commit, the check wrapped the entire switch() block,
which was dropped with the move to a separate function.
Restore the checking.
Signed-off-by: Mihail Konev <k.mvc@ya.ru>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
There are two ways to separate multiple files in XKB include statements:
'+' will cause the later file to override the first in case of conflict,
while '|' will cause it augment it (this is done by xkbcomp). '!' is
unrelated here.
Currently, if someone tries to use '|' in a rule instead of '+', it
won't have any effect. Since '|' is practically never used, this wasn't
noticed.
Signed-off-by: Ran Benita <ran234@gmail.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Regression introduce by ac164e5887 which calls
XkbUpdateAllDeviceIndicators() with two NULL arguments. A few layers down into
the stack and we triggered a NULL-pointer dereference. In theory a NULL cause
is acceptable since we don't actually change modifier state here. Instead of
updating all places to check for NULL just set the cause to the client
request and go to the pub.
https://bugs.freedesktop.org/show_bug.cgi?id=96384
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Daniel Stone <daniels@collabora.com>
The xserver generates the key repeat by itself.
But when used with another server processing inputs first (e.g. a
Wayland compositor), the other server may be busy dealing with some
other things and not queue up key release events in time.
Add a vfunc in XkbSrvInfo to possibly add a check before re-emitting a
keypress event in the AccessX timer handler, so that the key repeat has
a chance to be denied if the server processing the input is not ready.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
This makes the code more consistent with other versions of
out-of-queue event processing
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
This removes all of the SIGIO handling support used for input
throughout the X server, preparing the way for using threads for input
handling instead.
Places calling OsBlockSIGIO and OsReleaseSIGIO are marked with calls
to stub functions input_lock/input_unlock so that we don't lose this
information.
xfree86 SIGIO support is reworked to use internal versions of
OsBlockSIGIO and OsReleaseSIGIO.
v2: Don't change locking order (Peter Hutterer)
v3: Comment weird && FALSE in xf86Helper.c
Leave errno save/restore in xf86ReadInput
Squash with stub adding patch (Peter Hutterer)
v4: Leave UseSIGIO config parameter so that
existing config files don't break (Peter Hutterer)
v5: Split a couple of independent patch bits out
of kinput.c (Peter Hutterer)
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
When NumLock is on and a new keymap is applied, the next modifier state
change will turn off that LED (but leave the state enabled). The cause
for this is a bit convoluted:
* the SLI explicitState is copied from the current state in
ProcXkbGetKbdByName. Thus, if NumLock is on, that state is 0x2.
* on the next modifier key press (e.g. Shift), XkbApplyState() calls into
XkbUpdateIndicators() -> XkbUpdateLedAutoState() to update SLIs (if any)
for the currently changed modifier. But it does so with a mask only for
the changed modifier (i.e. for Shift).
* XkbUpdateLedAutoState() calculates the state based on this mask and
ends up with 0 because we don't have a Shift LED and we masked out the
others.
* XkbUpdateLedAutoState() compares that state with the previous state
(which is still 0x2) and then proceeds to turn the LED off
This doesn't happen in the normal case because either the mask
encompasses all modifiers or the state matches of the masked-out
modifiers matches the old state.
Avoid this issue by forcing an SLI update after changing the keymap.
This updates the sli->effectiveState and thus restores everything to
happy working order.
https://bugzilla.redhat.com/show_bug.cgi?id=1047151
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Daniel Stone <daniels@collabora.com>
-Wlogical-op now tells us:
devices.c:1685:23: warning: logical ‘and’ of equal expressions
Reviewed-by: Julien Cristau <jcristau@debian.org>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Wrong use of the mask here caused a beep whenever a key was rejected but
also when it was released after being accepted. Fix the mask to check
for the correct enabled controls.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Peter Hutterer
Samuel Thibault
Ray Strode
Adam Jackson
Povilas Kanapickas
Michal Srb
Matthieu Herrb
Alan Coopersmith
Ignacio Casal Quinteiro
Peter Harris
Dave Airlie
Samir Benmendil
Giuseppe Bilotta
Eric Anholt
Daniel Martin
Keith Packard
Matt Turner
Mihail Konev
Ran Benita
Olivier Fourdan