mcstrans: fix out-of-bounds read in parse_raw sensitivity parsing - #533
mcstrans: fix out-of-bounds read in parse_raw sensitivity parsing#533netliomax25-code wants to merge 1 commit into
Conversation
parse_raw() advanced the cursor with raw += numdigits(sens) + 1 after sscanf(raw, "s%u", &sens), assuming the consumed length equals the digit count of the parsed value. sscanf %u accepts a leading minus, so a level like "s-1" stores UINT_MAX and numdigits() returns 10 while only two characters were read; raw jumps 11 bytes past a 4-byte buffer and the following *raw == ':' test reads out of bounds. The range reaches parse_raw from trans_context()/untrans_context() -> extract_range() -> compute_trans_from_raw() for any context sent to the mcstransd socket; trans_context splits the range on its first dash, so a high level such as "s0-s-1" arrives here as "s-1". parse_category() uses the same numdigits-based advance at two sites, shielded today only by the MAX_CATS checks. Use sscanf %n to advance by the characters actually consumed at all three sites and drop numdigits(). Signed-off-by: netliomax25-code <netliomax25@gmail.com>
williamcroberts
left a comment
williamcroberts
left a comment
There was a problem hiding this comment.
Reviewed not tested, but it looks good. I had no idea that sscand supports %n.
This needs to go through mailing list like all patches and as @stephensmalley mentioned, your signoff and author details need to be your name.
Negative diffstat for the win, my favorite kind of patches.
|
Sounds good.
|
|
Posted to the mailing list with a corrected Signed-off-by line; is there a reason you haven't fixed this on your end? |
3 participants
Repro: send mcstransd a context whose range high level is "s-1" (e.g. the range "s0-s-1"); trans_context() splits the range on its first dash and calls parse_raw("s-1").
Cause: parse_raw() advances the cursor with raw += numdigits(sens) + 1 after sscanf(raw, "s%u", &sens). sscanf %u accepts a leading minus, so "s-1" stores UINT_MAX into sens and numdigits() returns 10 while only two characters were consumed. raw moves 11 bytes past the 4-byte allocation and the next *raw == : test reads out of bounds. The string is attacker-controlled: it is the MLS range of any context handed to the mcstransd socket via trans_context()/untrans_context() -> extract_range() -> compute_trans_from_raw(). parse_category() advances with the same numdigits construct at two sites; those are currently shielded only by the >= MAX_CATS checks.
Fix: advance by the number of characters sscanf actually consumed via %n at all three sites and drop numdigits(). Validated with an ASAN build of the parse_raw sensitivity logic: input "s-1" reports a heap-buffer-overflow read 7 bytes past the 4-byte buffer before the change, and runs clean after; "s5", "s5:c1.c3" and "s0" parse unchanged.