Some Bad News and Some Good News
Michael Howard
Secure Windows Initiative
October 21, 2002
Summary: Michael Howard discusses the finer points of scrubbing secret data from memory, as well as expounding on mitigating cross-site scripting issues using the HttpOnly cookie extension. (8 printed pages)
People are so predictable! If you ever said to me, "I have some good news, and I have some bad news, which do you want first?" I always opt for the bad news first. I think it's because I'm an eternal optimist and I figure that if you give me the good news last, it'll temper the bad news.
So, here's the bad news first.
The Bad News: Scrubbing Secrets in Memory
It's generally considered good practice to remove secret data from memory when it's no longer needed, which helps reduce the chance that sensitive data will find its way into a page file or a crash dump file. Take a look at the code below, compiled with Microsoft Visual C++® .NET, and see if you can you find the flaw?
BOOL DoStuff() {
char pPwd[64];
size_t cchPwd = sizeof(pPwd) / sizeof(pPwd[0]);
BOOL fOK = false;
if (GetPassword(pPwd, &cchPwd))
fOK = DoSecretStuff(pPwd, cchPwd);
memset(pPwd, 0, sizeof(pPwd));
return fOK;
}
I'll give you a tip—there's nothing wrong with the code. It's written as you would expect. The user's password is retrieved (how it is retrieved doesn't matter for this example), and then some sensitive operation is performed using the password. Once the task is completed, all bytes in the password are scrubbed.
The problem is not in the code, but rather in the compiled code. Take a look at the assembly output from this function. I've cleaned it up a little to remove the unnecessary stuff and make it readable.
?DoStuff PROC NEAR
; Line 14
sub esp, 68 ; 00000044H
mov eax, DWORD PTR ___security_cookie
xor eax, DWORD PTR __$ReturnAddr$[esp+64]
push esi
mov DWORD PTR __$ArrayPad$[esp+72], eax
; Line 19
lea eax, DWORD PTR _pPwd$[esp+72]
push 64 ; 00000040H
push eax
xor esi, esi
call ?GetPassword
add esp, 8
test eax, eax
je SHORT $L30117
; Line 20
push 64 ; 00000040H
lea ecx, DWORD PTR _pPwd$[esp+76]
push ecx
call ?DoSecretStuff
add esp, 8
pop esi
; Line 25
mov ecx, DWORD PTR __$ArrayPad$[esp+68]
xor ecx, DWORD PTR __$ReturnAddr$[esp+64]
add esp, 68 ; 00000044H
jmp @__security_check_cookie@4
$L30117:
mov ecx, DWORD PTR __$ArrayPad$[esp+72]
xor ecx, DWORD PTR __$ReturnAddr$[esp+68]
mov eax, esi
pop esi
add esp, 68 ; 00000044H
jmp @__security_check_cookie@4
?DoStuff ENDP
Focus on the code between lines 19 and 25. The rest is function prolog and epilog code setting up and tearing down the stack frame, as well as determining if there has been a stack-based buffer overrun. Is something missing? Where's the call to memset? The optimizer has removed it! Now before you run into the streets shouting out that this is a bug in the optimizer, it's not. The optimizer is doing its job. It realizes the local buffer variable pPwd is written to, but never read from again, so it simply removes the function call that manipulates it. It's classic dead store removal, and I have witnessed this optimization in many C and C++ compilers, including offerings from Microsoft, Borland, and GNU.
Since we don't want secrets floating around in memory, let's take a look at some solutions.
- Touch the secret data after the call to memset.
- Replace memset with code that is not optimized out.
- Turn off optimizations for this code.
Touch the Secret Data After the Call to Memset
Touching the secret data is easy to do. You simply add a line of code to the function to read the data in memory, but be wary of the optimizer again. You can fool the optimizer by casting the pointer to a volatile pointer because volatile pointers can be manipulated outside the scope of the application they are not optimized out by the compiler.
Changing the last part of the code to that shown below will keep the optimizer at bay:
memset(pPwd, 0, sizeof(pPwd));
*(volatile char*)pPwd = *(volatile char *)pPwd;
return fOK;
Replace Memset with Code That Is Not Optimized Out
When this issue was discovered during the Windows Security Push, a new version of ZeroMemory (a wrapper over memset) was written that is not optimized out, and it's available in newer versions of WinBase.h. If you don't have the updated Platform SDK, here's what the code looks like (it's nothing more than inline code):
#ifndef FORCEINLINE
#if (MSC_VER >= 1200)
#define FORCEINLINE __forceinline
#else
#define FORCEINLINE __inline
#endif
#endif
...
FORCEINLINE PVOID SecureZeroMemory(
void *ptr, size_t cnt) {
volatile char *vptr = (volatile char *)ptr;
while (cnt) {
*vptr = 0;
vptr++;
cnt--;
}
return ptr;
}
This code uses the volatile trick to prevent the compiler from optimizing out the code. Do not simply replace all calls to ZeroMemory or memset with this code; this code is much slower. Find where you are dealing with secret or sensitive data in your code and scrub that data with SecureZeroMemory.
Turn Off Optimizations for this Code
The problem with the previous two examples is that they only work well today. The optimizer developers are always looking at ways to squeeze that last ounce of size and speed from your code, and who knows, three years from now, there may be a way to optimize volatile pointer code safely. A clean way to solve the issue is to create one source code file that contains the functions that scrub data and turn off optimizations for that file. A simple way to achieve this in Visual C++ is to add the following line at the start of the file:
#pragma optimize("g",off)
This will turn off global optimizations for this file until the compiler detects a different #pragma optimize directive in the file. Global optimizations, -Og (implied by –Ox, -O1 and –O2), are what Visual C++ uses to remove dead stores. However, remember, global optimizations are a good thing, so keep the code in this file to a minimum.
You can also place optimize("",off) and optimize("",on) around the function calls in question.
Enough of the bad news; let's look at the good news.
The Good News: Mitigating Cross-Site Scripting Issues
During the Windows Security Push a number of us got together to discuss ways of mitigating attacks, or at least reducing the chance of attack, and the potential for damage if an attack were to occur. One hot topic was cross-site scripting attacks (XSS). I'm not going to outline XSS issues here, but if you are not aware of what they are you should read When Output Turns Bad: Cross-Site Scripting Explained.
I've heard many discussions, okay arguments, where the server guys point the finger at the browser guys saying, "You shouldn't execute script code like this," and the browser guys shooting back, "Well, you should only give us well-formed output." And on and on it goes, and the problem never gets solved. Frankly, I don't care to lay blame. It's our collective clients at risk, so we should all pitch in to fix the issue. If all the time and energy spent pointing fingers and accusing people were spent fixing issues, I wouldn't need this column and Writing Secure Code wouldn't be on anyone's bookshelf.
The Microsoft Internet Explorer team came up with the idea of marking cookies as scriptless. Think about what cookies are for a moment. They are opaque blobs understood by the server, and not the client, and normally they should not be accessed by client code. Most XSS attacks, but not all, involve cookie data disclosure, and if a server could mark a cookie indicating that it should never be accessed by the client, and have the client browser enforce this policy, it would help mitigate the issue. Note, this does not mean you can continue to write sloppy XSS-ridden code. Think of this cookie option as a small insurance policy that helps reduce information disclosure threats and nothing more.
If Internet Explorer 6.0 SP1 (available in Microsoft Windows® XP SP1 and at the Windows Update Site) detects a cookie marked HttpOnly and some client side script code, such as JavaScript, attempts to read the cookie (document.cookie, for example), Internet Explorer returns an empty string, thus preventing the attack by preventing the malicious code in the XSS attack from sending the data back to a malicious site. Of course, the cookie is passed to and from the originating server as normal; the browser using script code just can't read it.
A cookie is set on the client with an HTTP response header. The following shows the syntax used in this header:
Set-Cookie: <name>=<value>[; <name>=<value>]
[; expires=<date>][; domain=<domain_name>]
[; path=<some_path>][; secure][; HttpOnly]
Note, the HttpOnly attribute is not case sensitive.
You can set the HttpOnly option by updating ASP and ASP.NET pages with constructs such as:
Response.AddHeader("Set-Cookie","Name=Michael; path=/; HttpOnly; Expires=" + CStr(Now))
And:
HttpCookie cookie = new HttpCookie("Name", "Michael");
cookie.Path = "/; HttpOnly";
Response.Cookies.Add(cookie);
Or you can create an ISAPI filter that performs the task automatically for all outbound cookies with the following code:
#include <windows.h>
#include <httpfilt.h>
#include "tchar.h"
#include "strsafe.h"
// Portion of HttpOnly
DWORD WINAPI HttpFilterProc(
PHTTP_FILTER_CONTEXT pfc,
DWORD dwNotificationType,
LPVOID pvNotification) {
// Hard coded cookie length (2k bytes)
CHAR szCookie[2048];
DWORD cbCookieOriginal = sizeof(szCookie) / sizeof(szCookie[0]);
DWORD cbCookie = cbCookieOriginal;
HTTP_FILTER_SEND_RESPONSE *pResponse =
(HTTP_FILTER_SEND_RESPONSE*)pvNotification;
CHAR *szHeader = "Set-Cookie:";
CHAR *szHttpOnly = "; HttpOnly";
if (pResponse->GetHeader(pfc,szHeader,szCookie,&cbCookie)) {
if (SUCCEEDED(StringCchCat(szCookie,
cbCookieOriginal,
szHttpOnly))) {
if (!pResponse->SetHeader(pfc,
szHeader,
szCookie)) {
// Fail securely - send no cookie!
pResponse->SetHeader(pfc,szHeader,"");
}
} else {
pResponse->SetHeader(pfc,szHeader,"");
}
}
return SF_STATUS_REQ_NEXT_NOTIFICATION;
}
I know you know this, but I'll say it anyway—this does not solve the XSS issue! It only mitigates some vulnerabilities and it is not a replacement for good server-side programming techniques.
A Plea to Other Browser Vendors
We urge other browser vendors to adopt the HttpOnly cookie extension, not just Internet Explorer, as this provides a degree of protection to our collective customers using a fairly low-tech approach. I say this because there is no way we can expect all server applications to be fixed any time soon, and HttpOnly is a reasonable effective solution to a tricky problem.
Spot the Security Flaw
I think everyone spotted the bug in the last article. It was a buffer overrun, but a little different from the usual vulnerability. Rather than using a buffer copying function, such as mempy or strcpy, this code allows the attacker to write data anywhere in memory based on the variables index and d.
Now to this month's flaw. This code is from a service that runs as SYSTEM, and it makes file-based requests on behalf of its users.
bool WritePipeDataToFile(HANDLE hPipe) {
bool fDataWritten = false;
ImpersonateNamedPipeClient(hPipe);
HANDLE hFile = CreateFile(...);
if (hFile != INVALID_HANDLE_VALUE) {
BYTE buff[1024];
DWORD cbRead = 0;
if (ReadFile(hPipe,
buff,
sizeof(buff),
&cbRead,
NULL)) {
DWORD cbWritten = 0;
if (WriteFile(hFile,
buff,
cbRead,
&cbWritten,
NULL)) {
if (cbRead == cbWritten)
fDataWritten = true;
}
}
if (hFile) CloseHandle(hFile);
}
RevertToSelf();
return fDataWritten;
}
Michael Howard is a Senior Security Program Manager in the Secure Windows Initiative group at Microsoft and is the coauthor of Writing Secure Code and the main author of Designing Secure Web-based Applications for Windows 2000. His main focus in life is making sure people design, build, test, and document nothing short of a secure system. His favorite line is "One person's feature is another's exploit."