Skip to content

parse_ref_updates: validate SHAs are hex, not just length 40 #398

Description

@Gravirei

Summary

parse_ref_updates in crates/gitlawb-node/src/api/repos.rs accepts a 40-byte string as a SHA as long as its length is 40. It never verifies the bytes are [0-9a-f]. A peer-supplied pkt-line with non-hex content (e.g. ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ <new-sha> refs/heads/main) is silently accepted as a RefUpdate.

Location

crates/gitlawb-node/src/api/repos.rs:3197

if parts.len() == 3 && parts[0].len() == 40 && parts[1].len() == 40 {
    updates.push(RefUpdate {
        old_sha: parts[0].to_string(),
        new_sha: parts[1].to_string(),
        ref_name: parts[2].to_string(),
    });
}

Why it matters

new_sha (and old_sha) flow unchecked from this parser into:

  • RefUpdateEvent.new_sha published on gossipsub (crates/gitlawb-node/src/api/repos.rs:2767)
  • branch_cid DB upsert keyed on new_sha (:2754)
  • Peer sync-notify JSON (:1936)
  • RefUpdateBroadcast over broadcast channels (:2792)

So opaque, non-hex garbage becomes a canonical-looking identifier across the gossip wire, peer sync notifications, and persisted branch-CID rows. The git layer's pack validation still gates the actual write — this isn't a write-surface compromise — but it pollutes the wire format and persistence layer, which can cause downstream lookup/join failures (e.g. cid_map.get(new_sha) returning None for what looks like a valid SHA to a careless consumer) and obscures real pushes in logs.

Repro (conceptual)

Construct a pkt-line body:

00d6ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/heads/main\0capabilities\n0000

(where 00d6 is the 4-hex pkt-line length and the body is the non-hex old SHA + valid new SHA + refname). parse_ref_updates will accept both 40-byte strings as SHAs and emit a RefUpdate whose old_sha is ZZZZ... and new_sha is aaa....

Suggested fix

Replace the length-only check with a hex check:

fn is_hex_sha(s: &str) -> bool {
    s.len() == 40 && s.bytes().all(|b| b.is_ascii_hexdigit())
}

if parts.len() == 3 && is_hex_sha(parts[0]) && is_hex_sha(parts[1]) {
    updates.push(RefUpdate { ... });
}

Add a unit test covering:

  • A valid 40-hex old SHA + 40-hex new SHA → accepted.
  • A 40-byte non-hex old SHA → dropped.
  • A 39-byte / 41-byte SHA → dropped.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    crate:nodegitlawb-node — the serving node and REST APIkind:bugDefect fix — wrong or unsafe behaviorsev:mediumDegraded but workaround existssubsystem:apiNode REST API request/response surfacesubsystem:peersPeer announce, discovery, and registrysubsystem:storageBlob/object store, Arweave, IPFS, archives

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions