Skip Menu |
 
Ticket metadata
The Basics
Id: 3321
Status: resolved
Priority: 0/
Queue: OpenSSL-Bugs

Custom Fields
Milestone: (no value)
Subsystem: TLS/SSL
Severity: Critical
Broken in: (no value)

People
Owner: Kurt Roeckx (no email address)
Requestors: David Ramos
Cc:
AdminCc:

More about the requestors

David Ramos

Comments about this user: No comment entered about this user
Groups this user belongs to
  • Everyone
  • Unprivileged

New reminder:
Subject:
Owner:
Due:

Dates
Created: Mon Apr 21 17:40:08 2014
Starts: Not set
Started: Sat Apr 26 14:54:07 2014
Last Contact: Mon May 12 02:01:11 2014
Due: Not set
Closed: Mon May 12 02:01:11 2014
Updated: Mon May 12 02:01:12 2014 by Matt Caswell



Subject: NULL pointer dereference with SSL_MODE_RELEASE_BUFFERS flag
Date: Fri, 18 Apr 2014 13:02:17 -0400
To: rt@openssl.org
From: David Ramos <daramos@stanford.edu>
Download (untitled) / with headers
text/plain 1.5k
Hello,

Our UC-KLEE tool found a NULL pointer dereference bug in do_ssl3_write (ssl/s3_pkt.c) when an alert is pending and the SSL_MODE_RELEASE_BUFFERS flag is used. This bug affects the latest 1.0.1 branch.

The code for do_ssl3_write() first checks whether the write buffer is NULL:
644 if (wb->buf == NULL)
645 if (!ssl3_setup_write_buffer(s))
646 return -1;

It then dispatches any pending alerts:
653 /* If we have an alert to send, lets send it */
654 if (s->s3->alert_dispatch)
655 {
656 i=s->method->ssl_dispatch_alert(s);

This call to ssl3_dispatch_alert() calls do_ssl3_write() again:
1501 i = do_ssl3_write(s, SSL3_RT_ALERT, &s->s3->send_alert[0], 2, 0);

Which calls ssl3_write_pending():
852 /* we now just need to write the buffer */
853 return ssl3_write_pending(s,type,buf,len);

Which releases the write buffer if SSL_MODE_RELEASE_BUFFERS is used:
894 if (s->mode & SSL_MODE_RELEASE_BUFFERS &&
895 SSL_version(s) != DTLS1_VERSION && SSL_version(s) != DTLS1_BAD_VER)
896 ssl3_release_write_buffer(s);

When control returns back to the original do_ssl3_write() call, wb->buf has been set to NULL (*after* the NULL check). The NULL pointer dereference then occurs at:
743 *(p++)=type&0xff;

A second check is necessary after the call to ssl->dispatch_alert(), or a counter could be added to ssl_st to avoid releasing the buffers if any callers are performing writes.

-David
Subject: Re: [openssl.org #3321] NULL pointer dereference with SSL_MODE_RELEASE_BUFFERS flag
Date: Sat, 26 Apr 2014 14:53:57 +0200
To: rt@openssl.org
From: Kurt Roeckx <kurt@roeckx.be>
Download (untitled) / with headers
text/plain 2.1k
There is a potentional patch for this in libresll, you can see it
at:
http://anoncvs.estpak.ee/cgi-bin/cgit/openbsd-src/commit/lib/libssl?id=e76e308f1fab2253ab5b4ef52a1865c5ffecdf21


Kurt


On Mon, Apr 21, 2014 at 05:40:09PM +0200, David Ramos via RT wrote:
Show quoted text
> Hello,
>
> Our UC-KLEE tool found a NULL pointer dereference bug in do_ssl3_write (ssl/s3_pkt.c) when an alert is pending and the SSL_MODE_RELEASE_BUFFERS flag is used. This bug affects the latest 1.0.1 branch.
>
> The code for do_ssl3_write() first checks whether the write buffer is NULL:
> 644 if (wb->buf == NULL)
> 645 if (!ssl3_setup_write_buffer(s))
> 646 return -1;
>
> It then dispatches any pending alerts:
> 653 /* If we have an alert to send, lets send it */
> 654 if (s->s3->alert_dispatch)
> 655 {
> 656 i=s->method->ssl_dispatch_alert(s);
>
> This call to ssl3_dispatch_alert() calls do_ssl3_write() again:
> 1501 i = do_ssl3_write(s, SSL3_RT_ALERT, &s->s3->send_alert[0], 2, 0);
>
> Which calls ssl3_write_pending():
> 852 /* we now just need to write the buffer */
> 853 return ssl3_write_pending(s,type,buf,len);
>
> Which releases the write buffer if SSL_MODE_RELEASE_BUFFERS is used:
> 894 if (s->mode & SSL_MODE_RELEASE_BUFFERS &&
> 895 SSL_version(s) != DTLS1_VERSION && SSL_version(s) != DTLS1_BAD_VER)
> 896 ssl3_release_write_buffer(s);
>
> When control returns back to the original do_ssl3_write() call, wb->buf has been set to NULL (*after* the NULL check). The NULL pointer dereference then occurs at:
> 743 *(p++)=type&0xff;
>
> A second check is necessary after the call to ssl->dispatch_alert(), or a counter could be added to ssl_st to avoid releasing the buffers if any callers are performing writes.
>
> -David
>
> ______________________________________________________________________
> OpenSSL Project http://www.openssl.org
> Development Mailing List openssl-dev@openssl.org
> Automated List Manager majordomo@openssl.org
>
Subject: Re: [openssl.org #3321] NULL pointer dereference with SSL_MODE_RELEASE_BUFFERS flag
Date: Fri, 2 May 2014 22:30:52 +0200
To: rt@openssl.org
From: Kurt Roeckx <kurt@roeckx.be>
Download (untitled) / with headers
text/plain 478b
On Fri, May 02, 2014 at 06:53:06PM +0000, mancha wrote:
Show quoted text
> Kurt Roeckx via RT <rt <at> openssl.org> writes:
> >
> > There is a potentional patch for this in libresll, you can see it
> > at:
> > http://anoncvs.estpak.ee/cgi-bin/cgit/openbsd-src/commit
> > /lib/libssl?id=e76e308f1fab2253ab5b4ef52a1865c5ffecdf21
> >
> > Kurt
>
> Hello.
>
> This issue has been assigned CVE-2014-0198. Any news on an
> OpenSSL fix?

I've just created github pull request #94 for that.


Kurt
This patch looks like a bit of a kludge to me. Release a buffer only to then immediately set it up again. Compare with this commit on master:
https://github.com/openssl/openssl/commit/3ef477c69f2fd39549123d7b0b869029b46cf989

I think a backport of this might be more appropriate.

Matt

CC: daramos@stanford.edu
Subject: Re: [openssl.org #3321] NULL pointer dereference with SSL_MODE_RELEASE_BUFFERS flag
Date: Sat, 3 May 2014 11:43:31 +0200
To: rt@openssl.org
From: Kurt Roeckx <kurt@roeckx.be>
Download (untitled) / with headers
text/plain 438b
On Sat, May 03, 2014 at 01:14:47AM +0200, Matt Caswell via RT wrote:
Show quoted text
> This patch looks like a bit of a kludge to me. Release a buffer only to then
> immediately set it up again. Compare with this commit on master:
> https://github.com/openssl/openssl/commit/3ef477c69f2fd39549123d7b0b869029b46cf989
>
> I think a backport of this might be more appropriate.

Yes. As far as I can see the master branch didn't have this
problem.


Kurt
Many thanks David for your report.

This is fixed in commit b107586c0c3447ea22dba8698ebbcd81bb29d48c on 1.0.1 branch. Similar commit for 1.0.0 (0.9.8 does not support SSL_MODE_RELEASE_BUFFERS)
I've also made the same change to 1.0.2 and master. This wasn't strictly necessary because these versions do not have this problem, but I applied it for consistency reasons.

I came full circle on my thinking on this. I prefer the solution applied in master/1.0.2. However I felt it was too much of a change in the stable branch to backport these changes. The code in this area is quite complex with various recursions etc going on - it would be too easy to misstep.

Matt