The XKM_OUTPUT_DIR folder by default is defined as ${datadir}/X11/xkb/compiled
and it is usually defined as /var/lib/xkb or %{_localstatedir}/lib/xkb by
distributions. If X is executed as non-root it won't have permissions to write
into that folder. If we fallback directly to /tmp we might get name collisions:
```
> Error: Cannot open "/tmp/server-10.xkm" to write keyboard description
> Exiting
```
Where the file /tmp/server-10.xkm already exists but is owned by another user
that previously executed X and had the display number 10. This is specially
problematic when exeuting Xvfb.
Before falling back to /tmp/ check first the XDG_RUNTIME_DIR.
This is to make sure the hardware gets the device states regardless
whether the internal state has changed or not, to overcome situations
that device LEDs are out of sync e.g. switching between VTs.
Signed-off-by: Yao Wei (魏銘廷) <yao.wei@canonical.com>
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>
_XkbCheckRequestBounds assumes that from..to is at least one byte.
However, request strings can be empty, causing spurious failures in
XkbGetKbdByName calls. To avoid this, before checking bounds make
sure that the length is nonzero.
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>
Each string length field was accessed before checking whether that byte
was actually part of the client request. No real harm here since it
would immediately fail with BadLength anyway, but let's be correct here.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
This request accessed &stuff[1] before length-checking everything. The
check was performed afterwards so invalid requests would return
BadLength anyway, but let's do this before we actually access the
memory.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
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>
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>
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>
Sick of fighting vim and git from trying to add this fix with every
commit iteration...
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
This header merely defines the various protocol request handlers, so
let's rename it to something less generic and remove its include from
all the files that don't actually need it (which is almost all of them).
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
Let's move this to where all the other protocol handlers are.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
As can be seen in diff, nOut is always 0 here. The code was likely
copy-pasted from comparisons further below.
Fixes LGTM warning "Comparison is always false because nOut <= 0."
Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru>
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>
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>
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.
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>
Peter Hutterer
Alessandro Bono
Benno Schulenberg
Yao Wei
FeepingCreature
Konstantin Kharlamov
Samuel Thibault
Povilas Kanapickas
Ray Strode
Adam Jackson
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