[Bug 40742] cmd.exe: buffer overflow while parsing qualifiers

[wine-bugs at winehq.org]

https://bugs.winehq.org/show_bug.cgi?id=40742

--- Comment #5 from Brian <jbb.rose at yahoo.com> ---
Jason,

Thanks for dusting off this bug!  I responded briefly via email last week, but
apparently that doesn't work for updating bugs.  Sorry.

Here's what I think is going on in the code:

WCMD_execute() calls WCMD_parse(), passing the variable p as its first
parameter ("s") and the global quals[] array as its second parameter ("q"). 
WCMD_parse() scans the entire string in s, and accumulates all the qualifiers
in q, just like the comments say.  So the amount of space needed in q is
limited only by the length of s.  That is, q needs to be sized large enough to
hold essentially all of s, not just a single qualifier.

In WCMD_execute(), p is set from WCMD_skip_leading_spaces(), which just returns
its first parameter, possibly incremented to skip whitespace. 
WCMD_skip_leading_spaces() is passed &whichcmd[count], where count is the
length of the command name.  So p is essentially the same length as whichcmd.  

whichcmd, in turn, is set from WCMD_skip_leading_spaces(cmd).  And cmd is set
from new_cmd.  new_cmd is allocated via heap_alloc(MAXSTRING * sizeof(WCHAR)),
and filled from command, which is passed into WCMD_execute().  That strongly
suggests that cmd, whichcmd, p, q, and quals[] all must be sized to hold
MAXSTRING characters.

Looking further back in the call chain, things get kind of complicated, but it
appears that command is allocated in WCMD_ReadAndParseLine(), which deals in
buffers sized by MAXSTRING.  MAXSTRING seems to be the maximum size of an input
line, and hence a command.

In short, I think the issue is that quals[] needs to be big enough to hold
*all* the qualifiers on a command, not just one of them.  So MAX_PATH (260)
isn't big enough... it really should be MAXSTRING (8192), as used for all the
other buffers related to command processing.

I haven't explicitly tried reproducing it lately, but the affected code hasn't
changed between 1.9.11 and current HEAD.  But whether or not overflowing
quals[] causes a crash depends on luck, the input data, and where the linker
happens to place quals[] relative to other data.

Thanks,
Brian

-- 
Do not reply to this email, post in Bugzilla using the
above URL to reply.
You are receiving this mail because:
You are watching all bug changes.