emersion / emersion/mako

Use-after-free when `replaces_id` and a synchronous tag name the same notification

Aperta Adatta ai principianti
#653 0 commenti 1 reazione 0 assegnatari Vedi su GitHub
Lingua principale
C
Stelle
3.3k
Fork
173
Metriche di merge delle PR
Nessuna PR unita negli ultimi 30g

Descrizione

## Summary

If a `Notify` call carries **both** a `replaces_id` pointing at a still-live notification **and** an `x-canonical-private-synchronous` / `x-dunst-stack-tag` hint, mako frees the notification it is currently populating and then keeps using it. The result is a use-after-free that reliably aborts the daemon inside `apply_each_criteria()`.

This is not limited to exotic configs — it reproduces with the stock configuration, because the global criteria always specifies `button_bindings.left`, whose `action_name` is a heap string.

## Environment

- mako 1.11.0 (Arch Linux `mako 1.11.0-1`)
- glibc 2.44, niri / wlroots, `libnotify` `notify-send`
- Also present in `master` — `dbus/xdg.c:362-371` is unchanged, and the code has been this way since the tag feature landed in 4a30dfb (v1.6).

## Steps to reproduce

With the **default config** (no `~/.config/mako/config` needed):

```sh
ID=$(notify-send --app-name=test --expire-time=30000 --print-id \
--hint=string:x-canonical-private-synchronous:mytag "a" "first")

notify-send --app-name=test --expire-time=30000 \
--hint=string:x-canonical-private-synchronous:mytag \
--replace-id=$ID "a" "second"
```

The second call must arrive while the first notification is still on screen.

**Expected:** the notification is replaced.

**Actual:** mako aborts.

```
Failed to show notification: GDBus.Error:org.freedesktop.DBus.Error.NoReply: Remote peer disconnected
```

```
mako[485361]: free(): chunks in smallbin corrupted
systemd-coredump: Process 485361 (mako) of user 1000 dumped core.
```

The exact glibc diagnostic varies with heap state; I have captured both
`free(): double free detected in tcache 2` and `free(): chunks in smallbin corrupted`
from the same code path.

## Root cause

In `handle_notify()` (`dbus/xdg.c`), a `replaces_id` naming a live notification causes mako to **reuse that notification object**, which is still linked into `state->notifications`:

```c
if (replaces_id > 0) {
notif = get_notification(state, replaces_id); /* xdg.c:107 */
}
if (notif) {
reset_notification(notif); /* xdg.c:112 — style is NOT reset */
}
```

Further down, once `notif->tag` has been parsed from the hints, the tag-replacement block searches the same list:

```c
if (notif->tag) {
struct mako_notification *replace_notif =
get_tagged_notification(state, notif->tag, app_name); /* xdg.c:364 */
if (replace_notif) {
notif->id = replace_notif->id;
wl_list_insert(&replace_notif->link, ¬if->link);
destroy_notification(replace_notif); /* xdg.c:368 */
replaces_id = notif->id;
}
}
```

Because `notif` is itself in `state->notifications` and carries that exact tag and app name, `get_tagged_notification()` returns **`notif` itself**. So:

1. `wl_list_insert(¬if->link, ¬if->link)` self-links the list entry.
2. `destroy_notification(notif)` runs `finish_style()` — freeing `button_bindings.*.command` and `.action_name` — and then `free(notif)`.
3. Execution continues on the freed `notif` into `apply_each_criteria()` (`xdg.c:385`).
4. `apply_style()` → `copy_binding()` → `finish_binding()` frees those binding strings a second time.

`finish_binding()` frees without clearing, which is fine on its own but leaves nothing to catch the reuse:

```c
static void finish_binding(struct mako_binding *binding) {
free(binding->command); /* config.c:147 */
free(binding->action_name); /* config.c:148 */
}
```

### Backtrace (stock config)

```
#10 __GI___libc_free (mem=) at malloc.c:3184
#11 finish_binding (binding=0x556004e8f520) at config.c:148
#12 copy_binding (dst=0x556004e8f520, src=0x556004e838f0) at config.c:165
#13 apply_style (target=, style=0x556004e83800) at config.c:381
#14 apply_each_criteria (criteria_list=0x7ffdc3a060e0, notif=0x556004e8f410) at criteria.c:435
#15 handle_notify (msg=0x5560051797a0, ...) at dbus/xdg.c:385
#16 method_callbacks_run (...) at sd-bus/bus-objects.c:522
```

