DD3-LoRa-Bridge-MultiSender/CODE_REVIEW.md

# Code Review: DD3 LoRa Bridge MultiSender
**Date:** March 11, 2026  
**Reviewer:** Security Analysis  
**Focus:** Buffer overflows, memory issues, security risks, and bugs

---

## Executive Summary
The codebase is generally well-written with good defensive programming practices. Most critical vulnerabilities are mitigated through bounds checking and safe API usage. However, there are several issues ranging from minor to moderate severity that should be addressed.

---

## Critical Issues

### 1. ⚠️ **No HTTPS/TLS - Credentials transmitted in plaintext**
**Severity:** CRITICAL  
**File:** [web_server.cpp](src/web_server.cpp)  
**Issue:** The web server runs on plain HTTP (port 80) without any encryption.
- WiFi credentials, MQTT credentials, and API authentication are sent in plaintext
- All data exchanges (history, configuration, status) are unencrypted
- An attacker on the network can easily capture credentials and impersonate users
- User login credentials transmitted via HTTP Basic Auth are also vulnerable

**Impact:** Complete loss of confidentiality for all sensitive data  
**Recommendation:**
- Implement HTTPS/TLS support on the ESP32 web server
- Consider at minimum disabling HTTP when HTTPS is available
- Alternatively, restrict web access to local network only with firewall rules
- Document this limitation prominently

