You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@attglobal.net> on 2003/07/16 17:21:38 UTC
[PATCH] SSL: stop trying to talk to client after a negotiation failure?
I turned on SSLVerifyClient and my client wasn't set up properly and I
started getting server segfaults because we try to read on the
connection after some SSL-related data has been cleaned up.
#2 0x8091753 in ssl_io_filter_input (f=0x825d760, bb=0x825f440,
mode=AP_MODE_GETLINE, block=APR_BLOCK_READ,
readbytes=0) at ssl_engine_io.c:1231
1231 if ((status = ssl_io_filter_connect(inctx->filter_ctx)) !=
APR_SUCCESS) {
(gdb) p inctx->filter_ctx
$1 = (ssl_filter_ctx_t *) 0x0
I haven't tried very hard to see how inctx->filter_ctx can get cleared,
but first I wonder about the big picture: Is continued I/O reasonabe
after the negotiation failure? It *seems* like the clients I'm playing
with (wget, Mozilla) have given up at that point anyway.
If it isn't reasonable, then c->aborted should be set.
This is very closely related to PR 21370, where the reporter has a few
extra lines in his patch which I did not need.
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=21370
(If there is anything good about this patch, the reporter is
responsible. If anything bad, he is completely innocent.)
Index: modules/ssl/ssl_engine_kernel.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_engine_kernel.c,v
retrieving revision 1.82.2.6
diff -u -r1.82.2.6 ssl_engine_kernel.c
--- modules/ssl/ssl_engine_kernel.c 16 May 2003 18:12:18 -0000
1.82.2.6
+++ modules/ssl/ssl_engine_kernel.c 16 Jul 2003 15:10:48 -0000
@@ -696,6 +696,7 @@
ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
"Re-negotiation request failed");
+ r->connection->aborted = 1;
return HTTP_FORBIDDEN;
}
@@ -710,6 +711,7 @@
"Re-negotiation handshake failed: "
"Not accepted by client!?");
+ r->connection->aborted = 1;
return HTTP_FORBIDDEN;
}
}
RE: [PATCH] SSL: stop trying to talk to client after a negotiation failure?
Posted by Sander Striker <st...@apache.org>.
> From: Jeff Trawick [mailto:trawick@attglobal.net]
> Sent: Thursday, July 17, 2003 12:45 PM
[...]
> I will be off the net for 10 days starting this afternoon and have some
> crucial stuff to do in the meantime (find passport, pack, etc.). If I
> haven't committed this stuff yet, go for it. If I have, my apologies to
> everyone if something breaks :)
heh heh. Don't worry about it. Have a good trip ;).
Sander
Re: [PATCH] SSL: stop trying to talk to client after a negotiation
failure?
Posted by Jeff Trawick <tr...@attglobal.net>.
Joe Orton wrote:
> * ssl_engine_io.c (ssl_filter_write, ssl_io_filter_output): Don't
> dereference the BIOs in filter_ctx when filter_ctx->pssl is NULL.
>
> Index: ssl_engine_io.c
> ===================================================================
+1 for your patch... it is a cleaner patch than what the PR reporter
provided in http://nagoya.apache.org/bugzilla/show_bug.cgi?id=21370
With sufficient caffeine, I see that the patch I posted to set
c->aborted in a couple of places is different than the one the PR
reporter had posted (I said it was his because I thought they were the
same and I couldn't recall whether I came up with the idea before/after
I saw his patch :) )
So my patch posted earlier in this thread is a second patch to commit...
There is still part of the PR reporter's patch that is unaccounted for:
Diff:
diff -c -r1.2 -r1.3
*** ssl_engine_io.c 2003/04/16 14:14:39 1.2
--- ssl_engine_io.c 2003/07/03 11:36:24 1.3
***************
*** 1112,1117 ****
--- 1122,1129 ----
inctx->rc = APR_EGENERAL;
}
+ /* 2.7.2003/hk,mv: handshake failed, close the connection */
+ c->aborted=1;
return ssl_filter_io_shutdown(filter_ctx, c, 1);
}
***************
*** 1153,1158 ****
--- 1165,1172 ----
error ? error : "unknown");
ssl_log_ssl_error(APLOG_MARK, APLOG_INFO, c->base_server);
+ /* 2.7.2003/hk,mv: no client cert, close the
connection
*/
+ c->aborted=1;
return ssl_filter_io_shutdown(filter_ctx, c, 1);
}
}
I didn't need this change in my testing, but it looks to me that it is
proper for c->aborted to be set on this path, and that
ssl_filter_io_shutdown() should do the setting instead of putting it
here since any time ssl_filter_io_shutdown() is called it is appropriate
for c->aborted to be set.
I will be off the net for 10 days starting this afternoon and have some
crucial stuff to do in the meantime (find passport, pack, etc.). If I
haven't committed this stuff yet, go for it. If I have, my apologies to
everyone if something breaks :)
Re: [PATCH] SSL: stop trying to talk to client after a negotiation failure?
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jul 16, 2003 at 11:21:38AM -0400, Jeff Trawick wrote:
> I turned on SSLVerifyClient and my client wasn't set up properly and I
> started getting server segfaults because we try to read on the
> connection after some SSL-related data has been cleaned up.
>
> #2 0x8091753 in ssl_io_filter_input (f=0x825d760, bb=0x825f440,
> mode=AP_MODE_GETLINE, block=APR_BLOCK_READ,
> readbytes=0) at ssl_engine_io.c:1231
>
> 1231 if ((status = ssl_io_filter_connect(inctx->filter_ctx)) !=
> APR_SUCCESS) {
>
> (gdb) p inctx->filter_ctx
> $1 = (ssl_filter_ctx_t *) 0x0
>
> I haven't tried very hard to see how inctx->filter_ctx can get cleared,
> but first I wonder about the big picture: Is continued I/O reasonabe
> after the negotiation failure? It *seems* like the clients I'm playing
> with (wget, Mozilla) have given up at that point anyway.
>
> If it isn't reasonable, then c->aborted should be set.
I agree it's not reasonable; the SSL alert has been sent so nothing more
should be done. That patch was necessary but not sufficient to cure the
segfaults for me in this case - I also needed the below change. Do you
want to check these in together?
* ssl_engine_io.c (ssl_filter_write, ssl_io_filter_output): Don't
dereference the BIOs in filter_ctx when filter_ctx->pssl is NULL.
Index: ssl_engine_io.c
===================================================================
RCS file: /store/cvs/root/httpd-2.0/modules/ssl/ssl_engine_io.c,v
retrieving revision 1.109
diff -u -p -r1.109 ssl_engine_io.c
--- ssl_engine_io.c 22 May 2003 19:41:32 -0000 1.109
+++ ssl_engine_io.c 17 Jul 2003 09:54:03 -0000
@@ -780,8 +780,7 @@ static apr_status_t ssl_filter_write(ap_
apr_size_t len)
{
ssl_filter_ctx_t *filter_ctx = f->ctx;
- bio_filter_out_ctx_t *outctx =
- (bio_filter_out_ctx_t *)(filter_ctx->pbioWrite->ptr);
+ bio_filter_out_ctx_t *outctx;
int res;
/* write SSL */
@@ -789,6 +788,7 @@ static apr_status_t ssl_filter_write(ap_
return APR_EGENERAL;
}
+ outctx = (bio_filter_out_ctx_t *)filter_ctx->pbioWrite->ptr;
res = SSL_write(filter_ctx->pssl, (unsigned char *)data, len);
if (res < 0) {
@@ -1362,8 +1362,7 @@ static apr_status_t ssl_io_filter_output
{
apr_status_t status = APR_SUCCESS;
ssl_filter_ctx_t *filter_ctx = f->ctx;
- bio_filter_in_ctx_t *inctx = (bio_filter_in_ctx_t *)
- (filter_ctx->pbioRead->ptr);
+ bio_filter_in_ctx_t *inctx;
if (f->c->aborted) {
apr_brigade_cleanup(bb);
@@ -1375,6 +1374,7 @@ static apr_status_t ssl_io_filter_output
return ap_pass_brigade(f->next, bb);
}
+ inctx = (bio_filter_in_ctx_t *)filter_ctx->pbioRead->ptr;
/* When we are the writer, we must initialize the inctx
* mode so that we block for any required ssl input, because
* output filtering is always nonblocking.