OpenSSH and the dangers of unused code
Unused code is untested code, which probably means that it harbors bugs—sometimes significant security bugs. That lesson has been reinforced by the recent OpenSSH "roaming" vulnerability. Leaving a half-finished feature only in the client side of the equation might seem harmless on a cursory glance but, of course, is not. Those who mean harm can run servers that "implement" the feature to tickle the unused code. Given that the OpenSSH project has a strong security focus (and track record), it is truly surprising that a blunder like this could slip through—and keep slipping through for roughly six years.
The first notice of the bug was posted by Theo de Raadt on January 14. He noted that an update was coming soon and that users could turn off the experimental client roaming feature by setting the undocumented UseRoaming configuration variable to "no". The update was announced by Damien Miller later that day. It simply disabled the roaming feature entirely, though it also fixed a few other security bugs as well. The problems have been present since the roaming feature was added to the client (but not the server) in OpenSSH 5.4, which was released in March 2010.
The bug was found by Qualys, which put out a
detailed
advisory that described two separate flaws, both of which were in the
roaming code. The first is by far the most dangerous; it is an information
leak that can provide the server with a copy of the client system's private
SSH keys (CVE-2016-0777). The second is a buffer overflow (CVE-2016-0778)
that is "unlikely to have any
real-world impact
" because it relies on two non-default options
being used by the client (ProxyCommand and either
ForwardAgent or ForwardX11)
The private keys of an SSH client are, of course, the most important secret that is used to authenticate the client to a server where the corresponding public key has been installed. An attacker who has that private key can authenticate to any of the servers authorized by the user, assuming that there is no second authentication factor required. So they can effectively act as that user on the remote host(s). It should be noted that password-protected private keys are leaked in their encrypted form, which would still allow an attacker to try to break the passphrase offline. Also, if an agent such as ssh-agent is used, no key material is leaked.
The Qualys advisory includes patches to the OpenSSH server that implement a proof of concept of what a malicious server could do. The proof of concept is incomplete as there are environment-variable parameters used in the examples in the advisory that are not present in that code (notably, "heap_massaging:linux").
At its core, the problem in the client code (aside from still being present long after the server side was removed) is that it uses a server-supplied length to determine the size of a buffer to allocate—without much in the way of sanity checks. It also allocates the buffer using malloc(), which doesn't clear the memory being allocated.
The roaming feature is meant to handle the case when the SSH connection is lost (due to a transient problem of some sort) and allow the client to reconnect transparently. The client stores data that it has sent, but may not yet have been received by the server (and might get lost during the interruption). After the reconnect, the server can request that the client "resend" a certain number of bytes—even if the client never sent that many bytes. The server-controlled offset parameter can be used to trick the client into sending the entire contents of the buffer even though it has not written anything to it, thus leaking the data that was previously stored there.
So malicious servers can offer roaming to clients during the key-exchange phase, disconnect the client, then request a whole buffer's worth of data be "resent" after reconnection. There are some conditions that need to be met in order to exploit the flaw that are described in the advisory, such as "heap massaging" to force malloc() to return sensitive data and guessing the client send buffer size. But Qualys was able to extract some private key information from clients running on a number of different systems (including OpenBSD, FreeBSD, CentOS, and Fedora).
Qualys initially believed that the information leak would not actually leak private keys for a few different reasons. For one, the leak is from memory that has been freed, but is recycled in a subsequent allocation, rather than reading data beyond the end of a buffer, such as in a more-typical buffer overflow. In addition, OpenSSH took some pains to clear the sensitive data from memory.
It turns out that some of those attempts to clear sensitive information (like private keys) out of memory using memset() and bzero() were optimized away by some compilers. Clang/LLVM and GCC 5 use an optimization known as "dead store elimination" that gets rid of store operations to memory that is never read again. Some of the changes in the OpenSSH update are to use explicit_bzero() to avoid that optimization in sensitive places.
But a much bigger factor in disclosing the key information is the use of the C library's standard I/O functions—in this case fopen() and friends. The OpenSSH client uses those functions to read in the key files from the user's .ssh directory; they do buffered I/O, which means they have their own internal buffers that are allocated and freed as needed. On Linux, that's not a problem because the GNU C library (Glibc) effectively cleanses the buffers before freeing them. But on BSD-based systems, freed buffers will contain data from previous operations.
It is not entirely clear why Qualys was able to extract key information on
Linux systems given the Glibc behavior. The advisory does note that there
may be other ways for the key material to leak "as suggested by
the CentOS and Fedora examples at the end of this section
".
Beyond that, OpenSSH versions from 5.9 onward read() the private key in 1KB chunks into a buffer that is grown using realloc(). Since realloc() may return a newly allocated buffer, that can leave partial copies of the key information in freed memory. Chris Siebenmann has analyzed some of the lessons to be learned from OpenSSH's handling of this sensitive data.
Interactive SSH users who were communicating with a malicious server might well have noticed a problem, though. The OpenSSH client prints a message, "[connection suspended, press return to resume]", whenever a server disconnect is detected. Since causing a disconnect is part of tickling the bug, that message will appear. It would likely cause even a non-savvy user to wonder—and perhaps terminate the connection with Ctrl-C, which would not leak any key information.
But a large number of SSH sessions are not interactive. Various backup scripts and the like use SSH's public-key authentication to authenticate to the server and do their jobs, as does the SSH-based scp command. As Qualys showed, those can be tricked into providing the needed carriage return to resume the connection. Thus they are prime targets for an attack using this vulnerability.
While the bug is quite serious, it is hard to believe it wouldn't have been found if both sides of the roaming feature had been rolled out. Testing and code inspection might have led the OpenSSH developers to discover these problems far earlier. It was presumably overlooked because there was no server code, so it "couldn't hurt" to have the code still present in the client. Enabling an experimental feature by default is a little harder to understand.
For a project that "is developed with the same rigorous security
process that the OpenBSD group is famous for
", as the OpenSSH security page
notes, it is truly a remarkable oversight. It also highlights a lack of
community code review. We are sometimes a bit smug in the open-source
world because we can examine all of the security-sensitive code
running on our systems. But it appears that even for extremely important
tools like OpenSSH, the "can" does not always translate into "do". It
would serve us well to change that tendency.
Companies and organizations like Qualys are likely to
have done multiple code audits on the OpenSSH code over the last six
years. Attackers too, of course. The latter are not going to publish what
they find, but security researchers generally do. A high-profile bug like
this in a security tool that is in widespread use is exactly the kind of
bug they are looking for, so it is surprising this was missed (in white hat
communities, anyway) for so long. In hindsight, leaving the unused code
in the client seems obviously wrong—that's a lesson we can all stand to
relearn.
| Index entries for this article | |
|---|---|
| Security | OpenSSH |
Posted Jan 21, 2016 6:12 UTC (Thu)
by danielpf (guest, #4723)
[Link] (1 responses)
Posted Jan 22, 2016 16:52 UTC (Fri)
by dsommers (subscriber, #55274)
[Link]
Posted Jan 21, 2016 9:19 UTC (Thu)
by mjthayer (guest, #39183)
[Link] (24 responses)
We regularly hear about the damage caused by clever compiler optimisations in interesting situations. I wonder how high the actual benefits of those optimisations are, and whether it would not make sense to limit their use to code areas where they are actually likely to be useful. It could be done naively with existing compilers by putting performance-critical code into separate source files and compiling all other files with low optimisation settings (which would also probably pay back in compile time and energy savings).
Would/could/should Clang and GCC's undefined behaviour sanitiser catch this sort of problem?
Posted Jan 21, 2016 10:02 UTC (Thu)
by dunlapg (guest, #57764)
[Link] (1 responses)
It's likely though, that what would happen is that basically all distros would enable the flag by default, making most of the super-clever work of compiler authors of very limited scope.
Does anyone know anybody who works with the gcc / llvm team, to suggest such a thing?
Posted Jan 21, 2016 21:59 UTC (Thu)
by flussence (guest, #85566)
[Link]
Posted Jan 21, 2016 10:12 UTC (Thu)
by andresfreund (subscriber, #69562)
[Link] (5 responses)
Posted Jan 21, 2016 10:20 UTC (Thu)
by mjthayer (guest, #39183)
[Link] (4 responses)
I thought that the problem was that the code authors forgot to mark the store as having side effects, not that they could not have easily done it if they had thought of it.
Posted Jan 28, 2016 13:09 UTC (Thu)
by Wol (subscriber, #4433)
[Link] (3 responses)
It's down to the compiler to warn them that it's ignoring their instructions!
Okay, maybe one doesn't want too many optimisation warnings, but for the compiler to effective DELETE LINES OF CODE without warning the programmers that the resulting program doesn't actually do what they asked it to, is a compiler bug - optimisation or no optimisation.
Put it another way - THE CODE THE PROGRAMMERS WROTE is either wrong, or meaningful, so for the compiler to just lose it without warning is a compiler error. Saying that the code authors need to add extra instructions to force the compiler to "do as I say" is, as soon as you put it like that, obviously wrong. And something that even an EXPERIENCED programmer is likely to get wrong.
Cheers,
Posted Jan 28, 2016 16:16 UTC (Thu)
by nybble41 (subscriber, #55106)
[Link]
The compiler doesn't have any way to see whether the code was actually what the programmer wrote, or just something embedded in a macro and placed there by the preprocessor. In the latter case the line may make perfect sense in some situations, but have no effect in others. For inlined functions the compiler has a bit more information available (at least during the initial stages, well before the dead store elimination pass) but even there the programmer may just have been writing the code generically—which is simply good coding practice, even if the store is not meaningful in the context of the current compilation unit.
The current behavior is better for the vast majority of real-world programs, where these warnings about standard and expected optimizations would just add noise and perhaps mask more serious issues. For those rare cases where you absolutely need to make sure data isn't retained in memory, there are well-known techniques available to force the store to take place. If you don't know about those techniques and the need to use them, you probably shouldn't be writing such code in the first place.
Posted Jan 29, 2016 17:20 UTC (Fri)
by warrax (subscriber, #103205)
[Link] (1 responses)
See, programming languages are partly defined by this thing called "semantics" (operational or otherwise). The semantics of C say that a 'store' which cannot be observed is irrelevant[1]. Therefore the compiler is free to elide it -- as long as it can prove that nobody can observe it.
(Hence the existence of the "volatile" keyword.)
[1] Well, I'm not actually 100% sure that that's what the standard says, but if it didn't then compiler vendors *wouldn't* be doing this optimization in the first place.
Posted Jan 29, 2016 21:05 UTC (Fri)
by wahern (subscriber, #37304)
[Link]
Here's an excerpt from C11 (draft n1570). Elsewhere the standard says that an object's lifetime ends after a call to free (or realloc). See 6.2.4p2. You then have to rely on the definition (3.4.3p1) and application of undefined behavior to say that any side-effect is unobservable after the object's lifetime has ceased.
It's noteworthy that the last line, "[t]his is the observable behavior of the program", was absent from C99. You're still left to connect the dots about what observable behavior means in relationship to side-effects and, thus, allowable optimizations. Nowhere else is "observable behavior" explicitly mentioned except in the section on atomics, though that fact alone is suggestive of both the intended meaning as well as reasons why the language is so terse and circumspect in this respect.
Posted Jan 21, 2016 10:12 UTC (Thu)
by epa (subscriber, #39769)
[Link] (13 responses)
Posted Jan 21, 2016 10:22 UTC (Thu)
by mjthayer (guest, #39183)
[Link] (1 responses)
Again, I thought this was one of the things that the undefined behaviour sanitiser is supposed to help with, but I don't know if it would have caught this or not.
Posted Jan 23, 2016 1:25 UTC (Sat)
by lsl (subscriber, #86508)
[Link]
USan (-fsanitize=undefined) also has some bounds checks, but AFAIK only those where size information is available at compile time and which are thus (relatively) cheap to do.
Posted Jan 21, 2016 13:20 UTC (Thu)
by NAR (subscriber, #1313)
[Link] (9 responses)
My gut feeling is that the compiler should warn the programmer that "Hey, you're writing data that will be never read. Are you sure you want to do it?" instead of silently removing that particular code. Then the programmer could make his intention clear ("it's sensitive data, I want to clear it") if it's important - or can remove the code if it's really unnecessary. Removing code silently is not a good idea. I don't know if there's already such warning though.
Posted Jan 21, 2016 13:39 UTC (Thu)
by andresfreund (subscriber, #69562)
[Link] (3 responses)
My general feeling about a lot of these complains is that they're a bit one-sided. One one hand many of the same people complaining about such optimizations also complain how nobody cares about optimizations anymore. On the other hand they complain if compiler authors doing part of that work, removing the need for much more failure-prone and more hardware specific (i.e. badly aging) hand optimizations.
Most of the compiler authors aren't sitting there like a bond villain, saying "oh, shiny, an undefined behaviour, let's screw the user. Har. Har. Har.". They write code to infer value ranges and such, and optimize the code accordingly. In the vast majority of the cases that's exactly what code authors want.
Posted Jan 21, 2016 13:57 UTC (Thu)
by mjthayer (guest, #39183)
[Link] (1 responses)
By the 90-10 rule, you can assume that 90 percent of the code does not need optimising at all, either by hand or by the compiler. I much appreciate the compiler optimising those places which do need it, though sometimes I wonder whether pointing out potential hand optimisations instead might not be better still in some (many?) cases. Not that it is likely to happen soon, so I think I can safely wonder.
Posted Jan 21, 2016 14:05 UTC (Thu)
by andresfreund (subscriber, #69562)
[Link]
And humans are notoriously bad at prediction whether code belongs in the 10 or 90 percent bucket. It also changes over time. Whether it's 5-30% that need to be optimized in my experience also hugely depends on the type of project - and tentatively many C projects have a higher percentage of code needing to be optimized than code in higher level languages. And part of the reason why it's often a small percentage needing optimization is because the default code generation is good enough - but that already includes an optimizing compiler.
> I wonder whether pointing out potential hand optimisations instead might not be better still in some (many?) cases
Hand optimizations suck big time. They make code harder to read, and they don't work across existing platforms, not to speak of future ones.
Posted Jan 21, 2016 22:40 UTC (Thu)
by sionescu (subscriber, #59410)
[Link]
I have trouble believing that.
Posted Jan 21, 2016 13:48 UTC (Thu)
by pizza (subscriber, #46)
[Link] (3 responses)
There are warnings for such things (-Wunused turns them all on) but it's not as comprehensive as it could be, especially with respect to dead code elimination -- honestly that might create more noise than signal. But as I've mentioned above, dead store and dead code elimination isn't something that GCC does by default; it has to be requested on the command line. (-O1 and greater turns them on -- and people turn on optimizations because it tends to make things considerably faster..)
For C code I personally maintain, I turn on -Wall -Wextra along with -Os, and it doesn't get released until there are no remaining warnings. I also run it through static analysers, which tends to be more explicit about dead code and stores.
Posted Jan 22, 2016 5:17 UTC (Fri)
by dirtyepic (guest, #30178)
[Link]
It doesn't really matter what the default is if absolutely no one uses it. No distro builds their packages with -O0. If you want to use FORTIFY_SOURCE, which almost every distro enables by default these days, you have to build with optimization. I've even seen several examples over the years of code that doesn't even work unless optimization is enabled. (Yes, these are usually compiler bugs. No, they don't usually get fixed because no one actually cares.)
Posted Jan 23, 2016 5:32 UTC (Sat)
by marcH (subscriber, #57642)
[Link] (1 responses)
Just like many other people I guess, I have some slightly painful memories of never ending arguments about: - the "optimal" set of compiler warnings that finds bugs without blocking development; - stupi^H black and white -Werror policies, etc. Only now I realize that the answer to the question "Do we enable this brand new warning that comes with some false positives?" should be: "Both yes and no."
Posted Jan 28, 2016 13:16 UTC (Thu)
by Wol (subscriber, #4433)
[Link]
So there was never a problem about "we can't get rid of warning X", but stuff DID get fixed. When I first started C programming we had a load of code that was "flaky" because it didn't even get compiled with -W, but I switched it on, fixed the bugs, ramped it up a level, fixed again, ... and a lot of the flakiness went away :-)
Cheers,
Posted Jan 21, 2016 17:01 UTC (Thu)
by iabervon (subscriber, #722)
[Link]
There are also a lot of cases where locations will be read only if some #defines are set; it's nice to be able to sprinkle statements that maintain debugging information through a block of code, and then ultimately use it only in an #ifdef, and rely on the compiler to remove the code in particular builds that don't need it.
It would be nice if a compiler could warn about lines of source that can be removed every time they would generate code, regardless of anything passed in with -D or in config.h, but that's really not tractable in C, given what the preprocessor can do.
Posted Jan 21, 2016 20:19 UTC (Thu)
by mrshiny (guest, #4266)
[Link]
Posted Jan 21, 2016 10:27 UTC (Thu)
by mjthayer (guest, #39183)
[Link]
Slightly off-topic, but something compiler writers could do would be do introduce heuristics to judge how much performance a particular optimisation was likely to achieve before carrying it out, and saving compile time and energy by dropping low-value optimisations. This would have the benefit of being an interesting task for compiler writers, and one which looks good in a release change log.
Posted Jan 21, 2016 20:01 UTC (Thu)
by arielb1@mail.tau.ac.il (guest, #94131)
[Link]
Posted Jan 21, 2016 9:19 UTC (Thu)
by NAR (subscriber, #1313)
[Link] (20 responses)
This is where boringcc should be able to help?
Posted Jan 21, 2016 10:10 UTC (Thu)
by lacos (guest, #70616)
[Link]
Posted Jan 21, 2016 12:36 UTC (Thu)
by szbalint (guest, #95343)
[Link] (7 responses)
"Optimized away" is such a nice euphemism for "security feature critically compromised by insane compiler".
Posted Jan 21, 2016 13:38 UTC (Thu)
by pizza (subscriber, #46)
[Link]
I would tend to agree...
> "Optimized away" is such a nice euphemism for "security feature critically compromised by insane compiler".
No, it's not insane at all. Instead, I'd ask this question -- what compiler flags were selected by the person/makefile/script/etc that compiled the software? If they requested anything other than -O0 (ie no optimizations) then the compiler can't be faulted for doing what was asked of it.
(dead store elimination is '-fdse' on GCC, and is pulled in at -O1 or greater, as is dead code elimination)
Posted Jan 21, 2016 19:17 UTC (Thu)
by apoelstra (subscriber, #75205)
[Link] (5 responses)
Rust uses the LLVM optimizer, and has no way to track when data is moved, meaning that sensitive data can't be guaranteed to be zeroed in Rust any easier than in C. There's been a fair bit of noise about this, but it's not clear how best to handle it.
Posted Jan 23, 2016 13:03 UTC (Sat)
by aggelos (subscriber, #41752)
[Link] (4 responses)
I guess the point was that Rust (presumably w/o unsafe blocks) would not allow the programmers of an SSH implementation to express a memory safety violation, not that it would be easier to somewhat reliably implement a mitigation technique against a specific exploitation of such a vulnerability. Or? That said, I'm curious about your
Posted Jan 23, 2016 19:36 UTC (Sat)
by apoelstra (subscriber, #75205)
[Link] (3 responses)
Posted Jan 23, 2016 20:31 UTC (Sat)
by Cyberax (✭ supporter ✭, #52523)
[Link] (2 responses)
You can get the described effect if you use a stack array. But the solution is pretty simple: don't use them.
Posted Jan 23, 2016 20:52 UTC (Sat)
by apoelstra (subscriber, #75205)
[Link] (1 responses)
Did I say otherwise anywhere? I didn't mean to. I was using a Box as a simple example of a non-Copy type.
>You can get the described effect if you use a stack array.
I'm not sure what you mean here. The "desired effect" was to demonstrate move semantics.
Posted Jan 24, 2016 2:01 UTC (Sun)
by aggelos (subscriber, #41752)
[Link]
I'd like to keep discussing the issues with tooling that might make zeroing of sensitive data unreliable, but at this point I can only marvel at how the discussion in the comments concerns itself with the (un)reliability of a mitigation for a very specific exploit of one, out of various different kinds of, memory safety vulnerabilities. To the point where one person assumed that the mention of rust had to do with /that/, instead of the bigger picture (happens to be your comment too but this is coincidental, especially given that your later responses in this subthread were in response to my direct question). Seems disturbingly like a case of missing the (vast) forest of memory safety vulnerabilities for this particular tree (and then the next one, and the one after that :/), to my eyes at least.
Posted Jan 21, 2016 17:45 UTC (Thu)
by gnb (subscriber, #5132)
[Link] (10 responses)
extern void *memset (void *__s, int __c, size_t __n) __THROW __nonnull ((1));
which doesn't seem to contain any information that would allow the compiler to deduce that if the caller never looks at __s the call can be thrown away. Is the compiler simply assuming that a function called memset must have those semantics? Or do some systems have memset as an inline or macro that would allow it to decide that the write doesn't need doing?
Posted Jan 21, 2016 18:41 UTC (Thu)
by nybble41 (subscriber, #55106)
[Link] (9 responses)
If you don't want gcc to treat memset() et al. specially, you can use the -ffreestanding option to override most assumptions about C standard library behavior.
This has implications well beyond low-level functions like memset(). For example, you may find that calls to printf() with constant format strings and no arguments are converted into equivalent calls to puts(). (Checked with gcc 4.9.3 using -O1.)
Posted Jan 21, 2016 19:15 UTC (Thu)
by jimparis (guest, #38647)
[Link]
Posted Jan 22, 2016 13:18 UTC (Fri)
by madscientist (subscriber, #16861)
[Link] (7 responses)
Posted Jan 22, 2016 14:51 UTC (Fri)
by nybble41 (subscriber, #55106)
[Link] (6 responses)
What you actually have to do is implement your own volatile_memset() function which takes a "volatile void*" and is guaranteed to overwrite the target memory area. Or (somewhat less portably) you can trick the compiler into thinking that the memory is actually used:
memset(&x, 0, sizeof x);
No code is generated for the empty __asm__ block, but the compiler must make sure the data is available at that point just the same. The nice thing about implementing it this way is that the compiler can still optimize the memset() call, for example by replacing it with inline store instructions, but the effect will be preserved. Of course, the __asm__ block is not standard C and your mileage with other compilers will vary.
Posted Jan 22, 2016 15:25 UTC (Fri)
by PaXTeam (guest, #24616)
[Link] (5 responses)
Posted Jan 22, 2016 15:54 UTC (Fri)
by nybble41 (subscriber, #55106)
[Link] (4 responses)
The constraint I used, `"m" (x)`, specifies that the content of `x` is read, not just the address, so no `"memory"` clause is necessary.The GCC documentation actually suggests the "m" constraint as an alternative to "memory" when you need to access memory but wish to avoid the overhead of a full memory barrier (See https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Clob..., just before section 6.44.3.3.)
Posted Jan 22, 2016 16:24 UTC (Fri)
by PaXTeam (guest, #24616)
[Link] (3 responses)
error: memory input 0 is not directly addressable
Posted Jan 22, 2016 17:45 UTC (Fri)
by nybble41 (subscriber, #55106)
[Link] (2 responses)
What kind of expression were you trying to use? I don't have a copy of GCC 4.5 handy (it is eight years old, after all), but that form is documented in the manual for GCC 4.5.4 (https://gcc.gnu.org/onlinedocs/gcc-4.5.4/gcc/Extended-Asm...), with the same notes about being an alternative to the "memory" constraint. GCC 4.9 accepts it at least for variables of integer, array, struct, and union types. The expression does need to be an lvalue, of course.
Posted Jan 22, 2016 19:44 UTC (Fri)
by PaXTeam (guest, #24616)
[Link] (1 responses)
$ gcc-4.5.4 -O2 -x c -c - -o /dev/null <<EOF
void foo(int x) {
<stdin>: In function 'foo':
PS: gcc 4.5 is from 2010.
Posted Jan 22, 2016 20:57 UTC (Fri)
by nybble41 (subscriber, #55106)
[Link]
It looks like those versions have a bug in the implementation of the "m" constraint for array types. It works if you wrap the array in a struct:
__asm__ ("" :: "m" (*(struct { __typeof(buf) x; } *)&buf));
A bit awkward, but it does work in at least GCC 4.4, 4.9, and 5.3 as well as Clang 3.6, and the extra syntax could be hidden with a macro.
> PS: gcc 4.5 is from 2010.
So it is. And 4.5.4 is from 2012. So why does the copyright statement in the documentation for 4.5.4 (https://gcc.gnu.org/onlinedocs/gcc-4.5.4/gcc/) indicate that it was last updated in 2008?
Posted Jan 21, 2016 12:23 UTC (Thu)
by PaXTeam (guest, #24616)
[Link]
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
Wol
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
5.1.2.3 Program execution
1 The semantic descriptions in this International Standard describe the behavior of an
abstract machine in which issues of optimization are irrelevant.
2 Accessing a volatile object, modifying an object, modifying a file, or calling a function
that does any of those operations are all side effects, which are changes in the state of
the execution environment. Evaluation of an expression in general includes both value
computations and initiation of side effects. Value computation for an lvalue expression
includes determining the identity of the designated object.
...
4 In the abstract machine, all expressions are evaluated as specified by the semantics. An
actual implementation need not evaluate part of an expression if it can deduce that its
value is not used and that no needed side effects are produced (including any caused by
calling a function or accessing a volatile object).
...
6 The least requirements on a conforming implementation are:
-- Accesses to volatile objects are evaluated strictly according to the rules of the abstract
machine.
-- At program termination, all data written into files shall be identical to the result that
execution of the program according to the abstract semantics would have produced.
-- The input and output dynamics of interactive devices shall take place as specified in
7.21.3. The intent of these requirements is that unbuffered or line-buffered output
appear as soon as possible, to ensure that prompting messages actually appear prior to
a program waiting for input.
This is the observable behavior of the program.
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
"Memory need not be zeroed if it can never be read again,"
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
- Yes for the "static analysis" compile pass that produces metrics;
- No for the production, -Werror pass.
OpenSSH and the dangers of unused code
Wol
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
OpenSSH and the dangers of unused code
"It turns out that some of those attempts to clear sensitive information (like private keys) out of memory using memset() and bzero() were optimized away by some compilers."
Unsecure optimization?
Unsecure optimization?
Unsecure optimization?
Unsecure optimization?
Unsecure optimization?
Unsecure optimization?
has no way to track when data is moved
Do you mean that the optimizer could move sensitive data to different memory locations or simply that some data might have been copied to registers and survived the memset() there?Unsecure optimization?
Do you mean that the optimizer could move sensitive data to different memory locations or simply that some data might have been copied to registers and survived the memset() there?
In Rust, passing by value (rather than through pointers) is very common. This nominally results in a memcpy, though often this can be optimized away. For values that might contain references or other things that have non-duplicatable semantic information attached, the original binding becomes unusable. This is called "moving".
For example,
let x = Box::new(5u32); // creates a box, which cannot be copied as it is the unique owner of its contained value
myfunction(x);
if *x == 5 { // compile error! x was moved when myfunction was called
println! ("hello world");
}
The semantics of this program is that I created a value of type Box<32> on the stack, then memcpy'd it into the stack of myfunction. What the optimizer will likely do here (possibly even with no optimizations on, I don't know) is to create the value on the stack of myfunction in the first place, avoiding the memcpy. However, it is under no obligation to do so.
What this means in practical terms is that Rust code has a ton of "maybe I'll memcpy, maybe I won't", and the common case is that it avoids doing so. But if it does, then sensitive data might wind up in an inaccessible memory location. And allowing detecting this by calling a "move constructor" or something like this would mean that a clear "optimize to no-op" opportunity is lost, which would kill performance of Rust code.
Also there is an argument that it would complicate an already complicated language, for a feature that does not have many use cases.
Unsecure optimization?
Unsecure optimization?
Unsecure optimization?
Did I say otherwise anywhere? I didn't mean to. I was using a Box as a simple example of a non-Copy type.
Fair enough, though your example did have me wondering until I read the comments below.
Unsecure optimization?
Unsecure optimization?
For GCC specifically, this page lists some of the functions that may be treated as builtins. There are a lot!
Unsecure optimization?
All that's necessary is to declare the memset target to be Unsecure optimization?
volatile, then the compiler is not allowed to optimize away any access to it. I would imagine that this would be one of the very first things anyone dealing with sensitive memory would do.
Unsecure optimization?
__asm__ ("" :: "m" (x));
Unsecure optimization?
Unsecure optimization?
Unsecure optimization?
Unsecure optimization?
Unsecure optimization?
#include <string.h>
char buf[10];
int i;
for (i=0; i<sizeof(buf); ++i)
buf[i]=x++;
memset(buf,0,sizeof(buf));
// asm("" : : "r"(buf));
asm("" : : "m"(buf));
// asm("" : : : "memory");
}
EOF
<stdin>:10:18: error: memory input 0 is not directly addressable
Unsecure optimization?
OpenSSH and the dangers of unused code