**Code:** [web_server.cpp L580-620](src/web_server.cpp#L576) - All `server.send()` calls use HTTP

---

### 2. ⚠️ **Default weak credentials - "admin/admin"**
**Severity:** HIGH  
**File:** [config.h](include/config.h#L83)  
**Issue:** 
```cpp
constexpr const char *WEB_AUTH_DEFAULT_USER = "admin";
constexpr const char *WEB_AUTH_DEFAULT_PASS = "admin";
```

**Impact:** Default accounts are easily guessable; most users won't change them, especially in AP mode where `WEB_AUTH_REQUIRE_AP = false` (no auth required)  
**Recommendation:**
- Force users to create strong credentials during initial setup
- Generate random default credentials (or use MAC address-based credentials)
- Never store credentials in plain-text constants
- In AP mode, either enable auth or display a security warning

---

## High Priority Issues

### 3. ⚠️ **AP mode has no authentication**
**Severity:** HIGH  
**File:** [config.h](include/config.h#L82), [web_server.cpp](src/web_server.cpp#L115)  
**Issue:** 
```cpp
constexpr bool WEB_AUTH_REQUIRE_AP = false;  // AP mode has NO authentication!
```
When device acts as an access point, all endpoints can be accessed without any authentication.

**Impact:** Any device that connects to the AP can access all functionality:
- Download meter data and history
- Change WiFi/MQTT configuration
- Change web UI credentials
- Affect system behavior

**Recommendation:**
- Require authentication even in AP mode
- Or implement a time-limited "setup mode" that requires initial password setup
- Display a prominent warning on AP mode UI

---

### 4. ⚠️ **Integer overflow potential in history bin allocation**
**Severity:** MEDIUM  
**File:** [web_server.cpp](src/web_server.cpp#L767)  
**Code:**
```cpp
uint32_t bins = (static_cast<uint32_t>(days) * 24UL * 60UL) / res_min;
if (bins == 0 || bins > SD_HISTORY_MAX_BINS) {
    // error handling...
    return;
}
```

**Issue:** While bounds checks are in place, the multiplication `days * 24 * 60` uses 32-bit math after casting. Although mitigated by `SD_HISTORY_MAX_DAYS = 30` and `SD_HISTORY_MIN_RES_MIN = 1`, the order of operations could be unsafe with different constants.

**Current Safety:** The bounds check at [L776](src/web_server.cpp#L776) prevents allocation of more than 4000 bins. Max days (30) × 24 × 60 = 43,200 bins, but then divided by res_min (minimum 1), result is capped at 4000.

**Recommendation:**
- Reorder the multiplication to avoid overflow: `((days * 24) * 60) / res_min` → safer to do: `(days / res_min_in_days) * minutes_per_day` to prevent intermediate overflow
- Or explicitly check: `if (days > UINT32_MAX / (24 * 60)) { error; }`

---

### 5. ⚠️ **Potential memory leak in history processing on error**
**Severity:** MEDIUM  
**File:** [web_server.cpp](src/web_server.cpp#L779)  
**Code:**
```cpp
g_history.bins = new (std::nothrow) HistoryBin[bins];
if (!g_history.bins) {
    g_history.error = true;
    g_history.error_msg = "oom";
    server.send(200, "application/json", "{\"ok\":false,\"error\":\"oom\"}");
    return;
}
```

**Issue:** If a new history request is made while a previous request has error state with allocated `g_history.bins`, the `history_reset()` function properly cleans up. However, if the device loses power or crashes between allocation and cleanup, memory isn't freed (minor issue, but worth noting on embedded system).

**Mitigation:** The [history_reset()](src/web_server.cpp#L268) function properly cleans up on next use.

**Recommendation:** 
- Ensure `history_reset()` is always called before allocating new bins ✅ Already done at [L781](src/web_server.cpp#L781)

---

## Medium Priority Issues

### 6. ⚠️ **String buffer size assumptions in CSV line parsing**
**Severity:** MEDIUM  
**File:** [web_server.cpp](src/web_server.cpp#L298)  
**Code:**
```cpp
char line[160];
size_t n = g_history.file.readBytesUntil('\n', line, sizeof(line) - 1);
line[n] = '\0';
```

**Issue:** If SD card contains a line longer than 160 bytes (minus 1 for null terminator), the function will silently truncate data and re-attempt. The CSV data format is expected to be compact, but if corrupted files exist, this could cause parsing failures.

**Mitigation:** The function gracefully handles parse failures with `if (!history_parse_line(line, ts, p)) { continue; }` and returns false on oversized fields at [L323](src/web_server.cpp#L323).

**Recommendation:**
- This is acceptable for the use case. Consider logging truncation warnings if SERIAL_DEBUG_MODE is enabled.

---

### 7. ⚠️ **CSV injection vulnerability in meter data logging**
**Severity:** MEDIUM (Low practical risk)  
**File:** [sd_logger.cpp](src/sd_logger.cpp#L107)  
**Code:**
```cpp
f.print(data.total_power_w, 1);  // Directly prints floating point
f.print(data.energy_total_kwh, 3);
```

**Issue:** If floating-point values could be controlled by attacker, they could potentially inject CSV/formula injection attacks (e.g., `=1+1` starts formula in Excel). The power_w values are calculated from meter readings, so this has LOW practical risk.

**Impact:** Low - values come from trusted LoRa devices, not user input  
**Recommendation:**
- If you want to be extra safe, sanitize by checking first character: if value starts with `=`, `+`, `@`, or `-`, prefix with single quote or space
- For now, this is acceptable given the trusted data source

---

## Low Priority Issues / Best Practice Recommendations

### 8. ℹ️ **Path construction could use better validation**
**Severity:** LOW  
**File:** [web_server.cpp](src/web_server.cpp#L179)  
**Code:**
```cpp
static bool sanitize_sd_download_path(String &path, String &error) {
    // ... checks for "..", "\", "//" ...
    if (!path.startsWith("/dd3/")) {
        error = "prefix";
        return false;
    }
}
```

**Assessment:** ✅ **Path traversal protection is GOOD**
- Checks for `..` (parent directory)
- Checks for `\` (backslash)
- Checks for `//` (double slashes)
- Requires `/dd3/` prefix
- Limits path length to 160 characters

The implementation is solid. No changes needed.

---

### 9. ℹ️ **HTML escaping is properly implemented**
**Severity:** N/A  
**File:** [html_util.cpp](src/html_util.cpp)  
**Assessment:** ✅ **XSS protection is GOOD**
```cpp
case '&': out += "&amp;"; break;
case '<': out += "&lt;"; break;
case '>': out += "&gt;"; break;
case '"': out += "&quot;"; break;
case '\'': out += "&#39;"; break;
```

All unsafe HTML characters are properly escaped. Good defensive programming.

---

### 10. ℹ️ **Buffer overflow checks are generally sound**
**Severity:** N/A  
**Files:** [meter_driver.cpp](src/meter_driver.cpp), [lora_transport.cpp](src/lora_transport.cpp)  
**Assessment:** ✅ **NO UNSAFE STRING FUNCTIONS FOUND**

- No `strcpy`, `strcat`, `sprintf`, `gets`, `scanf` used
- All buffer writes check bounds before writing
- Example from [meter_driver.cpp L50](src/meter_driver.cpp#L50):
  ```cpp
  if (n + 1 < sizeof(num_buf)) {  // Bounds check BEFORE write
      num_buf[n++] = c;
  }
  ```

- Example from [lora_transport.cpp L119](src/lora_transport.cpp#L119):
  ```cpp
  if (pkt.payload_len > LORA_MAX_PAYLOAD) {
      return false;  // Reject oversized payloads
  }
  memcpy(&buffer[idx], pkt.payload, pkt.payload_len);
  ```

---

### 11. ℹ️ **Zigzag encoding is correct**
**Severity:** N/A  
**File:** [payload_codec.cpp](src/payload_codec.cpp#L107)  
**Code:**
```cpp
uint32_t zigzag32(int32_t x) {
    return (static_cast<uint32_t>(x) << 1) ^ static_cast<uint32_t>(x >> 31);
}
```

**Assessment:** ✅ **CORRECT**
- Proper cast to uint32_t before shift avoids UB
- Standard protobuf zigzag encoding pattern
- Correctly handles signed integers

---

### 12. ℹ️ **Payload encoding/decoding has solid bounds checking**
**Severity:** N/A  
**File:** [payload_codec.cpp](src/payload_codec.cpp#L132-160)  
**Assessment:** ✅ **GOOD DEFENSIVE PROGRAMMING**

Examples of proper bounds checks:
```cpp
// Check maximum samples
if (in.n > kMaxSamples) return false;

// Check feature mask validity
if ((in.present_mask & ~kPresentMaskValidBits) != 0) return false;

// Check consistency
if (bit_count32(in.present_mask) != in.n) return false;

// Check monotonically increasing energy
if (in.energy_wh[i] < in.energy_wh[i - 1]) return false;

// Check for 32-bit overflow when adding deltas
uint64_t sum = static_cast<uint64_t>(out->energy_wh[i-1]) + delta;
if (sum > UINT32_MAX) return false;

// Check phase value ranges
if (value < INT16_MIN || value > INT16_MAX) return false;
```

Excellent work on defense-in-depth.

---

### 13. ℹ️ **LoRa frame validation is robust**
**Severity:** N/A  
**File:** [lora_transport.cpp](src/lora_transport.cpp#L126-180)  
**Assessment:** ✅ **GOOD**

- Validates minimum packet size
- Validates maximum packet size
- CRC verification
- Message kind validation
- Signal strength logging

---

### 14. ⚠️ **Time-based security: Minimum epoch check**
**Severity:** LOW  
**File:** [config.h](include/config.h#L81)  
**Code:**
```cpp
constexpr uint32_t MIN_ACCEPTED_EPOCH_UTC = 1769904000UL; // 2026-02-01 00:00:00 UTC
```

**Issue:** This constant is a static minimum and won't be appropriate over time. In 2030, this will reject legitimate timestamps from 2026-2029.

**Recommendation:**
- Calculate dynamically: `MIN_ACCEPTED_EPOCH = compile_time_epoch - 5_years` 
- Or use a configuration that can be updated via firmware
- Or accept any reasonable recent timestamp (e.g., >= 2020-01-01)

---

### 15. ℹ️ **Floating point NaN handling is correct**
**Assessment:** ✅ **GOOD**

The code properly uses `isnan()` throughout:
- [json_codec.cpp L13](src/json_codec.cpp#L13)
- [web_server.cpp L104](src/web_server.cpp#L104)
- [sd_logger.cpp L131](src/sd_logger.cpp#L131)

No integer division by zero issues detected either (checks for zero before division).

---

### 16. ℹ️ **Integer casting for power calculations handles overflow**
**Severity:** N/A  
**File:** [web_server.cpp](src/web_server.cpp#L97)  
**Code:**
```cpp
static int32_t round_power_w(float value) {
    if (isnan(value)) return 0;
    long rounded = lroundf(value);
    if (rounded > INT32_MAX) return INT32_MAX;  // Overflow protection
    if (rounded < INT32_MIN) return INT32_MIN;  // Underflow protection
    return static_cast<int32_t>(rounded);
}
```

**Assessment:** ✅ **EXCELLENT** - Defensive against both positive and negative overflows

---

## Summary Table

| ID | Issue | Severity | Category | Status |
|---|---|---|---|---|
| 1 | No HTTPS/TLS | CRITICAL | Security | ⚠️ Needs Fix |
| 2 | Weak default credentials | HIGH | Security | ⚠️ Needs Fix |
| 3 | AP mode no auth | HIGH | Security | ⚠️ Needs Fix |
| 4 | Integer overflow in bins | MEDIUM | Memory | ⚠️ Needs Review |
| 5 | Memory leak potential | MEDIUM | Memory | ✅ Mitigated |
| 6 | CSV line truncation | MEDIUM | Data Handling | ✅ Safe |
| 7 | CSV injection | MEDIUM | Security | ✅ Low Risk |
| 8 | Path traversal | LOW | Security | ✅ Well Protected |
| 9-16 | Best practices | N/A | Quality | ✅ GOOD |

---

## Recommendations for Fixes

### Immediate (Critical Path)
1. **Enable HTTPS** - Implement TLS on ESP32 web server
2. **Strengthen AP mode security** - Either enable auth or use time-limited setup mode
3. **Improve default credentials** - Generate strong defaults or force user configuration

### Short-term (High Priority)
4. **Fix integer overflow checks** - Add explicit overflow detection before bin allocation
5. **Document security limitations** - Clearly state that HTTPS is not available

### Long-term (Nice to Have)
6. **Add audit logging** - Log all configuration changes and data access
7. **Implement certificate pinning** - Once HTTPS is added
8. **Add device firmware signature verification** - Prevent unauthorized updates

---

## Testing Recommendations

```bash
# Verify no plaintext credentials in traffic
tcpdump -i <interface> port 80 or port 1883 | grep -i password

# Test path traversal protection
curl "http://device/sd/download?path=/etc/passwd"
curl "http://device/sd/download?path=/../../../"

# Test XSS protection
curl "http://device/sender/<img%20src=x%20onerror=alert(1)>"

# Test OOM handling with large history requests
curl "http://device/history/start?days=365&res=1"
```

---

## Overall Assessment

**Grade: B+ (Good with areas for improvement)**

**Strengths:**
- Solid use of safe APIs and standard library functions
- Excellent bounds checking throughout
- Good defensive programming practices
- CRC validation and format validation

**Weaknesses:**
- Lack of encryption (HTTPS)
- Weak default security posture
- No security in AP mode
- Need better overflow protection in integer arithmetic

The codebase demonstrates good engineering practices and would be production-ready once the critical HTTPS and authentication issues are addressed.