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

added
sev:lowCosmetic, cleanup, or nice-to-have
subsystem:identityDID/UCAN, http-sig auth, push authorization
kind:bugDefect fix — wrong or unsafe behavior
crate:attestgitlawb-attest — attestation and verification
on Aug 15, 2026

beardthelion commented on Aug 15, 2026

@beardthelion
CollaboratorAuthor

Correcting the exposure claim in the body above, which is stronger than the facts support.

I wrote that this is a "release-please-versioned publishable crate" and that the exposure is therefore "to a future or external consumer". The versioning half is accurate, the implication is not. gitlawb-attest is not on crates.io (the API returns "crate gitlawb-attest does not exist"), and neither is gitlawb-core. .github/workflows/release.yml has no cargo publish step at all: it ships npm packages, Docker images, and binaries. release-please only rewrites the version string, via a generic extra-files entry pointed at crates/gitlawb-attest/Cargo.toml, matching the # x-release-please-version annotation on line 4.

So there is no external consumer, because there has never been a publish. Combined with the zero reverse dependencies already noted, the accurate statement is that this crate is entirely unwired, not that it is a shipped library with a latent hazard.

That lowers the urgency rather than the correctness. The defect is still real and the fix is still one bound. But there is nothing to protect today, so this and #359 are best treated as preconditions for wiring the crate up rather than as work to schedule on their own. Whoever does that wiring should land both first, since the same reasoning that makes them harmless now makes them live the moment a request path reaches verify_signature.

Same correction applies to #359's severity reasoning, which leaned on the same "downstream users of the published crate" framing.

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