FileOpen + InitInterProcessComm: bound user-input string parsing#192
Open
nurdymuny wants to merge 2 commits into
Open
FileOpen + InitInterProcessComm: bound user-input string parsing#192nurdymuny wants to merge 2 commits into
nurdymuny wants to merge 2 commits into
Conversation
Two memory-safety fixes in the config-file ingest paths: 1. Kit/Source/iokit.c::FileOpen strcpy + strcat into a 1024-byte fixed buffer with no bound check on the inputs. A long working directory + long config filename passed through the same FileOpen() call overflows FileName[1024] on the stack. Replaced with snprintf + truncation check. 2. Source/42ipc.c::InitInterProcessComm Five fscanf calls used the unbounded '%s' / '%[^...]' conversion into fixed-size buffers (HostName[40], response[120], FileName[80], Prefix[80]). A hand-written or auto-generated Inp_IPC.txt with a field longer than the buffer overflows the stack frame. Added explicit width specifiers to every %s / %[^...] specifier so the conversion truncates instead of overrunning. These are the two functions on the IPC-config entry path; the same pattern recurs in 42init.c (LoadSC) and 42gl.c (Map / POV config parsers). I left those for a follow-up if you want this scope of change applied more broadly — happy to extend the PR.
…Open scj-hunt's per-target catalog flagged these three OpenFile copies at score 10.0 — same strcpy+strcat-into-FileName[80] pattern as iokit.c::FileOpen (FileName[1024]). Even smaller buffer makes the overflow easier to trigger from a long Path or File argument. All three converted to snprintf with the same truncation-detection shape used in iokit.c. The three tools (Mercator/DEM/Albedo cube converters) share the same OpenFile helper because of copy-paste; ideally they would refactor to call the canonical iokit.c::FileOpen, but that's an architectural change out of scope here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two memory-safety fixes on 42's config-file ingest paths. Closes #191.
1.
FileOpen—strcpy+strcat→snprintfwith truncation checkKit/Source/iokit.c::FileOpenbuilds the full path bystrcpy(FileName, Path); strcat(FileName, File)into a fixed 1024-byte stack buffer. A long working directory + long config filename overflows that buffer.FileOpenis on every config-file load path in 42, so this is broadly reachable.Switched to
snprintf(FileName, sizeof(FileName), "%s%s", Path, File)and added a truncation check that aborts with a clear error if the combined length exceeds 1023 bytes.2.
InitInterProcessComm— width specifiers on everyfscanf%s/%[^...]Source/42ipc.c::InitInterProcessCommreadsInp_IPC.txtwith fivefscanfcalls, every%s/%[^...]is unbounded. A malformed/long field in the config file overflows the receiving buffer (HostName[40],response[120],FileName[80],Prefix[80]).Added explicit width specifiers (
%119s,%39s,%79[^"],%119[^\n],%1[\n]) on every conversion so they truncate at the receiving-buffer size minus the NUL terminator instead of overflowing.Diff
Tests
Inp_IPC.txtwith a 60-byte hostname; before the fix that overflowsI->HostName[40]and trips stack-protector; after the fix the field is truncated to 39 bytes and processing continues.Scope
I kept this PR to the IPC entry-path so the review is small. The same unbounded
fscanf %spattern recurs inSource/42init.c(LoadSC) andSource/42gl.c(POV / Map config parsers) — happy to extend the same fix shape to those if you want a broader sweep in a follow-up.Linked
Closes #191.