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.

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: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