mirror of
https://github.com/permissionlesstech/bitchat.git
synced 2026-07-25 12:25:19 +00:00
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:
@@ -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)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -2712,21 +2712,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)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user