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

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

Bug ID: 40742
           Summary: cmd.exe: buffer overflow while parsing qualifiers
           Product: Wine
           Version: 1.9.11
          Hardware: x86
                OS: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: cmd
          Assignee: [e-mail]
          Reporter: [e-mail]
      Distribution: ---

Created attachment 54646
  --> https://bugs.winehq.org/attachment.cgi?id=54646
Batch file which triggers the problem

WCMD_parse() copies command qualifiers into the quals[] array.  The
array is MAX_PATH (260) characters long, but the input command can be
up to MAXSTRING (8192) characters long.  This can lead to buffer
overflows and crashes if a command has many qualifiers.

The attached try.bat file, taken from an actual command generated by
a cross-build system, reliably crashes wine 1.9.11 as built on SLES11SP2.

Increasing the size of quals[] to MAXSTRING characters fixes the problem.

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

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

--- Comment #1 from Brian  ---
Created attachment 54647
  --> https://bugs.winehq.org/attachment.cgi?id=54647
Patch fixing the size of the quals[] array

Patch to change the size of quals[] from MAX_PATH to MAXSTRING.

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

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

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #2 from [e-mail] ---
If you think this is correct way to fix this bug then send your patch into
wine-devel list. Patches arent merged from the bug tracker.

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

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

--- Comment #3 from [e-mail] ---
https://wiki.winehq.org/Submitting_Patches

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

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

Jason Edmeades  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #4 from Jason Edmeades  ---
Brian, I appreciate this is an ancient defect but I am just working through
some of the cmd related ones. In principle, your patch would be fine, but I am
struggling slightly in that it doesnt crash any longer on current wine (for
me).

Looking at the wcmd_parse routine, quals gets set when parsing finds a '/'
character and contains data until end of line (a null), a space or another '/'.

The problem I have is your script doesnt contain any long data. The longest
qualifier I can see is 116 characters, and that's way under the limit - hence
I'm thinking something else might be going on here

Are you still able to reproduce? I'd love to be able to get more info (a +cmd
trace for example).

It leaves me in a sticky position - I'd happily have supported your proposed
patch as its only using a tiny amount more memory for a static buffer, however 
 I really cannot see that what you say triggers it would have!

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

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

--- Comment #5 from Brian  ---
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

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

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

Jason Edmeades  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
           Assignee|[e-mail]        |[e-mail]
     Ever confirmed|0                           |1

--- Comment #6 from Jason Edmeades  ---
I agree - as I said, I have no problems with the patch, it was more the
attached batch shouldnt have triggered it as far as I could tell! Anyway, I've
verified with debug code that we do indeed overwrite beyond the end of the
allocated space (I never could trap, even with deliberate and massive command
quals) and will submit giving you credit.

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

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

--- Comment #7 from Jason Edmeades  ---
https://www.winehq.org/pipermail/wine-devel/2018-July/129537.html

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

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

Jason Edmeades  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |dce5f89e48cc63668b90fbf2a43
                   |                            |5cbb54bcf4571
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Jason Edmeades  ---
Committed

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

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

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #9 from Alexandre Julliard  ---
Closing bugs fixed in 3.13.

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

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

Michael Stefaniuc  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |3.0.x

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

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

Michael Stefaniuc  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|3.0.x                       |---

--- Comment #10 from Michael Stefaniuc  ---
Removing the 3.0.x milestone from bugs included in 3.0.3.