Fix critical security issues in fragment reassembly and file cleanup

Fragment Reassembly Race Condition (CRITICAL):
- Wrap all incomingFragments/fragmentMetadata access in collectionsQueue.sync
- Prevents concurrent modification crashes from multi-threaded access
- Minimizes lock contention by doing heavy work (reassembly/decode) outside locks
- Add upper bound check: reject fragments with total > 10,000 (DoS prevention)
- Add cumulative size validation before storing fragments (memory DoS prevention)

File Cleanup Path Traversal (CRITICAL):
- Use NSString.lastPathComponent to extract filename safely
- Prevents directory traversal attacks via malicious filenames
- Add path prefix validation before file deletion
- Now checks both incoming and outgoing directories (fixes disk leak)

Additional Protections:
- Fragment assemblies now limited by both count (128) and cumulative bytes (1MB)
- Explicit checks for "." and ".." filenames in cleanup
- Defense-in-depth: multiple validation layers
This commit is contained in:
jack
2025-10-15 17:37:19 +01:00
committed by islam
parent d7ca99ae5d
commit c3ce32ad07
2 changed files with 91 additions and 49 deletions
+36 -7
View File
@@ -1274,11 +1274,17 @@ final class BLEService: NSObject {
let originalType = packet.payload[12] let originalType = packet.payload[12]
let fragmentData = packet.payload.suffix(from: 13) let fragmentData = packet.payload.suffix(from: 13)
// Sanity checks // Sanity checks - add reasonable upper bound on total to prevent DoS
guard total > 0 && index >= 0 && index < total else { return } guard total > 0 && total <= 10000 && index >= 0 && index < total else { return }
// Store fragment // Compute fragment key for this assembly
let key = FragmentKey(sender: senderU64, id: fragU64) let key = FragmentKey(sender: senderU64, id: fragU64)
// Critical section: Store fragment and check completion status
var shouldReassemble: Bool = false
var fragmentsToReassemble: [Int: Data]? = nil
collectionsQueue.sync(flags: .barrier) {
if incomingFragments[key] == nil { if incomingFragments[key] == nil {
// Cap in-flight assemblies to prevent memory/battery blowups // Cap in-flight assemblies to prevent memory/battery blowups
if incomingFragments.count >= maxInFlightAssemblies { if incomingFragments.count >= maxInFlightAssemblies {
@@ -1292,13 +1298,35 @@ final class BLEService: NSObject {
fragmentMetadata[key] = (originalType, total, Date()) fragmentMetadata[key] = (originalType, total, Date())
SecureLogger.debug("📦 Started fragment assembly id=\(String(format: "%016llx", fragU64)) total=\(total)", category: .session) SecureLogger.debug("📦 Started fragment assembly id=\(String(format: "%016llx", fragU64)) total=\(total)", category: .session)
} }
// Check cumulative size before storing this fragment
let currentSize = incomingFragments[key]?.values.reduce(0) { $0 + $1.count } ?? 0
guard currentSize + fragmentData.count <= FileTransferLimits.maxPayloadBytes else {
// Exceeds size limit - evict this assembly
SecureLogger.warning("🚫 Fragment assembly exceeds size limit (\(currentSize + fragmentData.count) bytes), evicting", category: .security)
incomingFragments.removeValue(forKey: key)
fragmentMetadata.removeValue(forKey: key)
shouldReassemble = false
fragmentsToReassemble = nil
return
}
incomingFragments[key]?[index] = Data(fragmentData) incomingFragments[key]?[index] = Data(fragmentData)
SecureLogger.debug("📦 Fragment \(index + 1)/\(total) (len=\(fragmentData.count)) for id=\(String(format: "%016llx", fragU64))", category: .session) SecureLogger.debug("📦 Fragment \(index + 1)/\(total) (len=\(fragmentData.count)) for id=\(String(format: "%016llx", fragU64))", category: .session)
// Check if complete // Check if complete
if let fragments = incomingFragments[key], if let fragments = incomingFragments[key], fragments.count == total {
fragments.count == total { shouldReassemble = true
// Reassemble fragmentsToReassemble = fragments
} else {
shouldReassemble = false
fragmentsToReassemble = nil
}
}
// Heavy work outside lock: reassemble and decode
guard shouldReassemble, let fragments = fragmentsToReassemble else { return }
var reassembled = Data() var reassembled = Data()
for i in 0..<total { for i in 0..<total {
if let fragment = fragments[i] { if let fragment = fragments[i] {
@@ -1314,7 +1342,8 @@ final class BLEService: NSObject {
SecureLogger.error("❌ Failed to decode reassembled packet (type=\(originalType), total=\(total))", category: .session) SecureLogger.error("❌ Failed to decode reassembled packet (type=\(originalType), total=\(total))", category: .session)
} }
// Cleanup // Critical section: Cleanup completed assembly
collectionsQueue.sync(flags: .barrier) {
incomingFragments.removeValue(forKey: key) incomingFragments.removeValue(forKey: key)
fragmentMetadata.removeValue(forKey: key) fragmentMetadata.removeValue(forKey: key)
} }
+23 -10
View File
@@ -2694,21 +2694,34 @@ final class ChatViewModel: ObservableObject, BitchatDelegate {
} }
private func cleanupLocalFile(forMessage message: BitchatMessage) { private func cleanupLocalFile(forMessage message: BitchatMessage) {
let prefixes = ["[voice] ": "voicenotes/outgoing", // Check both outgoing and incoming directories for thorough cleanup
"[image] ": "images/outgoing", let prefixes = ["[voice] ", "[image] ", "[file] "]
"[file] ": "files/outgoing"] let subdirs = ["voicenotes/outgoing", "voicenotes/incoming",
guard let entry = prefixes.first(where: { message.content.hasPrefix($0.key) }) else { return } "images/outgoing", "images/incoming",
let filename = String(message.content.dropFirst(entry.key.count)).trimmingCharacters(in: .whitespacesAndNewlines) "files/outgoing", "files/incoming"]
guard !filename.isEmpty, let base = try? applicationFilesDirectory() else { return }
let target = base.appendingPathComponent(entry.value, isDirectory: true).appendingPathComponent(filename) guard let prefix = prefixes.first(where: { message.content.hasPrefix($0) }) else { return }
let rawFilename = String(message.content.dropFirst(prefix.count)).trimmingCharacters(in: .whitespacesAndNewlines)
guard !rawFilename.isEmpty, let base = try? applicationFilesDirectory() else { return }
// Security: Extract only the last path component to prevent directory traversal
let safeFilename = (rawFilename as NSString).lastPathComponent
guard !safeFilename.isEmpty && safeFilename != "." && safeFilename != ".." else { return }
// Try all possible locations (outgoing and incoming)
for subdir in subdirs {
let target = base.appendingPathComponent(subdir, isDirectory: true).appendingPathComponent(safeFilename)
// Security: Verify target is within expected directory before deletion
guard target.path.hasPrefix(base.path) else { continue }
// Security: Remove file directly (no TOCTOU race)
do { do {
try FileManager.default.removeItem(at: target) try FileManager.default.removeItem(at: target)
} catch CocoaError.fileNoSuchFile { } catch CocoaError.fileNoSuchFile {
// Expected // Expected - file not in this directory
} catch { } catch {
SecureLogger.error("Failed to cleanup \(filename): \(error)", category: .session) SecureLogger.error("Failed to cleanup \(safeFilename): \(error)", category: .session)
}
} }
} }