370 lines
11 KiB
Markdown
370 lines
11 KiB
Markdown
# ESP32-C5 Toolkit Firmware Bug Review & Fixes
|
|
|
|
**Date:** 2024
|
|
**Reviewer:** AI Code Review
|
|
**Firmware Version:** 2.0
|
|
|
|
## Executive Summary
|
|
|
|
Comprehensive review of ESP32-C5 Toolkit firmware identified **10 critical bugs**, **5 medium-severity issues**, and several improvements. The bugs range from memory leaks and race conditions to deadlock vulnerabilities and unsafe task management.
|
|
|
|
---
|
|
|
|
## 🔴 CRITICAL BUGS (Must Fix)
|
|
|
|
### 1. **MUTEX DEADLOCK in `frame_analyzer.c`**
|
|
**File:** `main/frame_analyzer.c:57-59`
|
|
**Severity:** CRITICAL
|
|
**Issue:** Deadlock when `frame_analyzer_capture_start()` calls `frame_analyzer_reset()` while already holding the mutex.
|
|
|
|
```c
|
|
// BUG: frame_analyzer_capture_start takes mutex, then calls reset which tries to take it again
|
|
void frame_analyzer_capture_start(...) {
|
|
if (analyzer_mutex && xSemaphoreTake(analyzer_mutex, portMAX_DELAY)) {
|
|
frame_analyzer_reset(); // ❌ This tries to take the same mutex again!
|
|
// ...
|
|
}
|
|
}
|
|
```
|
|
|
|
**Fix:** Extract reset logic to a non-mutex-protected internal function, or remove mutex from reset when called internally.
|
|
|
|
---
|
|
|
|
### 2. **UNSAFE TASK DELETION - Multiple Locations**
|
|
**Files:**
|
|
- `main/wifi_sniffer.c:203`
|
|
- `main/deauth_engine.c:265`
|
|
- `main/handshake_capture.c:249, 431, 436`
|
|
|
|
**Severity:** CRITICAL
|
|
**Issue:** `vTaskDelete()` called directly on potentially running tasks, which can corrupt task structures and cause crashes.
|
|
|
|
**ESP-IDF Best Practice:** Tasks should be gracefully stopped (set flag, wait for exit), or suspended before deletion.
|
|
|
|
**Example Bug:**
|
|
```c
|
|
// wifi_sniffer.c:203
|
|
if (channel_hopper_task_handle != NULL) {
|
|
vTaskDelete(channel_hopper_task_handle); // ❌ Unsafe - task may be executing
|
|
channel_hopper_task_handle = NULL;
|
|
}
|
|
```
|
|
|
|
**Fix:** Suspend task first, then delete:
|
|
```c
|
|
if (channel_hopper_task_handle != NULL) {
|
|
vTaskSuspend(channel_hopper_task_handle);
|
|
vTaskDelay(pdMS_TO_TICKS(10)); // Allow task to reach safe point
|
|
vTaskDelete(channel_hopper_task_handle);
|
|
channel_hopper_task_handle = NULL;
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
### 3. **MEMORY LEAK in `wifi_sniffer.c`**
|
|
**File:** `main/wifi_sniffer.c:144-149`
|
|
**Severity:** CRITICAL
|
|
**Issue:** Allocated `channel_param` memory leaked if `xTaskCreate()` fails.
|
|
|
|
```c
|
|
uint8_t *channel_param = malloc(sizeof(uint8_t));
|
|
if (channel_param) {
|
|
*channel_param = channel;
|
|
xTaskCreate(single_channel_retry_task, "channel_retry", 2048, channel_param, 5, &channel_hopper_task_handle);
|
|
// ❌ If xTaskCreate fails, channel_param is leaked!
|
|
}
|
|
```
|
|
|
|
**Fix:** Check return value and free on failure:
|
|
```c
|
|
uint8_t *channel_param = malloc(sizeof(uint8_t));
|
|
if (channel_param) {
|
|
*channel_param = channel;
|
|
BaseType_t ret = xTaskCreate(single_channel_retry_task, "channel_retry", 2048, channel_param, 5, &channel_hopper_task_handle);
|
|
if (ret != pdPASS) {
|
|
free(channel_param); // ✅ Free if task creation fails
|
|
}
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
### 4. **RACE CONDITION in Task Handle Check**
|
|
**Files:**
|
|
- `main/deauth_engine.c:258-265`
|
|
- `main/handshake_capture.c:430-437`
|
|
|
|
**Severity:** CRITICAL
|
|
**Issue:** Task handle checked and then deleted in separate operations, allowing race condition.
|
|
|
|
```c
|
|
// deauth_engine.c:258-265
|
|
while (attack_task_handle != NULL && wait_count < 50) {
|
|
vTaskDelay(pdMS_TO_TICKS(100));
|
|
wait_count++;
|
|
}
|
|
|
|
if (attack_task_handle != NULL) { // ❌ Race: handle could become NULL between check and delete
|
|
vTaskDelete(attack_task_handle);
|
|
attack_task_handle = NULL;
|
|
}
|
|
```
|
|
|
|
**Fix:** Store handle in local variable atomically:
|
|
```c
|
|
TaskHandle_t task_to_delete = NULL;
|
|
if (xSemaphoreTake(attack_mutex, portMAX_DELAY) == pdTRUE) {
|
|
task_to_delete = attack_task_handle;
|
|
attack_task_handle = NULL; // Clear while holding mutex
|
|
xSemaphoreGive(attack_mutex);
|
|
}
|
|
|
|
if (task_to_delete != NULL) {
|
|
vTaskSuspend(task_to_delete);
|
|
vTaskDelay(pdMS_TO_TICKS(10));
|
|
vTaskDelete(task_to_delete);
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
### 5. **PCAP BUFFER MEMORY EXHAUSTION**
|
|
**File:** `main/pcap_serializer.c:84-89`
|
|
**Severity:** CRITICAL
|
|
**Issue:** No size limit on PCAP buffer growth - can exhaust all available heap.
|
|
|
|
```c
|
|
// pcap_serializer.c:84
|
|
unsigned new_size = pcap_size + sizeof(pcap_record_header_t) + size;
|
|
uint8_t *new_buffer = realloc(pcap_buffer, new_size);
|
|
if (!new_buffer) {
|
|
ESP_LOGE(TAG, "Failed to reallocate PCAP buffer! PCAP may be incomplete.");
|
|
return; // ❌ No recovery, buffer left in inconsistent state
|
|
}
|
|
```
|
|
|
|
**Fix:** Add maximum size limit and better error handling:
|
|
```c
|
|
#define MAX_PCAP_SIZE (512 * 1024) // 512KB limit
|
|
|
|
unsigned new_size = pcap_size + sizeof(pcap_record_header_t) + size;
|
|
if (new_size > MAX_PCAP_SIZE) {
|
|
ESP_LOGW(TAG, "PCAP buffer limit reached (%u bytes), dropping frame", MAX_PCAP_SIZE);
|
|
return;
|
|
}
|
|
|
|
uint8_t *new_buffer = realloc(pcap_buffer, new_size);
|
|
if (!new_buffer) {
|
|
ESP_LOGE(TAG, "Failed to reallocate PCAP buffer");
|
|
// Optionally: discard oldest packets to free space
|
|
return;
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
### 6. **MISSING ERROR CHECK on Task Creation**
|
|
**File:** `main/deauth_engine.c:228`
|
|
**Severity:** CRITICAL
|
|
**Issue:** `xTaskCreate()` return value not checked - task may not have been created.
|
|
|
|
```c
|
|
xTaskCreate(dual_band_attack_task, "dual_attack", 8192, NULL, 5, &attack_task_handle);
|
|
// ❌ No error check - attack_running is true but task may not exist
|
|
```
|
|
|
|
**Fix:** Check return value:
|
|
```c
|
|
BaseType_t ret = xTaskCreate(dual_band_attack_task, "dual_attack", 8192, NULL, 5, &attack_task_handle);
|
|
if (ret != pdPASS) {
|
|
ESP_LOGE(TAG, "Failed to create attack task");
|
|
attack_running = false;
|
|
xSemaphoreGive(attack_mutex);
|
|
return false;
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
### 7. **WiFi MODE CHANGE CONFLICTS**
|
|
**Files:** Multiple files change WiFi mode without coordination
|
|
**Severity:** HIGH
|
|
**Issue:** Multiple components (sniffer, deauth, handshake capture) change WiFi mode independently, causing conflicts.
|
|
|
|
**Locations:**
|
|
- `wifi_sniffer.c:98` - Sets `WIFI_MODE_APSTA`
|
|
- `deauth_engine.c:95, 115` - Sets `WIFI_MODE_APSTA`, then `WIFI_MODE_STA`
|
|
- `handshake_capture.c:182, 268, 379, 451` - Multiple mode changes
|
|
|
|
**Fix:** Implement WiFi mode manager with reference counting:
|
|
```c
|
|
// Add to wifi_init.c or new wifi_manager.c
|
|
static int wifi_mode_ref_count = 0;
|
|
static SemaphoreHandle_t wifi_mode_mutex = NULL;
|
|
|
|
esp_err_t wifi_request_mode(wifi_mode_t requested_mode) {
|
|
if (wifi_mode_mutex == NULL) {
|
|
wifi_mode_mutex = xSemaphoreCreateMutex();
|
|
}
|
|
|
|
if (xSemaphoreTake(wifi_mode_mutex, portMAX_DELAY) != pdTRUE) {
|
|
return ESP_FAIL;
|
|
}
|
|
|
|
wifi_mode_ref_count++;
|
|
esp_err_t ret = esp_wifi_set_mode(requested_mode);
|
|
|
|
xSemaphoreGive(wifi_mode_mutex);
|
|
return ret;
|
|
}
|
|
|
|
void wifi_release_mode(void) {
|
|
if (xSemaphoreTake(wifi_mode_mutex, portMAX_DELAY) == pdTRUE) {
|
|
wifi_mode_ref_count--;
|
|
if (wifi_mode_ref_count <= 0) {
|
|
esp_wifi_set_mode(WIFI_MODE_APSTA); // Restore default
|
|
wifi_mode_ref_count = 0;
|
|
}
|
|
xSemaphoreGive(wifi_mode_mutex);
|
|
}
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
### 8. **CHANNEL SWITCHING RACE CONDITIONS**
|
|
**File:** `main/wifi_sniffer.c`, `main/deauth_engine.c`
|
|
**Severity:** HIGH
|
|
**Issue:** Multiple tasks/operations switch channels without coordination.
|
|
|
|
**Example:**
|
|
- Channel hopper task continuously changes channel
|
|
- Deauth engine changes channel for attacks
|
|
- Handshake capture sets specific channel
|
|
|
|
**Fix:** Implement channel lock mechanism or serialize channel operations.
|
|
|
|
---
|
|
|
|
### 9. **BUFFER OVERFLOW RISK in Frame Analyzer**
|
|
**File:** `main/frame_analyzer.c:235`
|
|
**Severity:** HIGH
|
|
**Issue:** EAPOL packet size not validated before memcpy.
|
|
|
|
```c
|
|
uint16_t eapol_len = sizeof(eapol_packet_header_t) + ntohs(eapol->header.packet_body_length);
|
|
if (eapol_len <= sizeof(captured_handshake.eapol)) {
|
|
memcpy(captured_handshake.eapol, eapol, eapol_len); // ⚠️ Should also check against actual frame size
|
|
}
|
|
```
|
|
|
|
**Fix:** Add bounds checking against actual received frame size.
|
|
|
|
---
|
|
|
|
### 10. **MISSING NULL CHECK before Memory Free**
|
|
**File:** `main/web_server.c:1759`
|
|
**Severity:** MEDIUM
|
|
**Issue:** Packet freed without null check (though malloc failure is handled).
|
|
|
|
```c
|
|
free(pkt); // pkt comes from malloc, should be checked
|
|
```
|
|
|
|
**Fix:** Add defensive check (though malloc failure is already handled, this is defensive):
|
|
```c
|
|
if (pkt) {
|
|
free(pkt);
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
## 🟡 MEDIUM SEVERITY ISSUES
|
|
|
|
### 11. **Inefficient RSSI Averaging**
|
|
**File:** `main/signal_analysis.c:46`
|
|
**Issue:** Simple average overwrites previous value instead of maintaining running average.
|
|
|
|
```c
|
|
ch->rssi = (ch->rssi + rssi) / 2; // Loses history
|
|
```
|
|
|
|
**Fix:** Use exponential moving average or maintain proper average with count.
|
|
|
|
---
|
|
|
|
### 12. **Hardcoded Credentials in Code**
|
|
**File:** Multiple files
|
|
**Issue:** WiFi AP credentials hardcoded (`"ESP32-C5-Toolkit"`, `"h4ck3rm4n"`).
|
|
|
|
**Fix:** Store in NVS or use configuration structure.
|
|
|
|
---
|
|
|
|
### 13. **No Timeout on Mutex Operations**
|
|
**Files:** Multiple
|
|
**Issue:** Many `portMAX_DELAY` timeouts - can hang forever if deadlock occurs.
|
|
|
|
**Fix:** Use reasonable timeouts and handle timeout errors gracefully.
|
|
|
|
---
|
|
|
|
### 14. **Missing WiFi Error Handling**
|
|
**Files:** Multiple
|
|
**Issue:** Several `esp_wifi_set_channel()` calls don't check return values.
|
|
|
|
**Example:** `deauth_engine.c:68`, `handshake_capture.c:389`
|
|
|
|
---
|
|
|
|
### 15. **Stack Size May Be Insufficient**
|
|
**Files:** Task creation calls
|
|
**Issue:** Some tasks use 2048-8192 bytes stack - may overflow with deep call stacks.
|
|
|
|
**Fix:** Monitor stack usage with `uxTaskGetStackHighWaterMark()` and adjust.
|
|
|
|
---
|
|
|
|
## ✅ RECOMMENDED IMPROVEMENTS
|
|
|
|
1. **Add Heap Monitoring:** Use `esp_get_minimum_free_heap_size()` to detect memory leaks
|
|
2. **Add Watchdog:** Implement task watchdog to detect hung tasks
|
|
3. **Improve Logging:** Add more detailed error context to log messages
|
|
4. **Add Unit Tests:** Critical path functions should have unit tests
|
|
5. **Documentation:** Add function-level documentation for all public APIs
|
|
6. **Error Recovery:** Implement better error recovery mechanisms (e.g., PCAP buffer full)
|
|
7. **Resource Limits:** Add configurable limits for packet queues, PCAP size, etc.
|
|
|
|
---
|
|
|
|
## 🔧 PRIORITY FIX ORDER
|
|
|
|
1. **IMMEDIATE:** Fix mutex deadlock (#1)
|
|
2. **IMMEDIATE:** Fix unsafe task deletions (#2)
|
|
3. **HIGH:** Fix memory leaks (#3)
|
|
4. **HIGH:** Add PCAP size limits (#5)
|
|
5. **MEDIUM:** Implement WiFi mode manager (#7)
|
|
6. **MEDIUM:** Fix channel switching coordination (#8)
|
|
7. **LOW:** Address other improvements
|
|
|
|
---
|
|
|
|
## 📝 NOTES
|
|
|
|
- All fixes should be tested on actual hardware
|
|
- Consider adding CONFIG options for buffer sizes
|
|
- Review ESP-IDF v6.0 migration guide for deprecated APIs
|
|
- Some "bugs" may be intentional for performance - verify before changing
|
|
|
|
---
|
|
|
|
## REFERENCES
|
|
|
|
- ESP-IDF Programming Guide v6.0
|
|
- FreeRTOS Task Management Best Practices
|
|
- ESP32-C5 Technical Reference Manual
|
|
|