Re: Unchecked Buffer

From: Ken Wickes [MS] (kenwic@online.microsoft.com)
Date: 01/30/03 

From: "Ken Wickes [MS]" <kenwic@online.microsoft.com>
Date: Thu, 30 Jan 2003 12:09:54 -0800

"Hector Santos" <nospam@nospam.com> wrote in message
news:eucV9LIyCHA.2536@TK2MSFTNGP09...
> > > >
> > > > TCHAR szBuffer[256];
> > > > GetWindowText(hWnd, szBuffer, sizeof(szBuffer));
> > >
> > > [clip]
> > >
> > > 2) this is an internal error and not one that is caused by an external
> > > interface point or input condition where foreign logic can be
inserted.
> > > In other words, the above is not a potential security design issue.
> > >
> >
> > It is a security issue. I can just set my window title to contain
object
> > code, then the code above executing as admin will overwrite its stack
with
> > my code and I can take control of an exe running under admin.
>
> Ok, good point.
>
> However, I have two more comments: :-)
>
> First, wouldn't this have be a malicious piece of software to begin with?
> Its a little different from lets say "harder to detect" software for
> "security vulnerabilities" which I believe was your original excellent
point
> when attempting to go over software to check for possible stack overflow
> conditions. In other words, the above two lines used in legimate code
is
> a coding bug, but it is not, in my view, a security issue. The computer
> virus epidemic the PC world is suffering from is one of
"interconnectivity"
> meaning how a non-malicious software reacts to chaotic outside world
input.

Well my example was just a typical overrung caused by ANSI/UNICODE
confusion. But it is still an issue. Consider this, IE sets its title bar
to contain the title of a web page, so a remote site is able to set window
text on the local machine where an app might GetWindowText. Also it's not
just GetWindowText, there are many many apis where one could make the same
mistake.

>
> Second, although I think your point is well taken, is it a bad example?
>
> Yes, I agree, the above harder to detect the syntax level. I was
thinking
> it should be picked up by the compiler. So I did some compiler testing
> myself.
>
> The result is that, no, the compiler will not pick this up for the
following
> reason:
>
> According to MSDN, GetWindowText() is implemented for UNICODE and ANSI
> under Windows NT/2000. So the microsoft compiler will correctly size
TCHAR
> as either 16 bits (UNICODE) or 8 bits (ANSI). GetWindowText() 3rd
paramter
> is the size. So even if TCHAR is unicode or ansi, the correct size is
> allocated on the local stack and the requested size is passed to
> GetWindowText().

No, the correct third param is number of characters including the NULL.

> So the problem, in my view, is not that of UNICODE vs ANSI. But rather
how
> you coded the 3rd parameter in GetWindowText().
>
> Regardless of it being UNICODE or ANSI , to be more programmatically
> correct, it should instead have logic that considers the null character.
> For example, this would be a more correct way to code this:
>
> TCHAR szBuffer[256];
> ZeroMemory(&szBuffer,sizeof(szBuffer));
> GetWindowText(hWnd, szBuffer, sizeof(szBuffer)-sizeof(TCHAR));

The correct coding is GetWindowText(hWnd, szBuffer, sizeof(szBuffer) /
sizeof(TCHAR));

>
> As you had it above, it can be overrun by 1 (ansi) or 2 (unicode) bytes .

It could be overrung by 256 bytes.

>
> This has to be taken into account. In my experience, when reviewing
> programmer's code, I see this as a common C programming mistake. The
NULL
> byte is not accounted for when using sizeof() in functions that request
> size.
>
> Of course, one of the neat things about C++ and MFC, is that there is a
C++
> version of GetWindowText() that removes the programming consideration of
> size.
>
>
> CString sBuffer;
> GetWindowText(sBuffer);

There are many ways. I prefer the COM convention where the caller allocs
the buffer.

> Its a higher overhead issue, if one wishes to that that into account, but
> it does resolve the programmer's requirement of checking for size.
>
> Nonethless, you are correct. It is a stack overflow problem that is
harder
> to detect at the software coding level. An "advanced Microsoft"
compiler
> is required to do "smarter parameter passing" checking. The compiler
will
> need to have the protocol header rule for "GetWindowText()" and see that
the
> buffer address is a fixed size local stack variable and check that the
> correct static request size was passed. The current compiler would not
be
> able to detect this. It should of, however in my view, be able to issue
a
> warning pointing out a potential type mismatch. The header ask for
LPTSTR.
> The compiler can detect that there is potential problem when a TCHAR
> variable is passed.
>
> In any case, Ken, I think we all get the point and understand what you
> were were pointing out. :-)
>
> See ya
>
> --
> Hector Santos
> Wildcat! Interactive Net Server
> http://www.santronics.com
>
> 

--
This posting is provided "AS IS" with no warranties, and confers no rights.

Relevant Pages

  • Re: Do buffers always start with the lowest memory address being the first element?
    ... > The C standard does not assume a downward-growing stack, ... > an upward-growing stack. ... C allows but does not require that the array produced ... > machine depends on both the C compiler and the machine. ...
    (comp.lang.c)
  • Re: switch statement, was compiler, status...
    ... Primary Register, Secondary Register, ... Stack, and abit of storage does it. ... This version of Small-C is copyrighted as a revision to Ron Cain's ... Croatia) is "Calculator Compiler" by Senko Rasik. ...
    (alt.lang.asm)
  • RE: Anyone looked at the canary stack protection in Win2k3?
    ... I wrote up a simple analysis of Microsoft's /GS compiler option for Visual C++ ... Compiler Security Optimizations ... In Chapter 1 you saw the simplest possible buffer overflow, ... checks to see that it is still alive when a vulnerable stack frame returns. ...
    (Vuln-Dev)
  • Re: Compilation of Code in Microsoft Visual Studio .NET and a couple of other Microsoft .NET questio
    ... I've just recently installed the Visual Studio .net Professional ... part of visual studio, not the compiler. ... then does that mean that I have found a security flaw in .NET or is it just ... Visual Studio .NET will that executable require the .NET framework to be ...
    (microsoft.public.dotnet.general)
  • Re: back online again...
    ... really "acceptable") convention. ... stack machine, but the design ... target with my compiler (may need a different name for this though, ... most of this code is not likely to ever go to object files anyways... ...
    (alt.lang.asm)