emersion / emersion/mako

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

Ouverte Adaptée aux débutants
#653 0 commentaires 1 réaction 0 personnes assignées Voir sur GitHub
Langage dominant
C
Étoiles
3.3k
Forks
173
Métriques de merge des PR
Aucune PR mergée en 30 j

Description

## 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.

Guide de contribution

Ouvrir le guide de contribution

Piste de recherche

Commencez dans dbus/xdg.c au niveau de handle_notify(), en particulier dans le chemin de remplacement de balise autour des lignes 362-379, et reproduisez l’échec avec les commandes notify-send fournies. Vérifiez qu’une notification qui correspond à elle-même n’est pas détruite, puis réexécutez les cas de remplacement et de balise seule listés afin de confirmer que le daemon reste actif et que les identifiants des notifications restent stables.

Rédigé par le modèle d'indexation à partir du texte de l'issue.

Évaluation

Stack technique
c
Domaine
desktop
Type d'issue
Bug
Difficulté
2/5
Temps estimé
1-3 heures
Activité
Calme
Clarté
Clairement spécifiée
Accessibilité débutants
72/100

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.