httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <>
Subject Re: SSL Input Filter bogosity
Date Thu, 31 Oct 2002 06:19:50 GMT
At 10:42 PM 10/30/2002, Justin Erenkrantz wrote:
>--On Wednesday, October 30, 2002 5:56 PM -0600 "William A. Rowe, Jr." <>
>>@@ -358,6 +357,12 @@
>>     SSLConnRec *sslconn = myConnConfig(inbio->f->c);
>>     int len = 0;
>>+    inbio->rc = APR_SUCCESS;
>>+    /* OpenSSL catches this case, so should we. */
>>+    if (!in)
>>+        return 0;
>>     /* XXX: flush here only required for SSLv2;
>>      * OpenSSL calls BIO_flush() at the appropriate times for
>>      * the other protocols.
>Why are we attempting to catch NULL pointer exceptions?  I don't believe that there are
any cases where our code could have a NULL ctx->buffer as those are well-defined char arrays.
 Within mod_ssl, there shouldn't be any other entry points to the OpenSSL read calls but by
entering this path with ctx->buffer, so I don't see why we should bother.  (Yes, photons
could hit, so what?)

OpenSSL 0.9.6g does so.  Why shouldn't we?

>>@@ -366,11 +371,11 @@
>>         BIO_bucket_flush(inbio->wbio);
>>     }
>>-    inbio->rc = APR_SUCCESS;
>>+    BIO_clear_retry_flags(bio);
>>     /* first use data already read from socket if any */
>>     if ((len = char_buffer_read(&inbio->cbuf, in, inl))) {
>>-        if ((len <= inl) || inbio->mode == AP_MODE_GETLINE) {
>>+        if (len >= inl) {
>>             return len;
>>         }
>>         inl -= len;
>Hmm, what is the point of this conditional change?  char_buffer_read shouldn't be returning
anything larger than inl.  To me, it actually sounds that this segment should just be returning
len all the time (as that conditional was *always* true).  No need for a conditional here.
 If there's anything that we've already read, just return it. Clear out that buffer - if the
user still wants more, then they can call again.  (Due to our structure, we're not sure if
the caller really wants all of inl or not - almost certainly not for AP_MODE_GETLINE.)

However, if we don't have inl worth of bytes, and they are sitting
ready (on the socket) shouldn't we fetch them?  Forget the GETLINE
bogosity, it means nothing to SSL.

>>@@ -396,21 +399,58 @@
>>                                        AP_MODE_READBYTES, 
>>                                        inl);
>>-            if ((inbio->rc != APR_SUCCESS) 
>>-            {
>>+            if (APR_STATUS_IS_EAGAIN(inbio->rc)
>>+                    || APR_STATUS_IS_EINTR(inbio->rc)) {
>>+                break;
>>+            }
>>+            if (inbio->rc != APR_SUCCESS) {
>>+                /* Unexpected errors discard the brigade */
>>+                apr_brigade_cleanup(inbio->bb);
>>+                inbio->bb = NULL;
>>+                return -1;
>>+            }
>>+            if (APR_BRIGADE_EMPTY(inbio->bb)) {
>>+                /* Treat an empty brigade as a retry case */
>>                 break;
>>             }
>>         }
>We should do a conditional on the APR_BRIGADE_EMPTY() check if inbio->block is non-blocking.
 It's considered a design error if a filter returns an empty brigade on a blocking call.

Who said we are blocking?  This could be a SPECULATIVE call
with a NONBLOCKING request, no?

>>-        inbio->bucket = APR_BRIGADE_FIRST(inbio->bb);
>>+        bucket = APR_BRIGADE_FIRST(inbio->bb);
>>+        if (APR_BUCKET_IS_EOS(bucket)) {
>>+            if (len) {
>>+                /* Leave this EOS on the brigade */
>>+                break;
>>+            }
>>+            /* Consume the EOS and return 0, we already
>>+             * reset the retry flag above
>>+             */
>>+            apr_brigade_cleanup(inbio->bb);
>>+            inbio->bb = NULL;
>>+            return 0;
>>+        }
>Should the APR_BUCKET_IS_EOS rather be APR_BUCKET_IS_METADATA?  Not sure here.  Perhaps.

Hmmm.  I was thinking about the METADATA case.

Do you suppose we should percolate METADATA buckets back out
to the filter_read of SSL?  I suppose metadata should just go through

However, we have to react to EOS, since that's the end of the input
available to the SSL pump.

>>-        if (inbio->mode == AP_MODE_GETLINE) {
>>-            /* only read from the socket once in getline mode.
>>-             * since callers buffer size is likely much larger than
>>-             * the request headers.  caller can always come back 
>for more
>>-             * if first read didn't get all the headers.
>>-             */
>>-            break;
>>-        }
>Again, what is the rationale for removing this?  We don't have a predetermined size for
a GETLINE call - well, not 100% true, it sort of ends up being sizeof(ctx->buffer).  Yet,
I think that should at most translate to only one socket read.  Not sure why we would want
to do a full 8k read all the time.  The hope is that on a GETLINE mode, the odds are that
the lines are really short - one socket call should be sufficient to get mulitple lines. 
(Furthermore, we can probably 
>read from the buffer since multiple headers will be read at once!)

Getline means nothing in the context of fetching bytes off of
an SSL socket.  We must shove the raw bytes into the SSL
pump in order to return any sort of data (SPECULATIVE, READBYTES,
GETLINE, etc) from the SSL pump.  The raw bytes just need
to be fetched.

Of course, it's generally nonblocking, so if we don't get a hit
from the socket, the new code just returns whatever we got.

>Also, I'm not totally sold on the collasping of ssl_io_hook_read.  I think there's an
advantage to creating a fine line between the OpenSSL calls and our more abstract calls. 
But, whatever, no big deal.  I think shorter, concise functions are better...  -- justin

No, there really isn't.  It was impossible to look at inbio.rv when
we needed too, since ctx wasn't passed, but the SSL ctx.

We really need to watch the EAGAIN cases.  Right now, that is
what was most borked.  Problems in SSL only happen with some
reasonably busy traffic, interrupt and the EAGAIN situations.  If you
don't flood SSL, you won't see any problems today.


View raw message