Fix: issue #112 follow-ups — OTR strip, presence UAF, OMEMO load #130

Manually merged
jabber.developer merged 6 commits from fix/issue-112-followups into master 2026-06-20 10:30:41 +00:00
Collaborator

Correctness / safety fixes

  • Item 8 (OTR) — strip the whitespace tag by shifting the full message tail (incl. NUL), not tag_length bytes; previously the body was duplicated for messages longer than the tag.
  • Item 9 (xmpp/presence) — snapshot resource fields before connection_add_available_resource() takes ownership, removing a latent use-after-free in the own-presence path.
  • Item 10 (omemo) — propagate _omemo_finalize_identity_load failure on connect (log + cons_show_error + stop) instead of leaving OMEMO silently unavailable.
  • Item 1 (xmpp/message) — gate XEP-0359 stanza-id dedup on disco urn:xmpp:sid:0 (checks the by JID or its domain, since that's what profanity disco's on connect); falls back to no-dedup when caps are unknown. Covered by two functional tests (trusted vs untrusted server).
  • Item 2 (config/accounts) — narrow the group-name sanitizer to the characters GKeyFile actually forbids in headers ([ ] \n \r); = and # are valid there and stay, keeping read/write symmetric.

Build hardening (item 5) — Pikaur-safe

Re-introduces what 5459e78e8 attempted, in a form that coexists with Arch/Pikaur (the conflict that forced its revert in 0722dc9e3):

  • -D_FORTIFY_SOURCE=2 only when env ($CFLAGS/$CPPFLAGS) doesn't already set one and not under --coverage.
  • glib/gio CFLAGS routed through -isystem.
  • --enable-sanitizers (ASan+UBSan; unsigned-integer-overflow probed, skipped on gcc), off by default.
  • --enable-hardening gates -Wnull-dereference, off by default.
  • -Wsign-compare promoted from -Wno- to an active probed warning.

Plus the sign-conversion sites that surfaced as safe (pointer-diff → gsize via new g_diff_to_gsize helper, non-negative casts), and /vcard remove now validates its index through strtoi_range so a negative argument can't become a huge unsigned index.

Verified: default Debian build, --enable-sanitizers build, and an Arch build with /etc/makepkg.conf injected (Pikaur simulation) — all green, including the functional-test and unit-test binaries.

Out of scope / tracked separately

  • Items 7 + 11 (_migrate_to_v3 dedupe/FK) → #128, PR #129.
  • Item 3 (OTR interop matrix) — manual QA.
  • Full -Wconversion cleanup (~180 mostly-mechanical sign-conversion sites) — deferred; not enabled here because it would block -Werror builds.

Items 4 and 6 were resolved by investigation (no code change needed).

Refs #112
Fixes #112
Fixes #109

