* Out of bounds memory read in src/stdio/vfprintf.c @ 2018-06-28 17:20 Mark Winterrowd 2018-06-28 17:40 ` Rich Felker 0 siblings, 1 reply; 3+ messages in thread From: Mark Winterrowd @ 2018-06-28 17:20 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 972 bytes --] Hi all, I believe I have found an out of bounds memory read in vfprintf.c On line 509 in src/stdio/vfprintf.c in the current source tree head, you can observe the following snippet of code: /* Format specifier state machine */ st=0; do { if (OOB(*s)) goto inval; ps=st; st=states[st]S(*s++); } while (st-1<STOP); if (!st) goto inval; Note that on line 99 the OOB macro expands to the following test whether the argument falls outside of 'A' and 'z', written to use a single compare: #define OOB(x) ((unsigned)(x)-'A' > 'z'-'A') Unfortunately, the cast to unsigned binds tighter than the subtract from 'A', so if x is less than 'A', OOB will return false. This is common in the case of space, which has an ascii value of 32 compared to 'A' 's value of 65. This causes us to index into states with a negative value for its second dimension, causing us to index to an unpredictable location in states, possibly even off the beginning. Hope this helps, Mark Winterrowd [-- Attachment #2: Type: text/html, Size: 1802 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Out of bounds memory read in src/stdio/vfprintf.c 2018-06-28 17:20 Out of bounds memory read in src/stdio/vfprintf.c Mark Winterrowd @ 2018-06-28 17:40 ` Rich Felker 2018-06-28 17:52 ` Mark Winterrowd 0 siblings, 1 reply; 3+ messages in thread From: Rich Felker @ 2018-06-28 17:40 UTC (permalink / raw) To: musl On Thu, Jun 28, 2018 at 10:20:28AM -0700, Mark Winterrowd wrote: > Hi all, > > I believe I have found an out of bounds memory read in vfprintf.c > > On line 509 in src/stdio/vfprintf.c in the current source tree head, you > can observe the following snippet of code: > > /* Format specifier state machine */ > st=0; > do { > if (OOB(*s)) goto inval; > ps=st; > st=states[st]S(*s++); > } while (st-1<STOP); > if (!st) goto inval; > > Note that on line 99 the OOB macro expands to the following test whether > the argument falls outside of 'A' and 'z', written to use a single compare: > > #define OOB(x) ((unsigned)(x)-'A' > 'z'-'A') > Unfortunately, the cast to unsigned binds tighter than the subtract For this idiom, it's intentional that it bind higher. Here since x is small (char-range) anyway it doesn't matter, but in general the pattern (x-'A') could overflow, producing UB, if x weren't alreaady unsigned. > from 'A', so if x is less than 'A', > OOB will return false. This is common in the case of space, which has > an ascii value of 32 No, the result of (unsigned)(x)-'A' is unsigned, and in the case x<'A', it's a value larger than INT_MAX which is much larger than 'z'-'A'. > compared to 'A' 's value of 65. > > This causes us to index into states with a negative value for its > second dimension, causing us to > index to an unpredictable location in states, possibly even off the beginning. Did you test this? It's possible there's another mistake we're not seeing, but the above isn't one. Also note that passing an invalid format string is UB already, so any graceful handling of that is just hardening, not correctness. Rich ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Out of bounds memory read in src/stdio/vfprintf.c 2018-06-28 17:40 ` Rich Felker @ 2018-06-28 17:52 ` Mark Winterrowd 0 siblings, 0 replies; 3+ messages in thread From: Mark Winterrowd @ 2018-06-28 17:52 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 1990 bytes --] Hm, trying this with my native GCC compiler it does behave as you claim. I think I may have misdiagnosed the issue I was running into. I apologize for taking up your time. On Thu, Jun 28, 2018 at 10:41 AM Rich Felker <dalias@libc.org> wrote: > On Thu, Jun 28, 2018 at 10:20:28AM -0700, Mark Winterrowd wrote: > > Hi all, > > > > I believe I have found an out of bounds memory read in vfprintf.c > > > > On line 509 in src/stdio/vfprintf.c in the current source tree head, you > > can observe the following snippet of code: > > > > /* Format specifier state machine */ > > st=0; > > do { > > if (OOB(*s)) goto inval; > > ps=st; > > st=states[st]S(*s++); > > } while (st-1<STOP); > > if (!st) goto inval; > > > > Note that on line 99 the OOB macro expands to the following test whether > > the argument falls outside of 'A' and 'z', written to use a single > compare: > > > > #define OOB(x) ((unsigned)(x)-'A' > 'z'-'A') > > Unfortunately, the cast to unsigned binds tighter than the subtract > > For this idiom, it's intentional that it bind higher. Here since x is > small (char-range) anyway it doesn't matter, but in general the > pattern (x-'A') could overflow, producing UB, if x weren't alreaady > unsigned. > > > from 'A', so if x is less than 'A', > > OOB will return false. This is common in the case of space, which has > > an ascii value of 32 > > No, the result of (unsigned)(x)-'A' is unsigned, and in the case > x<'A', it's a value larger than INT_MAX which is much larger than > 'z'-'A'. > > > compared to 'A' 's value of 65. > > > > This causes us to index into states with a negative value for its > > second dimension, causing us to > > index to an unpredictable location in states, possibly even off the > beginning. > > Did you test this? It's possible there's another mistake we're not > seeing, but the above isn't one. Also note that passing an invalid > format string is UB already, so any graceful handling of that is just > hardening, not correctness. > > Rich > [-- Attachment #2: Type: text/html, Size: 2633 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-06-28 17:52 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-28 17:20 Out of bounds memory read in src/stdio/vfprintf.c Mark Winterrowd 2018-06-28 17:40 ` Rich Felker 2018-06-28 17:52 ` Mark Winterrowd
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).