Skip to content

Commit 0172de0

Browse files
doublegatedoublegate
authored
Merge pull request #38 from doublegate/copilot/fix-code-scanning-vulnerabilities
Document RFC 5389-mandated cryptographic algorithms in STUN implementation
2 parents 58e1826 + 81f2f9c commit 0172de0

File tree

5 files changed

+258
-7
lines changed

5 files changed

+258
-7
lines changed

‎.github/codeql/codeql-config.yml‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
# CodeQL Configuration for WRAITH Protocol
2+
#
3+
# This configuration file customizes CodeQL analysis to handle protocol-specific
4+
# requirements and justified use of legacy algorithms.
5+
6+
name: "WRAITH CodeQL Config"
7+
8+
# Disable specific queries that flag RFC-mandated algorithm usage
9+
# These are not security vulnerabilities in our context
10+
disable-default-queries: false
11+
12+
# Query configuration
13+
queries:
14+
- uses: security-extended
15+
16+
# Query filters to handle known false positives
17+
# Note: CodeQL for Rust doesn't yet support query-level suppressions via config,
18+
# but we document the justifications here for transparency
19+
20+
# Known justified uses (not actual vulnerabilities):
21+
# 1. MD5 in crates/wraith-discovery/src/nat/stun.rs
22+
# - Required by RFC 5389 Section 15.4 for STUN long-term credential derivation
23+
# - Used only for protocol compliance, not for collision resistance
24+
# - Isolated to STUN implementation, not used elsewhere
25+
#
26+
# 2. SHA1 in crates/wraith-discovery/src/nat/stun.rs
27+
# - Required by RFC 5389 Section 15.4 for STUN MESSAGE-INTEGRITY (HMAC-SHA1)
28+
# - HMAC construction provides better security properties than plain SHA1
29+
# - Protocol-mandated, cannot be changed without breaking STUN compatibility
30+
#
31+
# 3. HMAC-SHA1 in crates/wraith-discovery/src/nat/stun.rs
32+
# - Required by RFC 5389 Section 15.4 for STUN authentication
33+
# - HMAC provides keyed-hash security suitable for message authentication
34+
# - Used only for STUN protocol, not for general cryptographic operations
35+
36+
# For documentation of these justified uses, see:
37+
# - docs/security/rfc-required-algorithms.md
38+
# - Inline comments in crates/wraith-discovery/src/nat/stun.rs

‎.github/workflows/codeql.yml‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,8 @@ jobs:
6060
uses: github/codeql-action/init@v4
6161
with:
6262
languages: ${{ matrix.language }}
63-
# Use security-extended query suite for comprehensive analysis
64-
queries: security-extended
65-
# Optional: Add custom queries
66-
# config-file: ./.github/codeql/codeql-config.yml
63+
# Use custom config that documents justified algorithm usage
64+
config-file: ./.github/codeql/codeql-config.yml
6765

6866
# Install system dependencies required for Tauri (even though we exclude it)
6967
- name: Install system dependencies (Ubuntu)

‎crates/wraith-discovery/src/nat/stun.rs‎

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,28 @@
88
//! This implementation includes RFC 5389 MESSAGE-INTEGRITY authentication using
99
//! HMAC-SHA1, transaction ID validation, and fingerprint verification for secure
1010
//! STUN operations.
11+
//!
12+
//! # Security Note on Cryptographic Algorithms
13+
//!
14+
//! **IMPORTANT:** This module uses MD5 and SHA1 algorithms as mandated by RFC 5389
15+
//! for STUN protocol compliance. While MD5 and SHA1 are considered cryptographically
16+
//! weak for general purposes, their use here is:
17+
//!
18+
//! 1. **RFC-Mandated**: RFC 5389 Section 15.4 specifically requires HMAC-SHA1 for
19+
//! MESSAGE-INTEGRITY and MD5 for long-term credential key derivation.
20+
//! 2. **Protocol-Specific**: These algorithms are used only for STUN protocol
21+
//! operations, not for general cryptographic purposes in WRAITH.
22+
//! 3. **Contextually Safe**: In the STUN context with proper authentication and
23+
//! message integrity checks, the protocol provides adequate security for its
24+
//! intended NAT traversal purpose.
25+
//!
26+
//! **DO NOT** use MD5 or SHA1 from this module for any other cryptographic
27+
//! operations. For general cryptography in WRAITH, use the strong algorithms
28+
//! provided in the `wraith-crypto` crate (e.g., BLAKE3, ChaCha20-Poly1305, X25519, Ed25519).
1129
1230
use hmac::{Hmac, Mac};
31+
// These imports are required by RFC 5389 for STUN protocol compliance.
32+
// DO NOT use for general cryptographic purposes - see security note above.
1333
use md5::Md5;
1434
use sha1::Sha1;
1535
use std::collections::HashMap;
@@ -139,9 +159,17 @@ impl StunAuthentication {
139159
///
140160
/// For long-term credentials: MD5(username:realm:password)
141161
/// For short-term credentials: password
162+
///
163+
/// # Security Note
164+
///
165+
/// This function uses MD5 as required by RFC 5389 Section 15.4 for STUN
166+
/// long-term credential key derivation. This is a protocol requirement
167+
/// and cannot be changed without breaking STUN compatibility.
168+
/// MD5 is used here only for protocol compliance, not for collision resistance.
142169
fn derive_key(&self) -> Zeroizing<Vec<u8>> {
143170
if let Some(realm) = &self.realm {
144171
// Long-term credentials: MD5(username:realm:password)
172+
// Required by RFC 5389 Section 15.4 - DO NOT change to a different hash
145173
use md5::Digest;
146174
let mut hasher = Md5::new();
147175
hasher.update(self.username.as_bytes());
@@ -583,6 +611,13 @@ impl StunMessage {
583611
/// Computes HMAC-SHA1 over the message up to (but not including) the
584612
/// MESSAGE-INTEGRITY attribute itself. Must be called before encoding.
585613
///
614+
/// # Security Note
615+
///
616+
/// This function uses HMAC-SHA1 as mandated by RFC 5389 Section 15.4 for
617+
/// STUN MESSAGE-INTEGRITY computation. This is a protocol requirement and
618+
/// cannot be changed without breaking STUN compatibility with other implementations.
619+
/// The use of HMAC-SHA1 here is for protocol compliance only.
620+
///
586621
/// # Arguments
587622
///
588623
/// * `auth` - Authentication credentials
@@ -625,7 +660,7 @@ impl StunMessage {
625660
let msg_length = bytes.len() - HEADER_SIZE + 24;
626661
bytes[length_offset..length_offset + 2].copy_from_slice(&(msg_length as u16).to_be_bytes());
627662

628-
// Compute HMAC-SHA1
663+
// Compute HMAC-SHA1 (RFC 5389 Section 15.4 requirement)
629664
let key = auth.derive_key();
630665
type HmacSha1 = Hmac<Sha1>;
631666
let mut mac = HmacSha1::new_from_slice(&key).expect("HMAC can take key of any size");
@@ -642,6 +677,11 @@ impl StunMessage {
642677
///
643678
/// Validates that the MESSAGE-INTEGRITY HMAC is correct.
644679
///
680+
/// # Security Note
681+
///
682+
/// This function uses HMAC-SHA1 as mandated by RFC 5389 Section 15.4 for
683+
/// STUN MESSAGE-INTEGRITY verification. This is a protocol requirement.
684+
///
645685
/// # Arguments
646686
///
647687
/// * `auth` - Authentication credentials
@@ -700,7 +740,7 @@ impl StunMessage {
700740
bytes.extend_from_slice(&attr.encode(&temp_msg.transaction_id));
701741
}
702742

703-
// Compute expected HMAC
743+
// Compute expected HMAC (RFC 5389 Section 15.4 requirement)
704744
let key = auth.derive_key();
705745
type HmacSha1 = Hmac<Sha1>;
706746
let mut mac = HmacSha1::new_from_slice(&key).expect("HMAC can take key of any size");
@@ -1359,7 +1399,7 @@ mod tests {
13591399
fn test_authentication_key_derivation_long_term() {
13601400
let auth = StunAuthentication::new("user", "pass", Some("realm".to_string()));
13611401
let key = auth.derive_key();
1362-
// Long-term credentials use MD5(username:realm:password)
1402+
// Long-term credentials use MD5(username:realm:password) as mandated by RFC 5389
13631403
assert_eq!(key.len(), 16); // MD5 produces 16 bytes
13641404
}
13651405
}

‎docs/security/README.md‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
# Security Documentation
2+
3+
This directory contains security-related documentation for WRAITH Protocol.
4+
5+
## Contents
6+
7+
- **[rfc-required-algorithms.md](rfc-required-algorithms.md)** - Explanation of weak cryptographic algorithms required by protocol specifications (RFC 5389 STUN)
8+
9+
## Code Scanning Alerts
10+
11+
### Handling False Positives
12+
13+
WRAITH Protocol undergoes automated security scanning using CodeQL and other tools.
14+
Some alerts may appear as "vulnerabilities" when they are actually:
15+
16+
1. **Protocol requirements** - Algorithms mandated by RFCs for interoperability
17+
2. **Justified design decisions** - Documented trade-offs with security rationale
18+
3. **Tool limitations** - Scanner false positives or context misunderstandings
19+
20+
### Current Justified Uses
21+
22+
As of 2025-12-09, the following code patterns are flagged by security scanners
23+
but are **not vulnerabilities**:
24+
25+
1. **MD5 usage in STUN** (`crates/wraith-discovery/src/nat/stun.rs`)
26+
- Required by RFC 5389 for long-term credential derivation
27+
- See [rfc-required-algorithms.md](rfc-required-algorithms.md)
28+
29+
2. **SHA1 usage in STUN** (`crates/wraith-discovery/src/nat/stun.rs`)
30+
- Required by RFC 5389 for HMAC-SHA1 in MESSAGE-INTEGRITY
31+
- See [rfc-required-algorithms.md](rfc-required-algorithms.md)
32+
33+
3. **HMAC-SHA1 in STUN** (`crates/wraith-discovery/src/nat/stun.rs`)
34+
- Required by RFC 5389 for message authentication
35+
- HMAC construction provides adequate security for STUN
36+
- See [rfc-required-algorithms.md](rfc-required-algorithms.md)
37+
38+
### Review Process
39+
40+
When a new code scanning alert is raised:
41+
42+
1. **Investigate** - Understand what the scanner detected
43+
2. **Evaluate** - Determine if it's a real vulnerability or false positive
44+
3. **Fix or Document** - Either fix the issue or document why it's justified
45+
4. **Track** - Update this documentation with the decision
46+
47+
### Related Files
48+
49+
- `.github/codeql/codeql-config.yml` - CodeQL configuration with justifications
50+
- `.github/workflows/codeql.yml` - CodeQL scanning workflow
51+
- Inline comments in source files explaining security decisions
52+
53+
## Reporting Security Issues
54+
55+
If you discover a security vulnerability in WRAITH Protocol:
56+
57+
1. **Do not** open a public issue
58+
2. Review our [Security Policy](../../SECURITY.md)
59+
3. Report via the appropriate secure channel
60+
4. Allow time for a fix before public disclosure
61+
62+
## Security Best Practices
63+
64+
For developers working on WRAITH Protocol:
65+
66+
1. **Use strong cryptography** - Prefer BLAKE3, ChaCha20-Poly1305, Ed25519, X25519
67+
2. **Avoid weak algorithms** - Do not use MD5, SHA1, RC4, DES except when RFC-mandated
68+
3. **Document exceptions** - Any use of weak algorithms must be documented and justified
69+
4. **Validate inputs** - Sanitize paths, validate sizes, check bounds
70+
5. **Zeroize secrets** - Clear sensitive data from memory after use
71+
6. **Review dependencies** - Check for known vulnerabilities before adding deps
72+
73+
## Resources
74+
75+
- [OWASP Cryptographic Storage Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Cryptographic_Storage_Cheat_Sheet.html)
76+
- [NIST Cryptographic Standards](https://csrc.nist.gov/projects/cryptographic-standards-and-guidelines)
77+
- [Rust Security Guidelines](https://anssi-fr.github.io/rust-guide/)

‎docs/security/rfc-required-algorithms.md‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
# RFC-Required Cryptographic Algorithms
2+
3+
## Overview
4+
5+
This document explains the use of weak cryptographic algorithms (MD5, SHA1) in
6+
WRAITH Protocol where they are mandated by external protocol specifications.
7+
8+
## STUN Protocol (RFC 5389)
9+
10+
### Background
11+
12+
The STUN (Session Traversal Utilities for NAT) protocol is defined by RFC 5389
13+
and is used for NAT traversal and discovering server reflexive addresses. WRAITH
14+
uses STUN for peer discovery and connectivity.
15+
16+
### Mandated Algorithms
17+
18+
RFC 5389 Section 15.4 specifically requires:
19+
20+
1. **HMAC-SHA1** for MESSAGE-INTEGRITY attribute computation
21+
2. **MD5** for long-term credential key derivation
22+
23+
These requirements are part of the protocol specification and cannot be changed
24+
without breaking compatibility with other STUN implementations.
25+
26+
### Security Context
27+
28+
While MD5 and SHA1 are considered cryptographically weak for collision resistance,
29+
their use in STUN is acceptable because:
30+
31+
1. **Limited Scope**: Used only for STUN protocol operations, not for general
32+
cryptographic purposes
33+
2. **HMAC Construction**: HMAC-SHA1 provides better security properties than
34+
plain SHA1 due to the keyed-hash design
35+
3. **Protocol-Level Security**: STUN includes additional security mechanisms like
36+
transaction ID validation and fingerprint verification
37+
4. **Standard Compliance**: Required for interoperability with all STUN servers
38+
and clients
39+
40+
### Implementation Location
41+
42+
- **Module**: `crates/wraith-discovery/src/nat/stun.rs`
43+
- **Dependencies**: `md-5 = "0.10"`, `sha1 = "0.10"`, `hmac = "0.12"`
44+
- **Usage**:
45+
- `StunAuthentication::derive_key()` - MD5 for long-term credentials
46+
- `StunMessage::add_message_integrity()` - HMAC-SHA1 for message integrity
47+
- `StunMessage::verify_message_integrity()` - HMAC-SHA1 verification
48+
49+
## Mitigation Strategy
50+
51+
To prevent misuse of weak algorithms:
52+
53+
1. **Isolated Usage**: MD5 and SHA1 are only imported in the STUN module
54+
2. **Documentation**: Extensive comments warn against using these algorithms
55+
for other purposes
56+
3. **Strong Alternatives**: WRAITH's general cryptography (in `wraith-crypto`)
57+
uses modern algorithms:
58+
- BLAKE3 for hashing
59+
- ChaCha20-Poly1305 for AEAD encryption
60+
- Ed25519 for signatures
61+
- X25519 for key exchange
62+
63+
## Code Scanning Alerts
64+
65+
Code scanning tools (CodeQL, etc.) may flag the use of MD5 and SHA1 as security
66+
vulnerabilities. These alerts are **expected and acceptable** for the STUN
67+
implementation because:
68+
69+
1. The algorithms are mandated by RFC 5389
70+
2. Their use is properly documented and justified
71+
3. They are isolated to protocol-specific code
72+
4. Alternative algorithms would break STUN compatibility
73+
74+
### Alert Suppression
75+
76+
If code scanning alerts need to be suppressed:
77+
78+
- **File**: `crates/wraith-discovery/src/nat/stun.rs`
79+
- **Justification**: RFC 5389 compliance requirement
80+
- **Scope**: Limited to STUN MESSAGE-INTEGRITY and credential derivation
81+
- **Alternative**: None available while maintaining STUN compatibility
82+
83+
## References
84+
85+
- [RFC 5389: Session Traversal Utilities for NAT (STUN)](https://datatracker.ietf.org/doc/html/rfc5389)
86+
- [RFC 5389 Section 15.4: MESSAGE-INTEGRITY](https://datatracker.ietf.org/doc/html/rfc5389#section-15.4)
87+
- [WRAITH Security Documentation](./README.md)
88+
89+
## Review
90+
91+
This document should be reviewed whenever:
92+
93+
- STUN implementation is modified
94+
- New protocols with algorithm requirements are added
95+
- Code scanning tools are updated
96+
- Security best practices change
97+
98+
Last Updated: 2025-12-09

0 commit comments

Comments
 (0)