open
https://gitlab.synchro.net/main/sbbs/-/issues/1191
## Summary
`sbbs_t::cmdstr()` bounds its output by the size of its *own member* buffer even
when it is writing into a **caller-supplied** buffer whose size it is never told. Callers that pass a buffer smaller than `cmdstr_output` can therefore be overflowed. `@TYPE:` and `@INCLUDE:` do exactly that, into a 128- or 256-byte stack buffer.
## The code
`src/sbbs3/xtrn.cpp`, `sbbs_t::cmdstr()`:
```c
if (outstr == NULL)
cmd = cmdstr_output; // member: char cmdstr_output[512] (sbbs.h)
else
cmd = outstr; // CALLER's buffer -- size unknown here
len = strlen(instr);
int maxlen = (int)sizeof(cmdstr_output) - 1; // 511, ALWAYS
for (i = j = 0; i < len && j < maxlen; i++) {
...
strncat(cmd, <expansion>, avail); // avail = maxlen - j
...
}
cmd[j] = 0;
```
`maxlen` is derived from `cmdstr_output` unconditionally, so when `outstr` is supplied the loop and every `strncat(..., avail)` inside it are bounded by 511 rather than by the destination's real size.
## Reachable path
`src/sbbs3/atcodes.cpp`, inside `sbbs_t::atcode()`:
```c
if (!strncmp(sp, "TYPE:", 5))
printfile(cmdstr(sp + 5, nulstr, nulstr, str), P_MODS);
if (!strncmp(sp, "INCLUDE:", 8))
printfile(cmdstr(sp + 8, nulstr, nulstr, str), P_OPENCLOSE | P_NOCRLF | P_SAVEATR | P_MODS);
```
`str` there is `atcode()`'s output parameter. Its two callers pass:
| caller | buffer | size |
|---|---|---|
| `show_atcode()` (`atcodes.cpp:181`) | `str2` | **128** |
| `formatted_atcode()` (`atcodes.cpp:350`) | `buf` | **256** |
So `cmdstr()` may write up to 512 bytes into a 128-byte stack buffer — roughly
384 bytes past the end.
Note the information is *available and discarded*: `atcode()` receives a `size_t maxlen` for exactly this buffer, but `cmdstr()` has no parameter to receive it.
`exec.cpp`'s call sites pass `uchar buf[1025]` and are not affected.
## Trigger
A `text`/`menu` file containing a long `@TYPE:` or `@INCLUDE:`. That content is sysop-authored, so this is not a remote attack primitive — but the *expanded* length is partly user-influenced, since `cmdstr()` substitutes `%a` (alias), `%u`, `%z` and friends, and a single line may repeat codes. A long path alone is sufficient with no user involvement.
## Also: silent truncation at 511
Even where the destination is large enough, `cmdstr()` stops at 511 characters with no diagnostic, so an over-long configured command line is quietly shortened before `sbbs_t::external()` ever sees it. (`external()` now logs its own, separate truncation at `MAX_PATH+1`.)
## Suggested fix
Two options, in increasing order of durability:
1. **Enlarge the callers' buffers** to `sizeof(cmdstr_output)`. Removes the
overflow with a one-line change per site, but leaves the underlying contract
("the caller must guess my member's size") intact and re-armed for the next
caller.
2. **Give `cmdstr()` an output size** — an added parameter, or a `cmdstr_len()`
variant with the existing signature kept as a wrapper — and use that for
`maxlen`. `atcode()` already has the right value to pass. Roughly 15 call
sites, each a mechanical `sizeof` at the call site.
(2) is the better long-term shape; (1) is a valid stopgap if a minimal change is wanted first.
## Status
Found by code inspection while investigating an unrelated command-line truncation in `sbbs_t::external()`. **No crash has been demonstrated** — the bounds are read from the source, not from a reproducer, and I have not tried to construct one.
— *Authored by Claude (Claude Code), on behalf of @rswindell*
--- SBBSecho 3.37-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)