Bug#976216: xorg-server: CVE-2020-25712 CVE-2020-14360
On Wed, Dec 02, 2020 at 11:49:24AM +0100, Julien Cristau wrote:
> Hi,
>
> On Tue, Dec 01, 2020 at 05:58:56PM +0100, Salvatore Bonaccorso wrote:
> > The following vulnerabilities were published for xorg-server.
> >
> > CVE-2020-25712[0]:
> > | Fix XkbSetDeviceInfo() and SetDeviceIndicators() heap overflows
> >
> > CVE-2020-14360[1]:
> > | Check SetMap request length carefully
> >
> > If you fix the vulnerabilities please also make sure to include the
> > CVE (Common Vulnerabilities & Exposures) ids in your changelog entry.
> >
> I'm assuming we'll want this in buster-security?
>
> Proposed patch below, it's a clean cherry-pick from server-1.20-branch;
> Timo's working on updating sid to 1.20.10. This being xkb I'd prefer to
> wait a couple of days to watch out for any regressions before shipping
> the update in stable if possible, but we can get started with an upload
> and builds right away.
Hi,
I've uploaded the attached debdiff yesterday night, but had gotten too late to
loop you in, wanted to do that today :-)
I'll compare them with yours tonight, but I'd expect them to be identical given
how close buster is to upstream.
The update is running fine on my Buster workstation so far (and Salvatore did some
tests as well), but I had also been planned to hold it back until at least
tomorrow to check for regressions.
Ubuntu pushed an update yesterday and their LTS is also using 1.20, so that'll
be useful to keep an eye on for regressions as well.
Cheers,
Moritz
diff -u xorg-server-1.20.4/debian/changelog xorg-server-1.20.4/debian/changelog
--- xorg-server-1.20.4/debian/changelog
+++ xorg-server-1.20.4/debian/changelog
@@ -1,3 +1,9 @@
+xorg-server (2:1.20.4-1+deb10u2) buster-security; urgency=medium
+
+ * CVE-2020-14360 CVE-2020-25712
+
+ -- Moritz Mühlenhoff <jmm@debian.org> Tue, 01 Dec 2020 18:59:57 +0100
+
xorg-server (2:1.20.4-1+deb10u1) buster-security; urgency=high
* Non-maintainer upload by the Security Team.
only in patch2:
unchanged:
--- xorg-server-1.20.4.orig/xkb/xkb.c
+++ xorg-server-1.20.4/xkb/xkb.c
@@ -2366,6 +2366,93 @@
return (char *) wire;
}
+#define _add_check_len(new) \
+ if (len > UINT32_MAX - (new) || len > req_len - (new)) goto bad; \
+ else len += new
+
+/**
+ * Check the length of the SetMap request
+ */
+static int
+_XkbSetMapCheckLength(xkbSetMapReq *req)
+{
+ size_t len = sz_xkbSetMapReq, req_len = req->length << 2;
+ xkbKeyTypeWireDesc *keytype;
+ xkbSymMapWireDesc *symmap;
+ BOOL preserve;
+ int i, map_count, nSyms;
+
+ if (req_len < len)
+ goto bad;
+ /* types */
+ if (req->present & XkbKeyTypesMask) {
+ keytype = (xkbKeyTypeWireDesc *)(req + 1);
+ for (i = 0; i < req->nTypes; i++) {
+ _add_check_len(XkbPaddedSize(sz_xkbKeyTypeWireDesc));
+ if (req->flags & XkbSetMapResizeTypes) {
+ _add_check_len(keytype->nMapEntries
+ * sz_xkbKTSetMapEntryWireDesc);
+ preserve = keytype->preserve;
+ map_count = keytype->nMapEntries;
+ if (preserve) {
+ _add_check_len(map_count * sz_xkbModsWireDesc);
+ }
+ keytype += 1;
+ keytype = (xkbKeyTypeWireDesc *)
+ ((xkbKTSetMapEntryWireDesc *)keytype + map_count);
+ if (preserve)
+ keytype = (xkbKeyTypeWireDesc *)
+ ((xkbModsWireDesc *)keytype + map_count);
+ }
+ }
+ }
+ /* syms */
+ if (req->present & XkbKeySymsMask) {
+ symmap = (xkbSymMapWireDesc *)((char *)req + len);
+ for (i = 0; i < req->nKeySyms; i++) {
+ _add_check_len(sz_xkbSymMapWireDesc);
+ nSyms = symmap->nSyms;
+ _add_check_len(nSyms*sizeof(CARD32));
+ symmap += 1;
+ symmap = (xkbSymMapWireDesc *)((CARD32 *)symmap + nSyms);
+ }
+ }
+ /* actions */
+ if (req->present & XkbKeyActionsMask) {
+ _add_check_len(req->totalActs * sz_xkbActionWireDesc
+ + XkbPaddedSize(req->nKeyActs));
+ }
+ /* behaviours */
+ if (req->present & XkbKeyBehaviorsMask) {
+ _add_check_len(req->totalKeyBehaviors * sz_xkbBehaviorWireDesc);
+ }
+ /* vmods */
+ if (req->present & XkbVirtualModsMask) {
+ _add_check_len(XkbPaddedSize(Ones(req->virtualMods)));
+ }
+ /* explicit */
+ if (req->present & XkbExplicitComponentsMask) {
+ /* two bytes per non-zero explicit componen */
+ _add_check_len(XkbPaddedSize(req->totalKeyExplicit * sizeof(CARD16)));
+ }
+ /* modmap */
+ if (req->present & XkbModifierMapMask) {
+ /* two bytes per non-zero modmap component */
+ _add_check_len(XkbPaddedSize(req->totalModMapKeys * sizeof(CARD16)));
+ }
+ /* vmodmap */
+ if (req->present & XkbVirtualModMapMask) {
+ _add_check_len(req->totalVModMapKeys * sz_xkbVModMapWireDesc);
+ }
+ if (len == req_len)
+ return Success;
+bad:
+ ErrorF("[xkb] BOGUS LENGTH in SetMap: expected %ld got %ld\n",
+ len, req_len);
+ return BadLength;
+}
+
+
/**
* Check if the given request can be applied to the given device but don't
* actually do anything..
@@ -2617,6 +2704,11 @@
CHK_KBD_DEVICE(dev, stuff->deviceSpec, client, DixManageAccess);
CHK_MASK_LEGAL(0x01, stuff->present, XkbAllMapComponentsMask);
+ /* first verify the request length carefully */
+ rc = _XkbSetMapCheckLength(stuff);
+ if (rc != Success)
+ return rc;
+
tmp = (char *) &stuff[1];
/* Check if we can to the SetMap on the requested device. If this
@@ -6476,7 +6568,9 @@
unsigned changed,
int num,
int *status_rtrn,
- ClientPtr client, xkbExtensionDeviceNotify * ev)
+ ClientPtr client,
+ xkbExtensionDeviceNotify * ev,
+ xkbSetDeviceInfoReq * stuff)
{
xkbDeviceLedsWireDesc *ledWire;
int i;
@@ -6497,6 +6591,11 @@
xkbIndicatorMapWireDesc *mapWire;
XkbSrvLedInfoPtr sli;
+ if (!_XkbCheckRequestBounds(client, stuff, ledWire, ledWire + 1)) {
+ *status_rtrn = BadLength;
+ return (char *) ledWire;
+ }
+
namec = mapc = statec = 0;
sli = XkbFindSrvLedInfo(dev, ledWire->ledClass, ledWire->ledID,
XkbXI_IndicatorMapsMask);
@@ -6515,6 +6614,10 @@
memset((char *) sli->names, 0, XkbNumIndicators * sizeof(Atom));
for (n = 0, bit = 1; n < XkbNumIndicators; n++, bit <<= 1) {
if (ledWire->namesPresent & bit) {
+ if (!_XkbCheckRequestBounds(client, stuff, atomWire, atomWire + 1)) {
+ *status_rtrn = BadLength;
+ return (char *) atomWire;
+ }
sli->names[n] = (Atom) *atomWire;
if (sli->names[n] == None)
ledWire->namesPresent &= ~bit;
@@ -6532,6 +6635,10 @@
if (ledWire->mapsPresent) {
for (n = 0, bit = 1; n < XkbNumIndicators; n++, bit <<= 1) {
if (ledWire->mapsPresent & bit) {
+ if (!_XkbCheckRequestBounds(client, stuff, mapWire, mapWire + 1)) {
+ *status_rtrn = BadLength;
+ return (char *) mapWire;
+ }
sli->maps[n].flags = mapWire->flags;
sli->maps[n].which_groups = mapWire->whichGroups;
sli->maps[n].groups = mapWire->groups;
@@ -6611,7 +6718,7 @@
ed.deviceID = dev->id;
wire = (char *) &stuff[1];
if (stuff->change & XkbXI_ButtonActionsMask) {
- int nBtns, sz, i;
+ int nBtns, sz, i;
XkbAction *acts;
DeviceIntPtr kbd;
@@ -6623,7 +6730,11 @@
return BadAlloc;
dev->button->xkb_acts = acts;
}
+ if (stuff->firstBtn + stuff->nBtns > nBtns)
+ return BadValue;
sz = stuff->nBtns * SIZEOF(xkbActionWireDesc);
+ if (!_XkbCheckRequestBounds(client, stuff, wire, (char *) wire + sz))
+ return BadLength;
memcpy((char *) &acts[stuff->firstBtn], (char *) wire, sz);
wire += sz;
ed.reason |= XkbXI_ButtonActionsMask;
@@ -6644,7 +6755,8 @@
int status = Success;
wire = SetDeviceIndicators(wire, dev, stuff->change,
- stuff->nDeviceLedFBs, &status, client, &ed);
+ stuff->nDeviceLedFBs, &status, client, &ed,
+ stuff);
if (status != Success)
return status;
}
Reply to: