devs/cproof
1
2
Fork 1
Code Issues 43 Pull Requests 10 Actions Packages Projects 1 Releases Wiki Activity

WIP: security: harden untrusted-input handling (issue #148) #176

Draft
jabber.developer2 wants to merge 1 commits from fix/untrusted-input-148 into master
pull from: fix/untrusted-input-148
merge into: devs:master
Conversation 0 Commits 1 Files Changed 30 +415 -78
jabber.developer2 commented 2026-08-01 10:52:13 +00:00
Collaborator
Copy Link

Covers the input-validation and memory-safety tasks of issue #148 (T02, T10, T11). Every change is on a path that processes data controlled by a remote peer or by the server: presence and message handlers, the URL commands, the desktop notifier, the OX/OMEMO helpers and the disco cache.

T02 — crashes on malformed from (REQ-INP-01)

Four receive-path handlers dereferenced the result of jid_create() without a NULL check, so a stanza with a missing or malformed from terminated the client:

  • _presence_error_handler() — MUC join errors
  • _subscribed_handler() / _unsubscribed_handler() — subscription presence, reachable from any remote JID
  • _should_ignore_based_on_silence() — with silence.non-roster enabled this is on the path of every incoming message

Two MUC handlers (_muc_user_self_handler, _muc_user_occupant_handler) got the same guard, defensively: the caller validates today, but the handlers are one refactor away from being reachable directly. _subscribe_handler() logged the missing attribute and then continued into the NULL dereference anyway — it now returns.

T11 — untrusted input reaching the shell, the terminal and the storage layer

  • /url open, /url save (REQ-INP-06) — the URL comes out of a received message and was passed to the configured opener command with no scheme check. Both commands now accept only http, https and aesgcm; a file:, javascript: or data: URL is refused with an error line. User-visible behaviour change.
  • macOS notifier (REQ-INP-07) — the notification text was interpolated into a shell command line and handed to system(), so a message body containing `, $(…) or ; executed as a command. Replaced with g_spawn_async() and an argv, where the message is a single argument the shell never sees.
  • MAM result ids (REQ-INP-05) — live <stanza-id> elements are already gated on the archive announcing XEP-0359 support; MAM results were not, so any server could inject ids that end up stored as stable identifiers. The same gate now applies, falling back to the account's own bare JID when the archive is implicit.
  • Terminal escapes and bidi overrides (REQ-INP-08) — str_neutralize_untrusted() replaces C0/C1 control characters (including ESC) and the bidi embedding/override/isolate ranges with U+FFFD. It runs in _clean_incoming_message(), i.e. after decryption, so OTR, PGP and OX plaintext is covered by the same pass. Tab and newlines are kept, as is LRM/RLM (U+200E/U+200F) — legitimate RTL text needs them.
  • JID validation (REQ-INP-02) — added coverage for the localpart/domainpart/resourcepart/total length boundaries and for invalid UTF-8.

T10 — memory safety

  • Unsafe string APIs (REQ-MEM-03) — strcpy/strcat/alloca in the OX signcrypt helper and sprintf in the OMEMO AESGCM fragment builder, the last-activity handler and the MUC trigger printer replaced with g_strdup_printf, g_strndup and g_snprintf. gcry_malloc_secure() may return NULL under a secure-memory limit; that return is now checked.
  • OMEMO key buffers (REQ-MEM-04) — the three bundle getters allocated with malloc() and fed the result straight into memcpy(). They now use g_malloc, which aborts rather than returning NULL, with an explicit zero-length case since g_malloc(0) does return NULL. The matching free() calls in omemo_bundle_publish() became g_free().
  • Variable-length arrays (REQ-MEM-09) — all VLAs removed and -Werror=vla enforced in both build systems, so a stack array sized from peer input cannot come back. Two of the removed VLAs were remote-sized: the disco#info feature count in caps_add_by_ver() (now g_malloc_n) and a word length in the line-wrapping code (which only ever needed room for one UTF-8 character). Enabling the flag also surfaced, in the plugin autocompleter bindings, a one-past-the-end stack write and a leak on the argument-validation early return.

Resolve #148

Covers the input-validation and memory-safety tasks of issue #148 (T02, T10, T11). Every change is on a path that processes data controlled by a remote peer or by the server: presence and message handlers, the URL commands, the desktop notifier, the OX/OMEMO helpers and the disco cache. ## T02 — crashes on malformed `from` (REQ-INP-01) Four receive-path handlers dereferenced the result of `jid_create()` without a NULL check, so a stanza with a missing or malformed `from` terminated the client: - `_presence_error_handler()` — MUC join errors - `_subscribed_handler()` / `_unsubscribed_handler()` — subscription presence, reachable from any remote JID - `_should_ignore_based_on_silence()` — with `silence.non-roster` enabled this is on the path of *every* incoming message Two MUC handlers (`_muc_user_self_handler`, `_muc_user_occupant_handler`) got the same guard, defensively: the caller validates today, but the handlers are one refactor away from being reachable directly. `_subscribe_handler()` logged the missing attribute and then continued into the NULL dereference anyway — it now returns. ## T11 — untrusted input reaching the shell, the terminal and the storage layer - **`/url open`, `/url save` (REQ-INP-06)** — the URL comes out of a received message and was passed to the configured opener command with no scheme check. Both commands now accept only `http`, `https` and `aesgcm`; a `file:`, `javascript:` or `data:` URL is refused with an error line. *User-visible behaviour change.* - **macOS notifier (REQ-INP-07)** — the notification text was interpolated into a shell command line and handed to `system()`, so a message body containing `` ` ``, `$(…)` or `;` executed as a command. Replaced with `g_spawn_async()` and an argv, where the message is a single argument the shell never sees. - **MAM result ids (REQ-INP-05)** — live `<stanza-id>` elements are already gated on the archive announcing XEP-0359 support; MAM results were not, so any server could inject ids that end up stored as stable identifiers. The same gate now applies, falling back to the account's own bare JID when the archive is implicit. - **Terminal escapes and bidi overrides (REQ-INP-08)** — `str_neutralize_untrusted()` replaces C0/C1 control characters (including ESC) and the bidi embedding/override/isolate ranges with U+FFFD. It runs in `_clean_incoming_message()`, i.e. after decryption, so OTR, PGP and OX plaintext is covered by the same pass. Tab and newlines are kept, as is LRM/RLM (U+200E/U+200F) — legitimate RTL text needs them. - **JID validation (REQ-INP-02)** — added coverage for the localpart/domainpart/resourcepart/total length boundaries and for invalid UTF-8. ## T10 — memory safety - **Unsafe string APIs (REQ-MEM-03)** — `strcpy`/`strcat`/`alloca` in the OX signcrypt helper and `sprintf` in the OMEMO AESGCM fragment builder, the last-activity handler and the MUC trigger printer replaced with `g_strdup_printf`, `g_strndup` and `g_snprintf`. `gcry_malloc_secure()` may return NULL under a secure-memory limit; that return is now checked. - **OMEMO key buffers (REQ-MEM-04)** — the three bundle getters allocated with `malloc()` and fed the result straight into `memcpy()`. They now use `g_malloc`, which aborts rather than returning NULL, with an explicit zero-length case since `g_malloc(0)` does return NULL. The matching `free()` calls in `omemo_bundle_publish()` became `g_free()`. - **Variable-length arrays (REQ-MEM-09)** — all VLAs removed and `-Werror=vla` enforced in both build systems, so a stack array sized from peer input cannot come back. Two of the removed VLAs were remote-sized: the disco#info feature count in `caps_add_by_ver()` (now `g_malloc_n`) and a word length in the line-wrapping code (which only ever needed room for one UTF-8 character). Enabling the flag also surfaced, in the plugin autocompleter bindings, a one-past-the-end stack write and a leak on the argument-validation early return. Resolve #148
jabber.developer2 added 1 commit 2026-08-01 10:52:13 +00:00
security: harden untrusted-input handling (issue #148)
All checks were successful
CI Code / Check spelling (pull_request) Successful in 14s
Details
CI Code / Check coding style (pull_request) Successful in 26s
Details
CI Code / Code Coverage (pull_request) Successful in 3m29s
Details
CI Code / Linux (debian) (pull_request) Successful in 5m13s
Details
CI Code / Linux (ubuntu) (pull_request) Successful in 5m16s
Details
CI Code / Linux (arch) (pull_request) Successful in 7m34s
Details
250703a0bf 
T02: guard the receive-path handlers that dereferenced jid_create()
without a NULL check — MUC join errors, subscribed/unsubscribed
presence and, with silence.non-roster enabled, every incoming message.
A stanza with a missing or malformed 'from' crashed the client
(REQ-INP-01)

T11: restrict /url open and /url save to http, https and aesgcm, so a
received file:, javascript: or data: URL is refused (REQ-INP-06); spawn
terminal-notifier through g_spawn_async with an argv instead of
building a shell command for system() (REQ-INP-07); apply the XEP-0359
disco gate to MAM result ids, as live stanza-ids already do
(REQ-INP-05); replace control and bidi-reordering characters in
incoming message bodies with U+FFFD before they reach the terminal, the
logs and the database, keeping LRM/RLM for legitimate RTL text
(REQ-INP-08); cover JID part-length boundaries and invalid UTF-8
(REQ-INP-02)

T10: replace strcpy/strcat/alloca and sprintf with g_strdup_printf and
g_snprintf (REQ-MEM-03); allocate the OMEMO key buffers with g_malloc
so a failed allocation cannot reach the following memcpy (REQ-MEM-04);
remove the variable-length arrays and enforce -Werror=vla. Two of them
were sized from remote input: the disco#info feature count and a chat
message word length. The flag also caught a one-past-the-end write and
a leak in the plugin autocompleter bindings (REQ-MEM-09)
All checks were successful
CI Code / Check spelling (pull_request) Successful in 14s
Required
Details
CI Code / Check coding style (pull_request) Successful in 26s
Required
Details
CI Code / Code Coverage (pull_request) Successful in 3m29s
Required
Details
CI Code / Linux (debian) (pull_request) Successful in 5m13s
Required
Details
CI Code / Linux (ubuntu) (pull_request) Successful in 5m16s
Required
Details
CI Code / Linux (arch) (pull_request) Successful in 7m34s
Required
Details
This pull request is marked as a work in progress.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/untrusted-input-148:fix/untrusted-input-148
git checkout fix/untrusted-input-148
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
Compat/Breaking
Breaking change that won't be backward compatible
Kind/Bug
Something is not working
Kind/Documentation
Documentation changes
Kind/Enhancement
Improve existing functionality
Kind/Feature
New functionality
Kind/Security
This is security issue
Kind/Testing
Issue or pull request related to testing
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Low
4
The priority is low
Priority
Medium
3
The priority is medium
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
admin jabber.developer jabber.developer2
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: devs/cproof#176
No description provided.
Delete Branch "fix/untrusted-input-148"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?