Date: Thu, 10 Apr 2014 03:00:00 -0700 From: "Ronald F. Guilmette" <rfg@tristatelogic.com> To: freebsd-security@freebsd.org Subject: Re: Heartbleed, a few naive questions Message-ID: <42638.1397124000@server1.tristatelogic.com> In-Reply-To: <53463A2E.90905@delphij.net>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <53463A2E.90905@delphij.net>, Xin Li <delphij@delphij.net> wrote: >On 4/9/14, 10:28 PM, Ronald F. Guilmette wrote: >> 1) Why does OpenSSL even contain a function called >> "OPENSSL_malloc"? Does anyone other than me think that it might >> perhaps have been a better choice to provide only a function called >> "OPENSSL_calloc"? > >For this bug, doing calloc() makes no difference. I would very much like to know how you reached that conclusion. I may be wrong in this instance... I have been, on more than one occasion in my lifetime... but I do disagree with your conclusion in this instance. The bug is described in some great detail here: http://blog.existentialize.com/diagnosis-of-the-openssl-heartbleed-bug.html What I believe to the the text of the buggy code in question appears to be available here: http://code.woboq.org/crypto/openssl/ssl/d1_both.c.html For the information being leaked to an attacker by this bug, where exactly do _you_ believe it is coming from, if not out of a buffer which has been allocated (out of recycled and non-cleared memory) by the call to OPENSSL_malloc? >Is doing calloc() a >good practice in general? Sometimes it is, sometimes it merely slows >things down. I agree entirely. If data leakage is occurring within the program that I use to crop my vacation pictures, the world will not be terribly the worse for it. but that was not the question I was asking. Rather, I was asking, albeit indirectly, whether a program or library, such as OpenSSL, which is primarily a security-forcused tool, and upon which a significant fraction of online humanity depends for its security, is deserving of a "belt and suspenders" sytle of defensive programming. >> 2) Not that this would actually have eliminated the bug, but... >> >> Was there some compelling reason why space for the "buffer" at >> issue was allocated via a call to "OPENSSL_malloc", rather than, >> say, simply being declared as a function-local "auto" >> char[1+2+65536+padding] ? > >Automatic arrays are introduced with C99. I am sorry that I failed to be clear. My question has/has nothing to do what what you are calling "automatic arrays" and what I would prefer to call "dynamically sized arrays". If you look at the actual code you will see that "padding" is not really a variable quantity. Rather it appears to have a fixed value (16). So the real question I had intended to pose was actually: "Why wasn't space for the buffer obtained by a simple local variable declaration of the following general form?" enum { padding = 16 }; auto char buffer[1+2+65536+padding]; or more simply: auto char buffer[1+2+65536+16]; >By the way, how do you benefit from allocating from stack than from >heap for relatively large chunk of memory? Doing so is unambiguously more efficient, and provides an absolute assurance that the memory will be fully and properly reclaimed at the instant the containing function exits, however just with respect to the security issues alone, I must admit that this might possibly not make any difference at all, specifically in relation to memory leakage of those secrets which belong *to the server itself*. However depending on how (or whether) memory which is allocated to hold execution stacks is pre-zeroed (before being given to a process or a thread) and depending on whether each SSL session gets its own separate thread (and thus also its own private execution stack) allocating buffers out of the stack, rather than out of the (common) process- wide heap might at least prevent leakage of secrets belonging to one client to a different client. (And by the way, it would not be a terribly difficult thing to add to any C compiler a new option that, when used during compilation of select "mission critical" secirity software, would have the effect of generating code for each function that completely zeros at least the statically-sized part of the new stack frame upon entry to the function, or conversely would generate code which would zero the current frame just before function exit. Such an option might be useful when compiling critical security code, such as the OpenSSL library, and if it had been available and used in conjunction with allocation of the problematic buffer on the stack, as I suggested, I believe that those steps, taken together would have eliminated even the possibility of leakage of server-specific secrets. Such an approach might seem extreme to some, but would it have been worse that the current mess the net finds itself in?) >> 3) Not that this would actually have eliminated the bug, but... >> >> After making some effort to do so, I have been unable to find any >> information online which would serve to document the initial state >> of the blocks of memory allocated by calls to pthread_create(). >> Would any of you happen to know if said blocks are or, conversely, >> are not cleared to zeros prior to execution of the created >> thread(s)? > >I don't think I understood this question... I'll try again. A call to pthread_create() causes space to be allocated for an execution stack for the newly created thread. I was asking if that space is cleared to zeros (by pthread_create) before it is handed over to the nely created thread. My hope and belief is that it is, but if it is, that fact does not seem to have made its way into the relevant man page. >> 4) If, as has been suggested in some quarters, the _actual_ size >> of the heartbeat client-supplied payload may be correctly >> determined without any reference whatsoever to the _content_ of the >> request packet, then why is/was it the case that the SSL protocol >> specified that such packets should contain a payload length >> specification... a value that, as we all now know, may be spoofed >> with disasterous consequences? > >Do you have any reference here? I'm not an expert in OpenSSL code but >looking at the specification, it seems like, without the payload >length there is no way to figure out what length the padding is. Please review the detailed description of the bug provided here: http://blog.existentialize.com/diagnosis-of-the-openssl-heartbleed-bug.html "Note that the actual length in the SSLv3 record is not checked." ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This is the crux of the matter. The actual received record is this big: ->.<- Unfortunately however there is a field within the record which says how big it is _alleged_ to be... by the client (aka the attacker). The value within that field can be a lie. It can claim that the received record is actually this big: ->...........................................................<- The problem is that the buggy OpenSSL code believed the lie, rather than believing the actual size of the record it received, which it also had ready access to. My question was about the specification of the SSL protocol. I do wonder why the authors/developers of that protocol engineered it in such a way that a client was even able to lie to the server about how much data he had actually sent. Why was that field even present in the protocol?? It was clearly not needed, since... as is now clear... servers have other (and accurate) ways of deducing the _actual_ lenth of the _actual_ payload. In effect it seems to me that the very design of the protocol may be to blame here. Is the SSL protocol, as currently defined, walking around with a giant "KICK ME" sign taped to its back? The protocol provided the ability... if not to say the outright INVITATION... to lie to the server. Then we all wake up one day and learn that, lo and behold, some nefarious people might actually be doing that. Now, everybody is running around expressing shock and surprise. I'm not sure that I understand that. Having read a couple of explanations of this bug now, the whole thing seems (a) inevitable and (b) fundamentally the fault of the protocol specification itself. And yet various online commentators seem all too ready and willing to lay the blame on the C language itself, almost as if *it* created the flawed protocol, or developed the bad code. >From where I am sitting, nothing could be further from the truth. >> P.S. Looking just now at the man page for execve(2) I could not >> help but notice that it neglects to say a single word about the >> state of the memory comprising the initial page of the (new) >> execution stack... >... >New pages are always initialized to zeros before access is given to >process, Right. My point is that it wouldn't kill anyone to explicitly say that, specifically in the man pages for exec*() and also for brk()/sbrk(). (The latter is relevant to the heap, while the former is relevant to the stack. In both cases, new space is zeroed before being given to the process. These facts are undoubtedly widely known but are nontheless currently UNDOCUMENTED within the relevant FreeBSD man pages. This should be corrected. Worse yet, looking now at my dog-eared copy of IEEE 1003.1-1990 I see that even the official POSIX.1 standard entirely dropped the ball by also and likewise failing to even mention that the stack space given to the new process after a call to exec*() may be relied upon to contain something other than flotsam and jetsum left over from previously demised processes. The fact that the POSIX comittee was guilty of this shameful omission is no excuse for the FreeBSD team to repeat their mistake.) >Hope this helps. Should you find anything that we need to correct in >the manual page to help others please speak up. I just did. Regards, rfg
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?42638.1397124000>