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

WIP: security: validate <delay>'s from and <stanza-id>'s by attributes #97

Draft
jabber.developer2 wants to merge 1 commits from fix/delay-timestamp-validation into master
pull from: fix/delay-timestamp-validation
merge into: devs:master
Conversation 2 Commits 1 Files Changed 3 +49 -15
jabber.developer2 commented 2026-02-21 15:27:32 +00:00
Collaborator
Copy Link

Summary

Two related security fixes for incoming stanza validation per XEP-0203 §4 and XEP-0359 §5.

Problem

1. Timestamp spoofing via <delay> (XEP-0203)

A remote client can inject a <delay xmlns='urn:xmpp:delay' stamp='...'> element with an arbitrary timestamp into a message stanza. Profanity accepted any <delay> regardless of its from attribute, so a malicious peer could make their message appear at any position in chat history — past or future.

XEP-0203 §4 (Security Considerations) states:

An entity MUST check the 'from' attribute to ensure that the entity that added the delay is an entity it trusts.

2. Archive-ID spoofing via <stanza-id> (XEP-0359)

A remote client can inject a <stanza-id xmlns='urn:xmpp:sid:0' id='...' by='...'> element with a forged archive ID. Profanity read the id attribute without checking by, so an attacker could cause MAM deduplication breakage or message omission in history backends that key on archive_id.

XEP-0359 §5 (Security Considerations) states:

Entities MUST check the 'by' attribute to ensure it was set by an acceptable entity.

Fix

XEP-0203: <delay> from validation

All three incoming message handlers now use a trust-ordered fallback chain instead of accepting the first <delay> found:

Handler Fallback order
_handle_chat our server domain → sender's domain → now_local()
_handle_groupchat room bare JID → room domain → our domain → now_local()
_handle_muc_private_message our domain → sender's domain

New helper: stanza_get_oldest_delay_from(stanza, from) — same as stanza_get_oldest_delay() but filters <delay>/<x> children by the from attribute. The unfiltered version is refactored to delegate with from=NULL.

The MAM path (_handle_mam) is deliberately unchanged — <delay> inside <forwarded> comes from our own server's archive and is trusted by construction.

XEP-0359: <stanza-id> by validation

Handler Accepted by value
_handle_groupchat room bare JID
_handle_chat our server domain

Changed files

  • stanza.c — add stanza_get_oldest_delay_from(), NULL-safety for timestamp comparison
  • stanza.h — declare stanza_get_oldest_delay_from(), add STANZA_ATTR_BY

Fixes #102

## Summary Two related security fixes for incoming stanza validation per XEP-0203 §4 and XEP-0359 §5. ## Problem ### 1. Timestamp spoofing via `<delay>` (XEP-0203) A remote client can inject a `<delay xmlns='urn:xmpp:delay' stamp='...'>` element with an arbitrary timestamp into a message stanza. Profanity accepted any `<delay>` regardless of its `from` attribute, so a malicious peer could make their message appear at any position in chat history — past or future. XEP-0203 §4 (Security Considerations) states: > An entity MUST check the 'from' attribute to ensure that the entity that added the delay is an entity it trusts. ### 2. Archive-ID spoofing via `<stanza-id>` (XEP-0359) A remote client can inject a `<stanza-id xmlns='urn:xmpp:sid:0' id='...' by='...'>` element with a forged archive ID. Profanity read the `id` attribute without checking `by`, so an attacker could cause MAM deduplication breakage or message omission in history backends that key on `archive_id`. XEP-0359 §5 (Security Considerations) states: > Entities MUST check the 'by' attribute to ensure it was set by an acceptable entity. ## Fix ### XEP-0203: `<delay>` from validation All three incoming message handlers now use a trust-ordered fallback chain instead of accepting the first `<delay>` found: | Handler | Fallback order | |---------|---------------| | `_handle_chat` | our server domain → sender's domain → `now_local()` | | `_handle_groupchat` | room bare JID → room domain → our domain → `now_local()` | | `_handle_muc_private_message` | our domain → sender's domain | New helper: `stanza_get_oldest_delay_from(stanza, from)` — same as `stanza_get_oldest_delay()` but filters `<delay>`/`<x>` children by the `from` attribute. The unfiltered version is refactored to delegate with `from=NULL`. The MAM path (`_handle_mam`) is **deliberately unchanged** — `<delay>` inside `<forwarded>` comes from our own server's archive and is trusted by construction. ### XEP-0359: `<stanza-id>` by validation | Handler | Accepted `by` value | |---------|-------------------| | `_handle_groupchat` | room bare JID | | `_handle_chat` | our server domain | ## Changed files - stanza.c — add `stanza_get_oldest_delay_from()`, NULL-safety for timestamp comparison - stanza.h — declare `stanza_get_oldest_delay_from()`, add `STANZA_ATTR_BY` - Fixes #102
jabber.developer2 added 1 commit 2026-02-21 15:27:33 +00:00
security: validate <delay> from and <stanza-id> by attributes
All checks were successful
CI Code / Check spelling (pull_request) Successful in 20s
Details
CI Code / Check coding style (pull_request) Successful in 35s
Details
CI Code / Code Coverage (pull_request) Successful in 4m47s
Details
CI Code / Linux (debian) (pull_request) Successful in 6m8s
Details
CI Code / Linux (ubuntu) (pull_request) Successful in 6m14s
Details
CI Code / Linux (arch) (pull_request) Successful in 6m26s
Details
5f39dd4939 
XEP-0203 §4 (Delayed Delivery):
Remote clients could inject <delay xmlns='urn:xmpp:delay'> elements
with arbitrary timestamps, causing messages to appear at incorrect
positions in chat history.  The spec mandates checking the 'from'
attribute to verify the delay was added by a trusted source.