### Correctness / safety fixes - **Item 8 (OTR)** — strip the whitespace tag by shifting the full message tail (incl. NUL), not `tag_length` bytes; previously the body was duplicated for messages longer than the tag. - **Item 9 (xmpp/presence)** — snapshot `resource` fields before `connection_add_available_resource()` takes ownership, removing a latent use-after-free in the own-presence path. - **Item 10 (omemo)** — propagate `_omemo_finalize_identity_load` failure on connect (log + `cons_show_error` + stop) instead of leaving OMEMO silently unavailable. - **Item 1 (xmpp/message)** — gate XEP-0359 stanza-id dedup on disco `urn:xmpp:sid:0` (checks the `by` JID or its domain, since that's what profanity disco's on connect); falls back to no-dedup when caps are unknown. Covered by two functional tests (trusted vs untrusted server). - **Item 2 (config/accounts)** — narrow the group-name sanitizer to the characters GKeyFile actually forbids in headers (`[ ] \n \r`); `=` and `#` are valid there and stay, keeping read/write symmetric. ### Build hardening (item 5) — Pikaur-safe Re-introduces what `5459e78e8` attempted, in a form that coexists with Arch/Pikaur (the conflict that forced its revert in `0722dc9e3`): - `-D_FORTIFY_SOURCE=2` only when env (`$CFLAGS`/`$CPPFLAGS`) doesn't already set one and not under `--coverage`. - glib/gio CFLAGS routed through `-isystem`. - `--enable-sanitizers` (ASan+UBSan; `unsigned-integer-overflow` probed, skipped on gcc), off by default. - `--enable-hardening` gates `-Wnull-dereference`, off by default. - `-Wsign-compare` promoted from `-Wno-` to an active probed warning. Plus the sign-conversion sites that surfaced as safe (pointer-diff → `gsize` via new `g_diff_to_gsize` helper, non-negative casts), and `/vcard remove` now validates its index through `strtoi_range` so a negative argument can't become a huge unsigned index. Verified: default Debian build, `--enable-sanitizers` build, and an Arch build with `/etc/makepkg.conf` injected (Pikaur simulation) — all green, including the functional-test and unit-test binaries. ### Out of scope / tracked separately - Items 7 + 11 (`_migrate_to_v3` dedupe/FK) → #128, PR #129. - Item 3 (OTR interop matrix) — manual QA. - Full `-Wconversion` cleanup (~180 mostly-mechanical sign-conversion sites) — deferred; not enabled here because it would block `-Werror` builds. Items 4 and 6 were resolved by investigation (no code change needed). Refs #112 Fixes #112 Fixes #109
jabber.developer2 added 1 commit 2026-05-27 11:30:54 +00:00
fix: address issue #112 follow-ups (OTR strip, presence UAF, OMEMO load)
All checks were successful
CI Code / Check coding style (pull_request) Successful in 33s
CI Code / Check spelling (pull_request) Successful in 18s
CI Code / Linux (ubuntu) (pull_request) Successful in 4m46s
CI Code / Linux (arch) (pull_request) Successful in 5m36s
CI Code / Linux (debian) (pull_request) Successful in 6m49s
CI Code / Code Coverage (pull_request) Successful in 6m57s
77cc33eedd
- otr: strip the OTR whitespace tag by shifting the full remaining tail
  (including the trailing NUL) instead of tag_length bytes, which duplicated
  the body for messages longer than the tag (#8).
- xmpp: snapshot resource name/status/priority/presence before
  connection_add_available_resource() transfers ownership of the Resource, so
  the later reads can no longer become a use-after-free (#9).
- omemo: propagate _omemo_finalize_identity_load failure on connect — mark the
  context unloaded, log it, surface a cons_show_error, and stop instead of
  silently leaving OMEMO unavailable (#10).

Refs #112
jabber.developer2 added 1 commit 2026-06-05 11:49:30 +00:00
fix: more issue #112 follow-ups (stanza-id disco gate, account sanitizer)
All checks were successful
CI Code / Check spelling (pull_request) Successful in 19s
CI Code / Check coding style (pull_request) Successful in 32s
CI Code / Linux (debian) (pull_request) Successful in 4m50s
CI Code / Linux (ubuntu) (pull_request) Successful in 5m7s
CI Code / Code Coverage (pull_request) Successful in 7m14s
CI Code / Linux (arch) (pull_request) Successful in 10m31s
148b022b35
- xmpp: gate XEP-0359 stanza-id dedup on disco urn:xmpp:sid:0. Before
  honoring <stanza-id by='…'/> for archive_id dedup, check that the entity
  in `by` (or its domain — that's what profanity disco's on connect)
  announces the namespace; fall back to no-dedup when caps are unknown,
  matching the XEP §6 disco-gate (#1).
- accounts: narrow group-name sanitizer to only the characters GKeyFile
  actually forbids in group headers (`[`, `]`, `\n`, `\r`); `=` and `#` are
  valid inside a header and stay (#2).
- functional tests cover both branches of the disco gate — replayed
  stanza-id is flagged as duplicate when sid:0 is announced, and not
  flagged when it isn't.

Refs #112
jabber.developer2 added 1 commit 2026-06-14 14:07:01 +00:00
build: Pikaur-safe hardening flags, opt-in sanitizers, -Wsign-compare
Some checks failed
CI Code / Check spelling (pull_request) Successful in 17s
CI Code / Check coding style (pull_request) Successful in 33s
CI Code / Linux (ubuntu) (pull_request) Failing after 4m7s
CI Code / Linux (arch) (pull_request) Failing after 4m56s
CI Code / Linux (debian) (pull_request) Failing after 6m33s
CI Code / Code Coverage (pull_request) Failing after 6m48s
f910eecb38
Re-introduce the build hardening that 5459e78e8 attempted, in a form that
coexists with Arch/Pikaur builds (the conflict that forced its revert in
0722dc9e3).

configure.ac:
- Add -D_FORTIFY_SOURCE=2 only when neither $CFLAGS nor $CPPFLAGS already
  define _FORTIFY_SOURCE (makepkg injects its own) and not under --coverage.
- Route glib/gio CFLAGS through -isystem so their macros don't trip our -W set.
- Add --enable-sanitizers (ASan + UBSan; unsigned-integer-overflow probed,
  skipped on gcc) for a dedicated CI job, off by default.
- Add --enable-hardening to gate -Wnull-dereference (false positives under
  -O2 on some toolchains), off by default.
- Promote -Wsign-compare from -Wno- to an active probed warning.

Also fix the sign-conversion sites that -Wsign-compare/-Wconversion cleanup
surfaced as safe (pointer-diff to gsize via new g_diff_to_gsize helper,
non-negative int casts), and validate the /vcard remove index through
strtoi_range so a negative argument can no longer become a huge unsigned
index.

Refs #112
jabber.developer2 added 1 commit 2026-06-14 14:29:53 +00:00
test: fix -Wsign-compare in proftest _mkdir_recursive loop
All checks were successful
CI Code / Check spelling (pull_request) Successful in 16s
CI Code / Check coding style (pull_request) Successful in 31s
CI Code / Code Coverage (pull_request) Successful in 2m42s
CI Code / Linux (debian) (pull_request) Successful in 4m50s
CI Code / Linux (ubuntu) (pull_request) Successful in 4m57s
CI Code / Linux (arch) (pull_request) Successful in 5m51s
0e5ff2e1ef
The functional-test harness compared a signed loop index against strlen()'s
size_t, which the newly promoted -Wsign-compare turns into a -Werror failure
when the test binaries are built. Make the index size_t.

Refs #112
jabber.developer2 changed title from WIP: Fix: issue #112 follow-ups — OTR strip, presence UAF, OMEMO load to Fix: issue #112 follow-ups — OTR strip, presence UAF, OMEMO load 2026-06-16 11:22:49 +00:00
jabber.developer requested changes 2026-06-16 12:02:42 +00:00
Dismissed
jabber.developer left a comment
Owner

Generally this PR introduces nice improvements. However, I'd like to discuss further the nature of multiple changes to ensure their necessity and alignment with our goals. Nonetheless, the PR is good in its nature, as well as documentation and level of detail.

Generally this PR introduces nice improvements. However, I'd like to discuss further the nature of multiple changes to ensure their necessity and alignment with our goals. Nonetheless, the PR is good in its nature, as well as documentation and level of detail.
@@ -10068,0 +10066,4 @@
int index;
auto_gchar gchar* err_msg = NULL;
if (!strtoi_range(args[1], &index, 0, INT_MAX, &err_msg)) {
cons_show("%s", err_msg);

I think that cons_show_error would be more appropriate.

I think that `cons_show_error` would be more appropriate.
jabber.developer marked this conversation as resolved
src/common.c Outdated
@@ -692,12 +692,13 @@ get_file_paths_recursive(const char* path, GSList** contents)
gchar*
get_random_string(int length)
{
g_assert(length >= 0);

g_assert usage should be discussed.

`g_assert` usage should be discussed.
jabber.developer marked this conversation as resolved
src/common.h Outdated
@@ -161,0 +163,4 @@
static inline gsize
g_diff_to_gsize(const void* end, const void* start)
{
g_assert(end >= start);

That does not appear as an appropriate solution. In debug mode it would crash the app, while in prod it would be ignored. Both ways introduce additional point of failure, further discussion is needed.

That does not appear as an appropriate solution. In debug mode it would crash the app, while in prod it would be ignored. Both ways introduce additional point of failure, further discussion is needed.
jabber.developer marked this conversation as resolved
src/otr/otr.c Outdated
@@ -279,3 +279,3 @@
tag_length = 32;
}
memmove(whitespace_base, whitespace_base + tag_length, tag_length);
// Shift the remainder of the message (including the trailing NUL)

The comment explains the nature of change, which is a known antipattern.

The comment explains the nature of change, which is a known antipattern.
jabber.developer marked this conversation as resolved
@@ -622,3 +622,3 @@
if (g_strcmp0(xmpp_presence->jid->barejid, my_jid->barejid) == 0) {
connection_add_available_resource(resource);
// Snapshot everything we read from `resource` before ownership is

nit: Ideally we want more concise 1-2 line comments instead. Further discussion is other required.

nit: Ideally we want more concise 1-2 line comments instead. Further discussion is other required.
jabber.developer marked this conversation as resolved
@@ -120,3 +120,3 @@
_mkdir_recursive(const char* dir)
{
int i;
size_t i;

Why is it even defined out of loop scope? We can move it inside of the for body for readability.

Why is it even defined out of loop scope? We can move it inside of the `for` body for readability.
jabber.developer marked this conversation as resolved
jabber.developer2 added 1 commit 2026-06-16 15:03:53 +00:00
fix(review): address PR #130 review
All checks were successful
CI Code / Check spelling (pull_request) Successful in 18s
CI Code / Check coding style (pull_request) Successful in 34s
CI Code / Linux (debian) (pull_request) Successful in 5m12s
CI Code / Linux (ubuntu) (pull_request) Successful in 5m17s
CI Code / Linux (arch) (pull_request) Successful in 6m4s
CI Code / Code Coverage (pull_request) Successful in 7m27s
c073ff3566
- proftest: declare the _mkdir_recursive loop index inside the for.
- xmpp/presence: shorten the resource-snapshot comment to one line.
- command/cmd_funcs: report the /vcard remove index error via cons_show_error.
- common: replace g_assert in g_diff_to_gsize with a log_error + return 0
  (moved out of the header into common.c); take get_random_string length as
  size_t instead of asserting non-negative.
- otr: drop the change-narrating comment on the whitespace memmove.

Refs #112
jabber.developer2 added 1 commit 2026-06-19 08:48:48 +00:00
fix(omemo): guard NULL fingerprint decode and log failures (CWE-476/457)
All checks were successful
CI Code / Check spelling (pull_request) Successful in 19s
CI Code / Check coding style (pull_request) Successful in 34s
CI Code / Code Coverage (pull_request) Successful in 2m36s
CI Code / Linux (arch) (pull_request) Successful in 5m25s
CI Code / Linux (debian) (pull_request) Successful in 7m10s
CI Code / Linux (ubuntu) (pull_request) Successful in 7m11s
d1dfd57a49
Absorbs the still-relevant null-pointer guards from #107 and extends them for consistency across all _omemo_fingerprint_decode() callers:
- _omemo_fingerprint_decode(): return NULL on NULL input instead of dereferencing it via strlen().
- omemo_is_trusted_identity(): bail out (FALSE) when decode fails and zero-init fingerprint_len, instead of passing a NULL buffer / reading an uninitialized length into signal_buffer_append().
- omemo_trust(): bail out when decode fails (previously unchecked; under OOM it would save a bogus 1-byte identity).

Every decode failure is now logged (omemo_is_trusted_identity / omemo_trust / omemo_untrust) instead of failing silently; omemo_trust also surfaces a user-facing error.

The cmd_funcs.c hunks from #107 are dropped: master already has the strtok guard, and its fdopen guard is more complete (it closes the fd).
Author
Collaborator

Adsorb #107

Adsorb #107
jabber.developer2 force-pushed fix/issue-112-followups from d1dfd57a49 to f63bf57872 2026-06-19 09:46:13 +00:00 Compare
jabber.developer approved these changes 2026-06-20 10:01:50 +00:00
jabber.developer left a comment
Owner

LGTM

LGTM
Author
Collaborator

Suggested squash comment

fix: OTR/presence/OMEMO correctness, stanza-id disco gate, build hardening

- OTR: strip the whitespace tag by shifting the full message tail incl. the NUL, not tag_length bytes, so the body is no longer duplicated for messages longer than the tag.
- presence: snapshot the resource fields before connection_add_available_resource() takes ownership, removing a use-after-free in the own-presence path.
- OMEMO: propagate _omemo_finalize_identity_load() failure on connect (log + cons_show_error + stop) instead of leaving OMEMO silently unavailable.
- OMEMO: guard NULL fingerprint decode in _omemo_fingerprint_decode / omemo_is_trusted_identity / omemo_trust and log every decode failure instead of failing silently.
- stanza-id: gate XEP-0359 dedup on disco urn:xmpp:sid:0 (the `by` JID or its domain), falling back to no-dedup when caps are unknown; add functional tests for trusted vs untrusted server.
- accounts: narrow the group-name sanitizer to the characters GKeyFile forbids in headers ([ ] \n \r), keeping `=` and `#`, so read/write stays symmetric.
- build: re-introduce compiler/sanitizer flags in a Pikaur-safe form (opt-in sanitizers, -Wsign-compare) and fix the resulting -Wsign-compare warnings (incl. proftest _mkdir_recursive).fix: OTR/presence/OMEMO correctness, stanza-id disco gate, build hardening
Suggested squash comment ``` fix: OTR/presence/OMEMO correctness, stanza-id disco gate, build hardening - OTR: strip the whitespace tag by shifting the full message tail incl. the NUL, not tag_length bytes, so the body is no longer duplicated for messages longer than the tag. - presence: snapshot the resource fields before connection_add_available_resource() takes ownership, removing a use-after-free in the own-presence path. - OMEMO: propagate _omemo_finalize_identity_load() failure on connect (log + cons_show_error + stop) instead of leaving OMEMO silently unavailable. - OMEMO: guard NULL fingerprint decode in _omemo_fingerprint_decode / omemo_is_trusted_identity / omemo_trust and log every decode failure instead of failing silently. - stanza-id: gate XEP-0359 dedup on disco urn:xmpp:sid:0 (the `by` JID or its domain), falling back to no-dedup when caps are unknown; add functional tests for trusted vs untrusted server. - accounts: narrow the group-name sanitizer to the characters GKeyFile forbids in headers ([ ] \n \r), keeping `=` and `#`, so read/write stays symmetric. - build: re-introduce compiler/sanitizer flags in a Pikaur-safe form (opt-in sanitizers, -Wsign-compare) and fix the resulting -Wsign-compare warnings (incl. proftest _mkdir_recursive).fix: OTR/presence/OMEMO correctness, stanza-id disco gate, build hardening ```
jabber.developer manually merged commit 830479cf20 into master 2026-06-20 10:30:41 +00:00
Sign in to join this conversation.
No description provided.