Skip to content

[CALCITE-7674] Add Avatica threat model - #313

Open
rubenada wants to merge 3 commits into
apache:mainfrom
rubenada:CALCITE-7674
Open

[CALCITE-7674] Add Avatica threat model#313
rubenada wants to merge 3 commits into
apache:mainfrom
rubenada:CALCITE-7674

Conversation

@rubenada

Copy link
Copy Markdown
Contributor

CALCITE-7674 Add Avatica threat model

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed this against the rubric we use for scanner-facing models. Short version: it passes comfortably, and it's stronger than most first drafts we see — including several we've written ourselves. Every section a scanning agent depends on is present and substantive: scope and the Calcite boundary split, attacker profiles, the P1–P5 properties, always/never-a-vulnerability lists, downstream responsibilities, and triage dispositions.

Three things I'd single out as unusually good, because they're the parts that actually change triage outcomes:

  • The Inputs table maps every input to a governing rule and then closes the set — "A report that reaches a sink not covered here is a model gap." That catch-all is what stops the table going stale silently.
  • Model gap is a disposition with a feedback loop — escalate, rule, then update this document so the next report of its kind is no longer a gap. Most models stop at the escalation.
  • The DoS section is honest about missing controls rather than overclaiming, and pairs with a Known limitation disposition instead of pretending the bound exists.

Five things worth your judgement, roughly in order of value:

1. Does this model cover apache/calcite-avatica-go? (substantive — the one I'd most want answered)

The model is JVM-centric throughout: P1 says "client JVM", the assets list has "The Avatica client's JVM", and the class-loading sections are Class.forName / asSubclass. The Go client speaks the same wire protocol from a completely different codebase, where none of that applies. Right now a Go-client finding lands as Model gap by default. One sentence either way — "this model covers the Java server and JDBC driver; the Go client is not covered" or "…and applies to the Go client except where it names JVM mechanisms" — closes it before the first report rather than after.

2. "Always a vulnerability" #7 vs the DoS section.

Item 7 says single-frame resource exhaustion is a vulnerability; the DoS section says treat those gaps as known limitations not per-report vulnerabilities until controls land. Known limitation in the dispositions reconciles them, but a reporter reading item 7 alone will expect Valid and feel bait-and-switched. A clause on item 7 — "except where Denial of service records the bound as not yet landed" — makes the two agree on the page.

3. SECURITY.md link text points at a file that doesn't exist.

[THREAT_MODEL.md](site/_docs/security_threat_model.md) — the path is right, the label names a non-existent THREAT_MODEL.md. Cosmetic, but the label is what a human follows.

4. The CVE-2020-13955 entry doesn't state its rule.

The section says these are recorded "as ground truth for the rule they establish", and CVE-2022-36364 does exactly that — it grounds the surprising-class-loading rule. The 2020 entry currently reads "a Calcite Avatica issue in an earlier release", which establishes nothing. I've deliberately not guessed what it was; you'll know whether it grounds a rule already in the model or should just be dropped.

5. Status: draft for PMC discussion; not yet ratified — worth flipping when this merges, so the merged document doesn't describe itself as unratified.

None of these block anything. Happy to push any of them as commits on this branch if that's easier than doing it yourself — your call, it's your PR.

@rubenada

Copy link
Copy Markdown
Contributor Author

Thanks a lot for the review @potiuk !
Regarding your comments:

  1. This model does not cover apache/calcite-avatica-go, Avatica-go would require its own model, and for the moment this is out of the scope.
  2. I've added your proposed clause, please take a look.
  3. Well spotted. Fixed.
  4. I double checked, and actually that CVE was related to Calcite, not Avatica, so I've removed it.
  5. I've removed that "status" mention, so that it's ready when we decide to merge this; but I'd like to have at least one other PMC approval.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants