docs: Add comprehensive security assessment report - #408
Conversation
This security assessment examines the Kitsune2 P2P networking library with a focus on remotely exploitable vulnerabilities. The assessment identified 2 critical, 6 high, 5 medium, and 4 low severity issues. Critical findings: - CRIT-1: Unbounded peer store growth - CRIT-2: Protobuf deserialization without size pre-checks High-priority findings include unbounded op store growth, JSON deserialization vulnerabilities, and fetch queue management issues. The report also documents several positive security practices including gossip round limits, signature verification, comprehensive blocking system, and proper timeout mechanisms. Recommendations include adding resource limits, size validation, and improved rate limiting across multiple layers.
WalkthroughAdds a new Kitsune2 security assessment document (SECURITY_ASSESSMENT.md v0.4.0-dev.0) analyzing the remote attack surface of P2P/DHT networking. The report enumerates severity-classified findings (2 Critical, 5 High, 5 Medium, 4 Low), attacker scenarios, evidence, remediation steps, testing plans, and a risk trajectory. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The following will be added to the changelog [0.4.0-dev.1] - 2025-12-11Documentation
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
SECURITY_ASSESSMENT.md (1)
288-310: Optional: Replace weak intensifiers with precise adjectives.Lines 294 ("extremely large"), 367 ("properly cleaned"), and 522 ("very large") use over-used intensifiers. For a security report, precision over intensifiers strengthens the writing:
- Line 294: "extremely large vectors" → "unbounded vectors"
- Line 367: "properly cleaned up" → "thoroughly cleaned up" or "completely cleaned up"
- Line 522: "very large disconnect reasons" → "large disconnect reasons" (intensifier unnecessary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
SECURITY_ASSESSMENT.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T20:27:59.798Z
Learnt from: ThetaSinner
Repo: holochain/kitsune2 PR: 305
File: crates/core/src/factories/core_fetch/message_handler.rs:0-0
Timestamp: 2025-08-14T20:27:59.798Z
Learning: In the Holochain Kitsune2 codebase, channel closure errors in fetch message handlers are primarily expected during shutdown scenarios, reducing their practical impact compared to queue-full conditions.
Applied to files:
SECURITY_ASSESSMENT.md
🪛 LanguageTool
SECURITY_ASSESSMENT.md
[style] ~294-~294: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...ing. An attacker can send messages with extremely large vectors causing memory exhaustion durin...
(EN_WEAK_ADJECTIVE)
[style] ~367-~367: Consider an alternative adverb to strengthen your wording.
Context: ... - Memory leaks if session state is not properly cleaned up Recommended Fix: - Add ...
(PROPERLY_THOROUGHLY)
[style] ~522-~522: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: .... While connections are closing anyway, very large disconnect reasons could cause issues. ...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
SECURITY_ASSESSMENT.md
808-808: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
820-820: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
840-840: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
851-851: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
SECURITY_ASSESSMENT.md (6)
1-100: Comprehensive assessment with strong structure.The document effectively communicates the security posture of Kitsune2 with clear severity stratification, specific code locations, realistic attack scenarios, and actionable remediation guidance. The inclusion of positive security findings balances the risk narrative and acknowledges existing controls. Well done.
39-376: Detailed and evidence-backed findings across severity levels.Each critical and high-severity finding includes specific code locations, realistic attack scenarios, impact analysis, and concrete remediation steps. The mapping between problems (e.g., unbounded peer/op stores, hardcoded limits) and solutions (e.g., LRU eviction, configurable bounds) is clear and actionable.
Consider spot-checking a few code location references (e.g., lines 260–306 in
mem_peer_store.rsfor CRIT-1, line 189 inmem_op_store.rsfor HIGH-1) against the actual codebase to confirm the evidence snippets match current code.
378-577: Medium and low findings appropriately prioritized.The medium-severity findings (hardcoded limits, timestamp bounds, state machine complexity, rate limiting gaps) are reasonable, and low-severity items (validation, counters, metrics) correctly recognize edge cases and observability. Prioritization focuses on the highest-impact resource exhaustion issues first.
580-707: Strong cross-cutting analysis with pattern synthesis.The vulnerability patterns section effectively synthesizes findings across categories (crashes, oversized payloads, unbounded storage, resource exhaustion, loops/recursion), acknowledges existing mitigations (gossip round limits, timeouts, blocking system), and identifies systematic gaps. This meta-level analysis helps future reviewers understand failure modes and design principles.
237-285: HIGH-3 finding is consistent with operational context.The emphasis on queue-full conditions (rather than channel closure errors) as the primary DoS vector aligns with prior learnings indicating that channel closure in fetch handlers is primarily expected during shutdown and is lower impact than queue-full scenarios. The document correctly prioritizes the operational vulnerability (silent request drops) over transient shutdown errors.
709-787: Recommendations are actionable and well-prioritized.The three-tier prioritization (immediate, soon, improvements) provides clear guidance. Concrete configuration defaults and specific testing scenarios (fuzz, load, adversarial, integration) enable implementation teams to proceed with confidence. Recommended fixes map directly to identified vulnerabilities.
|
|
||
| ## HIGH SEVERITY | ||
|
|
||
| ### HIGH-1: Op Store JSON Deserialization Vulnerability |
There was a problem hiding this comment.
Op store is not used in production. Demote this or delete altogether.
|
|
||
| --- | ||
|
|
||
| ### HIGH-2: No Bounds on Op Store Size |
The in-memory op store (mem_op_store) with JSON deserialization is only used for development and testing purposes. Production integrations provide their own persistent op store implementation. Changes: - Removed HIGH-1 (JSON deserialization vulnerability) - Renumbered HIGH-2 through HIGH-6 to HIGH-1 through HIGH-5 - Updated severity counts: 2 critical, 5 high (was 6), 5 medium, 4 low - Updated all references to renumbered findings - Added note in vulnerability patterns section clarifying mem_op_store is development-only - Updated recommended actions to reflect changes This addresses code review feedback correctly identifying that the in-memory op store is not part of the production attack surface.
Deploying kitsune2 with
|
| Latest commit: |
9d526df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fe1dcdac.kitsune2.pages.dev |
| Branch Preview URL: | https://security-assessment-2025-12.kitsune2.pages.dev |
|
✔️ f2eeaa1...9d526df - Conventional commits check succeeded. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
SECURITY_ASSESSMENT.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
SECURITY_ASSESSMENT.md
[style] ~240-~240: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...ing. An attacker can send messages with extremely large vectors causing memory exhaustion durin...
(EN_WEAK_ADJECTIVE)
[style] ~313-~313: Consider an alternative adverb to strengthen your wording.
Context: ... - Memory leaks if session state is not properly cleaned up Recommended Fix: - Add ...
(PROPERLY_THOROUGHLY)
[style] ~468-~468: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: .... While connections are closing anyway, very large disconnect reasons could cause issues. ...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
SECURITY_ASSESSMENT.md
754-754: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
766-766: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
786-786: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
797-797: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: docker-build-bootstrap-auth-test
- GitHub Check: nix-build (ubuntu-24.04-arm)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: docker-build-bootstrap
- GitHub Check: test-windows (windows-latest)
- GitHub Check: nix-build (ubuntu-latest)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
SECURITY_ASSESSMENT.md (1)
138-181: Clarify production relevance of HIGH-1 (Op Store) finding.The HIGH-1 finding presents unbounded op store growth as a high-severity issue, but the document later clarifies (line 541-542) that the in-memory op store is "only used for development/testing and is not exposed in production integrations." This creates a disconnect: a high-priority finding shouldn't apply to non-production code.
Address the past review feedback to clarify or demote this finding by either:
- Adding a note within HIGH-1 explaining it's development-only
- Demoting it to LOW/informational severity
- Removing it entirely if it poses no production risk
Example updated opening for option 1:
### HIGH-1: No Bounds on Op Store Size **Severity:** High **Attack Vector:** Remote **Component:** `crates/core/src/factories/mem_op_store.rs` **Description:** -The op store has no limits on the number or total size of ops it will store. An attacker can send unlimited ops through gossip or publish protocols. +The op store has no limits on the number or total size of ops it will store. An attacker can send unlimited ops through gossip or publish protocols. + +**NOTE:** The in-memory implementation (mem_op_store.rs) is used for development/testing only; production integrations provide their own persistent op store. However, the pattern is documented here as a template concern for custom implementations.
| ## Appendix: Attack Scenarios | ||
|
|
||
| ### Scenario 1: Memory Exhaustion via Peer Store | ||
| ``` | ||
| 1. Attacker generates 100,000 unique keypairs | ||
| 2. For each keypair, create agent info with expires_at = now + 1 year | ||
| 3. Sign each agent info properly | ||
| 4. Send agent infos via gossip Agents messages | ||
| 5. Victim stores all 100K agent infos (no limit) | ||
| 6. Each agent info ~1KB = 100MB minimum | ||
| 7. Repeat from multiple peer URLs to evade per-peer limits | ||
| Result: OOM crash | ||
| ``` | ||
|
|
||
| ### Scenario 2: Gossip Round State Exhaustion (MITIGATED) | ||
| ``` | ||
| **NOTE: This attack is mitigated by max_concurrent_accepted_rounds (default 10)** | ||
|
|
||
| Theoretical attack if limit were disabled: | ||
| 1. Attacker controls 1000 peer URLs (different IPs/relays) | ||
| 2. From each URL, initiate gossip round | ||
| 3. Don't complete rounds - let them timeout | ||
| 4. Before timeout (15s), initiate more rounds | ||
| 5. Gossip state HashMap grows to 1000+ entries | ||
| 6. Each entry contains DHT snapshots, arc sets, etc. | ||
| 7. Memory consumption: 1000 rounds * ~100KB = 100MB | ||
|
|
||
| Actual result with default config: | ||
| - Only 10 concurrent accepted rounds allowed | ||
| - 11th attacker receives "Busy" response | ||
| - Limited to ~1MB memory for gossip state | ||
| Status: MITIGATED by existing controls | ||
| ``` | ||
|
|
||
| ### Scenario 3: Large Message Flood | ||
| ``` | ||
| 1. Attacker sends 1MB protobuf messages repeatedly | ||
| 2. Each message is valid but maximum size | ||
| 3. 1000 messages/second = 1GB/s deserialization load | ||
| 4. CPU pegged on protobuf decode | ||
| 5. Memory spikes from allocation during decode | ||
| Result: DoS via resource exhaustion | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| **End of Report** |
There was a problem hiding this comment.
Fix markdown formatting violations in appendix and conclusion.
The code blocks lack language specifications and the final section uses emphasis instead of a proper heading, violating markdown lint rules and reducing accessibility.
Apply this diff:
### Scenario 1: Memory Exhaustion via Peer Store
-```
+```text
1. Attacker generates 100,000 unique keypairs
2. For each keypair, create agent info with expires_at = now + 1 year
3. Sign each agent info properly
4. Send agent infos via gossip Agents messages
5. Victim stores all 100K agent infos (no limit)
6. Each agent info ~1KB = 100MB minimum
7. Repeat from multiple peer URLs to evade per-peer limits
Result: OOM crashScenario 2: Gossip Round State Exhaustion (MITIGATED)
- +text
NOTE: This attack is mitigated by max_concurrent_accepted_rounds (default 10)
Theoretical attack if limit were disabled:
...
Status: MITIGATED by existing controls
### Scenario 3: Large Message Flood
-```
+```text
1. Attacker sends 1MB protobuf messages repeatedly
2. Each message is valid but maximum size
3. 1000 messages/second = 1GB/s deserialization load
4. CPU pegged on protobuf decode
5. Memory spikes from allocation during decode
Result: DoS via resource exhaustion
-End of Report
+## End of Report
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
754-754: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
766-766: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
786-786: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
797-797: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In SECURITY_ASSESSMENT.md around lines 751 to 797, the appendix uses code fences
without language tags and the final section uses bold emphasis instead of a
heading; update the three fenced code blocks to use text (replace with
Message Flood block) and replace the final bold "**End of Report**" with a
proper markdown heading "## End of Report".
|
|
||
| --- | ||
|
|
||
| ### HIGH-4: Bootstrap Server Body Size Limit Too Small |
There was a problem hiding this comment.
If anything I imagine 1KB is bigger than necessary, but its not an attack vector.
|
|
||
| **Impact:** | ||
| - Legitimate agent infos may be rejected if signed data exceeds 1KB | ||
| - No rate limiting on 413 responses |
There was a problem hiding this comment.
This is actually an issue - we should have rate limiting on all requests.
|
|
||
| --- | ||
|
|
||
| ### HIGH-5: Lack of Gossip Session ID Validation |
|
|
||
| --- | ||
|
|
||
| ### MED-2: Timestamp Manipulation in Peer Meta Store |
There was a problem hiding this comment.
We're setting these timestamps ourselves so this isn't an attack.
| - Protocol deadlocks | ||
|
|
||
| **Recommended Fix:** | ||
| - Add comprehensive fuzz testing of gossip protocol |
There was a problem hiding this comment.
Yes to fuzz testing, but the rest I don't think are necessary unless the fuzz testing turns up something. The state machine logic is clear and probably as simple as possible.
|
|
||
| --- | ||
|
|
||
| ### MED-4: DHT Hash Collision Handling Not Documented |
There was a problem hiding this comment.
We're only using XOR for:
- Creating "aggregate" hashes to more efficiently compare our dht state with peers. Collision isn't relevant there.
- Generating the location byte postfix, which are explicitly a reduction of the hash space and are fine to collide.
I don't think there are risks from either use. I don't know if we can or should care about general hash collision from content-addressed data. The only solution would be to increase the hash length I think.
| - Connection table exhaustion | ||
|
|
||
| **Recommended Fix:** | ||
| - Add connection rate limiting per peer |
There was a problem hiding this comment.
I think rate limiting at connection level is the generalized solution, there are probably a bunch of places where it could be exploited, not specific to preflight.
|
|
||
| --- | ||
|
|
||
| ### LOW-4: Missing Metrics for Attack Detection |
There was a problem hiding this comment.
Some of this might be useful to expose to apps, but Idk if logs are sufficient.
mattyg
left a comment
mattyg
left a comment
There was a problem hiding this comment.
Nice, I think it would be better to extract the useful ones into issues rather than committing this into the repo where it will rot.
|
Another issue not flagged here is that I don't think we have a limit on the number of open connections. |
3 participants
This security assessment examines the Kitsune2 P2P networking library with a focus on remotely exploitable vulnerabilities. The assessment identified 2 critical, 6 high, 5 medium, and 4 low severity issues.
Critical findings:
High-priority findings include unbounded op store growth, JSON deserialization vulnerabilities, and fetch queue management issues.
The report also documents several positive security practices including gossip round limits, signature verification, comprehensive blocking system, and proper timeout mechanisms.
Recommendations include adding resource limits, size validation, and improved rate limiting across multiple layers.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.