[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: wine-bugs(a)winehq.org Reporter: jbb.rose(a)yahoo.com 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. -- 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.
https://bugs.winehq.org/show_bug.cgi?id=40742 --- Comment #1 from Brian <jbb.rose(a)yahoo.com> --- 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. -- 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.
https://bugs.winehq.org/show_bug.cgi?id=40742 winetest(a)luukku.com changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |winetest(a)luukku.com --- Comment #2 from winetest(a)luukku.com --- 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. -- 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.
https://bugs.winehq.org/show_bug.cgi?id=40742 --- Comment #3 from winetest(a)luukku.com --- https://wiki.winehq.org/Submitting_Patches -- 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.
https://bugs.winehq.org/show_bug.cgi?id=40742 Jason Edmeades <us(a)edmeades.me.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |us(a)edmeades.me.uk --- Comment #4 from Jason Edmeades <us(a)edmeades.me.uk> --- 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! -- 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.
https://bugs.winehq.org/show_bug.cgi?id=40742 --- Comment #5 from Brian <jbb.rose(a)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.
https://bugs.winehq.org/show_bug.cgi?id=40742 Jason Edmeades <us(a)edmeades.me.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |NEW Assignee|wine-bugs(a)winehq.org |us(a)edmeades.me.uk Ever confirmed|0 |1 --- Comment #6 from Jason Edmeades <us(a)edmeades.me.uk> --- 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. -- 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.
https://bugs.winehq.org/show_bug.cgi?id=40742 --- Comment #7 from Jason Edmeades <us(a)edmeades.me.uk> --- https://www.winehq.org/pipermail/wine-devel/2018-July/129537.html -- 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.
https://bugs.winehq.org/show_bug.cgi?id=40742 Jason Edmeades <us(a)edmeades.me.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |dce5f89e48cc63668b90fbf2a43 | |5cbb54bcf4571 Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #8 from Jason Edmeades <us(a)edmeades.me.uk> --- Committed -- 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.
https://bugs.winehq.org/show_bug.cgi?id=40742 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #9 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 3.13. -- 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.
https://bugs.winehq.org/show_bug.cgi?id=40742 Michael Stefaniuc <mstefani(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|--- |3.0.x -- 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.
https://bugs.winehq.org/show_bug.cgi?id=40742 Michael Stefaniuc <mstefani(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|3.0.x |--- --- Comment #10 from Michael Stefaniuc <mstefani(a)winehq.org> --- Removing the 3.0.x milestone from bugs included in 3.0.3. -- 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.
participants (1)
-
wine-bugs＠winehq.org