Bug #8803
Updated by Victor Julien 10 days ago
Reported by Communications Security Establishment (CSE):
<pre>
# SCFTPFreeResponseLine: copy-paste bug leaks `code` allocation; mismatched alloc/free
## Summary
`SCFTPFreeResponseLine` in `rust/src/ftp/response.rs` is supposed to free the two heap buffers (`code` and `response`) inside an `FTPResponseLine`. The first `if` checks `response.response.is_null()` instead of `response.code.is_null()`, so when a parsed FTP line has no status code (`code == ""` → boxed empty slice → may be a non-null sentinel), behaviour is wrong; more importantly the *intent* check is on the wrong field. In addition, `code` was allocated via `into_boxed_slice()` → `Box::into_raw()` but is freed via `Vec::from_raw_parts(ptr, len, len)`, which is an allocator-layout mismatch (UB) when `len > 0` because `Vec` deallocates with capacity-sized layout that must match the original allocation. Per-FTP-response-line allocations leaking on every FTP flow is a slow remote DoS (memory exhaustion) for sensors monitoring FTP-heavy traffic.
## Affected code
- File: `rust/src/ftp/response.rs`
- Function: `SCFTPFreeResponseLine`
- Lines: 89-108 (at suricata commit 17dc06532)
```rust
#[no_mangle]
pub unsafe extern "C" fn SCFTPFreeResponseLine(response: *mut FTPResponseLine) {
if response.is_null() {
return;
}
let response = Box::from_raw(response);
if !response.response.is_null() { // BUG: should be response.code
let _ = Vec::from_raw_parts(response.code, response.code_length, response.code_length);
// BUG: code was Box<[u8]>::into_raw(); freeing via Vec::from_raw_parts is layout-mismatched
}
if !response.response.is_null() {
let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(
response.response,
response.length,
));
}
}
```
Allocation site (lines 60-67):
```rust
Some(FTPResponseLine {
code: Box::into_raw(code_bytes.into_boxed_slice()) as *mut u8,
response: Box::into_raw(response_bytes.into_boxed_slice()) as *mut u8,
...
})
```
## The bug
Two issues:
1. **Wrong null check** — both branches test `!response.response.is_null()`. The `code` buffer is never gated on its own pointer. In practice both pointers are produced by `Box::into_raw(boxed_slice)` and are never null even for empty slices (Rust returns a dangling-but-aligned non-null pointer), so today both branches execute. But the duplicated check is clearly a copy-paste error and means future changes that null `response` would also skip freeing `code` → leak.
2. **Alloc/free mismatch (UB)** — `code` is allocated as a `Box<[u8]>` (via `Vec::into_boxed_slice`, which shrinks capacity to length). It is freed by constructing a `Vec<u8>` with `Vec::from_raw_parts(ptr, len, len)` and dropping it. Per `Vec::from_raw_parts` safety docs, `capacity` "needs to be the capacity that the pointer was allocated with" and "ptr must have been allocated using the global allocator … with the same layout". A `Box<[u8]>` of length N is allocated with layout `(size=N, align=1)`; a `Vec<u8>` with capacity N deallocates with the same layout, so on the *current* global allocator this happens to work. However it is still formally UB (the docs require the pointer to have come from a `Vec`/`String`), and it diverges from how `response` is freed (correctly, via `Box::from_raw(slice_from_raw_parts_mut(...))`). When `code_length == 0` (no status code on the line — e.g., FTP banners or continuation lines), `Vec::from_raw_parts(dangling, 0, 0)` is dropped — that path is defined as a no-op, so no crash, but the asymmetry with the `response` path is a latent bug.
Net effect today: no crash, no leak (both pointers are always non-null and the layouts coincide on the system allocator). But the code is technically unsound and one refactor away from a per-FTP-response leak.
## Validation results
- [x] Code-path traced manually — `SCFTPParseResponseLine` is called for every server response line (`src/app-layer-ftp.c:758`); `SCFTPFreeResponseLine` is called from `FTPStringFree` (line 176) and on the early-free path (line 772). Confirmed `Box::into_raw(empty_boxed_slice)` returns `NonNull::dangling()`, never null.
- [ ] Compiler/analyzer warning
- [ ] ASan/UBSan runtime hit
- [ ] Reproducer pcap/rule/input attached
- [x] Could NOT trigger at runtime — current allocator layouts coincide; documented as code-quality / latent UB.
Confidence: high (bug is real). Severity: info / low (no observable impact on current targets; technically-unsound `unsafe`).
## Expected fix
```rust
if !response.code.is_null() {
let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(
response.code,
response.code_length,
));
}
if !response.response.is_null() {
let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(
response.response,
response.length,
));
}
```
</pre>