Document and justify all CodeQL security findings as acceptable
for CyberChef's specific use case as a security analysis tool.
CodeQL Findings Analysis:
--------------------------
Analyzed 6 open HIGH severity findings:
✅ 3x Incomplete string escaping - Already reviewed (lgtm tags)
✅ 2x DOM innerHTML usage - False positive (hardcoded content)
✅ 1x Weak password hash - Intentional tool behavior
All findings are ACCEPTED - No code fixes required.
Changes:
--------
1. CODEQL_FINDINGS_ASSESSMENT.md (NEW)
- Comprehensive analysis of all 6 findings
- Detailed justification for each
- Security context and threat model
- Comparison: Tool vs Production App
- ~500 lines of documentation
2. SECURITY.md (UPDATED)
- Added "Security Context and Threat Model" section
- Explained CyberChef's unique security posture
- Documented known CodeQL exceptions
- Guidance for reviewing future findings
- Linked to detailed assessment
3. src/web/waiters/BindingsWaiter.mjs (UPDATED)
- Added CodeQL suppression comments
- lgtm [js/xss-through-dom] annotations
- Explanatory comments for reviewers
- No functional changes
Findings Summary:
-----------------
Issue #1-3: Incomplete String Escaping (HIGH)
Location: PHPDeserialize.mjs, JSONBeautify.mjs, Utils.mjs
Status: ✅ ACCEPTED - Already marked with lgtm tags
Reason: Intentional behavior for encoding/decoding tools
Action: None - existing annotations are sufficient
Issue #4-5: DOM Text Reinterpreted as HTML (HIGH)
Location: BindingsWaiter.mjs:300-301
Status: ✅ FALSE POSITIVE - Hardcoded content only
Reason: All data-help attributes are static strings in source code
Help text intentionally contains HTML for formatting
No user input flows to these attributes
Action: Added suppression comments for documentation
Issue #6: Insufficient Password Hash (HIGH)
Location: DeriveEVPKey.mjs:72
Status: ✅ ACCEPTED - Already marked with lgtm tag
Reason: This is a KEY DERIVATION TOOL, not an auth system
Users control iteration count (1-999999)
Weak settings are intentional for compatibility/testing
Action: None - existing annotation is sufficient
Security Context:
-----------------
CyberChef is a CLIENT-SIDE TOOL for security analysts, NOT:
❌ Multi-tenant web application
❌ User data storage system
❌ Authentication system
❌ Production backend service
Therefore:
✅ Weak crypto is INTENTIONAL (reverse engineering)
✅ Deserialization is INTENTIONAL (data analysis)
✅ XSS risk is MINIMAL (single-user, client-side)
✅ "Vulnerabilities" are often FEATURES
Threat Model:
-------------
Traditional web app security doesn't fully apply:
- Users ARE the "attackers" (security professionals)
- All input/output is user-controlled
- No persistent storage
- No multi-user environment
- Offline-capable by design
Risk Assessment:
----------------
ACTUAL Risk: LOW
- No remote exploitation vectors
- No data exfiltration risk
- No privilege escalation
- Client-side only
PERCEIVED Risk: HIGH (by static analysis)
- Tools flag intentional behavior
- Context-unaware scanning
- Production app assumptions
Recommendations:
----------------
Future CodeQL Reviews:
1. Check for lgtm annotations first
2. Consider CyberChef's tool context
3. Refer to CODEQL_FINDINGS_ASSESSMENT.md
4. Ask: "Is this a feature or a bug?"
DO NOT:
❌ "Fix" lgtm-annotated code without review
❌ Remove intentional weak crypto
❌ Break encoding/decoding functionality
❌ Apply production app security to tool
DO:
✅ Review actual user input flows
✅ Check for NEW patterns (not documented)
✅ Validate external library usage
✅ Monitor dependency vulnerabilities
Documentation:
--------------
- CODEQL_FINDINGS_ASSESSMENT.md: Detailed technical analysis
- SECURITY.md: Security policy with threat model
- Code comments: Inline suppression justifications
This commit ensures future contributors understand why these
findings are not security issues in CyberChef's context.
No functional changes. Documentation only.
Resolves two TODO comments by implementing a compression method lookup
table similar to the existing cipher suites and extensions lookups.
Now displays human-readable names (null, DEFLATE, LZS) instead of
raw numeric values when parsing TLS compression methods.
Changes:
- Added COMPRESSION_METHODS_LOOKUP constant with standard TLS compression methods
- Updated parseServerHello to use lookup for compression method display
- Updated parseCompressionMethods to use lookup for compression method display
Addresses bug report #2008
Added explicit support for octal IP addresses.
Changed approach to IPv4 regex to be string manipulation generated.
Added some unit tests for IP address parsing - probably not full coverage.
Added lookahead and lookbehind tricks to resolve warned issue that 1.2.3.256 would still be extracted as 1.2.3.25. Now only accepts valid IP addresses. Warning replaced with clause about infinite length dotted decimal forms.