This one is worth highlighting — it is not a harmless double free of a dead pointer. The stale chunk had already been **recycled into another live allocation** by the time it was freed again:

```
(gdb) p *dst
$1 = {action = MAKO_BINDING_INVOKE_ACTION, command = 0x0,
action_name = 0x5560051eb3e0 "monospace 10"} <-- freed "default" chunk, now the font string
(gdb) p *src
$2 = {action = MAKO_BINDING_INVOKE_ACTION, command = 0x0,
action_name = 0x556004e83ad0 "default"}
```

`dst->action_name` was `strdup("default")` from `init_default_style()`; after the premature `destroy_notification()` the chunk was reallocated for the font string `"monospace 10"`, which `copy_binding()` then frees while it is still in use.

With a config that sets an `exec` binding, the same crash lands one line earlier on `free(binding->command)` (`config.c:147`), and the dead `notif`'s `app_name` / `summary` / `tag` all read back as mangled tcache pointers.

## Suggested fix

A notification cannot meaningfully replace itself — when `replace_notif == notif`, the desired end state already holds: the id is correct and the list position is unchanged. Skipping the block is sufficient:

```diff
if (notif->tag) {
// Find and replace the existing notfication with a matching tag
struct mako_notification *replace_notif = get_tagged_notification(state, notif->tag, app_name);
- if (replace_notif) {
+ if (replace_notif && replace_notif != notif) {
notif->id = replace_notif->id;
wl_list_insert(&replace_notif->link, ¬if->link);
destroy_notification(replace_notif);
replaces_id = notif->id;
}
}
```

`replaces_id` is left equal to `notif->id` in that case, so the `if (replaces_id != notif->id)` guard at `xdg.c:379` correctly continues to skip `insert_notification()`.

Optionally, having `finish_binding()` null the pointers after freeing would make this class of bug fail loudly at the second use rather than corrupting the heap:

```c
static void finish_binding(struct mako_binding *binding) {
free(binding->command);
free(binding->action_name);
binding->command = NULL;
binding->action_name = NULL;
}
```

## Verification of the fix

Built 1.11.0 with only the guard above applied and re-ran, against the same config:

| Case | Result |
| --- | --- |
| `--replace-id` + matching tag (the repro) | no crash, id preserved |
| 5 rapid rounds, both mechanisms | no crash, id stable, no stacking |
| tag-only replacement | still replaces, id stable |
| `--replace-id` only, no tag | still replaces, id stable |

## Reachability

For completeness, since a use-after-free invites the question — I could not find a path that crosses a privilege boundary. Triggering it requires a caller that sets a synchronous tag hint **and** a `replaces_id` naming a live notification, and the obvious candidates for a less-privileged caller do not:

- **`org.freedesktop.portal.Notification`** — does forward `replaces_id`, but constructs a fixed hint set (`desktop-entry`, `urgency`) and does not pass arbitrary hints through, so a sandboxed app cannot inject the tag. Confirmed on the wire and by three back-to-back `AddNotification` calls with an identical id: no crash. (`notif->tag` is `""` there, which `get_tagged_notification()` already skips via its `strlen() != 0` check.)
- **Firefox 153** — reaches the daemon through libnotify directly, and neither `libxul.so` nor `libnotify.so.4` contains either hint name, so a web page's JS notification `tag` is not mapped onto the fdo synchronous hints.

So in practice this needs a cooperating same-user client, which makes it a robustness bug rather than a security issue — filing it accordingly rather than through a security contact. It is still a real use-after-free that frees a live allocation, and the affected strings are attacker-influenced in length and content, so it seemed worth writing up carefully.

## Notes

Clients that remember a notification id *and* set a stable per-application tag hit this whenever an update arrives before the previous popup expires — a natural pattern for progress/status notifications. Sending only the tag is a working client-side workaround, since mako already carries the old id across a tag replacement.

Guida per i contributori

Apri la guida per i contributori

Direzione di ricerca

Start in dbus/xdg.c at handle_notify(), especially the tag-replacement path around lines 362-379, and reproduce the failure with the supplied notify-send commands. Verify that a notification matching itself is not destroyed, then rerun the listed replacement and tag-only cases to confirm the daemon stays alive and notification IDs remain stable.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Valutazione

Stack tecnologico
c
Ambito
desktop
Tipo di issue
Bug
Difficoltà
2/5
Tempo stimato
1-3 ore
Stato di attività
Tranquilla
Chiarezza
Specificata chiaramente
Idoneità per principianti
72/100

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.