Security hardening: bound FASL sizes, refuse unsafe socket buffers, shell-quote save-lisp paths (S2–S10 except S1/S6) - #1776
Conversation
|
Heads-up on a merge hazard in This PR's 1. objcopy invocation. This PR hardcodes the x86-64 target: OBJCOPY_BINARY " --input-target binary --output-target elf64-x86-64"
" --binary-architecture i386 "On aarch64 those values produce #if defined(__x86_64__)
const char* snapshot_objcopy_target = "elf64-x86-64";
const char* snapshot_objcopy_arch = "i386";
#elif defined(__aarch64__)
const char* snapshot_objcopy_target = "elf64-littleaarch64";
const char* snapshot_objcopy_arch = "aarch64";
#endif
cmd = OBJCOPY_BINARY " --input-target binary --output-target " +
std::string(snapshot_objcopy_target) + " --binary-architecture " +
snapshot_objcopy_arch + " " +
shell_quote(filename) + " " + shell_quote(obj_filename) + ...2. Darwin link command. This PR's version drops the absolute cmd = CXX_BINARY " " BUILD_LINKFLAGS " -o" + shell_quote(snapshot_data->_FileName) + ...
cmd = CXX_BINARY " " BUILD_LINKFLAGS " -L" + shell_quote(snapshot_data->_LibDir) +
" -o" + shell_quote(snapshot_data->_FileName) + ...Note Verified with both halves combined: clean from-scratch builds and full suites on macOS arm64 / LLVM 22 / native (1979 successes, 0 unexpected failures) and x86-64 Linux / LLVM 18 / bytecode (1979, 0 unexpected failures, 13 consecutive runs). Nothing to change in this PR; just flagging it so whoever merges it does not resolve the conflict by taking one side. |
Two file-supplied sizes in loadltv flowed straight into allocation/iteration
with no checking, so one byte of a malformed FASL could overflow the stack or
allocate tens of GB.
op_bignum:
- mp_limb_t limbs[std::abs(ssize)] was a stack VLA sized straight from
the file (stack overflow / smash).
- std::abs(INT64_MIN) is undefined behaviour.
- No upper bound on the limb count.
Reject INT64_MIN, cap at 1<<20 limbs (~8 MB bignum, very generous), and use
std::vector instead of a VLA.
op_array:
- total *= dim could overflow size_t over up to 255 dimensions of u16, then
the wrapped-around small value was passed to make_vector / make_mdarray
while the per-element fill loop still walked the full unwrapped count =
read/write past the allocated buffer.
Replace with __builtin_mul_overflow and cap total at 1G elements.
Two issues in the sockets FFI layer that exported low-level socket primitives
to Lisp:
safe_buffer_pointer (used by ll-socket-receive / ll-socket-send-*):
- The (vector t) branch (ComplexVector_T_O) let the caller hand recvfrom/
send a vector of *boxed Lisp pointers*. recvfrom would then scribble raw
network bytes over the GC's tagged pointer values, corrupting the heap.
- Its capacity check did size=(size+divisor-1)/divisor on a uint, which
wrapped when length (signed int, passed in from Lisp) was negative or
huge; the resulting small size then passed the bounds check and the
syscall ran with the original signed length cast to size_t = multi-GB OOB.
Refuse non-byte/char buffers altogether (high-level wrappers already use
strings or (simple-array (unsigned-byte 8))), and reject negative size at
the entry. Change the parameter type from uint to int so the negativity
check is well-defined.
FD_SET / FD_CLR / FD_ISSET (sockets_internal__fdset_*):
- Took fd from Lisp with no range check. FD_SET on an fd outside
[0, FD_SETSIZE) writes out of bounds of the fixed bit array.
Range-check fd via a shared check_fdset_fd helper.
serve_event_internal__ll_fd_set / ll_fd_isset took fd from Lisp and called FD_SET / FD_ISSET with no bounds check; an fd outside [0, FD_SETSIZE) writes or reads out of bounds of the fixed bit array.
The snapshot save/load code built shell command lines by string concatenation
and ran them through system() / popen():
- popen("nm \"" + filename + "\"", "r") (loadLibrarySymbolLookup)
- system(OBJCOPY_BINARY " ... " + filename + ...) (Linux objcopy)
- system(CXX_BINARY " ... -o" + _FileName + ...) (Linux+Darwin link)
A filename / library-dir containing shell metacharacters (or just a double
quote) would inject commands -- for example save-lisp-and-die with
:executable t :file "foo; rm -rf ~/work; #"
would run the rm at link time.
Add a small shell_quote helper (single-quote, escape embedded ' as '\\'')
and wrap every user-controlled value (filename, _FileName, _LibDir,
obj_filename) before interpolating it. BUILD_LINKFLAGS / BUILD_LIB / the
SNAPSHOT_* macros come from clasp's own config.h, not from Lisp, and stay
unquoted. mangled_name is already restricted to [A-Za-z0-9_] and stays
unquoted.
The exec-environment build loop did:
environ[j] = (char*)malloc(l + 1);
memcpy(environ[j], ss.c_str(), l);
j++;
environ[j][l] = 0;
The null-terminate fired AFTER j++, so it wrote into environ[j] (the next,
still-uninitialized pointer slot) at offset l = wild write at an
uncontrolled address + the actual string was never NUL-terminated. Move the
terminate before j++.
The LLVM symbol-lookup callback strcpy'd the result of a stringstream into a fixed 1 KB buffer (global_LLVMSymbolLookupCallbackBuffer[CALLBACK_BUFFER_SIZE]). The stringstream can exceed that -- e.g. when dbg_safe_repr emits a long object printout -- so the strcpy could overflow. Replace with a bounded memcpy that truncates to CALLBACK_BUFFER_SIZE-1 bytes and NUL-terminates. This is a JIT debug/diagnostic path, so a clipped name is acceptable.
lu_procname's give-up branch (when the function name didn't fit even at
nbytes=4096) did:
char fnamedot[nbytes + 3];
fnamedot[0] = '\0';
strcat(fnamedot, fname);
strcat(fnamedot, "...");
This assumes fname is NUL-terminated, but unw_get_proc_name returned
UNW_ENOMEM -- meaning the buffer was too small, and libunwind may have
filled all nbytes bytes without writing a terminator. strcat then reads
off the end of fname into the surrounding stack until it finds a NUL,
and writes that (potentially much longer) content into fnamedot = OOB
read + write.
Bound the read with strnlen and build the result with std::string instead
of strcat, eliminating both hazards. Backtrace formatting is called from
the debugger, which is exactly the path you do not want to crash.
1 participant
Summary
Six small, file-local security fixes addressing pre-existing vulnerabilities at the FASL loader, sockets FFI, save-lisp-and-die build path, exec-environment construction, and the LLVM symbol-lookup callback. Each commit is independent and reviewable on its own.
Threat-model framing
clasp has no internal security boundary by design — loading a FASL is arbitrary code execution, and any Lisp code already has full
ext:system/ FFI. These fixes therefore matter at real trust boundaries: loading FASLs from a less-trusted source, feeding network bytes to the socket primitives, or passing externally-influenced paths tosave-lisp-and-die.The fixes
loadltv: validate FASL-supplied bignum and array sizesop_bignumdidmp_limb_t limbs[std::abs(ssize)]— stack VLA sized from a file byte, withstd::abs(INT64_MIN)UB and no upper bound. (S7)op_array'stotal *= dimcould overflowsize_tthen be passed tomake_vector/make_mdarraywhile the per-element fill walked the full unwrapped count → read/write past the bufferINT64_MIN, cap limbs at1<<20, heap-allocate viastd::vector; replacetotal *= dimwith__builtin_mul_overflowand cap at1<<30elementssockets: refuse unsafe buffer types + range-check fd_set fdssafe_buffer_pointer's(vector t)branch letrecvfrom/sendscribble raw network bytes over a vector of boxed Lisp pointers (GC heap corruption); its(size+divisor-1)/divisorcapacity check wrapped on a negativelengthcast touint. (S8)FD_SET/FD_CLR/FD_ISSETfrom Lisp had no range check, writing/reading out of bounds forfd >= FD_SETSIZE(simple-array (unsigned-byte 8)), reject negative size, change the param type toint; range-check fd in all three operatorsserve-event: range-check fd_set fdsFD_SET/FD_ISSETissue in serve-event's variantssave-lisp-and-die: shell-quote interpolated pathssystem(...)andpopen(...)built command lines by string concatenation withfilename/_FileName/_LibDir.(save-lisp-and-die :executable t :file "foo; rm -rf ~/work; #")would run thermat link timeshell_quotehelper (single-quote + escape internal'), wrap every user-controlled value before interpolation; build-time-constantBUILD_LINKFLAGS/BUILD_LIBstay unquoted, andmangled_nameis already restricted to[A-Za-z0-9_]unixsys: terminate env strings before advancing the indexenviron[j]=malloc(...); memcpy(...); j++; environ[j][l]=0;— the NUL-terminate fired afterj++, so it wrote into the next, still-uninitialized pointer slot at offsetl(wild write) and the actual string was never terminatedj++Bound the LLVM symbol-lookup callback buffer copystrcpy(global_LLVMSymbolLookupCallbackBuffer /* [1024] */, stringstream.str().c_str())— buffer overflow whendbg_safe_repremits a long object printout (JIT debug path)memcpythat truncates toCALLBACK_BUFFER_SIZE-1and NUL-terminatesWhat this PR does not do
DEBUG_VIRTUAL_MACHINEoff (release), VM bounds checks are compiled out, so a malformed FASL can do anything the VM can do. Adding a verifier is a much larger, separate design project — out of scope here.Testing
Full clasp regression suite byte-identical to the pre-change baseline after the combined set: 1877 pass, 0 bus errors / segfaults / memory corruption. (Same 4 pre-existing/expected failures listed in
set-unexpected-failures.lisp, unrelated.) The suite exercises FASL loading, the sockets FFI, andsave-lisp-and-diepaths.🤖 Generated with Claude Code