Обсуждение: Bogus-looking SSL code in postmaster wait loop

The postmaster contains this code just before it waits for input:

#ifdef USE_SSL       for (curr = DLGetHead(PortList); curr; curr = DLGetSucc(curr))       {           if (((Port *)
DLE_VAL(curr))->ssl&&               SSL_pending(((Port *) DLE_VAL(curr))->ssl) > 0)           {               no_select
=true;               break;           }       }       if (no_select)           FD_ZERO(&rmask);    /* So we don't
accept()anything below */
 
#endif

I am not sure exactly what SSL_pending() is defined to mean, but as
near as I can tell, whenever SSL_pending() returns true, the postmaster
will completely ignore every other input-ready condition.  This spells
"denial of service" from where I sit: a nonresponsive SSL client will
cause the postmaster to freeze up for all other clients.

Can anyone who knows about SSL defend or even explain the above code?
I am strongly inclined to just dike it out.
        regards, tom lane

Magnus Hagander <mha@sollentuna.net> writes:
> SSL_pending() returns true when there is data in the SSL buffer of the
> socket.
> The problem is that since SSL uses block cipher, even if you read one just
> byte from the socket (using ssl_read), OpenSSL will read a complete block
> from the network, in order to be able to decrypt it. In this case, the rest
> of that block will sit in the (SSL *)-structure. If you then select() the
> socket, it will *not* return that there is more data available, because that
> data has already been read from the network layer. Therefor, the select()
> call would block *even though there is more data available* on that socket.

OK.  In that case the existing code is actually broken, because what
will happen as soon as SSL_pending returns true is that the select will
*never* complete.

> struct timeval tv;
> tv.tv_sec = 0;
> tv.tv_usec = 0;
> if (select(nSockets, &rmask, &wmask, (fd_set *) NULL,
>     no_select?&tv:(struct timeval *)NULL) < 0)

> That way, select() would block *only* if there is nothing on the sockets
> *and* nothing in the SSL buffers.

This looks reasonable to me, and should avoid the DOS issue.  We don't
want to skip the select() entirely, else we'd be ignoring our other
clients.

I'll put this in (with comments ;-)) unless there are objections from
the floor ...
        regards, tom lane

> The postmaster contains this code just before it waits for input:
> 
> #ifdef USE_SSL
>         for (curr = DLGetHead(PortList); curr; curr = DLGetSucc(curr))
>         {
>             if (((Port *) DLE_VAL(curr))->ssl &&
>                 SSL_pending(((Port *) DLE_VAL(curr))->ssl) > 0)
>             {
>                 no_select = true;
>                 break;
>             }
>         }
>         if (no_select)
>             FD_ZERO(&rmask);    /* So we don't accept() 
> anything below */
> #endif
> 
> I am not sure exactly what SSL_pending() is defined to mean, but as
> near as I can tell, whenever SSL_pending() returns true, the 
> postmaster
> will completely ignore every other input-ready condition.  This spells
> "denial of service" from where I sit: a nonresponsive SSL client will
> cause the postmaster to freeze up for all other clients.
> 
> Can anyone who knows about SSL defend or even explain the above code?
> I am strongly inclined to just dike it out.

SSL_pending() returns true when there is data in the SSL buffer of the
socket.
The problem is that since SSL uses block cipher, even if you read one just
byte from the socket (using ssl_read), OpenSSL will read a complete block
from the network, in order to be able to decrypt it. In this case, the rest
of that block will sit in the (SSL *)-structure. If you then select() the
socket, it will *not* return that there is more data available, because that
data has already been read from the network layer. Therefor, the select()
call would block *even though there is more data available* on that socket.

That would be an explanation, I think. I agree it's not an ideal situation.
Perhaps it could be replaced with a check that made it do something like
this around the actual select statement?


...
struct timeval tv;
tv.tv_sec = 0;
tv.tv_usec = 0;
if (select(nSockets, &rmask, &wmask, (fd_set *) NULL,no_select?&tv:(struct timeval *)NULL) < 0)
...

That way, select() would block *only* if there is nothing on the sockets
*and* nothing in the SSL buffers. If there is data in *either* sockets *or*
SSL buffers, it will go through to the loop that reads the data.

With this, completely removing the if(no_select) FD_ZERO part of the code.


This was a quick suggestion, so it may be flawed :-) Comments?


//Magnus