A security vulnerability report arrived that reported a read-after-free bug.
Launch the program under Application Verifier with strict heap enforcement, and it crashes with a use-after-free. This can lead to arbitrary code execution or information disclosure.
☑ Bounty requested
Let’s see what we have:
std::string GetThemeName();
void Vulnerable()
{
auto theme = GetThemeName().c_str();
if (strcmp(theme, "Light") == 0) {
SetLightTheme();
} else if (strcmp(theme, "Dark") == 0) {
SetDarkTheme();
} else {
SetDefaultTheme();
}
}
We indeed have a use-after free bug here, because we are using the c_str() after the temporary std::string has destructed. This leaves us with a pointer to already-freed data, and attempting to compare it to other strings will result in a read-after-free.¹
The claim is that this is possible remote code execution or data disclosure. Does that make sense?
The value read from the already-freed memory is not used to control execution flow directly. It’s not a vtable or a function pointer or index into a jump table, or anything like that, so there’s no code injection opportunity here. The only code that can run is code that was written with the intent of being run. At best, you can cause the wrong code to run.
As for information disclosure, the value read from the already-freed memory is compared against two other values. The string in the already-freed memory is not directly disclosed. The only clue you get is that the theme changes if the already-freed memory contains either of the two magic strings. The information disclosure is therefore one bit of information: “Does the freed memory contain one of the magic strings?”
At best, the program gets the wrong answer to the question, and the user gets the wrong theme.
This code is running at program startup, where there are no competing threads, so the memory is not going to be overwritten during the small window between the free and the use. Therefore, in practice, the original string is still there, and the value will still be correct.
If you are very unlucky (or if bad luck is forced upon you by verification tools), the memory will be unmapped, and the read will crash with an access violation. In that case, you have a denial of service.
Okay, let’s do our assessment.
Who is the attacker?
The finder never said, but maybe we can say that the attacker is somebody who emails you a file that this program uses, trying to induce the read-after-free.
Who is the victim?
Again, the finder never identified the victim, but the victim appears to be the recipient of the file who tries to open it. If the victim is unlucky, the theme is wrong. If the victim is very unlucky, the program crashes. And if the victim preconfigured their system to force extremely bad luck, the program always crashes at startup.
What has the attacker gained?
As far as I can tell, the attacker has gained nothing.
The victim might get the wrong theme, but they would have gotten the wrong theme even without any help from the attacker, seeing as the attacker has done nothing to make the error more likely than it would have been without any effort from the attacker.
Even if the victim experiences a denial of service due the program crashing at launch, this is the same crash that would have occurred without any action from the attacker. (And in the extreme case of having enabled memory verification tools, it is a self-inflicted denial of service.)
On top of all that, the results of the defect are never communicated back to the attacker, so the attacker gains no information.
What they found was a bug, but not a security vulnerability.
This is basically a script-kiddie vulnerability report. They used some tools, followed the instructions from a Web site on how to set up the tools, and with that configuration, they were able to identify a problem and submitted a security vulnerability report without bothering to evaluate what their crash actually told them.
Of the time I spend investigating security vulnerability reports, a good chunk of the time goes into assessing what the attacker actually gained. In many cases, the answer is “Nothing.”
And this is an assessment the finder really should have done before submitting the report. You can’t claim to have found a vulnerability without being able to articulate how it can be exploited.
¹ Let’s ignore the small string optimization. I’ve simplified the code for expository purposes.
This makes me wonder if either the person who wrote the code didn’t trust the optimiser or they did that in an ill thought out attempt to avoid an extra read.
Well I guess information disclosure is app verifier version y or whatever wasn’t run before version x of app meh was released.
Am I the only one, after reading this, who wants to see another blog post showing and explaining us a serious real security vulnerability? Most of the time they are not published even after being fixed. Or they are published with so many technical terms that only a security expert is able to understand it. It even hasn’t to be a recent security vulnerability.
You are not alone! Every time I read one of the “dubious security vulnerability report” articles I wish we also had a few examples written up in a similar fashion of bug reports that indeed turned out to be security vulnerabilities. But I can also think of several reasons why Microsoft might not wish such things to be published.
I agree that it wouldn’t have to be a recent issue (in fact I’m sure we would want to avoid anything recent), but something from the XP days or even Windows 9x would still be interesting.
Based on the C++ I've learned purely from reading your blog, I'm guessing the obvious fix would be to keep the result of GetThemeName() alive by doing something like:
<code>
If that's correct, is there a way to do it without introducing that extra verbosity and the otherwise unused variable? My C#-trained brain considers a variable that's declared, used immediately once, and never again, to be a code smell unless it adds clarity/readability, which is the opposite of what an extra variable does here! I understand that my instinctive reaction doesn't apply in an RAII language, but I'm still curious if there's...
There is one thing that adding extra variables can do in C++ is give the optimiser some hints.
Taking the above situation as is, then if you do:
<code>
Then the optimiser knows that you intend to use the pointer and stores it in a register. It then actually removes the theme variable entirely since it isn't needed. This is where the irony is. Simpler code with extra variables may actually optimise better than clever code because the optimiser is able to see what you are doing.
I have seen interesting performance increases where using a temporary performs better than a long string...
The fix is literally to KISS:<code>
This should in theory compile to the same assembly as the author's intended code. I can't think about a reason why the author had to use C-style string operations either.
Edit: If the author thinks getting the theme name should not introduce a potential allocation and a copy (say, because the set of possible theme names is closed and you can actually list all possible values as string literals, so that they live statically in the compiled binary's text section), then that can't possibly be achieved without changing the return type of itself...
My example was an oversimplification. The return type of GetThemeName was actually a winrt::hstring, but I used std::string to keep things more tightly focused on the issue of temporary lifetime rather than making it look like a C++/WinRT-specific problem.
That is the right answer.
Unfortunately even after 40 years, C++ still suffers too much from C culture, leading to these results.
The way to do it without the otherwise unused variable is to stop trying to work with a pointer.
The most obvious way is to avoid using strcmp to do the comparison. Then you would store the result of GetThemeName() entirely and do the comparison on that. I'd have to check on my references, but I would imagine std::string has an equality operator for char*. This option would have minimal changes to verbosity.
Another alternative would be to store the result into a more appropriate type that could provide a char* but would capture the data. That will...
My favourite C++ string library has a named method to compare against a
const CharT (&)[N].