- stanza.c: add stanza_get_oldest_delay_from(stanza, from) that
  filters <delay>/<x> children by the 'from' attribute; refactor
  stanza_get_oldest_delay() to delegate with from=NULL.  Add
  NULL-safety for tmp timestamp in comparison logic.
- stanza.h: declare stanza_get_oldest_delay_from().
- message.c (_handle_chat): replace unfiltered stanza_get_delay()
  with trust-ordered fallback: our domain → sender domain → now.
- message.c (_handle_groupchat): replace stanza_get_oldest_delay()
  with fallback: room bare JID → room domain → our domain → now.
- message.c (_handle_muc_private_message): same pattern.

MAM path deliberately unchanged — <delay> inside <forwarded> is
already from our own server's archive.

XEP-0359 §5 (Unique and Stable Stanza IDs):
The <stanza-id> element was accepted without checking the 'by'
attribute, allowing a remote client to inject forged archive IDs.

- stanza.h: add STANZA_ATTR_BY define.
- message.c (_handle_groupchat): only accept <stanza-id> when
  by == room bare JID.
- message.c (_handle_chat): only accept <stanza-id> when
  by == our server domain.
jabber.developer2 changed title from security: validate <delay> from and <stanza-id> by attributes to WIP: security: validate <delay> from and <stanza-id> by attributes 2026-02-21 15:28:22 +00:00
jabber.developer changed title from WIP: security: validate <delay> from and <stanza-id> by attributes to WIP: security: validate `<delay>`'s `from` and `<stanza-id>`'s `by` attributes 2026-03-12 20:26:31 +00:00
jabber.developer2 force-pushed fix/delay-timestamp-validation from 5f39dd4939 to 7fb926c78a 2026-05-16 09:24:43 +00:00 Compare
jabber.developer2 added 1 commit 2026-05-16 09:43:24 +00:00
fix(review): drop <stanza-id by> checks overlapping with PR #105
Some checks failed
CI Code / Check spelling (pull_request) Successful in 17s
Details
CI Code / Check coding style (pull_request) Successful in 30s
Details
CI Code / Code Coverage (pull_request) Failing after 10m2s
Details
CI Code / Linux (debian) (pull_request) Failing after 12m31s
Details
CI Code / Linux (ubuntu) (pull_request) Failing after 12m41s
Details
CI Code / Linux (arch) (pull_request) Failing after 13m25s
Details
ffc626a276 
After review of PR #105 (upstream-full merge) it became clear the
<stanza-id by> validation for MUC and 1:1 chat is already added there
via upstream profanity. Keeping both versions would conflict on merge.

Removed from this PR:
  - _handle_groupchat <stanza-id by == room JID> check
    (identical to PR #105)
  - _handle_chat <stanza-id by == my_domain> check
    (PR #105 uses equals_our_barejid(by) instead — defer to upstream)
  - STANZA_ATTR_BY define (no longer referenced)

Kept (PR #105 does NOT touch these):
  - XEP-0203 §4 <delay from=?> validation in _handle_groupchat,
    _handle_chat, _handle_muc_private_message
  - stanza_get_oldest_delay_from() helper + NULL-tmp guard bug fix
    in stanza_get_oldest_delay()

Also applied review fixes:
  - Add g_date_time_new_now_local() fallback in
    _handle_muc_private_message so timestamp is never NULL
  - Change stanza_get_delay_from signature to const char* const from,
    eliminating (gchar*) casts at call sites
jabber.developer2 commented 2026-05-16 09:48:03 +00:00
Author
Collaborator
Copy Link

Update after overlap analysis with PR #105

After reviewing PR #105 (merge/upstream-full), I noticed it already adds <stanza-id by> validation upstream. To avoid merge conflicts, this PR was reduced to only the parts that don't overlap with #105:

Kept (novel to this PR)

  • XEP-0203 §4 <delay from=?> validation in _handle_groupchat, _handle_chat, _handle_muc_private_message — PR #105 still uses unvalidated stanza_get_delay(stanza)
  • stanza_get_oldest_delay_from() helper + NULL-tmp guard bug-fix in stanza_get_oldest_delay()

Removed (already in PR #105 or different semantics there)

  • _handle_groupchat <stanza-id by == room JID> — identical to upstream version in #105
  • _handle_chat <stanza-id by == my_domain> — #105 uses equals_our_barejid(by) instead. We defer to the upstream interpretation rather than fight it; if XEP-0359-strict checking is wanted, that can be a follow-up against the merged upstream code
  • STANZA_ATTR_BY define — no longer referenced after the above removals

Review fixes applied

  • Added g_date_time_new_now_local() fallback in _handle_muc_private_message (was missing, would have left timestamp = NULL on the path through)
  • Changed stanza_get_delay_from signature gchar* → const char* const, removed (gchar*) casts

Merge order

Land PR #105 first, then this PR will rebase cleanly on the resulting master.

## Update after overlap analysis with PR #105 After reviewing PR #105 (`merge/upstream-full`), I noticed it already adds `<stanza-id by>` validation upstream. To avoid merge conflicts, this PR was reduced to **only the parts that don't overlap with #105**: ### Kept (novel to this PR) - **XEP-0203 §4 `<delay from=?>` validation** in `_handle_groupchat`, `_handle_chat`, `_handle_muc_private_message` — PR #105 still uses unvalidated `stanza_get_delay(stanza)` - `stanza_get_oldest_delay_from()` helper + NULL-`tmp` guard bug-fix in `stanza_get_oldest_delay()` ### Removed (already in PR #105 or different semantics there) - `_handle_groupchat <stanza-id by == room JID>` — identical to upstream version in #105 - `_handle_chat <stanza-id by == my_domain>` — #105 uses `equals_our_barejid(by)` instead. We defer to the upstream interpretation rather than fight it; if XEP-0359-strict checking is wanted, that can be a follow-up against the merged upstream code - `STANZA_ATTR_BY` define — no longer referenced after the above removals ### Review fixes applied - Added `g_date_time_new_now_local()` fallback in `_handle_muc_private_message` (was missing, would have left `timestamp = NULL` on the path through) - Changed `stanza_get_delay_from` signature `gchar*` → `const char* const`, removed `(gchar*)` casts ### Merge order Land **PR #105 first**, then this PR will rebase cleanly on the resulting master.
jabber.developer2 force-pushed fix/delay-timestamp-validation from ffc626a276 to 55c73ee21a 2026-06-19 08:15:29 +00:00 Compare
jabber.developer2 commented 2026-06-19 08:18:55 +00:00
Author
Collaborator
Copy Link

Rebased on master; deferring — strict <delay> from-validation regresses history/export timestamps.

State of this branch

  • Rebased onto current master (was behind and conflicting) and squashed into one commit.
  • The XEP-0359 <stanza-id> by half of the original PR is dropped: master already validates by, and #130 adds the disco-trust gate on top. What remains is purely the XEP-0203 <delay> from validation (stanza_get_oldest_delay_from() + from-filtered wiring in the three live handlers; MAM <forwarded> stays unfiltered).
  • Builds clean (-Werror, full features); unit tests pass 644/644.

Why this is not ready to merge

make check fails 9 functional tests in test_export_import.c (export_sqlite_to_flatfile, import_flatfile_to_sqlite, bidirectional_merge_to_flatfile, bidirectional_merge_to_sqlite, roundtrip_full_cycle, export_timestamps_preserved, import_timestamps_preserved, export_timestamps_non_utc, muc_export_sqlite_to_flatfile). All of them inject <delay xmlns='urn:xmpp:delay' stamp='…'/> without a from attribute; the new filter rejects any from-less delay, so the stamp is dropped and the message is timestamped "now". These tests are green on master, so this is a real regression of the history/export-import feature, not a test artifact.

Real-world fragility (beyond the tests)

  • Offline messages (mod_offline): the <delay> from varies across servers — bare server domain, account bare JID, or omitted (see the existing gh#1190 workaround at message.c that already checks both barejid and domainpart). Exact-match on the server domain loses the timestamp for any of these variants, collapsing the whole offline backlog to receive-time.
  • MUC history (XEP-0045 §7.2.15): compliant rooms stamp from=room-bare-JID (handled), but bridges/transports and non-standard rooms may stamp differently → history timestamps lost.
  • Gateways/transports: a <delay> stamped with the bridge's JID matches neither our domain nor the peer's domain → timestamp lost.
  • Fail-open: in the 1:1 path connection_get_domain() is passed straight to stanza_get_delay_from(); if it returns NULL, filtering is silently bypassed (any delay accepted).
  • Weak for live 1:1: the peer controls the whole stanza, so a malicious sender can simply set from='<our-domain>' (or omit from) to bypass the check — the validation is most meaningful only where a trusted intermediary adds the delay (MUC history, MAM).

Suggested path forward

Scope the strict from validation to the contexts where it is sound — MUC history and MAM <forwarded> (delay added by a trusted intermediary, from well-defined). For live 1:1, accept leniently: trust a delay whose from is absent or in a trusted set (our server domain, our bare JID, the peer's domain, the room JID/domain), and reject only a clearly foreign third-party from. That preserves timestamps in real-world cases while keeping the protection where it actually holds.

**Rebased on `master`; deferring — strict `<delay>` `from`-validation regresses history/export timestamps.** ### State of this branch - Rebased onto current `master` (was behind and conflicting) and squashed into one commit. - The XEP-0359 `<stanza-id>` `by` half of the original PR is **dropped**: `master` already validates `by`, and #130 adds the disco-trust gate on top. What remains is purely the XEP-0203 `<delay>` `from` validation (`stanza_get_oldest_delay_from()` + from-filtered wiring in the three live handlers; MAM `<forwarded>` stays unfiltered). - Builds clean (`-Werror`, full features); unit tests pass 644/644. ### Why this is not ready to merge `make check` fails 9 functional tests in `test_export_import.c` (`export_sqlite_to_flatfile`, `import_flatfile_to_sqlite`, `bidirectional_merge_to_flatfile`, `bidirectional_merge_to_sqlite`, `roundtrip_full_cycle`, `export_timestamps_preserved`, `import_timestamps_preserved`, `export_timestamps_non_utc`, `muc_export_sqlite_to_flatfile`). All of them inject `<delay xmlns='urn:xmpp:delay' stamp='…'/>` **without a `from` attribute**; the new filter rejects any `from`-less delay, so the stamp is dropped and the message is timestamped "now". These tests are green on `master`, so this is a real regression of the history/export-import feature, not a test artifact. ### Real-world fragility (beyond the tests) - **Offline messages (mod_offline):** the `<delay>` `from` varies across servers — bare server domain, account bare JID, or omitted (see the existing `gh#1190` workaround at `message.c` that already checks both barejid and domainpart). Exact-match on the server domain loses the timestamp for any of these variants, collapsing the whole offline backlog to receive-time. - **MUC history (XEP-0045 §7.2.15):** compliant rooms stamp `from=room-bare-JID` (handled), but bridges/transports and non-standard rooms may stamp differently → history timestamps lost. - **Gateways/transports:** a `<delay>` stamped with the bridge's JID matches neither our domain nor the peer's domain → timestamp lost. - **Fail-open:** in the 1:1 path `connection_get_domain()` is passed straight to `stanza_get_delay_from()`; if it returns `NULL`, filtering is silently bypassed (any delay accepted). - **Weak for live 1:1:** the peer controls the whole stanza, so a malicious sender can simply set `from='<our-domain>'` (or omit `from`) to bypass the check — the validation is most meaningful only where a trusted intermediary adds the delay (MUC history, MAM). ### Suggested path forward Scope the strict `from` validation to the contexts where it is sound — MUC history and MAM `<forwarded>` (delay added by a trusted intermediary, `from` well-defined). For live 1:1, accept leniently: trust a delay whose `from` is absent or in a trusted set (our server domain, our bare JID, the peer's domain, the room JID/domain), and reject only a clearly foreign third-party `from`. That preserves timestamps in real-world cases while keeping the protection where it actually holds.
Some checks failed
CI Code / Check spelling (pull_request) Successful in 19s
Required
Details
CI Code / Check coding style (pull_request) Successful in 34s
Required
Details
CI Code / Code Coverage (pull_request) Failing after 9m56s
Required
Details
CI Code / Linux (arch) (pull_request) Failing after 13m41s
Required
Details
CI Code / Linux (debian) (pull_request) Failing after 15m0s
Required
Details
CI Code / Linux (ubuntu) (pull_request) Failing after 15m9s
Required
Details
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/delay-timestamp-validation:fix/delay-timestamp-validation
git checkout fix/delay-timestamp-validation
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#97
No description provided.
Delete Branch "fix/delay-timestamp-validation"

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?