Hi all,
The following AI generated code analysis reports were sent to me for dwm, dmenu
and st.
I've already dug through some of the findings and fixed what I think are the
most important issues.
I'm sending these findings so people can see it, evaluate it and give their
patches or what needs addressing.
Before the thread derails: if you want to have a discussion about AI for code
analysis, its usefulness or uselessness please make a separate thread about it.
Again: this is only about the findings which were sent to me.
A few notes:
- st recursion / deadlock in ttywriteraw() and ttyread().
This has been reported before, but I cannot reproduce the issue or write
an elegant patch solution. I consider it a potential issue still.
- Findings in dwm and dmenu about monitor/display setups: I would like more
people
especially with more exotic setups to look at it.
Multi-monitor setups, different resolutions/rotations and hotplugging weird
configurations could be improved?
- dwm showhide recursion with many clients: a patch for this is welcome. But
it should not break any existing behaviour. If we can avoid recursion that
would be nice, even though I think spawning "100k tiny clients" is
unrealistic.
- I'm not sure the notes about XSendEvent or spoofing events is a security
issues. And it could break useful tools that simulate keypresses or events
for
example (like xdotool?).
- I don't think we should handle all possible (input) errors, because we assume
the user uses his/her brain.
Thank you,
Report below:
================================================================================
dwm - SECURITY REVIEW FINDINGS
================================================================================
Date reviewed : 2026-09-11
Repository : C:\dwm (branch master, HEAD 44dbc68)
Scope : dwm.c, drw.c, drw.h, util.c, util.h, config.def.h, transient.c
Reviewer : static source review (no build, no runtime fuzzing)
Code changed : NONE. Every diff below is ILLUSTRATIVE ONLY and has not been
applied. Line numbers refer to HEAD 44dbc68; hunk offsets are
approximate and may need `patch -l` / manual application.
--------------------------------------------------------------------------------
THREAT MODEL USED
--------------------------------------------------------------------------------
dwm is the X11 window manager: it is the most privileged client on the display
and its death ends the user's session. Every other X client connected to the
same display is UNTRUSTED for the purposes of this review, because in X11 any
client can:
* set arbitrary properties on its own windows (WM_NAME, _NET_WM_NAME,
WM_CLASS, WM_NORMAL_HINTS, WM_HINTS, _NET_WM_STATE, ...), and
* call XSendEvent() to deliver a SYNTHETIC event to any window, including the
root window and dwm's own bar window, as long as dwm has selected the
corresponding event mask there.
dwm selects this mask on the root window (dwm.c:1607-1611):
SubstructureRedirectMask|SubstructureNotifyMask|ButtonPressMask
|PointerMotionMask|EnterWindowMask|LeaveWindowMask|StructureNotifyMask
|PropertyChangeMask
and this mask on each bar window (dwm.c:1825):
ButtonPressMask|ExposureMask
So forged ConfigureNotify / EnterNotify / LeaveNotify / MotionNotify /
PropertyNotify / ButtonPress / Expose events ARE delivered to dwm. Of the 14
installed handlers, only unmapnotify() (dwm.c:1811) inspects ev->send_event.
That asymmetry is the root cause of findings SEC-01, SEC-02 and SEC-03.
Forged KeyPress is NOT currently reachable: dwm never selects KeyPressMask on
the root or bar windows (it relies on XGrabKey), so a synthetic KeyPress has no
recipient. See SEC-11 note.
Severity is availability/integrity-weighted for a window manager: "crash the
WM" == "kill the user's whole graphical session and every unsaved document in
it", so it is rated High, not Low.
--------------------------------------------------------------------------------
SUMMARY TABLE
--------------------------------------------------------------------------------
ID Severity Component Issue
-------- --------- ---------------------------- ------------------------------
SEC-01 High dwm.c configurenotify() Forged root ConfigureNotify ->
fatal X BadValue, dwm exits
SEC-02 High dwm.c buttonpress() Forged ButtonPress on bar ->
invokes button actions,
including spawn()
SEC-03 Medium dwm.c enternotify() Forged EnterNotify -> focus
theft / keystroke capture
SEC-04 Medium dwm.c updatesizehints() / Unsanitized WM_NORMAL_HINTS ->
applysizehints() signed overflow, div-by-zero,
inf->int conversion
SEC-05 Medium dwm.c arrangemon(), strncpy() with sizeof(dest)
createmon(), setlayout() leaves ltsymbol[16]
unterminated -> OOB read
SEC-06 Low/Med drw.c drw_text() Hostile window title -> per
redraw XftFontMatch storm and
unbounded font cache
SEC-07 Low dwm.c showhide() Unbounded recursion, one frame
per window -> stack exhaustion
SEC-08 Low dwm.c gettextprop() Leak of XmbTextPropertyToText
List() allocation
SEC-09 Low dwm.c updatenumlockmask() Unchecked XGetModifierMapping()
-> NULL deref
SEC-10 Low dwm.c updategeom(), NULL deref when the monitor
cleanupmon() list empties / desyncs
SEC-11 Low dwm.c clientmessage() No format check, no origin
restriction on _NET_WM_STATE
SEC-12 Low dwm.c spawn() Unchecked fork(), inherited fds
leak to children, dmenumon
one-byte mis-encode
SEC-13 Info dwm.c applyrules() Placement rules keyed on
client-spoofable WM_CLASS/title
================================================================================
SEC-01 Forged root ConfigureNotify kills dwm (and corrupts screen geometry)
================================================================================
Severity : High (availability - full session loss)
Location : dwm.c:552-578 configurenotify()
CWE : CWE-346 Origin Validation Error; CWE-20 Improper Input Validation
Description
-----------
configurenotify() acts on any ConfigureNotify whose ev->window == root without
checking ev->send_event:
dwm.c:561 if (ev->window == root) {
dwm.c:562 dirty = (sw != ev->width || sh != ev->height);
dwm.c:563 sw = ev->width;
dwm.c:564 sh = ev->height;
dwm.c:565 if (updategeom() || dirty) {
dwm.c:566 drw_resize(drw, sw, bh);
dwm.c:567 updatebars();
...
dwm.c:572 XMoveResizeWindow(dpy, m->barwin, m->wx, m->by,
m->ww, bh);
dwm has StructureNotifyMask selected on the root window (dwm.c:1609), so a
synthetic ConfigureNotify sent by ANY client is delivered here and is
indistinguishable from a real server-generated one.
The attacker fully controls sw and sh. With width == 0:
sw = 0
-> drw_resize(drw, 0, bh) -> XCreatePixmap(..., 0, bh, depth) (drw.c:76)
-> X_CreatePixmap / BadValue
and, in the non-Xinerama path, updategeom() sets mons->mw = mons->ww = 0
-> XMoveResizeWindow(dpy, m->barwin, ..., 0, bh)
-> X_ConfigureWindow / BadValue
BadValue is not in the ignore list in xerror() (dwm.c:2101-2110), so the next
XSync()/XNextEvent() dispatches it to:
dwm.c:2111 fprintf(stderr, "dwm: fatal error: ...");
dwm.c:2113 return xerrorxlib(dpy, ee); /* Xlib default handler: exit() */
dwm terminates. All managed clients are left unmanaged and the session is over.
Non-zero-but-bogus geometries are also damaging without crashing: sw/sh feed
the `interact` branch of applysizehints() (dwm.c:322-330) and, without
Xinerama, the entire monitor rectangle (dwm.c:1931-1936), so a client can
relocate/resize the bar and push windows off-screen.
Failure scenario
----------------
1. Any unprivileged client on the same display (a browser tab helper, a
sandboxed app with $DISPLAY, a compromised tray icon) runs:
XConfigureEvent ce = {0};
ce.type = ConfigureNotify;
ce.display = d; ce.event = root; ce.window = root;
ce.width = 0; ce.height = 0;
XSendEvent(d, root, False, StructureNotifyMask, (XEvent *)&ce);
XFlush(d);
2. dwm prints "dwm: fatal error: request code=53, error code=2" and exits.
Illustrative fix
----------------
Mirror what unmapnotify() already does, and validate the geometry:
--- a/dwm.c
+++ b/dwm.c
@@ -558,6 +558,13 @@ configurenotify(XEvent *e)
int dirty;
/* TODO: updategeom handling sucks, needs to be simplified */
if (ev->window == root) {
+ /* Any client may XSendEvent() a ConfigureNotify for the root
+ * window (we select StructureNotifyMask there). Only the server
+ * may tell us the screen changed size, and a bogus geometry
+ * makes XCreatePixmap()/XMoveResizeWindow() fail with BadValue,
+ * which xerror() treats as fatal. */
+ if (ev->send_event || ev->width <= 0 || ev->height <= 0)
+ return;
dirty = (sw != ev->width || sh != ev->height);
sw = ev->width;
sh = ev->height;
Defence in depth: also add BadValue-on-CreatePixmap tolerance to xerror(), and
clamp in drw_resize():
--- a/drw.c
+++ b/drw.c
@@ -66,10 +66,13 @@ void
drw_resize(Drw *drw, unsigned int w, unsigned int h)
{
if (!drw)
return;
+ /* XCreatePixmap rejects a zero extent with BadValue */
+ w = MAX(w, 1);
+ h = MAX(h, 1);
drw->w = w;
drw->h = h;
if (drw->drawable)
XFreePixmap(drw->dpy, drw->drawable);
(drw.c already includes util.h, so MAX is available.)
================================================================================
SEC-02 Forged ButtonPress on the bar invokes configured actions, incl. spawn()
================================================================================
Severity : High (integrity - unauthenticated triggering of WM commands)
Location : dwm.c:417-457 buttonpress()
CWE : CWE-346 Origin Validation Error
Description
-----------
Each bar window is created with ButtonPressMask (dwm.c:1825), and buttonpress()
never checks ev->send_event. The handler derives a click region purely from
attacker-supplied ev->window / ev->x / ev->state / ev->button and then calls
the matching entry of buttons[]:
dwm.c:433 if (ev->window == selmon->barwin) {
dwm.c:437 while (ev->x >= x && ++i < LENGTH(tags));
dwm.c:443 else if (ev->x > selmon->ww - (int)TEXTW(stext) + lrpad - 2)
dwm.c:444 click = ClkStatusText;
...
dwm.c:453 for (i = 0; i < LENGTH(buttons); i++)
dwm.c:454 if (click == buttons[i].click && buttons[i].func && ...
dwm.c:456 buttons[i].func(...);
With the shipped config.def.h this already reaches spawn():
config.def.h:108 { ClkStatusText, 0, Button2, spawn, {.v = termcmd } },
so any client can make dwm fork+exec `st`. It can also drive view(),
toggleview(), tag(), toggletag(), setlayout() and zoom() (config.def.h:105-115)
to reorganise the user's workspace. The severity rises sharply for the very
common patch/customisation of binding SHCMD("...") (config.def.h:57) to a bar
button - then a forged ButtonPress is arbitrary command execution as the user.
Note that repeated forged presses of ClkStatusText/Button2 are also a fork bomb
vector: one shell command executed per event, with no rate limit.
Failure scenario
----------------
1. Client learns selmon->barwin (it is a child of root; XQueryTree lists it, or
read _NET_CLIENT_LIST neighbours; the bar has WM_CLASS "dwm"/"dwm",
dwm.c:1827).
2. Client sends:
XButtonEvent be = {0};
be.type = ButtonPress; be.window = barwin; be.root = root;
be.x = bar_width - 5; /* status-text region */
be.button = Button2; be.state = 0; be.same_screen = True;
XSendEvent(d, barwin, False, ButtonPressMask, (XEvent *)&be);
3. dwm spawns termcmd (or whatever is bound), repeatedly, on demand.
Illustrative fix
----------------
--- a/dwm.c
+++ b/dwm.c
@@ -423,6 +423,10 @@ buttonpress(XEvent *e)
Monitor *m;
XButtonPressedEvent *ev = &e->xbutton;
+ /* never act on a forged press: buttons[] can spawn() processes */
+ if (ev->send_event)
+ return;
+
click = ClkRootWin;
/* focus monitor if necessary */
if ((m = wintomon(ev->window)) && m != selmon) {
================================================================================
SEC-03 Forged EnterNotify lets any client steal input focus
================================================================================
Severity : Medium (confidentiality - keystroke capture)
Location : dwm.c:759-776 enternotify()
CWE : CWE-346 Origin Validation Error; CWE-1021 Improper Restriction of
Rendered UI Layers (UI redress / input redirection)
Description
-----------
dwm selects EnterWindowMask on the root window (dwm.c:1608) and on every
managed client (dwm.c:1071). enternotify() filters on ev->mode/ev->detail but
not on ev->send_event:
dwm.c:766 if ((ev->mode != NotifyNormal || ev->detail == NotifyInferior)
&& ev->window != root)
dwm.c:767 return;
dwm.c:768 c = wintoclient(ev->window);
...
dwm.c:775 focus(c); /* -> setfocus() -> XSetInputFocus() */
A synthetic EnterNotify with mode == NotifyNormal, detail == NotifyAncestor and
window == the attacker's own managed window therefore causes dwm to move the
keyboard focus to the attacker, with the pointer never having moved. Every
subsequent keystroke - including one typed into what the user still believes is
their terminal, password prompt, or ssh session - is delivered to the attacker's
window.
Because dwm's focus-follows-mouse model means focus changes are routine and
produce only a border-colour change, this is visually very cheap to hide: the
attacker can steal focus, buffer keystrokes, and send a second forged
EnterNotify to hand focus back.
Failure scenario
----------------
1. User is typing a passphrase into a terminal.
2. Attacker client (already mapped, e.g. a 1x1 window on the same tag) sends a
synthetic EnterNotify naming its own window.
3. dwm calls XSetInputFocus() on the attacker window; the rest of the
passphrase is typed into the attacker.
Illustrative fix
----------------
--- a/dwm.c
+++ b/dwm.c
@@ -763,7 +763,10 @@ enternotify(XEvent *e)
Monitor *m;
XCrossingEvent *ev = &e->xcrossing;
- if ((ev->mode != NotifyNormal || ev->detail == NotifyInferior) &&
ev->window != root)
+ /* a forged crossing event would hand keyboard focus to the sender */
+ if (ev->send_event)
+ return;
+ if ((ev->mode != NotifyNormal || ev->detail == NotifyInferior) &&
ev->window != root)
return;
c = wintoclient(ev->window);
m = c ? c->mon : wintomon(ev->window);
The same one-line guard is worth adding to motionnotify() (dwm.c:1129, forged
motion silently switches selmon) and expose() (dwm.c:779, forged exposures are
a cheap redraw-loop CPU sink, and each redraw runs the drw_text() path of
SEC-06).
================================================================================
SEC-04 Unsanitized WM_NORMAL_HINTS -> integer overflow, div-by-zero, inf->int
================================================================================
Severity : Medium (availability; undefined behaviour)
Location : dwm.c:1961-2003 updatesizehints(); dwm.c:313-379 applysizehints()
CWE : CWE-190 Integer Overflow; CWE-369 Divide By Zero; CWE-20
Description
-----------
updatesizehints() copies every field of the client-controlled XSizeHints
straight into the Client struct with no range check:
dwm.c:1971 c->basew = size.base_width;
dwm.c:1979 c->incw = size.width_inc;
dwm.c:1984 c->maxw = size.max_width;
dwm.c:1997 c->mina = (float)size.min_aspect.y / size.min_aspect.x;
dwm.c:1998 c->maxa = (float)size.max_aspect.x / size.max_aspect.y;
These are plain `int`s written by the client via XSetWMNormalHints(), so they
may be negative, zero, or INT_MAX. applysizehints() then does signed arithmetic
on them:
(a) Signed overflow - dwm.c:351-352 and dwm.c:362-363:
*w -= c->basew; *h -= c->baseh;
With c->basew == INT_MIN + k this overflows (UB; in practice wraps).
(b) Division by zero / inf->int - dwm.c:355-359:
if (c->mina > 0 && c->maxa > 0) {
if (c->maxa < (float)*w / *h)
*w = *h * c->maxa + 0.5;
After (a), *h can be 0 (the earlier `*h = MAX(1, *h)` at dwm.c:321 and
`if (*h < bh) *h = bh;` at dwm.c:341 both happen BEFORE the subtraction),
so `(float)*w / *h` is a division by zero. Separately, min_aspect.x == 0
makes c->mina == +inf at dwm.c:1997, and `*w = *h * c->maxa + 0.5`
converts +inf/NaN to int - undefined behaviour, in practice INT_MIN.
(c) SIGFPE - dwm.c:366-369:
if (c->incw) *w -= *w % c->incw;
c->incw is only tested for != 0. With c->incw == -1 and *w == INT_MIN
(reachable via the wraparound in (a): pick base_width == INT_MIN + w0),
`INT_MIN % -1` traps on x86-64 (integer-divide #DE -> SIGFPE) and dwm
dies immediately with no X error involved.
Every one of these is reached on the normal path: manage() -> updatesizehints()
(dwm.c:1069) for any newly mapped window, and again on every resize through
resize() -> applysizehints() (dwm.c:345-347).
Failure scenario
----------------
A client maps a window after:
XSizeHints h = {0};
h.flags = PResizeInc | PBaseSize | PMinSize;
h.width_inc = -1; h.height_inc = -1;
h.base_width = INT_MIN + 500; /* window is 500px wide */
h.base_height = 0;
XSetWMNormalHints(d, win, &h);
dwm computes *w == INT_MIN, evaluates INT_MIN % -1, and takes SIGFPE. Session
over. A variant with min_aspect.x = 0 instead produces an inf->int conversion
and absurd window geometry rather than a crash.
Illustrative fix
----------------
Clamp at the trust boundary (where the hints are read), and re-establish the
>= 1 invariant before the aspect/increment maths:
--- a/dwm.c
+++ b/dwm.c
@@ -1961,43 +1961,49 @@ void
updatesizehints(Client *c)
{
long msize;
XSizeHints size;
if (!XGetWMNormalHints(dpy, c->win, &size, &msize))
/* size is uninitialized, ensure that size.flags aren't used */
size.flags = PSize;
+ /* WM_NORMAL_HINTS is entirely client controlled: clamp every field to
+ * a sane, non-negative range so that the signed arithmetic in
+ * applysizehints() cannot overflow, divide by zero, or trap. */
if (size.flags & PBaseSize) {
- c->basew = size.base_width;
- c->baseh = size.base_height;
+ c->basew = MAX(0, MIN(size.base_width, sw));
+ c->baseh = MAX(0, MIN(size.base_height, sh));
} else if (size.flags & PMinSize) {
- c->basew = size.min_width;
- c->baseh = size.min_height;
+ c->basew = MAX(0, MIN(size.min_width, sw));
+ c->baseh = MAX(0, MIN(size.min_height, sh));
} else
c->basew = c->baseh = 0;
if (size.flags & PResizeInc) {
- c->incw = size.width_inc;
- c->inch = size.height_inc;
+ c->incw = MAX(0, MIN(size.width_inc, sw));
+ c->inch = MAX(0, MIN(size.height_inc, sh));
} else
c->incw = c->inch = 0;
if (size.flags & PMaxSize) {
- c->maxw = size.max_width;
- c->maxh = size.max_height;
+ c->maxw = MAX(0, size.max_width);
+ c->maxh = MAX(0, size.max_height);
} else
c->maxw = c->maxh = 0;
if (size.flags & PMinSize) {
- c->minw = size.min_width;
- c->minh = size.min_height;
+ c->minw = MAX(0, MIN(size.min_width, sw));
+ c->minh = MAX(0, MIN(size.min_height, sh));
} else if (size.flags & PBaseSize) {
- c->minw = size.base_width;
- c->minh = size.base_height;
+ c->minw = MAX(0, MIN(size.base_width, sw));
+ c->minh = MAX(0, MIN(size.base_height, sh));
} else
c->minw = c->minh = 0;
- if (size.flags & PAspect) {
+ if ((size.flags & PAspect)
+ && size.min_aspect.x > 0 && size.max_aspect.y > 0) {
c->mina = (float)size.min_aspect.y / size.min_aspect.x;
c->maxa = (float)size.max_aspect.x / size.max_aspect.y;
} else
c->maxa = c->mina = 0.0;
--- a/dwm.c
+++ b/dwm.c
@@ -352,6 +352,9 @@ applysizehints(Client *c, int *x, int *y, int *w, int *h,
int interact)
if (!baseismin) { /* temporarily remove base dimensions */
*w -= c->basew;
*h -= c->baseh;
}
+ /* the subtraction above may have driven these to <= 0 */
+ *w = MAX(1, *w);
+ *h = MAX(1, *h);
/* adjust for aspect limits */
if (c->mina > 0 && c->maxa > 0) {
Behaviour note: clamping base/min/inc to the screen dimensions is a deliberate
policy choice - a legitimate client asking for a base size larger than the
screen will now be treated as asking for the screen size.
================================================================================
SEC-05 strncpy(dst, src, sizeof dst) leaves Monitor.ltsymbol unterminated
================================================================================
Severity : Medium (out-of-bounds read; gated on config.h content)
Location : dwm.c:398 arrangemon(); dwm.c:645 createmon(); dwm.c:1517
setlayout()
CWE : CWE-170 Improper Null Termination; CWE-125 Out-of-bounds Read
Description
-----------
Monitor.ltsymbol is `char ltsymbol[16]` (dwm.c:114). All three copies into it
pass the FULL size of the destination to strncpy():
dwm.c:398 strncpy(m->ltsymbol, m->lt[m->sellt]->symbol, sizeof
m->ltsymbol);
dwm.c:645 strncpy(m->ltsymbol, layouts[0].symbol, sizeof m->ltsymbol);
dwm.c:1517 strncpy(selmon->ltsymbol, selmon->lt[selmon->sellt]->symbol,
sizeof selmon->ltsymbol);
strncpy() does not NUL-terminate when strlen(src) >= n. A layout symbol of 16
or more characters therefore leaves ltsymbol without a terminator, and the
subsequent consumers all treat it as a C string:
dwm.c:732 w = TEXTW(m->ltsymbol); -> drw_fontset_getwidth ->
strlen-like walk
dwm.c:734 drw_text(drw, x, 0, w, bh, lrpad / 2, m->ltsymbol, 0);
dwm.c:441 else if (ev->x < x + TEXTW(selmon->ltsymbol))
drw_text() then walks past the 16-byte field, reading the adjacent struct
members (mfact, nmaster, num, by, mx...) and, once it runs off the end of the
heap allocation, unmapped memory -> SIGSEGV. Before crashing it RENDERS those
bytes into the bar, i.e. it discloses adjacent heap contents on screen.
Contrast dwm.c:1123, which gets this right: monocle() uses snprintf().
Reachability: layouts[] comes from config.h, so this is not remotely
attacker-controlled in a stock build. It is, however, a live foot-gun for the
patch-and-recompile workflow dwm is designed around - many popular layout
patches ship multi-byte/decorated symbols (e.g. "[]=" replaced with a padded
Unicode glyph string) that exceed 16 bytes once encoded in UTF-8.
Failure scenario
----------------
config.h contains `{ "||||||||||||||||", tile }` (16 chars). On the first
arrange(), drawbar() calls TEXTW(m->ltsymbol) which reads past ltsymbol[15]
into the Monitor struct and paints the raw bytes of mfact/nmaster into the
status bar; with a Monitor allocated at the end of a heap page it faults.
Illustrative fix
----------------
--- a/dwm.c
+++ b/dwm.c
@@ -395,7 +395,8 @@ void
arrangemon(Monitor *m)
{
- strncpy(m->ltsymbol, m->lt[m->sellt]->symbol, sizeof m->ltsymbol);
+ strncpy(m->ltsymbol, m->lt[m->sellt]->symbol, sizeof m->ltsymbol - 1);
+ m->ltsymbol[sizeof m->ltsymbol - 1] = '\0';
if (m->lt[m->sellt]->arrange)
m->lt[m->sellt]->arrange(m);
}
@@ -643,5 +644,6 @@ createmon(void)
m->lt[0] = &layouts[0];
m->lt[1] = &layouts[1 % LENGTH(layouts)];
- strncpy(m->ltsymbol, layouts[0].symbol, sizeof m->ltsymbol);
+ strncpy(m->ltsymbol, layouts[0].symbol, sizeof m->ltsymbol - 1);
+ m->ltsymbol[sizeof m->ltsymbol - 1] = '\0';
return m;
}
@@ -1515,5 +1517,6 @@ setlayout(const Arg *arg)
if (arg && arg->v)
selmon->lt[selmon->sellt] = (Layout *)arg->v;
- strncpy(selmon->ltsymbol, selmon->lt[selmon->sellt]->symbol, sizeof
selmon->ltsymbol);
+ strncpy(selmon->ltsymbol, selmon->lt[selmon->sellt]->symbol,
+ sizeof selmon->ltsymbol - 1);
+ selmon->ltsymbol[sizeof selmon->ltsymbol - 1] = '\0';
if (selmon->sel)
arrange(selmon);
A stronger alternative is a compile-time assert in the spirit of struct NumTags
(dwm.c:274), so an over-long symbol fails the build instead of corrupting the
bar at runtime.
================================================================================
SEC-06 Hostile window title -> XftFontMatch storm + unbounded font cache
================================================================================
Severity : Low/Medium (availability - CPU and memory exhaustion)
Location : drw.c:354-400 drw_text() fallback-font path
CWE : CWE-400 Uncontrolled Resource Consumption
Description
-----------
For each codepoint with no glyph in the loaded fonts, drw_text() performs a
fontconfig match and appends the result to drw->fonts permanently:
drw.c:359-366 /* 128-entry, 2-slot negative cache */
if (nomatches[h0] == utf8codepoint || nomatches[h1] == ...)
goto no_match;
drw.c:382 match = XftFontMatch(drw->dpy, drw->screen, fcpattern,
&result);
drw.c:390-392 for (curfont = drw->fonts; curfont->next; curfont =
curfont->next);
curfont->next = usedfont;
Two problems, both driven by fully client-controlled text (a window title read
in updatetitle(), dwm.c:2016, or the root-window status string read in
updatestatus(), dwm.c:2008):
1. The negative cache is 128 entries with only two hash slots per codepoint
(drw.c:263, drw.c:362-363). A title cycling through a few hundred distinct
unmapped codepoints thrashes it, so nearly every character triggers a full
FcConfigSubstitute + FcDefaultSubstitute + XftFontMatch on EVERY bar
redraw. That is a fontconfig traversal per character per redraw, inside
dwm's single-threaded event loop - the WM becomes unresponsive.
2. Fonts that DO match are appended to drw->fonts and never freed until exit
(only drw_free()/drw_fontset_free() release them). The list is also walked
linearly for every character (drw.c:294), so cost grows with list length.
Redraws are themselves client-triggerable: propertynotify() -> updatetitle() ->
drawbar() (dwm.c:1248-1252), or forged Expose events (see SEC-03 note).
Failure scenario
----------------
A client sets _NET_WM_NAME to ~200 codepoints drawn from unassigned planes
(e.g. U+E0100..U+E01CF) and then rewrites the property in a loop. Each rewrite
costs dwm hundreds of XftFontMatch calls; the WM stops servicing input while
RSS climbs with each newly opened fallback font.
Illustrative mitigation
-----------------------
This one has no clean one-liner; the sketch below caps the fallback chain and
is a behaviour trade-off (very exotic scripts will render as the invalid glyph
once the cap is hit):
--- a/drw.c
+++ b/drw.c
@@ -11,6 +11,8 @@
#define UTF_INVALID 0xFFFD
+/* bound the fallback chain: it is grown from client-supplied text */
+#define MAXFALLBACKS 32
@@ -387,10 +389,13 @@ no_match:
if (match) {
usedfont = xfont_create(drw, NULL, match);
- if (usedfont && XftCharExists(drw->dpy,
usedfont->xfont, utf8codepoint)) {
- for (curfont = drw->fonts;
curfont->next; curfont = curfont->next)
- ; /* NOP */
+ for (nfallbacks = 0, curfont = drw->fonts;
curfont->next;
+ curfont = curfont->next)
+ nfallbacks++;
+ if (usedfont && nfallbacks < MAXFALLBACKS
+ && XftCharExists(drw->dpy, usedfont->xfont,
utf8codepoint)) {
curfont->next = usedfont;
} else {
xfont_free(usedfont);
nomatches[nomatches[h0] ? h1 : h0] =
utf8codepoint;
(declare `unsigned int nfallbacks;` alongside the other locals at drw.c:251.)
Enlarging `nomatches` (drw.c:263) from 128 entries materially reduces the
XftFontMatch rate for the same cost in bss.
================================================================================
SEC-07 showhide() recurses once per window - stack exhaustion
================================================================================
Severity : Low (availability; requires many windows)
Location : dwm.c:1629-1645 showhide()
CWE : CWE-674 Uncontrolled Recursion
Description
-----------
showhide() walks the monitor's stack list by self-recursion, one frame per
client, in both branches:
dwm.c:1639 showhide(c->snext); /* visible: show, then recurse */
dwm.c:1642 showhide(c->snext); /* hidden: recurse, then hide */
The list length is the number of managed windows, which any client can inflate
by mapping windows in a loop (manage() imposes no limit). arrange() calls
showhide() on every layout change, so once the list is deep enough the next
arrange() overflows the stack. Compiled at -O2 GCC does not reliably turn the
visible branch into a tail call (there is work after the call in the hidden
branch), so the frames are real.
drw_fontset_free() (drw.c:160-167) has the same shape, but its list length is
bounded by the font cache, so it only matters in combination with SEC-06.
Failure scenario
----------------
A client maps ~100k tiny windows on one tag. The user presses MODKEY+t; dwm
recurses 100k frames deep in showhide() and receives SIGSEGV on stack
overflow.
Illustrative fix
----------------
Iterate. Hidden windows are moved off-screen, so the original bottom-up
ordering for that branch is not observable:
--- a/dwm.c
+++ b/dwm.c
@@ -1629,17 +1629,17 @@ void
showhide(Client *c)
{
- if (!c)
- return;
- if (ISVISIBLE(c)) {
- /* show clients top down */
- XMoveWindow(dpy, c->win, c->x, c->y);
- if ((!c->mon->lt[c->mon->sellt]->arrange || c->isfloating) &&
!c->isfullscreen)
- resize(c, c->x, c->y, c->w, c->h, 0);
- showhide(c->snext);
- } else {
- /* hide clients bottom up */
- showhide(c->snext);
- XMoveWindow(dpy, c->win, WIDTH(c) * -2, c->y);
- }
+ /* iterative: the stack list length is client controlled */
+ for (; c; c = c->snext) {
+ if (ISVISIBLE(c)) {
+ /* show clients top down */
+ XMoveWindow(dpy, c->win, c->x, c->y);
+ if ((!c->mon->lt[c->mon->sellt]->arrange ||
c->isfloating)
+ && !c->isfullscreen)
+ resize(c, c->x, c->y, c->w, c->h, 0);
+ } else {
+ XMoveWindow(dpy, c->win, WIDTH(c) * -2, c->y);
+ }
+ }
}
================================================================================
SEC-08 gettextprop() leaks the XmbTextPropertyToTextList allocation
================================================================================
Severity : Low (memory leak, client-triggerable)
Location : dwm.c:908-929 gettextprop()
CWE : CWE-401 Missing Release of Memory after Effective Lifetime
Description
-----------
dwm.c:922 } else if (XmbTextPropertyToTextList(dpy, &name, &list, &n)
>= Success && n > 0 && *list) {
dwm.c:923 strncpy(text, *list, size - 1);
dwm.c:924 XFreeStringList(list);
dwm.c:925 }
XFreeStringList() is inside the conjunction. If the conversion succeeds (return
value >= Success, which includes the positive "N characters could not be
converted" results) but `n == 0` or `*list == NULL`, `list` has been allocated
and is never freed. gettextprop() is on a client-driven hot path -
propertynotify() -> updatetitle() (dwm.c:1248-1252) runs on every WM_NAME /
_NET_WM_NAME change - so a client that can reach that state can leak on demand
until the OOM killer intervenes.
Illustrative fix
----------------
--- a/dwm.c
+++ b/dwm.c
@@ -918,9 +918,11 @@ gettextprop(Window w, Atom atom, char *text, unsigned int
size)
if (name.encoding == XA_STRING) {
strncpy(text, (char *)name.value, size - 1);
- } else if (XmbTextPropertyToTextList(dpy, &name, &list, &n) >= Success
&& n > 0 && *list) {
- strncpy(text, *list, size - 1);
- XFreeStringList(list);
+ } else if (XmbTextPropertyToTextList(dpy, &name, &list, &n) >= Success)
{
+ /* the list is allocated even when it turns out to be empty */
+ if (n > 0 && *list)
+ strncpy(text, *list, size - 1);
+ XFreeStringList(list);
}
text[size - 1] = '\0';
XFree(name.value);
================================================================================
SEC-09 updatenumlockmask() dereferences an unchecked XGetModifierMapping()
================================================================================
Severity : Low (NULL dereference)
Location : dwm.c:1945-1959 updatenumlockmask()
CWE : CWE-476 NULL Pointer Dereference
Description
-----------
dwm.c:1952 modmap = XGetModifierMapping(dpy);
dwm.c:1954 for (j = 0; j < modmap->max_keypermod; j++)
dwm.c:1955 if (modmap->modifiermap[i * modmap->max_keypermod + j]
XGetModifierMapping() returns NULL when the reply cannot be allocated. The
return value is never checked, so an allocation failure (which an X client can
help along by keeping the server and dwm under memory pressure - see SEC-06 and
SEC-08) turns into a NULL dereference. updatenumlockmask() runs from
grabbuttons() (dwm.c:934) on every focus change and from grabkeys()
(dwm.c:955), i.e. constantly.
Illustrative fix
----------------
--- a/dwm.c
+++ b/dwm.c
@@ -1948,7 +1948,8 @@ updatenumlockmask(void)
unsigned int i, j;
XModifierKeymap *modmap;
numlockmask = 0;
- modmap = XGetModifierMapping(dpy);
+ if (!(modmap = XGetModifierMapping(dpy)))
+ return;
for (i = 0; i < 8; i++)
================================================================================
SEC-10 NULL dereference when the monitor list empties or desyncs
================================================================================
Severity : Low (NULL dereference; requires an unusual RANDR/Xinerama state)
Location : dwm.c:498-512 cleanupmon(); dwm.c:1910-1924 and 1938-1941
updategeom()
CWE : CWE-476 NULL Pointer Dereference
Description
-----------
(a) cleanupmon() assumes `mon` is in the list:
dwm.c:506 for (m = mons; m && m->next != mon; m = m->next);
dwm.c:507 m->next = mon->next; /* m may be NULL here */
The loop guards against NULL but the dereference on the next line does not.
(b) updategeom()'s monitor-removal loop dereferences `m` unguarded, and the
same loop can empty the list entirely:
dwm.c:1912 for (m = mons; m && m->next; m = m->next);
dwm.c:1913 while ((c = m->clients)) { /* m may be NULL */
(c) If the removal loop does empty the list (nn == 0 reported by
XineramaQueryScreens), the tail of updategeom() leaves selmon NULL:
dwm.c:1939 selmon = mons; /* NULL */
dwm.c:1940 selmon = wintomon(root); /* recttomon() returns selmon ==
NULL */
Every later `selmon->...` (e.g. buttonpress() at dwm.c:433, focus() at
dwm.c:792) then faults.
Failure scenario
----------------
A RANDR/Xinerama transition that momentarily reports zero active screens (hot
unplug of the last output, or a driver glitch during suspend/resume) drives dwm
through (b)/(c) and it crashes on the next event.
Illustrative fix
----------------
--- a/dwm.c
+++ b/dwm.c
@@ -503,8 +503,9 @@ cleanupmon(Monitor *mon)
if (mon == mons)
mons = mons->next;
else {
for (m = mons; m && m->next != mon; m = m->next);
- m->next = mon->next;
+ if (m)
+ m->next = mon->next;
}
@@ -1910,7 +1911,9 @@ updategeom(void)
/* removed monitors if n > nn */
for (i = nn; i < n; i++) {
for (m = mons; m && m->next; m = m->next);
+ if (!m)
+ break;
while ((c = m->clients)) {
@@ -1938,6 +1941,8 @@ updategeom(void)
if (dirty) {
selmon = mons;
selmon = wintomon(root);
+ if (!selmon) /* never leave selmon dangling */
+ selmon = mons = mons ? mons : createmon();
}
return dirty;
(The last hunk is a sketch - the real invariant "mons is never empty" is better
enforced by refusing to remove the final monitor.)
================================================================================
SEC-11 clientmessage(): no format check, no origin restriction
================================================================================
Severity : Low
Location : dwm.c:514-531 clientmessage()
CWE : CWE-20 Improper Input Validation; CWE-346
Description
-----------
dwm.c:522 if (cme->message_type == netatom[NetWMState]) {
dwm.c:523 if (cme->data.l[1] == netatom[NetWMFullscreen]
dwm.c:524 || cme->data.l[2] == netatom[NetWMFullscreen])
cme->format is never checked. A client message sent with format 8 or 16 fills
the same 20-byte union, so data.l[1]/data.l[2] read reinterpreted bytes rather
than the atoms EWMH specifies. This is not memory-unsafe (the union is large
enough), but it means malformed messages can toggle fullscreen with values the
sender never intended to place there.
Separately, and by EWMH design, ANY client may send _NET_WM_STATE for ANY
window: a client can force an unrelated window (a password prompt, a video
call) into or out of fullscreen, and can set the urgency hint on any window
(dwm.c:527-529). This is spec-conformant, so it is noted for awareness rather
than as a defect - but it is the mechanism by which a hostile client can cover
the screen with its own window on demand.
Note for completeness: keypress() (dwm.c:1000) likewise never inspects
send_event, and keys[] includes spawn() and quit(). This is NOT currently
exploitable because dwm selects no KeyPressMask on the root or bar windows, so
a synthetic KeyPress has no recipient. Any patch that adds KeyPressMask to a
dwm-owned window turns this into a full command-execution bug; adding the
`if (ev->send_event) return;` guard now is cheap insurance.
Illustrative fix
----------------
--- a/dwm.c
+++ b/dwm.c
@@ -518,7 +518,7 @@ clientmessage(XEvent *e)
Client *c = wintoclient(cme->window);
- if (!c)
+ if (!c || cme->format != 32) /* EWMH mandates format 32 */
return;
if (cme->message_type == netatom[NetWMState]) {
================================================================================
SEC-12 spawn(): unchecked fork(), descriptor inheritance, dmenumon encoding
================================================================================
Severity : Low
Location : dwm.c:1647-1667 spawn()
CWE : CWE-252 Unchecked Return Value; CWE-403 Exposure of File
Descriptor to Unintended Control Sphere
Description
-----------
dwm.c:1652 if (arg->v == dmenucmd)
dwm.c:1653 dmenumon[0] = '0' + selmon->num;
dwm.c:1654 if (fork() == 0) {
dwm.c:1655 if (dpy)
dwm.c:1656 close(ConnectionNumber(dpy));
(a) fork() == -1 is not distinguished from the parent path, so under fork
exhaustion spawn() silently does nothing - no diagnostic, no feedback.
(b) The child closes only the X connection. Every other descriptor dwm
inherited from the session startup script (~/.xsession-errors, any log,
socket, or keyring/agent fd held open by .xinitrc) is passed to the
spawned program. dmenu/terminal launches are the most common path by
which an attacker-reachable process is created, so this is a real
least-privilege gap. Setting FD_CLOEXEC broadly, or closing the range
above stderr, is the fix.
(c) `dmenumon[0] = '0' + selmon->num` writes a single byte into
`static char dmenumon[2] = "0"` (config.def.h:60). No overflow, but for
monitor index >= 10 it writes ':', ';', ... and dmenu is handed a
non-numeric -m argument. Ten-plus-monitor setups exist.
Illustrative fix
----------------
--- a/dwm.c
+++ b/dwm.c
@@ -1647,12 +1647,20 @@ void
spawn(const Arg *arg)
{
struct sigaction sa;
+ pid_t pid;
- if (arg->v == dmenucmd)
+ if (arg->v == dmenucmd && selmon->num >= 0 && selmon->num < 10)
dmenumon[0] = '0' + selmon->num;
- if (fork() == 0) {
+ if ((pid = fork()) == -1) {
+ fprintf(stderr, "dwm: fork failed: %s\n", strerror(errno));
+ return;
+ }
+ if (pid == 0) {
if (dpy)
close(ConnectionNumber(dpy));
+ /* do not hand dwm's inherited descriptors to the child */
+ closefrom(STDERR_FILENO + 1); /* or a close_range()/loop
fallback */
setsid();
(`closefrom()` is BSD/glibc 2.34+; on older glibc use
`close_range(3, ~0U, 0)` or a getrlimit-bounded loop. A more portable
alternative is to set FD_CLOEXEC on descriptors as they are created.)
================================================================================
SEC-13 Placement rules are keyed on client-spoofable properties
(INFORMATIONAL)
================================================================================
Severity : Informational
Location : dwm.c:277-311 applyrules()
Description
-----------
dwm.c:295 if ((!r->title || strstr(c->name, r->title))
dwm.c:296 && (!r->class || strstr(class, r->class))
dwm.c:297 && (!r->instance || strstr(instance, r->instance)))
WM_CLASS and WM_NAME are set by the client itself, and the match is an
unanchored substring search. Any client can therefore opt in to any rule by
naming itself appropriately - e.g. claiming class "Firefox" to land on the
user's browser tag (config.def.h:31), or matching a rule that grants
isfloating/always-on-top placement. The string handling itself is sound
(c->name is NUL-terminated by gettextprop(), and `broken` is substituted when
the hints are absent).
This is inherent to X11 window rules rather than a dwm bug, but it means rules
must never be relied on as a trust or isolation boundary.
================================================================================
CHECKED AND FOUND SOUND
================================================================================
Recording these so the coverage of the review is auditable:
* getatomprop() (dwm.c:863-878) - the `nitems > 0 && format == 32` guard added
in c3dd6a8 is correct; reading `*(long *)p` is right for format 32 because
Xlib returns a long array regardless of the wire format. Requesting
`sizeof atom` (8) longs is an over-request, which is harmless.
* getstate() (dwm.c:890-906) - `n != 0 && format == 32` correctly prevents the
NULL/short read; XFree(NULL) on the not-found path is safe. The access-type
fix in 5c9f303 holds up.
* gettextprop() bounds (dwm.c:921, 926) - `strncpy(..., size - 1)` plus the
explicit `text[size - 1] = '\0'` is correct, and XGetTextProperty()
guarantees a NUL-terminated value, so the source-side read is in bounds.
(The only defect is the leak in SEC-08.)
* utf8decode() tables (drw.c:16-34) - `lens[*s >> 3]` indexes 0..31 into a
32-entry table and `leading_mask[len - 1]` / `overlong[len - 1]` index 0..3
into 4-entry tables, with the len == 0 case returning early. Surrogate,
overlong and out-of-range rejection at drw.c:41 is correct.
* unmapnotify() (dwm.c:1804-1816) - correctly distinguishes synthetic from
real unmaps. This is the pattern the handlers in SEC-01/02/03 should follow.
* struct NumTags (dwm.c:274) - the compile-time assert that tags fit a 31-bit
mask is sound, and every `arg->ui` is masked with TAGMASK before use
(dwm.c:1672, 1745, 1756, 2059), so the `1 << i` shifts cannot go out of
range.
* tile() (dwm.c:1687-1713) - the two divisors `MIN(n, m->nmaster) - i` and
`n - i` are provably >= 1 given the loop bounds, and incnmaster() clamps
nmaster at 0 (dwm.c:983), so the signed/unsigned comparison at dwm.c:1697 is
safe. No division by zero.
* sendevent() (dwm.c:1447-1470) - `while (!exists && n--)` terminates correctly
for n == 0 and does not index below zero.
* main() (dwm.c:2143-2165) - argv handling is bounded; the OpenBSD pledge() at
dwm.c:2157 is a genuine hardening win and its absence elsewhere is noted
below.
* transient.c - a standalone manual test program, not linked into dwm. No
findings.
* No use of system(), popen(), exec*p() with attacker-controlled argv, and no
format string is ever taken from client data (die()/fprintf() callers all
pass literals).
================================================================================
CROSS-CUTTING OBSERVATIONS
================================================================================
1. Add the missing origin check systematically. Ten of the fourteen handlers in
`handler[]` (dwm.c:245-260) act on data that may have arrived via
XSendEvent. A single audit pass adding `if (ev->send_event) return;` to
configurenotify, buttonpress, enternotify, motionnotify, expose,
propertynotify and keypress closes SEC-01, SEC-02, SEC-03 and pre-empts the
keypress variant, at a cost of about seven lines.
2. xerror()'s fail-closed default is load-bearing and currently over-strict for
BadValue. Any BadValue dwm provokes by feeding client-supplied numbers to
Xlib is fatal (SEC-01). Either validate the numbers (preferred) or add the
specific request_code/BadValue pairs to the ignore list - not BadValue
wholesale, which would mask real bugs.
3. Client-supplied integers cross the trust boundary unclamped. SEC-04 is the
acute case, but the same pattern appears in configurerequest() (dwm.c:589-
608: c->bw, c->x/y/w/h taken verbatim from the request) - those flow into
WIDTH()/HEIGHT() macros that add `2 * bw`, so a border_width near INT_MAX
overflows there too. A single sanitising accessor for "geometry value from
a client" would cover all of them.
4. Build hardening is absent. config.mk should carry, at minimum:
-D_FORTIFY_SOURCE=2 -fstack-protector-strong -fPIE -pie
-Wl,-z,relro,-z,now -Wl,-z,noexecstack
and, for development builds, -fsanitize=address,undefined - UBSan in
particular would have caught SEC-04's signed overflow and inf->int
conversion immediately. Consider also -Wall -Wextra -Wformat=2
-Wconversion; the codebase currently compiles clean under far less.
5. Privilege reduction is OpenBSD-only. The pledge() call at dwm.c:2157 has no
Linux counterpart. A seccomp-bpf filter after setup() (the syscall set dwm
needs post-initialisation is small: read/write/poll on the X socket, plus
fork/execve for spawn) would materially limit the impact of any memory-
safety bug found above.
6. No fuzzing harness exists. The highest-value next step is a synthetic X
client that drives dwm with randomised properties (WM_NORMAL_HINTS, WM_HINTS,
_NET_WM_STATE, long/exotic titles) and randomised XSendEvent traffic, run
against an ASan+UBSan build under Xephyr. SEC-04 and SEC-06 would surface in
minutes.
================================================================================
END OF FINDINGS
================================================================================
================================================================================
dmenu -- C SOURCE SECURITY REVIEW
================================================================================
Target : C:\dmenu (suckless dmenu 5.4)
Commit : 7175c48 "add -of and -ob arguments for outline colors options
for multi-selection" (branch master, clean tree)
Scope : dmenu.c (799 L), drw.c (471 L), stest.c (109 L), util.c (37 L),
arg.h, drw.h, util.h, config.def.h, plus the shipped shell
wrappers (dmenu_path, dmenu_run) and the build config.
Date : 2026-09-11
Method : Manual source audit (data-flow from the three untrusted inputs:
stdin item list, X11 keyboard/IM events, and the
PRIMARY/CLIPBOARD
selection) plus argv handling and build-hardening review.
IMPORTANT - STATUS OF THE PATCHES BELOW
No files in the repository were modified. The diffs in Part 2 are proposed
patches written by hand against the pristine tree at 7175c48. They have NOT
been applied and have NOT been compiled or tested. Apply them one at a time
with `patch -p1 < <hunk>` (patch resolves line-number offsets by context);
if you apply several, later hunk headers will be off by the earlier deltas.
THREAT MODEL / CAVEAT
dmenu is an unprivileged, non-setuid, single-user X11 client. It holds no
privilege boundary of its own, and X11 itself provides no isolation between
clients on the same display. Consequently nothing below is a
privilege-escalation bug. The realistic impacts are: (a) crashes / denial of
service driven by untrusted menu content or by the invoking arguments,
(b) out-of-bounds reads, and (c) exposure of typed secrets, which matters
because dmenu is widely used as a passphrase prompt (passmenu, pass-dmenu).
Findings are ranked by exploitability within that model, not by CVSS.
--------------------------------------------------------------------------------
SUMMARY
--------------------------------------------------------------------------------
ID Sev CWE Location Issue
---- ------ -------- ------------------------------
------------------------------
F1 HIGH CWE-476 dmenu.c:443 / dmenu.c:178 NULL-pointer dereference:
End key leaves curr ==
NULL,
drawmenu() then reads
curr->left. Reachable
crash.
F2 MED CWE-125 dmenu.c:661-667 Out-of-bounds read of
info[n] when the pointer
lies on no Xinerama screen.
F3 MED CWE-190 dmenu.c:637,743-745 `lines = MAX(lines, 0)` is
CWE-20 dead code (lines is
unsigned); atoi() lets -l
wrap; (lines+1)*bh
overflows.
F4 MED CWE-476 dmenu.c:696 XCreateIC() return value
unchecked -> NULL xic is
passed to
XmbLookupString().
F5 LOW CWE-457 dmenu.c:326,417 Uninitialised read of
buf[0]
when XmbLookupString()
returns XLookupKeySym.
F6 LOW CWE-191 drw.c:340-344 Unsigned underflow of `w`
in
measure-only mode on
invalid
UTF-8.
F7 LOW CWE-190 dmenu.c:559,562 Unchecked size multiply in
CWE-20 realloc(); getline() return
used without a `len > 0`
guard.
F8 LOW CWE-226 dmenu.c:36,99-112 Typed input (possibly a
CWE-528 passphrase) is left in a
static buffer and can reach
a core dump.
F9 MED CWE-426 dmenu_path:9-10 Empty PATH element makes
CWE-59 stest list $PWD binaries;
`tee` follows a symlinked
cache path.
F10 INFO CWE-1104 config.mk:27-29 No build hardening
(RELRO/PIE/FORTIFY/stack
protector).
F11 INFO CWE-401 dmenu.c:232-244, 693-697 Leaks: tokv, xim/xic never
released.
F12 INFO CWE-667 dmenu.c:788-790 `-f` grabs the keyboard
before reading stdin: a
stalled producer wedges the
whole X session's keyboard.
F13 INFO CWE-200 dmenu.c:217-218 `-w` (embed) skips the
keyboard grab entirely.
F14 INFO CWE-367 stest.c:28-48 access()-based tests are
CWE-807 TOCTOU-prone by design.
F15 INFO -- dmenu_run:2 Selection is piped straight
into $SHELL (by design, but
it is the impact multiplier
for F9).
CHECKED AND FOUND *NOT* VULNERABLE (documented so the next reviewer can skip):
* insert() (dmenu.c:284) -- the classic suspect. `strlen(text) + n` with a
negative ssize_t n wraps to a huge size_t and is correctly rejected by the
`> sizeof text - 1` test; dest = &text[cursor+n] and the length
`sizeof text - cursor - MAX(n,0)` both stay inside text[] for every call
site (paste, XK_u, XK_w, BackSpace, Delete). No overflow.
* match() (dmenu.c:240) -- `char buf[sizeof text]` + strcpy(buf, text) is
size-exact; the tokv realloc sizes tokn >= tokc before writing
tokv[tokc-1].
* keypress() XK_Tab (dmenu.c:519) -- strnlen(sel->text, sizeof text - 1)
bounds both the memcpy and the terminator write.
* utf8decode() (drw.c:14) -- lens[*s >> 3] is a 32-entry table indexed 0..31;
leading_mask[len-1] is bounded 0..3; the continuation loop stops on '\0'
and
returns the number of bytes actually consumed, so drw_font_getexts() never
reads past the terminator.
* nextrune() (dmenu.c:297) -- returns (size_t)-1 when cursor == 0, but every
caller is guarded by `cursor > 0` or an explicit `cursor == 0` early
return.
Latent trap, not currently reachable.
* readstdin() NUL terminator (dmenu.c:557,571) -- the `i + 1 >= itemsiz`
growth check keeps both index i and the sentinel index i valid.
* stest.c FLAG() / flag[26] -- ARGC() is validated with
strchr("abcdefghlpqrsuvwx", ...) before indexing, and ARGBEGIN guarantees
ARGC() != '\0', so `flag[(x)-'a']` cannot go out of bounds.
* die() (util.c:11) -- vfprintf with a caller-supplied format, but every one
of the 12 call sites passes a string literal; attacker-controlled data
(colour names, font names, window ids) is always an argument, never the
format. No format-string bug.
* drw_text() ellipsis recursion -- recursive calls with w == 0 are rejected
by
the `render && (!drw->scheme || !w)` guard, and ellipsis_len is always <=
the current utf8strlen, so XftDrawStringUtf8 never overruns utf8str.
================================================================================
PART 1 -- FINDINGS
================================================================================
--------------------------------------------------------------------------------
F1 HIGH -- NULL-pointer dereference via the End key (CWE-476)
--------------------------------------------------------------------------------
Where : dmenu.c:443 (root cause), dmenu.c:178 (crash site), dmenu.c:80-96
Impact: Reliable, remotely-influenced crash of dmenu. Availability only -- the
dereference is of a plain NULL, not of attacker-chosen memory, so this
is not exploitable for code execution.
The End handler walks `curr` to the last page:
dmenu.c:437-445
if (next) {
/* jump to end of list and position items in reverse */
curr = matchend;
calcoffsets();
curr = prev;
calcoffsets();
while (next && (curr = curr->right))
calcoffsets();
}
calcoffsets() computes the page break:
dmenu.c:85-95
if (lines > 0)
n = lines * bh;
else
n = mw - (promptw + inputw + TEXTW("<") + TEXTW(">"));
for (i = 0, next = curr; next; next = next->right)
if ((i += (lines > 0) ? bh : textw_clamp(next->text, n)) > n)
break;
If the very first item of the page is already wider than the budget `n`, the
loop breaks immediately with `next == curr`, i.e. non-NULL. The while loop at
:443 therefore keeps advancing, and when `curr` reaches `matchend` the
assignment `curr = curr->right` yields NULL, ends the loop, and leaves the
global `curr` NULL. drawmenu() is then called and takes the horizontal branch:
dmenu.c:174-178
} else if (matches) {
/* draw horizontal list */
x += inputw;
w = TEXTW("<");
if (curr->left) { <-- NULL dereference
Two ways to make `next == curr` hold for every page, both trivially reachable:
a) A long `-p` prompt. `promptw` grows without bound while `inputw` is fixed
at mw/3, so `n` at :88 goes negative. `n` is a signed int, so the
comparison `(i += textw_clamp(...)) > n` is true on the first item
regardless of its width. (Note textw_clamp() takes `unsigned int n`, so
the negative n converts to ~4e9 inside it and the clamp is a no-op --
the returned width is the real, small width, which is then compared
against the *signed* negative n.)
b) A single menu item wider than roughly 2/3 of the screen (n is about
mw - mw/3 minus the arrow widths). ~200 characters at a default font
size is enough. This is ordinary, non-malicious input: long paths, long
command lines, `pass` entry names.
Repro (a):
printf 'a\nb\n' | dmenu -p "$(printf 'x%.0s' $(seq 1 400))"
then press End -> segfault
Repro (b):
{ printf 'short\n'; printf 'y%.0s' $(seq 1 400); echo; } | dmenu
then press End -> segfault
Traced for the two-item case (i1 narrow, i2 wide): curr=matchend=i2 ->
calcoffsets sets next=i2,prev=i1 -> curr=prev=i1 -> calcoffsets sets next=i2 ->
loop: curr=i1->right=i2, calcoffsets sets next=i2 -> loop: curr=i2->right=NULL
-> exit with curr==NULL -> drawmenu -> curr->left.
Fix: two parts (Patch F1). Primary, at :443, never let `curr` become NULL --
advance only when `curr->right` exists. Secondary, defence in depth at :174,
require `curr` in the horizontal branch. The deeper design flaw -- that
calcoffsets() can fail to make forward progress when a single item does not fit
the budget -- is left alone deliberately: making it always advance at least one
item changes paging behaviour and belongs in a separate change.
--------------------------------------------------------------------------------
F2 MEDIUM -- Out-of-bounds read of the Xinerama screen array (CWE-125)
--------------------------------------------------------------------------------
Where : dmenu.c:659-667
Impact: Heap over-read of one XineramaScreenInfo (~20 bytes) past the end of
an Xlib allocation. The garbage is used as window geometry, so the
observable effect ranges from a mispositioned menu to a huge
XCreatePixmap/XCreateWindow request (BadAlloc / X-server memory
pressure). Not attacker-controlled input, so this is a robustness and
memory-safety defect rather than an attack primitive.
dmenu.c:659-667
/* no focused window is on screen, so use pointer location instead */
if (mon < 0 && !area && XQueryPointer(dpy, root, &dw, &dw, &x, &y, &di,
&di, &du))
for (i = 0; i < n; i++)
if (INTERSECT(x, y, 1, 1, info[i]) != 0)
break;
x = info[i].x_org;
y = info[i].y_org + (topbar ? 0 : info[i].height - mh);
mw = info[i].width;
If the loop completes without a match, `i == n` and info[n] is read -- one
element past the array XineramaQueryScreens() allocated. The loop does not
match when the pointer is inside the root window but outside every CRTC area.
That region exists whenever the monitor layout is not a perfect rectangle
(e.g. a 1920x1080 beside a 2560x1440 leaves an L-shaped dead zone inside the
root window's bounding box), and the pointer can be placed there by
XWarpPointer from any client on the display.
Second, smaller instance in the same statement: `n` is never checked to be
>= 1. If XineramaQueryScreens() ever returns a non-NULL array with n == 0,
neither of the two `for (j/i = 0; ... < n; ...)` loops runs, `i` stays 0, and
info[0] is read out of bounds.
Fix: Patch F2 -- reject n < 1 up front, and clamp `i` back to 0 when the
pointer matched no screen.
--------------------------------------------------------------------------------
F3 MEDIUM -- Dead bounds check + integer overflow on -l (CWE-190, CWE-20)
--------------------------------------------------------------------------------
Where : dmenu.c:637-638, dmenu.c:742-745, config.def.h:17
Impact: Unvalidated numeric argv; an X resource-exhaustion / BadValue path.
Requires control of dmenu's own command line, so the direct security
value is low -- but the dead check makes this a latent bug that any
future patch touching `lines` will fall into, which is why it is rated
MEDIUM rather than INFO.
`lines` is unsigned (config.def.h:17 `static unsigned int lines = 0;`), so the
guard at dmenu.c:637 is dead code -- `MAX(lines, 0)` on an unsigned value can
never change it:
dmenu.c:636-638
bh = drw->fonts->h + 2;
lines = MAX(lines, 0); <-- no-op, lines is unsigned
mh = (lines + 1) * bh; <-- overflows
The value arrives via atoi(), which has no error reporting and silently
accepts negatives and garbage:
dmenu.c:742-745
else if (!strcmp(argv[i], "-l")) /* number of lines in vertical list */
lines = atoi(argv[++i]);
else if (!strcmp(argv[i], "-m"))
mon = atoi(argv[++i]);
`dmenu -l -1` therefore sets lines = 4294967295. Two consequences:
* `(lines + 1) * bh` at :638 wraps unsigned to 0, so mh becomes 0 and
XCreateWindow(..., mh=0, ...) raises BadValue.
* `mh` is a plain `int`, so a large-but-legal item count also overflows it
signed (undefined behaviour) before reaching XCreatePixmap.
Partially mitigated today by `lines = MIN(lines, i);` at dmenu.c:572, which
clamps `lines` to the number of items read from stdin. That mitigation is
incidental -- it happens to run before setup() -- and it still permits
mh = (itemcount + 1) * bh, i.e. a ~100k-line stdin plus `-l -1` asks the X
server for a pixmap around 1.7 million pixels tall.
Fix: Patch F3 -- parse -l/-m with strtol and a range check, and replace the
dead MAX() with a real clamp that keeps (lines + 1) * bh inside the CARD16
range the X protocol actually allows for a window dimension.
--------------------------------------------------------------------------------
F4 MEDIUM -- XCreateIC() return value unchecked (CWE-476)
--------------------------------------------------------------------------------
Where : dmenu.c:696-697, used at dmenu.c:331
Impact: NULL-pointer dereference inside Xlib on the first keypress. Crash
before any user input is processed.
XOpenIM is checked, XCreateIC is not:
dmenu.c:693-697
if ((xim = XOpenIM(dpy, NULL, NULL, NULL)) == NULL)
die("XOpenIM failed: could not open input device");
xic = XCreateIC(xim, XNInputStyle, XIMPreeditNothing | XIMStatusNothing,
XNClientWindow, win, XNFocusWindow, win, NULL);
XCreateIC legitimately returns NULL -- an input method that does not support
the requested PreeditNothing/StatusNothing style, or a resource failure. The
NULL then goes straight into XmbLookupString():
dmenu.c:331
len = XmbLookupString(xic, ev, buf, sizeof buf, &ksym, &status);
Fix: Patch F4 -- die() on a NULL context, matching the XOpenIM handling
directly above it.
--------------------------------------------------------------------------------
F5 LOW -- Uninitialised stack read in keypress() (CWE-457)
--------------------------------------------------------------------------------
Where : dmenu.c:326 (declaration), dmenu.c:417 (read)
Impact: Undefined behaviour; reads one byte of uninitialised stack. Benign in
practice (the accompanying len is 0, so nothing is inserted), but it is
a real UB / MSan finding and the kind of thing that turns into a bug
when the surrounding code is edited.
dmenu.c:326 char buf[64];
dmenu.c:331 len = XmbLookupString(xic, ev, buf, sizeof buf, &ksym,
&status);
When XmbLookupString() reports status == XLookupKeySym it returns 0 and does
not write to `buf` at all. Execution continues into the keysym switch, and any
unhandled keysym falls through to the default arm:
dmenu.c:414-419
switch(ksym) {
default:
insert:
if (!iscntrl((unsigned char)*buf)) <-- buf is uninitialised
insert(buf, len); <-- len == 0 here
break;
`*buf` is read before anything has written it. The subsequent insert(buf, 0) is
a harmless self-memmove, so the only concrete consequence is the UB itself.
Fix: Patch F5 -- test `len > 0` before touching buf, and zero-initialise the
buffer at its declaration.
--------------------------------------------------------------------------------
F6 LOW -- Unsigned underflow of the width budget in drw_text() (CWE-191)
--------------------------------------------------------------------------------
Where : drw.c:340-344
Impact: `w` (unsigned int) wraps to ~4e9 in measure-only mode, defeating the
early-exit in the measuring loop. Result: wasted CPU walking the whole
string instead of stopping at the clamp, on any item containing invalid
UTF-8. No memory-safety consequence -- callers clamp the return value.
drw.c:340-345
if (utf8err && (!render || invalid_width < w)) {
if (render)
drw_text(drw, x, y, w, h, 0, invalid, invert);
x += invalid_width;
w -= invalid_width;
}
The `invalid_width < w` guard is short-circuited by `!render`, which is exactly
the mode reached from drw_fontset_getwidth_clamp() (drw.c:431) where `w` was
set to the small clamp value `n`:
drw.c:269-270
if (!render) {
w = invert ? invert : ~invert; /* w = n for the clamp path */
So with n smaller than the cached `invalid_width`, `w -= invalid_width`
underflows and every later `ew + tmpw > w` test becomes false. The function
still returns a clamped value (textw_clamp does MIN(w, n) at dmenu.c:64), so
output is correct; only the work is wasted. Attacker cost: put a stray 0x80
byte in a long menu item.
Note that in a pure measuring run `invalid_width` may still be 0, because it is
only initialised under `render` (drw.c:286-287) -- that path does not
underflow,
which is why this is easy to miss.
Fix: Patch F6 -- apply the `invalid_width < w` guard unconditionally.
--------------------------------------------------------------------------------
F7 LOW -- Unchecked allocation size and getline() length (CWE-190, CWE-20)
--------------------------------------------------------------------------------
Where : dmenu.c:556-563
Impact: On 32-bit builds, `itemsiz * sizeof(*items)` can wrap, producing an
undersized allocation and a subsequent heap overflow. Needs roughly
1.3e8 stdin lines to reach, so realloc() almost certainly fails first;
treat this as hardening rather than an exploitable bug.
dmenu.c:556-563
for (i = 0; (len = getline(&line, &linesiz, stdin)) != -1; i++) {
if (i + 1 >= itemsiz) {
itemsiz += 256;
if (!(items = realloc(items, itemsiz * sizeof(*items))))
die("cannot realloc %zu bytes:", itemsiz *
sizeof(*items));
}
if (line[len - 1] == '\n')
line[len - 1] = '\0';
Two small issues:
* the multiplication is unchecked (use reallocarray(), or test against
SIZE_MAX / sizeof(*items));
* `line[len - 1]` assumes len >= 1. glibc's getline() never returns 0, so
this is currently safe -- but note stest.c:86 writes the same loop
defensively as `while ((n = getline(...)) > 0)`. The inconsistency is worth
removing.
Fix: Patch F7 -- add the overflow check and the `len > 0` guard.
--------------------------------------------------------------------------------
F8 LOW -- Typed secrets retained in memory and exposed to core dumps
(CWE-226, CWE-528)
--------------------------------------------------------------------------------
Where : dmenu.c:36 (`static char text[BUFSIZ]`), dmenu.c:99-112 (cleanup)
Impact: Confidentiality. dmenu is a standard passphrase prompt (passmenu and
friends pipe it into `pass`), so text[] routinely holds a secret. It is
a static buffer in .bss that is never cleared, and core dumps are not
disabled, so a crash -- including the one in F1, which is reachable
with the *same* keystrokes a user makes while typing -- can write the
passphrase to disk via the system core handler.
dmenu.c:36 static char text[BUFSIZ] = "";
dmenu.c:99-112 cleanup() ungrabs, frees the item list and closes the
display; text[] is left intact.
Pasted clipboard contents (paste(), dmenu.c:531) land in the same buffer, as do
the `match()` scratch copy (`char buf[sizeof text]`, dmenu.c:235) and the IM
buffer (`char buf[64]`, dmenu.c:326), neither of which is scrubbed either.
This is a design-level gap rather than a coding error, and a complete fix is
out of scope for a source patch (it would also want mlock() to keep the buffer
out of swap, and the scratch copies handled). Patch F8 does the two cheap,
high-value parts: disable core dumps with setrlimit(RLIMIT_CORE, 0) and scrub
text[] in cleanup() using a volatile-qualified loop the compiler is not
permitted to optimise away (a plain memset() here is routinely elided).
--------------------------------------------------------------------------------
F9 MEDIUM -- dmenu_path: empty PATH element and symlink-following cache
(CWE-426, CWE-59)
--------------------------------------------------------------------------------
Where : dmenu_path:6, dmenu_path:9-10
Impact: Untrusted search path leading to command execution, because dmenu_run
pipes whatever is selected straight into $SHELL (F15).
dmenu_path (full file, 13 lines)
cachedir="${XDG_CACHE_HOME:-"$HOME/.cache"}"
cache="$cachedir/dmenu_run"
[ ! -e "$cachedir" ] && mkdir -p "$cachedir"
IFS=:
if stest -dqr -n "$cache" $PATH; then
stest -flx $PATH | sort -u | tee "$cache"
else
cat "$cache"
fi
(a) Empty PATH element -> current directory (CWE-426). With IFS=: and $PATH
unquoted, a leading ':', a trailing ':' or an embedded '::' produces an
empty argument to stest. stest resolves an empty path as the current
directory, so `stest -flx $PATH` lists every executable in $PWD under its
bare name, mixed indistinguishably into the menu. Since the same empty
PATH element also makes the shell resolve bare names against $PWD, picking
one of those entries in dmenu_run executes the file from the current
directory. Dropping a file named e.g. `ls` or `firefox` into any directory
the user will `cd` into is enough. An empty PATH element is a common
shell-profile accident, not something the attacker has to arrange.
(b) Cache written through an unvalidated path (CWE-59). `tee "$cache"` opens
the path with O_CREAT and follows symlinks. If $cache can be pre-created as
a symlink, tee truncates and overwrites the target with the executable
list. The path is inside the user's own cache directory, so this needs
either a pre-existing hostile symlink or a writable/attacker-influenced
XDG_CACHE_HOME -- hence MEDIUM, not HIGH.
(c) `mkdir -p "$cachedir"` uses the ambient umask; with a lax umask (002/022)
the cache is group- or world-writable, which turns (b) from conditional
into trivial on a shared host. The `[ ! -e ]` test is also wrong for a
dangling symlink (-e follows the link and reports false), so the mkdir is
skipped in exactly the case that matters.
Fix: Patch F9 -- filter empty PATH elements before handing them to stest,
create the cache directory with mode 700, and write the cache via a fresh
temp file under `umask 077` plus an atomic rename instead of tee-ing through
a path an attacker may have staged.
--------------------------------------------------------------------------------
F10 INFO -- No build hardening (CWE-1104)
--------------------------------------------------------------------------------
Where : config.mk:27-29
CPPFLAGS = -D_DEFAULT_SOURCE ... $(XINERAMAFLAGS)
CFLAGS = -std=c99 -pedantic -Wall -Os $(INCS) $(CPPFLAGS)
LDFLAGS = $(LIBS)
Missing: -D_FORTIFY_SOURCE=2, -fstack-protector-strong, full RELRO
(-Wl,-z,relro,-z,now), PIE (-fPIE / -pie), and -Wextra. -Wall alone also does
not flag the signed/unsigned comparison at dmenu.c:704
(`for (i = 0; i < du && ...)`, int vs unsigned int) that -Wextra would catch.
Fix: Patch F10. Note -fstack-clash-protection is GCC 8+/recent-clang and
x86/aarch64 only; drop it if the build target is older or on another arch.
_FORTIFY_SOURCE requires optimisation, which -Os already supplies.
--------------------------------------------------------------------------------
F11 INFO -- Resource leaks (CWE-401)
--------------------------------------------------------------------------------
* dmenu.c:232-244 -- the static `tokv` token vector grows across match() calls
and is never freed. Bounded by the longest input ever typed, so it is a
bookkeeping leak, not a DoS.
* dmenu.c:693-697 -- `xim` and `xic` are never released; cleanup() calls
neither XDestroyIC() nor XCloseIM().
Both are reclaimed at process exit. No patch proposed.
--------------------------------------------------------------------------------
F12 INFO -- `-f` holds the keyboard grab while reading stdin (CWE-667)
--------------------------------------------------------------------------------
Where : dmenu.c:788-790
if (fast && !isatty(0)) {
grabkeyboard();
readstdin();
With -f the keyboard is grabbed *before* stdin is drained. An X11 keyboard
grab is display-wide, so if the process feeding stdin stalls or never closes
its end, no other client -- including the window manager -- receives key
events, and there is no keyboard path to recover. The user must kill dmenu
from another input device or another session. This is the documented intent of
-f (grab early so no keystrokes are lost) and the trade-off is inherent, so
this is recorded for awareness rather than as a defect. No patch proposed.
--------------------------------------------------------------------------------
F13 INFO -- Embed mode does not grab the keyboard (CWE-200)
--------------------------------------------------------------------------------
Where : dmenu.c:217-218
if (embed)
return;
With `-w <windowid>`, grabkeyboard() returns immediately. The keyboard grab is
the only thing that stops other X clients from reading keystrokes intended for
dmenu, so anything typed while embedded -- including a passphrase -- can be
observed by any client on the display. The skip is required for embedding to
work at all, and X11 offers no per-client input isolation regardless, so this
is a property of the platform. Worth documenting in dmenu.1 next to -w if the
tool is used for secrets. No patch proposed.
--------------------------------------------------------------------------------
F14 INFO -- stest: TOCTOU-prone permission tests (CWE-367, CWE-807)
--------------------------------------------------------------------------------
Where : stest.c:28-48
test() stats a path, then calls access(path, R_OK/W_OK/X_OK) on it, then prints
the name; the caller (dmenu_run) later executes the result. The file can change
between the stat, the access and the exec. access() is additionally the wrong
primitive for an authorisation decision in general. Here there is no privilege
boundary -- stest is not setuid and runs as the same user that will run the
command -- so the race has no security consequence in the shipped usage.
Flagged
only because the pattern is a false positive magnet in automated scanners. Also
note test() follows symlinks unless -h is given. No patch proposed.
--------------------------------------------------------------------------------
F15 INFO -- dmenu_run executes the selection (by design)
--------------------------------------------------------------------------------
Where : dmenu_run:2
dmenu_path | dmenu "$@" | ${SHELL:-"/bin/sh"} &
The selected or typed line is handed to a shell, unquoted and uninterpreted --
that is the entire purpose of dmenu_run. It is listed here because it is the
impact multiplier that turns F9 from "the menu shows the wrong entries" into
command execution, and because it means anything that can influence dmenu's
stdin or the dmenu_path cache is one Enter keypress from running code. No patch
proposed; this is intended behaviour.
================================================================================
PART 2 -- PROPOSED PATCHES (NOT APPLIED, NOT COMPILED)
================================================================================
Each patch below is independent and is written against the pristine tree at
commit 7175c48. Apply with:
patch -p1 < <one of the hunks below>
Line numbers in the @@ headers are exact for a single patch applied to the
clean tree; if you apply several, `patch` will report and absorb the offsets.
Recommended order if applying all: F1, F4, F2, F5, F3, F7, F8, F6, F9, F10.
--------------------------------------------------------------------------------
Patch F1 -- NULL-pointer dereference via the End key
--------------------------------------------------------------------------------
--- a/dmenu.c
+++ b/dmenu.c
@@ -171,7 +171,7 @@ drawmenu(void)
/* draw vertical list */
for (item = curr; item != next; item = item->right)
drawitem(item, x, y += bh, mw - x);
- } else if (matches) {
+ } else if (matches && curr) {
/* draw horizontal list */
x += inputw;
w = TEXTW("<");
@@ -438,9 +438,11 @@ keypress(XKeyEvent *ev)
/* jump to end of list and position items in reverse */
curr = matchend;
calcoffsets();
curr = prev;
calcoffsets();
- while (next && (curr = curr->right))
+ /* advance only while there is a next item: assigning
+ * curr = curr->right would leave curr NULL, and
+ * drawmenu() dereferences curr->left */
+ while (next && curr->right) {
+ curr = curr->right;
calcoffsets();
+ }
}
sel = matchend;
--------------------------------------------------------------------------------
Patch F2 -- Out-of-bounds read of the Xinerama screen array
--------------------------------------------------------------------------------
--- a/dmenu.c
+++ b/dmenu.c
@@ -640,6 +640,8 @@ setup(void)
i = 0;
if (parentwin == root && (info = XineramaQueryScreens(dpy, &n))) {
+ if (n < 1)
+ die("Xinerama reported no screens");
XGetInputFocus(dpy, &w, &di);
if (mon >= 0 && mon < n)
i = mon;
@@ -659,7 +661,11 @@ setup(void)
/* no focused window is on screen, so use pointer location
instead */
if (mon < 0 && !area && XQueryPointer(dpy, root, &dw, &dw, &x,
&y, &di, &di, &du))
for (i = 0; i < n; i++)
if (INTERSECT(x, y, 1, 1, info[i]) != 0)
break;
+ /* the pointer can sit inside the root window but outside every
+ * screen (gaps in a mixed-resolution layout), leaving i == n;
+ * fall back to the first screen rather than reading info[n] */
+ if (i >= n)
+ i = 0;
x = info[i].x_org;
--------------------------------------------------------------------------------
Patch F3 -- Validate -l/-m; clamp the menu height
--------------------------------------------------------------------------------
--- a/dmenu.c
+++ b/dmenu.c
@@ -1,9 +1,12 @@
/* See LICENSE file for copyright and license details. */
#include <ctype.h>
+#include <errno.h>
+#include <limits.h>
#include <locale.h>
+#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <strings.h>
#include <time.h>
#include <unistd.h>
@@ -635,6 +638,9 @@ setup(void)
/* calculate menu geometry */
bh = drw->fonts->h + 2;
- lines = MAX(lines, 0);
- mh = (lines + 1) * bh;
+ /* X11 window dimensions are CARD16, so clamp instead of overflowing.
+ * The previous MAX(lines, 0) was dead code: lines is unsigned. */
+ if (bh > 0 && lines > UINT16_MAX / (unsigned int)bh - 1)
+ lines = UINT16_MAX / (unsigned int)bh - 1;
+ mh = (lines + 1) * bh;
#ifdef XINERAMA
i = 0;
@@ -712,5 +718,18 @@
}
+static int
+parseint(const char *s, int min, int max, const char *opt)
+{
+ char *end;
+ long n;
+
+ errno = 0;
+ n = strtol(s, &end, 10);
+ if (errno || end == s || *end || n < min || n > max)
+ die("invalid value for %s: %s", opt, s);
+ return (int)n;
+}
+
static void
usage(void)
{
@@ -742,5 +761,5 @@ main(int argc, char *argv[])
else if (!strcmp(argv[i], "-l")) /* number of lines in
vertical list */
- lines = atoi(argv[++i]);
+ lines = parseint(argv[++i], 0, INT_MAX, "-l");
else if (!strcmp(argv[i], "-m"))
- mon = atoi(argv[++i]);
+ mon = parseint(argv[++i], 0, INT_MAX, "-m");
else if (!strcmp(argv[i], "-p")) /* adds prompt to left of
input field */
Behaviour note: -m no longer accepts a negative argument. `mon` defaults to -1
meaning "auto-detect", and dmenu.1 documents -m as taking a monitor number
starting at 0, so a negative -m was only ever a confusing way to ask for the
default. Omit -m to get auto-detection.
--------------------------------------------------------------------------------
Patch F4 -- Check XCreateIC()
--------------------------------------------------------------------------------
--- a/dmenu.c
+++ b/dmenu.c
@@ -692,8 +692,9 @@ setup(void)
/* input methods */
if ((xim = XOpenIM(dpy, NULL, NULL, NULL)) == NULL)
die("XOpenIM failed: could not open input device");
- xic = XCreateIC(xim, XNInputStyle, XIMPreeditNothing | XIMStatusNothing,
- XNClientWindow, win, XNFocusWindow, win, NULL);
+ if (!(xic = XCreateIC(xim, XNInputStyle, XIMPreeditNothing |
XIMStatusNothing,
+ XNClientWindow, win, XNFocusWindow, win, NULL)))
+ die("XCreateIC failed: could not obtain input context");
XMapRaised(dpy, win);
--------------------------------------------------------------------------------
Patch F5 -- Do not read buf[] when XmbLookupString() wrote nothing
--------------------------------------------------------------------------------
--- a/dmenu.c
+++ b/dmenu.c
@@ -325,7 +325,7 @@ keypress(XKeyEvent *ev)
{
- char buf[64];
+ char buf[64] = "";
int len;
KeySym ksym = NoSymbol;
Status status;
len = XmbLookupString(xic, ev, buf, sizeof buf, &ksym, &status);
@@ -414,7 +414,7 @@ keypress(XKeyEvent *ev)
switch(ksym) {
default:
insert:
- if (!iscntrl((unsigned char)*buf))
+ if (len > 0 && !iscntrl((unsigned char)*buf))
insert(buf, len);
break;
case XK_Delete:
--------------------------------------------------------------------------------
Patch F6 -- Guard the width budget against unsigned underflow
--------------------------------------------------------------------------------
--- a/drw.c
+++ b/drw.c
@@ -337,7 +337,7 @@ drw_text(Drw *drw, int x, int y, unsigned int w, unsigned
int h, unsigned int lp
x += ew;
w -= ew;
}
- if (utf8err && (!render || invalid_width < w)) {
+ if (utf8err && invalid_width < w) {
if (render)
drw_text(drw, x, y, w, h, 0, invalid, invert);
--------------------------------------------------------------------------------
Patch F7 -- Check the allocation size and the getline() length
--------------------------------------------------------------------------------
--- a/dmenu.c
+++ b/dmenu.c
@@ -1,9 +1,10 @@
/* See LICENSE file for copyright and license details. */
#include <ctype.h>
#include <locale.h>
+#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <strings.h>
#include <time.h>
#include <unistd.h>
@@ -556,8 +557,10 @@ readstdin(void)
for (i = 0; (len = getline(&line, &linesiz, stdin)) != -1; i++) {
if (i + 1 >= itemsiz) {
itemsiz += 256;
+ if (itemsiz > SIZE_MAX / sizeof(*items))
+ die("too many items");
if (!(items = realloc(items, itemsiz * sizeof(*items))))
die("cannot realloc %zu bytes:", itemsiz *
sizeof(*items));
}
- if (line[len - 1] == '\n')
+ if (len > 0 && line[len - 1] == '\n')
line[len - 1] = '\0';
--------------------------------------------------------------------------------
Patch F8 -- Keep typed secrets out of core dumps and scrub on exit
--------------------------------------------------------------------------------
--- a/dmenu.c
+++ b/dmenu.c
@@ -1,9 +1,10 @@
/* See LICENSE file for copyright and license details. */
#include <ctype.h>
#include <locale.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <strings.h>
+#include <sys/resource.h>
#include <time.h>
#include <unistd.h>
@@ -98,7 +99,15 @@
+/* zeroing the compiler is not permitted to optimise away; a plain memset()
+ * on a buffer that is never read again is routinely elided */
+static void
+scrub(volatile char *p, size_t n)
+{
+ while (n--)
+ *p++ = '\0';
+}
+
static void
cleanup(void)
{
size_t i;
XUngrabKeyboard(dpy, CurrentTime);
+ scrub(text, sizeof text);
for (i = 0; i < SchemeLast; i++)
@@ -721,6 +730,7 @@ main(int argc, char *argv[])
int
main(int argc, char *argv[])
{
XWindowAttributes wa;
int i, fast = 0;
+ struct rlimit nocore = { 0, 0 };
for (i = 1; i < argc; i++)
@@ -767,4 +777,8 @@ main(int argc, char *argv[])
+ /* dmenu is commonly used as a passphrase prompt (passmenu): keep the
+ * contents of text[] out of core dumps */
+ setrlimit(RLIMIT_CORE, &nocore);
+
if (!setlocale(LC_CTYPE, "") || !XSupportsLocale())
fputs("warning: no locale support\n", stderr);
if (!(dpy = XOpenDisplay(NULL)))
die("cannot open display");
Incomplete by design: this does not scrub match()'s `char buf[sizeof text]`
(dmenu.c:235) or keypress()'s `char buf[64]` (dmenu.c:326), and it does not
mlock() text[] against swap. Those need a larger change; see F8.
--------------------------------------------------------------------------------
Patch F9 -- dmenu_path: drop empty PATH elements, write the cache safely
--------------------------------------------------------------------------------
--- a/dmenu_path
+++ b/dmenu_path
@@ -1,13 +1,29 @@
#!/bin/sh
cachedir="${XDG_CACHE_HOME:-"$HOME/.cache"}"
cache="$cachedir/dmenu_run"
-[ ! -e "$cachedir" ] && mkdir -p "$cachedir"
+# -e follows symlinks, so a dangling symlink skipped the mkdir entirely.
+# Create unconditionally, and not group/world writable.
+mkdir -p -m 700 "$cachedir" || exit 1
+
+# An empty element in PATH (a leading or trailing ':', or '::') means "the
+# current directory". stest would then list every executable in $PWD under its
+# bare name, and dmenu_run pipes the selection into $SHELL, which resolves that
+# bare name against $PWD too. Drop empty elements.
+cleanpath=
+oldifs=$IFS
+IFS=:
+for d in $PATH; do
+ [ -n "$d" ] || continue
+ cleanpath="${cleanpath:+$cleanpath:}$d"
+done
+IFS=$oldifs
IFS=:
-if stest -dqr -n "$cache" $PATH; then
- stest -flx $PATH | sort -u | tee "$cache"
+if stest -dqr -n "$cache" $cleanpath; then
+ # write to a fresh private temp file and rename, rather than letting
+ # tee(1) follow a symlink planted at $cache
+ tmp="$cache.$$"
+ rm -f "$tmp"
+ (umask 077; stest -flx $cleanpath | sort -u > "$tmp") || exit 1
+ mv -f "$tmp" "$cache"
+ cat "$cache"
else
cat "$cache"
fi
--------------------------------------------------------------------------------
Patch F10 -- Build hardening
--------------------------------------------------------------------------------
--- a/config.mk
+++ b/config.mk
@@ -26,4 +26,9 @@
# flags
-CPPFLAGS = -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE=700
-D_POSIX_C_SOURCE=200809L -DVERSION=\"$(VERSION)\" $(XINERAMAFLAGS)
-CFLAGS = -std=c99 -pedantic -Wall -Os $(INCS) $(CPPFLAGS)
-LDFLAGS = $(LIBS)
+CPPFLAGS = -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE=700
-D_POSIX_C_SOURCE=200809L -DVERSION=\"$(VERSION)\" $(XINERAMAFLAGS)
-D_FORTIFY_SOURCE=2
+# -fstack-clash-protection is GCC 8+/recent clang on x86 and aarch64 only;
+# drop it on other toolchains or architectures.
+HARDENFLAGS = -fstack-protector-strong -fstack-clash-protection -fPIE \
+ -Wformat -Wformat-security
+CFLAGS = -std=c99 -pedantic -Wall -Wextra -Os $(HARDENFLAGS) $(INCS)
$(CPPFLAGS)
+LDFLAGS = -Wl,-z,relro,-z,now -Wl,--as-needed -pie $(LIBS)
-Wextra will also surface the pre-existing signed/unsigned comparison at
dmenu.c:704 (`for (i = 0; i < du && dws[i] != win; ++i)`, int vs unsigned int).
Harmless in practice, but expect the warning.
================================================================================
VERIFICATION STATUS
================================================================================
Done : Manual read of all 1,557 lines of C plus headers, both shell wrappers,
Makefile and config.mk. F1 confirmed by hand-tracing the item list
through calcoffsets()/keypress() for the one-item and two-item cases.
NOT done: nothing was built or executed. This is a Windows host with no X11
toolchain, so the code was not compiled, the patches were not applied,
and the F1/F2 crashes were not reproduced against a running binary.
Before acting on this report, apply the patches on a Linux/BSD host
and confirm at minimum:
1. the two F1 repro commands segfault before the patch and do not
after it;
2. `make` is warning-clean with Patch F10 applied (modulo the
dmenu.c:704 -Wextra warning noted above);
3. normal operation is unaffected: paging with Home/End/PgUp/PgDn in
both -l and horizontal modes, a multi-byte/IM input path, and
Ctrl-y paste.
Suggested follow-up tooling: build with -fsanitize=address,undefined and drive
it with a fuzzed stdin corpus (long lines, invalid UTF-8, embedded NULs, 100k+
items) -- that path is where F6 and F7 live and where anything I missed in
drw_text()'s multi-font fallback loop would show up.
================================================================================
st - SECURITY REVIEW FINDINGS
================================================================================
Target : c:\st (suckless st terminal emulator)
Reviewed : st.c (2710 lines), x.c (2108 lines), st.h, win.h, config.def.h
Date : 2026-09-11
Method : manual static source review of the C code only.
Nothing was built, run, or fuzzed; no dependency/toolchain
analysis. The proposed patch at the end is UNCOMPILED.
Repo state : unmodified by this review (findings written to this file only).
BASELINE / PROVENANCE
This tree is upstream st 0.9.3 plus exactly three commits; "git diff
0.9.3..HEAD" is 30 lines:
0723b7e Disable bracked paste in reset (upstream)
688f70a st: guard tsetdirt() against zero-sized terminal
04ce0d6 fix async-unsafe error paths in sigchld()
All three are clean. The sigchld() rework is a genuine improvement: it
replaces die()/vfprintf() in a signal handler with _exit() and saves and
restores errno, removing async-signal-unsafety from the handler.
Every finding below is therefore a defect in upstream st, not a local
regression, and should be reported upstream to suckless.
SUMMARY
# Severity Finding Location
- -------- ---------------------------------------------------
-------------
1 HIGH Heap OOB write in tsetchar(): wide-glyph dummy cell st.c:1221
shifted into column 0 by DCH -> term.line[y][-1] st.c:1274
2 HIGH Heap OOB read in draw() -> unbounded OOB read on st.c:2690
dc.col[] via attacker-influenced colour index x.c:1404
3 HIGH ttywriteraw() re-enters ttyread(), corrupting its st.c:822
static parse buffer state; buflen can go negative st.c:876
4 MEDIUM Selection coordinates never revalidated on resize st.c:2578
-> OOB reads in getsel()/selsnap(), leakable to fd 1 st.c:584
5 LOW size_t underflow in stty() -> stack overflow of cmd st.c:748
6 LOW Unchecked XGetWMHints() NULL deref (crash on BEL) x.c:1758
Findings 1 and 2 are reachable from terminal output alone - a 12-byte
payload in anything the terminal displays (cat of an untrusted file, ssh
output, a curl response, a tailed log). Fix those first.
================================================================================
1. HIGH - Heap out-of-bounds write in tsetchar()
================================================================================
LOCATION
st.c:1221-1224 (sink) tsetchar()
st.c:1274 (root cause) tdeletechar()
CODE
st.c:1216
if (term.line[y][x].mode & ATTR_WIDE) {
if (x+1 < term.col) { <- guarded
term.line[y][x+1].u = ' ';
term.line[y][x+1].mode &= ~ATTR_WDUMMY;
}
} else if (term.line[y][x].mode & ATTR_WDUMMY) {
term.line[y][x-1].u = ' '; <- NO x > 0 guard
term.line[y][x-1].mode &= ~ATTR_WIDE;
}
MECHANISM
The missing guard is safe only under the invariant "an ATTR_WDUMMY cell
always has its ATTR_WIDE partner at x-1, therefore x >= 1". tputc()
establishes that invariant (st.c:2527-2537, which only sets ATTR_WDUMMY
at gp[1] when term.c.x+1 < term.col, so never at column 0).
tdeletechar() breaks it. It shifts the row left with no wide-glyph
fix-up:
st.c:1267
LIMIT(n, 0, term.col - term.c.x);
dst = term.c.x; src = term.c.x + n; size = term.col - src;
memmove(&line[dst], &line[src], size * sizeof(Glyph));
tclearregion(term.col-n, term.c.y, term.col-1, term.c.y);
With term.c.x == 0 the cell that lands in column 0 is whatever was at
column n - which can be an ATTR_WDUMMY. The trailing tclearregion() only
scrubs the tail of the row, so nothing resets the flag.
TRACE (term.col = 80, cursor at 0,0)
step input resulting state
1 U+4E00 wcwidth==2 -> line[0][0]=ATTR_WIDE, line[0][1]=ATTR_WDUMMY,
term.c.x=2
2 CR tcontrolcode -> tmoveto(0, y) -> term.c.x=0
3 CSI P tdeletechar(1): dst=0, src=1
-> line[0][0] is now ATTR_WDUMMY with no partner
4 A tputc: width=1, no wrap, MODE_INSERT off, 0+1 <= 80,
so tsetchar('A', attr, 0, 0) -> term.line[0][-1]
Payload (12 bytes): printf '\u4e00\r\033[PA'
IMPACT
Rows are bare xmalloc(col * sizeof(Glyph)) allocations (st.c:2625) and
Glyph is 16 bytes {Rune u; ushort mode; uint32_t fg; uint32_t bg}, so
term.line[y][-1] straddles the 16 bytes immediately before the row:
row-16 .. row-13 .u = ' ' (4-byte write)
row-12 .. row-11 .mode &= ~ATTR_WIDE (2-byte read-modify-write)
On a normal glibc heap that is the chunk header of the row allocation
(prev_size field). This is a heap-metadata corruption primitive driven
entirely by displayed output, i.e. by any untrusted data reaching the
terminal. No user interaction, no non-default configuration.
FIX
Guard the sink AND repair the invariant at the source. See patch: the
"x > 0 &&" guard in tsetchar(), plus a new tfixwide() helper called at
the shift seams in tdeletechar() and tinsertblank().
================================================================================
2. HIGH - Heap OOB read in draw() -> unbounded OOB read on dc.col[]
================================================================================
LOCATION
st.c:2688-2697 draw()
x.c:1404, x.c:1415 xdrawglyphfontspecs()
CODE
st.c:2688
LIMIT(term.ocx, 0, term.col-1);
LIMIT(term.ocy, 0, term.row-1);
if (term.line[term.ocy][term.ocx].mode & ATTR_WDUMMY)
term.ocx--; /* 0 -> -1 */
if (term.line[term.c.y][cx].mode & ATTR_WDUMMY)
cx--; /* 0 -> -1 */
...
xdrawcursor(cx, term.c.y, term.line[term.c.y][cx],
term.ocx, term.ocy, term.line[term.ocy][term.ocx]);
MECHANISM
Same root cause as finding 1 (ATTR_WDUMMY at column 0). The LIMIT() calls
run BEFORE the decrements, so they do not constrain the result: index 0
becomes -1 and a whole Glyph is read from before the row. Steps 1-3 of
the trace in finding 1 are sufficient; the 'A' is not needed.
The out-of-bounds Glyph then flows into rendering. The cursor glyph g is
sanitised (x.c:1542-1558 overwrite g.fg/g.bg with configured defaults),
but the old-cursor glyph og is NOT:
x.c:1404
fg = &dc.col[base.fg]; /* base.fg = 4 bytes of heap metadata */
x.c:1415
bg = &dc.col[base.bg];
dc.collen is 260 - LEN(colorname) is 260 because of the "[255] = 0"
designator at config.def.h:118, so MAX(260, 256) = 260. A chunk-size
field value such as 0x511 indexes roughly a thousand Color entries past
the array, and XftDrawRect() then dereferences it. IS_TRUECOL() only
diverts values with bit 24 set, so most header bit patterns take the
indexing path.
IMPACT
Wild read far outside dc.col[] -> crash, or a rendered colour derived
from out-of-bounds heap memory. Same trigger conditions as finding 1.
FIX
Move the bounds test into the conditions: "term.ocx > 0 &&" / "cx > 0 &&".
(cx is term.c.x, already within [0, term.col-1] by the tmoveto()
invariant, so the guard is purely about the decrement.)
================================================================================
3. HIGH - ttywriteraw() re-enters ttyread() and corrupts its static state
================================================================================
LOCATION
st.c:822-846 ttyread()
st.c:876-931 ttywriteraw() (nested calls at st.c:916 and st.c:925)
CODE
st.c:822
static char buf[BUFSIZ];
static int buflen = 0;
ret = read(cmdfd, buf+buflen, LEN(buf)-buflen);
...
buflen += ret;
written = twrite(buf, buflen, 0);
buflen -= written;
if (buflen > 0)
memmove(buf, buf + written, buflen);
MECHANISM
ttyread() keeps its parse buffer in statics and is not reentrant, but
ttywriteraw() calls it from inside its write loop, and twrite() reaches
ttywrite() through every query-response handler:
st.c:1660 DA (CSI c)
st.c:1793 DSR (CSI 5n)
st.c:1798 CPR (CSI 6n)
st.c:2290 DECID (0x9a)
st.c:2362 DECID (ESC Z)
st.c:1888 osc_color_response()
So the call graph contains:
ttyread -> twrite -> tputc -> csihandle -> ttywrite -> ttywriteraw
-> ttyread (nested)
Three consequences, all while the outer twrite() is still iterating buf:
a) The nested twrite(buf, buflen) restarts at index 0, RE-EXECUTING bytes
the outer frame already consumed. That duplicates output and re-emits
the same queries, so the recursion compounds rather than terminating.
b) The nested frame's memmove() shifts the buffer out from under the
outer frame's loop index.
c) The outer frame then computes "buflen -= written" against the nested
frame's already-updated buflen, so buflen can go NEGATIVE. The
"if (buflen > 0)" test skips the memmove but leaves the negative value
in the static. The next ttyread() then evaluates
read(cmdfd, buf + buflen, LEN(buf) - buflen)
with a destination BEFORE buf and a length LARGER than BUFSIZ: an
attacker-controlled out-of-bounds write into the preceding BSS.
TRIGGER
Ordinary conditions: a child that writes without draining stdin (cat, a
chatty build, a tight output loop) emitting repeated CSI c. Each query
makes st write 5 bytes into the pty that nobody reads; once the pty input
buffer fills, write() blocks, ttywriteraw() loops, cmdfd is readable, and
it recurses.
IMPACT
Stack exhaustion is the certain outcome (unbounded compounding
recursion). The buflen desync in (c) is the memory-corruption path.
FIX
A reentrancy flag so a nested ttyread() only buffers and never runs
twrite(); this also caps recursion depth at 2, removing the stack
exhaustion. A "buffer already full" early-out is required alongside it,
because read(fd, p, 0) returns 0 and st.c:834 treats 0 as EOF -> exit(0);
it returns 1 rather than 0 so that ttywriteraw()'s "lim" stays non-zero
and the write loop keeps making forward progress.
The proper long-term fix is to stop writing query responses synchronously
from inside twrite() and queue them for the run() loop instead.
Note the arithmetic stays correct with the guard: after a nested call
appends k bytes at the old offset,
buflen = (old_buflen + k) - written
is exactly the unparsed remainder plus the new bytes, contiguous at
offset "written", and buf + written + buflen == buf + old_buflen + k,
which is within bounds.
================================================================================
4. MEDIUM - Selection coordinates are never revalidated on resize
================================================================================
LOCATION
st.c:2578-2657 tresize()
st.c:584-637 getsel() (indexes term.line[y] at st.c:605, 608, 611)
st.c:512-582 selsnap() (indexes term.line[*y][*x] at st.c:524)
MECHANISM
tresize() reallocates term.line, term.alt, every row, term.dirty and
term.tabs, but touches nothing in "sel". The only thing that could clear
a selection is selclear() via the two tclearregion() calls at
st.c:2647-2652 - and when rows shrink, tclearregion(0, minrow, col-1,
row-1) has y1 > y2, gets swapped and LIMITed, and ends up clearing only
row row-1. The discarded rows are never visited. A selection anchored
below the new height, or right of the new width, survives with stale
coordinates.
TWO SINKS
a) getsel() indexes term.line[y] for y in [sel.nb.y, sel.ne.y]. With
nb.y >= term.row that reads past the reallocated Line* array and
dereferences a wild pointer - and whatever it reads is written to the
print fd. Reachable FROM THE STREAM, not just from a keybinding:
CSI 2 i -> tdumpsel() (st.c:1648), and iofd defaults to 1 (st.c:226),
so tprinter() writes to stdout unconditionally.
b) selsnap() does prevgp = &term.line[*y][*x] with *x from sel.ob.x.
After a column shrink, a double-click selection (sel.snap == SNAP_WORD)
whose ob.x exceeds the new term.col reads past the end of the
reallocated row. Reached via selscroll() -> selnormalize()
(st.c:1120) on ordinary scrolling output.
Sequence for (a): select text low in a tall window; shrink the window or
zoom the font (zoomabs -> cresize -> tresize); then any displayed output
containing CSI 2 i dumps out-of-bounds heap memory to stdout.
IMPACT
Out-of-bounds read / wild pointer dereference, with an information
disclosure path to fd 1. Requires a resize between the selection and the
trigger, hence MEDIUM rather than HIGH.
FIX
Clamp or drop the selection in tresize(). The patch drops it (selclear())
when either endpoint no longer fits, which is the conservative choice.
================================================================================
5. LOW - size_t underflow in stty() -> stack buffer overflow
================================================================================
LOCATION
st.c:737-758 stty()
CODE
st.c:739
char cmd[_POSIX_ARG_MAX], **p, *q, *s;
size_t n, siz;
...
siz = sizeof(cmd) - n;
for (p = args; p && (s = *p); ++p) {
if ((n = strlen(s)) > siz-1) <- siz is size_t
die("stty parameter length too long");
*q++ = ' ';
memcpy(q, s, n);
q += n;
siz -= n + 1;
}
*q = 0;
MECHANISM
The test admits n == siz-1, which drives siz to exactly 0. Bytes written
are then exactly sizeof(cmd) and q == cmd + sizeof(cmd), so the final
NUL store is already a one-byte overflow. With one more argument, siz-1
wraps to SIZE_MAX, the guard can never fire, and the memcpy() runs
unbounded past cmd. (Invariant: bytes written == sizeof(cmd) - siz.)
IMPACT
Stack buffer overflow, but reachable only via "st -l <line> <stty_args>",
so the input is the invoking user's own command line. st is not setuid,
so no privilege boundary is crossed - genuine bug, low severity.
FIX
Test "n + 1 >= siz", reserving both the separator and the trailing NUL.
================================================================================
6. LOW - Unchecked XGetWMHints() NULL dereference
================================================================================
LOCATION
x.c:1756-1763 xseturgency()
CODE
XWMHints *h = XGetWMHints(xw.dpy, xw.win);
MODBIT(h->flags, add, XUrgencyHint);
MECHANISM
XGetWMHints() returns NULL when the window has no WM_HINTS property.
xbell() calls xseturgency(1) whenever the window is unfocused (x.c:1768),
so a bare BEL in the output stream dereferences NULL.
IMPACT
Remote-triggerable crash (denial of service). No memory corruption.
FIX
NULL check with an XAllocWMHints() fallback.
================================================================================
LOWER-PRIORITY NOTES (no patch proposed)
================================================================================
N1 base64dec() silently accepts invalid input - st.c:361
base64_digits[] maps every unlisted byte to 0, indistinguishable from a
legitimate 'A', so corrupt data is decoded rather than rejected. The
ALLOCATION math is sound: in_len/4*3+1 exactly covers the worst case,
because base64dec_getc() consumes at least four source bytes per three
output bytes and non-printable skipping only reduces output. Reachable
only with allowwindowops enabled, which is 0 by default
(config.def.h:48).
N2 getsel() bufsize is int - st.c:594
(term.col+1) * (sel.ne.y-sel.nb.y+1) * UTF_SIZ overflows above roughly
5e8 cells. Not reachable through any real window geometry; widening to
size_t is free hardening.
N3 tdumpline() forms &bp[-1] - st.c:2117
When tlinelen() returns 0, end = &bp[-1]. Technically UB; benign in
practice because the loop guard rejects it immediately.
N4 "width" is left uninitialised in tputc() - st.c:2404-2411
When u >= 127, MODE_UTF8 is set and ISCONTROL(u) is true (e.g. u ==
0x7f), the else branch never assigns width. Currently dead: every path
that reads width is guarded by control == 0. One refactor away from
being live.
N5 tabspaces == 0 in config.h would hang st - st.c:1030, st.c:2634
"i += tabspaces" and "bp += tabspaces" loop forever. Configuration
robustness only; not attacker-controlled.
================================================================================
WHAT IS ALREADY HARDENED (verified present, no action needed)
================================================================================
* kpress(): the long-composed-input overflow fix is present, including the
XBufferOverflow check (x.c:1855-1861, upstream e5e9598).
* CSI b (REP) argument is capped at 65535 (st.c:1663, upstream 95f22c5).
* dc.col bounds checks in xgetcolor() and xsetcolorname() are present
(x.c:821, x.c:836, upstream a6bbc0c); dc.collen == 260 covers every
index tsetattr()/tdefcolor() can produce (indexed <= 255, truecolour
diverted by bit 24, defaults 256-259).
* csiparse()/strparse() respect their ESC_ARG_SIZ / STR_ARG_SIZ caps
(st.c:1163, st.c:2018).
* csiescseq.buf and the growable strescseq.buf both correctly reserve room
for the terminating NUL written by csiparse()/strparse()
(st.c:2474-2477 caps len at sizeof(buf)-1; st.c:2430 grows when
len+len >= siz, with a SIZE_MAX guard at st.c:2444).
* tclearregion(), tmoveto(), tsetscroll(), tscrollup(), tscrolldown(),
tinsertblank(), tdeletechar() and tputtab() all LIMIT() every
attacker-supplied CSI argument, including negatives.
* ttyread()'s UTF-8 continuation handling cannot stall: utf8decode()
returns 0 only when clen <= 3, so a full buffer always makes progress.
* OSC 52 clipboard writes are disabled by default (allowwindowops = 0).
* sigchld() is async-signal-safe after commit 04ce0d6.
================================================================================
PROPOSED PATCH
================================================================================
Covers findings 1-6. Extract the diff below to a file and apply from the
repository root:
git apply -p1 st-security-fixes.patch
STATUS: verified to apply cleanly to this tree with "git apply --check -p1".
NOT COMPILED and NOT TESTED - no toolchain analysis was in scope for this
review. Review and build before committing. Brace balance was checked
(st.c 300 -> 305 pairs, x.c unchanged).
--------------------------------------------------------------------------------
--- a/st.c
+++ b/st.c
@@ -176,6 +176,7 @@
static void tcursor(int);
static void tdeletechar(int);
static void tdeleteline(int);
+static void tfixwide(int, int);
static void tinsertblank(int);
static void tinsertblankline(int);
static int tlinelen(int);
@@ -745,7 +746,12 @@
q = cmd + n;
siz = sizeof(cmd) - n;
for (p = args; p && (s = *p); ++p) {
- if ((n = strlen(s)) > siz-1)
+ /*
+ * siz is unsigned, and "n > siz-1" admits n == siz-1, which
+ * drives siz to 0 so the next iteration compares against
+ * SIZE_MAX. Reserve the separator and the trailing NUL.
+ */
+ if ((n = strlen(s)) + 1 >= siz)
die("stty parameter length too long\n");
*q++ = ' ';
memcpy(q, s, n);
@@ -824,8 +830,22 @@
{
static char buf[BUFSIZ];
static int buflen = 0;
+ static int parsing = 0;
int ret, written;
+ /*
+ * ttywriteraw() re-enters ttyread() while twrite() is still on the
+ * stack: a DA/DSR/OSC-colour response fills the pty, write() blocks,
+ * and the drain path calls back in here. buf and buflen are static,
+ * so a nested twrite() replays bytes the outer frame has already
+ * consumed and desynchronises buflen -- it can end up negative, which
+ * turns the read() below into an out-of-bounds write before buf, and
+ * the replay makes the recursion compound instead of terminating.
+ * A nested call therefore only buffers; the outer frame parses.
+ */
+ if (buflen >= (int)LEN(buf))
+ return 1; /* full: keep ttywriteraw() progressing */
+
/* append read bytes to unprocessed bytes */
ret = read(cmdfd, buf+buflen, LEN(buf)-buflen);
@@ -836,11 +856,15 @@
die("couldn't read from shell: %s\n", strerror(errno));
default:
buflen += ret;
+ if (parsing)
+ return ret;
+ parsing = 1;
written = twrite(buf, buflen, 0);
buflen -= written;
/* keep any incomplete UTF-8 byte sequence for the next call */
if (buflen > 0)
memmove(buf, buf + written, buflen);
+ parsing = 0;
return ret;
}
}
@@ -1218,7 +1242,7 @@
term.line[y][x+1].u = ' ';
term.line[y][x+1].mode &= ~ATTR_WDUMMY;
}
- } else if (term.line[y][x].mode & ATTR_WDUMMY) {
+ } else if (x > 0 && (term.line[y][x].mode & ATTR_WDUMMY)) {
term.line[y][x-1].u = ' ';
term.line[y][x-1].mode &= ~ATTR_WIDE;
}
@@ -1258,6 +1282,37 @@
}
}
+/*
+ * A wide glyph occupies two cells: ATTR_WIDE at x and ATTR_WDUMMY at x+1.
+ * Shifting a row horizontally can separate the halves, and an ATTR_WDUMMY
+ * cell with no ATTR_WIDE partner at x-1 breaks the invariant that
+ * tsetchar() and draw() rely on -- at x == 0 both of them then index
+ * term.line[y][-1]. Turn any orphaned half back into a blank.
+ */
+void
+tfixwide(int y, int x)
+{
+ Glyph *line;
+
+ if (!BETWEEN(x, 0, term.col - 1))
+ return;
+
+ line = term.line[y];
+ if (line[x].mode & ATTR_WDUMMY) {
+ if (x == 0 || !(line[x-1].mode & ATTR_WIDE)) {
+ line[x].u = ' ';
+ line[x].mode &= ~ATTR_WDUMMY;
+ term.dirty[y] = 1;
+ }
+ } else if (line[x].mode & ATTR_WIDE) {
+ if (x + 1 >= term.col || !(line[x+1].mode & ATTR_WDUMMY)) {
+ line[x].u = ' ';
+ line[x].mode &= ~ATTR_WIDE;
+ term.dirty[y] = 1;
+ }
+ }
+}
+
void
tdeletechar(int n)
{
@@ -1273,6 +1328,10 @@
memmove(&line[dst], &line[src], size * sizeof(Glyph));
tclearregion(term.col-n, term.c.y, term.col-1, term.c.y);
+ /* the shift can strand one half of a wide glyph at the seams */
+ tfixwide(term.c.y, dst - 1);
+ tfixwide(term.c.y, dst);
+ tfixwide(term.c.y, term.col - n - 1);
}
void
@@ -1290,6 +1349,10 @@
memmove(&line[dst], &line[src], size * sizeof(Glyph));
tclearregion(src, term.c.y, dst - 1, term.c.y);
+ /* the shift can strand one half of a wide glyph at the seams */
+ tfixwide(term.c.y, src - 1);
+ tfixwide(term.c.y, dst);
+ tfixwide(term.c.y, term.col - 1);
}
void
@@ -2637,6 +2700,17 @@
/* update terminal size */
term.col = col;
term.row = row;
+ /*
+ * The selection survives a resize, but the rows and the Line array
+ * it indexes have just been reallocated and nothing else
+ * revalidates it -- the tclearregion() calls below never visit the
+ * discarded rows. Drop a selection that no longer fits instead of
+ * letting getsel()/selsnap() index out of bounds later.
+ */
+ if (sel.ob.x != -1 &&
+ (!BETWEEN(sel.ob.x, 0, col-1) || !BETWEEN(sel.ob.y, 0, row-1) ||
+ !BETWEEN(sel.oe.x, 0, col-1) || !BETWEEN(sel.oe.y, 0, row-1)))
+ selclear();
/* reset scrolling region */
tsetscroll(0, row-1);
/* make use of the LIMIT in tmoveto */
@@ -2687,9 +2761,9 @@
/* adjust cursor position */
LIMIT(term.ocx, 0, term.col-1);
LIMIT(term.ocy, 0, term.row-1);
- if (term.line[term.ocy][term.ocx].mode & ATTR_WDUMMY)
+ if (term.ocx > 0 && (term.line[term.ocy][term.ocx].mode & ATTR_WDUMMY))
term.ocx--;
- if (term.line[term.c.y][cx].mode & ATTR_WDUMMY)
+ if (cx > 0 && (term.line[term.c.y][cx].mode & ATTR_WDUMMY))
cx--;
drawregion(0, 0, term.col, term.row);
--- a/x.c
+++ b/x.c
@@ -1757,6 +1757,10 @@
{
XWMHints *h = XGetWMHints(xw.dpy, xw.win);
+ /* no WM_HINTS property set yet -> XGetWMHints() returns NULL */
+ if (h == NULL && (h = XAllocWMHints()) == NULL)
+ return;
+
MODBIT(h->flags, add, XUrgencyHint);
XSetWMHints(xw.dpy, xw.win, h);
XFree(h);
--------------------------------------------------------------------------------
end of findings
--
Kind regards,
Hiltjo
