Skip to content

gitlawb-attest's did:key parser is missing the length bound gitlawb-core already added, so base58 decode is unbounded #360

Description

@beardthelion

Two copies of the same parser, one hardened and one not.

crates/gitlawb-core/src/did.rs:83-97 carries the guard, added by commit 73bf132c ("resolve the key
on the unproven insert path, and bound the decode"), with the reasoning in the comment:

// Refuse an oversized id before decoding. base58 decoding is quadratic
// in its input, and both callers of this function take the string from
// an untrusted request...
const MAX_METHOD_ID_LEN: usize = 64;
if self.method_id().len() > MAX_METHOD_ID_LEN {
    return Err(Error::InvalidDid("did:key method-specific id too long".to_string()));
}

crates/gitlawb-attest/src/attestation.rs:196-206 is the second copy and has no such check:

let method_id = did.strip_prefix("did:key:")...?;
if !method_id.starts_with('z') { ... }
let (base, bytes) = multibase::decode(method_id).map_err(...)?;

The length checks at :212-216 (bytes.len() != 34) run after the decode, so they cannot help.

The cost is real

Measured against the pinned versions (bs58 0.5.1 via multibase 0.9.2, Cargo.lock lines 1978-1979 and
4698-4699), release build:

method-id length dense body leading-zero body
64 chars (a real did:key) 2.2 µs 920 ns
1 KB 95.8 µs 19.0 µs
10 KB 9.64 ms 1.16 ms
100 KB 941 ms 131 ms
1 MB 95.7 s 17.0 s

Ten times the length gives roughly a hundred times the work, which is the quadratic accumulator loop
in bs58-0.5.1/src/decode.rs:411-442. multibase-0.9.2/src/lib.rs:33-39 passes the input straight
through with no size check of its own.

The decode is reached from verify_signature (attestation.rs:98-107) after only a hex compare and
the type-grammar check, so nothing expensive gates it.

Why this is low and not a DoS

gitlawb-attest has zero reverse dependencies. It appears once in Cargo.lock as a package and in no
other crate's dependency list; gitlawb-node, gl, git-remote-gitlawb, and icaptcha-client each
depend on gitlawb-core only. Every caller of verifying_key_from_did_key today is one of the crate's
own tests.

So there is no handler, no body-size question, and no attestation-count question. The crate is a
release-please-versioned publishable member (release-please-config.json:15, version 0.7.1), so the
exposure is to a future or external consumer that wires it to a request path, which is precisely the
scenario did.rs documents.

Distinct from the small-order key work

#313, #315, and PR #314 concern accepting small-order Ed25519 keys, which is a property of
VerifyingKey::from_bytes and the X25519 conversion after a successful decode. This is decode cost
before any key material exists. Different function, different failure mode.

Fix direction

Apply core's bound to the attest copy: reject a method-id longer than MAX_METHOD_ID_LEN before
calling multibase::decode. Better still, have gitlawb-attest call gitlawb-core's parser rather
than keeping a second one, since the two have already drifted once and nothing would catch the next
drift. The crate does depend on gitlawb-core in dev-dependencies today, so promoting that is the
larger but more durable change.

Wider point worth recording

Both findings filed against this crate today (this and the sibling on verify_all) are the same
shape: a rule that exists elsewhere in the workspace is absent here, and nothing notices because the
crate has no consumers exercising it. If gitlawb-attest is meant to stay a published library ahead
of its in-tree use, it is worth deciding what keeps it in step with gitlawb-core, because right now
nothing does.

Metadata

Metadata

Assignees

No one assigned

    Labels

    crate:attestgitlawb-attest — attestation and verificationkind:bugDefect fix — wrong or unsafe behaviorsev:lowCosmetic, cleanup, or nice-to-havesubsystem:identityDID/UCAN, http-sig auth, push authorization

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions