buzz-sdk: build_add_member/build_remove_member accept overlong hex pubkeys and sign an unparseable p tag
- Dominant language
- Rust
- Stars
- 32.7k
- Forks
- 4.3k
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 253
Description
`buzz_sdk::builders::build_add_member` and `build_remove_member` validate their `target_pubkey` with a **minimum**-length check where the wire format requires an **exact** length, so an overlong all-hex string is accepted and signed into the `p` tag. The relay then cannot parse that `p` tag and rejects the write with `missing p tag`.
Found while reviewing #6372 (which touches a different builder path and is not affected). This is pre-existing on `main` — reproduced at `f88cda9eb886500ec7d205e1d265ac6f654aa433`.
Note for anyone who saw my earlier informal note on this: my first characterization ("no hex-alphabet check") was **wrong**, and the probe below is what corrected it. `check_hex_len` does check the hex alphabet. The defect is min-vs-exact length only.
## The check
`crates/buzz-sdk/src/builders.rs:44-52`:
```rust
/// Validate hex string has at least `min_len` hex characters.
fn check_hex_len(s: &str, min_len: usize, field: &str) -> Result<(), SdkError> {
if s.len() < min_len || !s.chars().all(|c| c.is_ascii_hexdigit()) {
return Err(SdkError::InvalidDiffMeta(format!(
"{field} must be at least {min_len} hex characters (got {:?})",
s
)));
}
Ok(())
}
```
It is the right helper for its two other call sites — abbreviated git shas at `builders.rs:333` (`commit_sha`, min 7) and `:335` (`parent_commit`, min 7), where "at least" is genuinely the contract. It is the wrong helper for a pubkey:
```rust
// builders.rs:576-590
pub fn build_add_member(channel_id: Uuid, target_pubkey: &str, role: Option) -> Result {
check_hex_len(target_pubkey, 64, "target_pubkey")?; // >= 64, not == 64
let mut tags = vec![
tag(&["h", &channel_id.to_string()])?,
tag(&["p", &target_pubkey.to_ascii_lowercase()])?,
];
...
}
// builders.rs:593-603 — build_remove_member, identical check
```
## Reproduction
Throwaway test in `crates/buzz-sdk/src/builders.rs` at `f88cda9eb`, `cargo test -p buzz-sdk -- --nocapture`, reverted after:
```
ADD len=100 hex -> ACCEPTED, p tag len 100 = ["abab…abab"]
RM len=100 hex -> ACCEPTED, p tag len 100 = ["abab…abab"]
ADD len=65 hex -> ACCEPTED
ADD len=64 non-hex -> REJECTED: InvalidDiffMeta("target_pubkey must be at least 64 hex characters (got \"zzzz…\")")
ADD len=63 hex -> REJECTED: InvalidDiffMeta("target_pubkey must be at least 64 hex characters (got \"aaa…\")")
BAN len=65 hex -> REJECTED: InvalidInput("target_pubkey must be a 64-character hex pubkey")
```
So: too short is caught, non-hex is caught, **too long is signed**. The last line is the same input against `build_moderation_ban`, which uses the exact-length helper and rejects it.
## Why exact length is the contract
1. **The relay requires exactly 64.** `crates/buzz-relay/src/handlers/side_effects.rs:2366-2379` (`extract_p_tag`, used by the kind-9000/9001 admin validator at `:363` and `:455`) hex-decodes and requires 32 bytes. `crates/buzz-relay/src/handlers/moderation_commands.rs:561-575` (`extract_p_tag_bytes`) requires `val.len() == 64 && all ascii hexdigit`. A 100-char tag decodes to 50 bytes and a 65-char tag fails to decode; both yield `None`, and the 9000 validator turns that into `Err("missing p tag")` — a confusing error for a caller that did supply a `p` tag.
2. **The SDK already documents this contract in its own test.** `crates/buzz-sdk/src/builders.rs:4383-4388`:
```rust
#[test]
fn moderation_ban_rejects_overlong_pubkey() {
// Relay `extract_p_tag_bytes` requires exactly 64 hex; the SDK must
// reject 65+ hex here rather than sign a `p` tag the relay drops.
let err = build_moderation_ban(&"a".repeat(65), None, None).unwrap_err();
assert!(matches!(err, SdkError::InvalidInput(_)));
}
```
`build_add_member`/`build_remove_member` are the two `p`-tag builders that do not honor it.
3. **Every sibling `p`-tag builder uses the exact-length helper**, `check_pubkey_hex` (`builders.rs:69-76`, `len() != 64 || !is_ascii_hexdigit`, returns the value lowercased, errors `InvalidInput`): `build_moderation_ban`/`unban`/`timeout`/`untimeout` (`:1751`, `:1764`, and following), `build_dm_open` (`:269-270`), issue-assignee tags (`:1231`), repo owner/recipient tags (`:988`, `:1036`, `:1045`, …). `check_hex_len` at 64 appears only in these two functions.
Secondary nit visible in the probe output: the error variant is `SdkError::InvalidDiffMeta`, which is a git-diff-metadata error, for a channel member pubkey. `check_pubkey_hex` returns `InvalidInput`.
## Exposure
Scoped to what I searched (`rg 'build_add_member|build_remove_member' -g '*.rs'` across the repo):
- `crates/buzz-cli/src/commands/channels.rs:991` and `:1008` — pre-validate with `validate_hex64` (exact 64, `crates/buzz-cli/src/validate.rs:29-36`). Not exposed.
- `crates/buzz-cli/src/commands/channels.rs:747` (template apply / roster) — passes `agent.pubkey` straight through with no length check. That value comes from the `d` tag of a relay-stored `KIND_MANAGED_AGENT` event via `extract_d_tag`, and `build_roster_resolution` only rejects it when empty (`channels.rs:453-455`). This is the one in-repo caller that would reach the builder with an unvalidated length; I did not construct a malformed managed-agent record, so I am claiming "unguarded path", not "demonstrated exploit".
- `desktop/src-tauri/src/**` calls its own `events::build_add_member`/`build_remove_member` (`desktop/src-tauri/src/events.rs:222`, `:239`), which use a local `check_pubkey` with an exact-64 check (`events.rs:84-92`). Desktop does not reach the SDK helper on this path.
No security claim: the relay rejects the event, so the practical impact is a signed-but-unusable event and a misleading `missing p tag` error rather than a bad membership write. I found no path where an overlong `p` tag results in a member being added.
## Suggested fix
Swap both call sites to the existing exact-length helper — no new helper, and it also lowercases so the manual `to_ascii_lowercase()` on the tag can use its return value:
```rust
let target_pubkey = check_pubkey_hex(target_pubkey, "target_pubkey")?;
let mut tags = vec![
tag(&["h", &channel_id.to_string()])?,
tag(&["p", &target_pubkey])?,
];
```
That also moves the error from `InvalidDiffMeta` to `InvalidInput`, matching the sibling builders. It is a behavior change for any caller currently passing 65+ hex chars, but such a caller's event is already rejected by the relay, so the change converts a remote failure into a local one.
Tests worth adding alongside, mirroring `moderation_ban_rejects_overlong_pubkey`: overlong rejected for both builders, exact-64 still accepted and lowercased, and the existing short/non-hex rejections preserved.
Happy to send that PR if you want it — it is a two-line change plus tests. Flagging rather than patching unilaterally since it tightens a public SDK signature's accepted input.
Contributor guide
Assessment
This issue has not been assessed yet.