FS_GETFILESPEC caused buffer overflows on too small buffers in 3 places:

[schinagl]

General

FS_GETFILESPEC caused buffer overflows on too small buffers in 3 places:

wfcomman.c wfdrives.c FS_GETFILESPEC was called with e.g. MAXFILENAMELEN, but the start of the WCHAR was moved back by lstrlen(szPath), thus causing a buffer overflow.

treectl.c FS_GETFILESPEC was called correctly but when this was executed close to 256 chars it failed in getting the filespec, because the string was truncated @MAXFILENAMELEN

Detail

• If you follow e.g the path from OpenOrEditSelection() it begins with allocating szPath with MAXPATHLEN+2.
• In wfcomman:674 CreateDirWindow() is called with szPath, which still is fine,
• in wfcomman.c:554 the already partially filled szPath is taken, and handed over to FS_GETFILESPEC with MAXFILENAMELEN.
• FS_GETFILESPEC is now allowed to append MAXFILENAMELEN to szPath (it really does this), which already contains e.g. 230 characters, and which definitely causes a buffer overflow.

The symptoms of this problem were already smelled by the authors, because in many places the path for CreateDirWindow() was already MAXPATHLEN * 2.

This issue is a problem when path are longer than 128 characters, but nowadays this happens often.

The same ugly (szPath + lstrlen(szPath) pattern was used in 2 other places, where I also fixed it. Well in treectl.c it didn't crash, but having path longer than 128 char caused a problem, because path were truncated to MAXPATHLEN

[schinagl]

Yes it solves a crash.

OpenOrEditSelection() provided a too small Buffer to CreateDirwindow(). And I had a crash there and needed to debug down to FS_Getselection and beyond to find the reason.

With this change lstrcat would only crash if

• one would pass a smaller path than MAXPATHLEN from OpenOrEditSelection()
• or you have filter larger than *.*

But as mentioned in the last comment it is not a 100% solution as long as we don't pass the length of the receiving path into CreateDirWindow

The previous code crashed/caused mem-corruptions when one was navigating pathes > 128 char.

This code crashes if you navigate pathes with 256 chars and a filter larger than *.*

The second is a lot less probable than the first.

[malxau]

OpenOrEditSelection() provided a too small Buffer to CreateDirwindow(). And I had a crash there and needed to debug down to FS_Getselection and beyond to find the reason.

Okay, so OpenOrEditSelection is still passing a buffer that's too small to CreateDirWindow. But what this change does is ensures that FS_GETFILESPEC always has MAXPATHLEN available, because that function firstly populates the entire directory, then later truncates to just the search pattern. So after this change, there's still an overflow if the search pattern doesn't fit, but it won't overflow trying to copy a second complete directory into a buffer temporarily?

Would it make sense to change FS_GETFILESPEC to use a stack buffer for the directory since it never returns it? There's a big "WARNING" in wftree.c for this, so clearly you weren't the first one to be affected. I'll try to dig up the origin of that comment tomorrow.

This code crashes if you navigate pathes with 256 chars and a filter larger than .

The second is a lot less probable than the first.

Yeah. Low expectations and all that. So many of these lstrcat calls seem completely unsafe.

[schinagl]

Okay, so OpenOrEditSelection is still passing a buffer that's too small to CreateDirWindow. But what this change does is ensures that FS_GETFILESPEC always has MAXPATHLEN available, because that function firstly populates the entire directory, then later truncates to just the search pattern. So after this change, there's still an overflow if the search pattern doesn't fit, but it won't overflow trying to copy a second complete directory into a buffer temporarily?

Yes

Would it make sense to change FS_GETFILESPEC to use a stack buffer for the directory since it never returns it? There's a big "WARNING" in wftree.c for this, so clearly you weren't the first one to be affected. I'll try to dig up the origin of that comment tomorrow.

Sure we could move this to FS_GETFILESPEC and have a local variable there, but I didn't want to touch this. There are invocations of FS_GETFILESPEC which are totally fine with the current behaviour. Why bother them with an extra lstrcpy()?

The problem is deeper in GetMDIWindowText(), but to solve it properly you have to pass the length to GetMDIWindowText() and thus change tons of invocations of GetMDIWindowText(). Sure this is then a 100% solution.

But I don't want to change so many locations. This issue was intended to be a simple improvement to make a crash less ( moving towards ~1% (guessing)) probable.

These thoughts further leads to the general point, that string handling is totally unsafe in Winfile without using xxxx_s functions and passing length. But introducing this to this old codebase introduces a lot of risk, and as usual if you do this you suffer in many places from missing error handling, which then also has to be introduced. You would start an avalance of changes.

So it is also a matter of ROI, and my solution was a way in the middle.

[malxau]

The problem is deeper in GetMDIWindowText(), but to solve it properly you have to pass the length to GetMDIWindowText() and thus change tons of invocations of GetMDIWindowText(). Sure this is then a 100% solution.

Doesn't GetMDIWindowText already take a length? It looks like it allocates a pessimistic buffer on the stack, truncates silently to fit within the supplied length, and copies out. Unfortunately this didn't help here because FS_GETFILESPEC supplied the wrong length, and the information it wants is at the tail of the string so truncation isn't right.

That makes me wonder though if GetMDIWindowText could be told which portion of the string the caller wants, so it can be buffered in this one place and the caller only needs a buffer large enough for the search specification.

These thoughts further leads to the general point, that string handling is totally unsafe in Winfile without using xxxx_s functions and passing length.

I think you already know this, but note by default xxxx_s just terminate the process on overflow, because handling it correctly is hard (eg the above - truncation avoids crashing but the logic is broken if the caller can't get the search criteria.) The real benefit of xxxx_s is manual review of buffer lengths at different points in the code - having to declare buffer sizes at different points enables humans to spot unsafe behavior. Personally I've found SAL useful for this too, since it allows a function to specify an expected buffer length on entry and validate at compile time that the callers provide it.

Running the analyzer on winfile would probably fail catastrophically but the good thing about using SAL is being able to add annotations with no functional behavior change means we can take some fairly large PRs that find problems, followed by more targeted PRs to fix problems.

I looked up the origin of the "WARNING" comment. It was added in 1992 as part of the NT port, so it's mildly amusing but not hugely enlightening. Prior to that the code might have been able to assume 8.3 limits for each component, which NT invalidated.

[schinagl]

Since we agree that the buffer length to FS_GETFILESPEC is wrong in CreateDirWindow(), I suggest to approve this PR.

It improves the situation very much and is a 99% solution for a reasonable effort.

[schinagl]

Basically this is a simple fix for a crash.

How do we proceed here? Is something missing?

[craigwi]

The issues are real and need to get fixed. FS_GETFILESPEC is an internal message; the implementation in wfsearch.c doesn't check the length, but the other cases look ok. In wftree.c is the comment:

// WARNING: Requires ((LPTSTR)lparam)[MAXPATHLEN] space!

The change in treectl.c does not appear to improve things since szPath is already big enough and the original call to FS_GETFILESPEC passed >= MAXPATHLEN space.

The proposed change in wfcommand.c is fine by ensuring FS_GETFILESPEC gets a big enough buffer.

For the change in wfdrives, given that the buffer in the original *2 length is local, you can just pass COUNTOF(szPath) - lstrlen(szPath) to FS_GETFILESPEC. That will ensure FS_GETFILESPEC gets a big enough buffer.

I suggest adding an Assert in all uses of FS_GETFILESPEC (wParam >= MAXPATHLEN) and check the length in wfsearch.

[schinagl]

General

The very FS_GETFILESPEC which is called from CreateDirWindow() (and caused my crashes) is not implemented in wfsearch.c but in wftree.c. There is one FS_GETFILESPEC in wfsearch.c, but it is not called in my case, don't want to touch this. Also don't know how to run into.... Anyhow in wftree.c::FS_GETFILESPEC the buffer is immediatley passed on to GetMDIWindowText() which does its length check.

Thus

• Reverted the changes in wftree.c, because it already provides the proper buffer length
• Changed the changes in wftree.c, to pass COUNTOF(szPath) - lstrlen(szPath) so that we have the proper buffer length in wftree.c::FS_GETFILESPEC
• Kept wfcommand.c as in the commit before, because this commit provides the proper buffer length to wftree.c::FS_GETFILESPEC (and was the problem, which initiated ths PR)

You can not assert in wftree.c::FS_GETFILESPEC as suggested because the buffer is in some cases larger than MAXPATHLEN and it is fine. It depends if the caller has MAXPATHLEN 2 or 3.

I didn't get what you mean with 'adding an Assert in all uses of FS_GETFILESPEC'. Places where Sendmessage(FS_GETFILESPEC, ... ) is issued? In these places you can calculate the proper buffer length, no need to assert. And the buffer length depends. Sometimes 2 or, 3

In a short:

It is important that all invocations of wftree.c::FS_GETFILESPEC specify the proper buffer length. This is acchieved now

[craigwims]

Looks good; thanks @schinagl.