Thread: Question regarding SSL code in backend and frontend

Question regarding SSL code in backend and frontend

From
Tatsuo Ishii
Date:
Hi,

While looking into SSL code in secure_read() of be-secure.c and
pqsecure_read() of fe-secure.c, I noticed subtle difference between
them. 

In secure_read:
----------------------------------------------------------        case SSL_ERROR_WANT_READ:        case
SSL_ERROR_WANT_WRITE:           if (port->noblock)            {                errno = EWOULDBLOCK;                n =
-1;               break;            }
 
#ifdef WIN32            pgwin32_waitforsinglesocket(SSL_get_fd(port->ssl),                                        (err
==SSL_ERROR_WANT_READ) ?                                FD_READ | FD_CLOSE : FD_WRITE | FD_CLOSE,
                INFINITE);
 
#endif            goto rloop;
----------------------------------------------------------

while in pqsecure_read:
----------------------------------------------------------        case SSL_ERROR_WANT_READ:            n = 0;
break;        case SSL_ERROR_WANT_WRITE:
 
            /*             * Returning 0 here would cause caller to wait for read-ready,             * which is not
correctsince what SSL wants is wait for             * write-ready.  The former could get us stuck in an infinite
    * wait, so don't risk it; busy-loop instead.             */            goto rloop;
 
----------------------------------------------------------

Those code fragment judges the return value from
SSL_read(). secure_read() does retrying when SSL_ERROR_WANT_READ *and*
SSL_ERROR_WANT_WRITE returned. However, pqsecure_read() does not retry
when SSL_ERROR_WANT_READ. It seems they are not consistent. Comments?
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp


Re: Question regarding SSL code in backend and frontend

From
Tom Lane
Date:
Tatsuo Ishii <ishii@postgresql.org> writes:
> Those code fragment judges the return value from
> SSL_read(). secure_read() does retrying when SSL_ERROR_WANT_READ *and*
> SSL_ERROR_WANT_WRITE returned. However, pqsecure_read() does not retry
> when SSL_ERROR_WANT_READ. It seems they are not consistent. Comments?

There's no particular reason why they should be consistent, I think.
The assumptions for nonblocking operation are different.

I rather wonder whether the #ifdef WIN32 bit in the backend isn't dead
code though.  If the port isn't in nonblock mode, we shouldn't really
get here at all, should we?
        regards, tom lane


Re: Question regarding SSL code in backend and frontend

From
Magnus Hagander
Date:
On Wed, Apr 4, 2012 at 17:57, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Tatsuo Ishii <ishii@postgresql.org> writes:
>> Those code fragment judges the return value from
>> SSL_read(). secure_read() does retrying when SSL_ERROR_WANT_READ *and*
>> SSL_ERROR_WANT_WRITE returned. However, pqsecure_read() does not retry
>> when SSL_ERROR_WANT_READ. It seems they are not consistent. Comments?
>
> There's no particular reason why they should be consistent, I think.
> The assumptions for nonblocking operation are different.
>
> I rather wonder whether the #ifdef WIN32 bit in the backend isn't dead
> code though.  If the port isn't in nonblock mode, we shouldn't really
> get here at all, should we?

Not in a position to look at the code right now, but first guess - we
*always* have the underlying socket in nonblocking mode on win32, so
we can deliver signals properly. It becomes blocking only through our
APIs, but the SSL stuff bypasses that in some places - such as this
one calling win32_waitforsinglesocket...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/


Re: Question regarding SSL code in backend and frontend

From
Tom Lane
Date:
Magnus Hagander <magnus@hagander.net> writes:
> On Wed, Apr 4, 2012 at 17:57, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I rather wonder whether the #ifdef WIN32 bit in the backend isn't dead
>> code though.  If the port isn't in nonblock mode, we shouldn't really
>> get here at all, should we?

> Not in a position to look at the code right now, but first guess - we
> *always* have the underlying socket in nonblocking mode on win32, so
> we can deliver signals properly.

Ah, I think you're right.  So actually, the retry looping is expected
to be never-invoked in the Unix case.  If it did happen, it'd be a busy
wait loop, which would probably be a bad thing ... but it shouldn't
happen, and not clear it's worth adding any code to consider the
possibility more carefully.
        regards, tom lane


Re: Question regarding SSL code in backend and frontend

From
Tatsuo Ishii
Date:
>> Those code fragment judges the return value from
>> SSL_read(). secure_read() does retrying when SSL_ERROR_WANT_READ *and*
>> SSL_ERROR_WANT_WRITE returned. However, pqsecure_read() does not retry
>> when SSL_ERROR_WANT_READ. It seems they are not consistent. Comments?
> 
> There's no particular reason why they should be consistent, I think.
> The assumptions for nonblocking operation are different.

Ok. Thanks.

BTW, usage of SSL_CTX_new() is different among frontend and backend as
well.

fe-secure.c:        SSL_context = SSL_CTX_new(TLSv1_method());
be-secure.c:        SSL_context = SSL_CTX_new(SSLv23_method());

In my understanding by using SSLV23_method, it is compatible with
SSLv2, SSLv3, and TLSv1 protocol. So it seems there's no particular
reason to use TLSv1_method(). Am I missing something?
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp


Re: Question regarding SSL code in backend and frontend

From
Magnus Hagander
Date:
On Thu, Apr 5, 2012 at 08:00, Tatsuo Ishii <ishii@postgresql.org> wrote:
>>> Those code fragment judges the return value from
>>> SSL_read(). secure_read() does retrying when SSL_ERROR_WANT_READ *and*
>>> SSL_ERROR_WANT_WRITE returned. However, pqsecure_read() does not retry
>>> when SSL_ERROR_WANT_READ. It seems they are not consistent. Comments?
>>
>> There's no particular reason why they should be consistent, I think.
>> The assumptions for nonblocking operation are different.
>
> Ok. Thanks.
>
> BTW, usage of SSL_CTX_new() is different among frontend and backend as
> well.
>
> fe-secure.c:            SSL_context = SSL_CTX_new(TLSv1_method());
> be-secure.c:            SSL_context = SSL_CTX_new(SSLv23_method());
>
> In my understanding by using SSLV23_method, it is compatible with
> SSLv2, SSLv3, and TLSv1 protocol. So it seems there's no particular
> reason to use TLSv1_method(). Am I missing something?

The reason is that TLS is more secure, I believe. It was changed in
commit 750a0e676e1f8f71bf1c6aba05d3264a7c57218b for backwards
compatibility.

If anything, we should be changing it to TLSv1 in both client and
server, since every client out there now should be using that anyway,
given that the client has been specifying it for a long time.

Unless I am also missing something? :-)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/


Re: Question regarding SSL code in backend and frontend

From
Tom Lane
Date:
Magnus Hagander <magnus@hagander.net> writes:
> If anything, we should be changing it to TLSv1 in both client and
> server, since every client out there now should be using that anyway,
> given that the client has been specifying it for a long time.

Huh?  libpq isn't every client.
        regards, tom lane


Re: Question regarding SSL code in backend and frontend

From
Magnus Hagander
Date:
On Fri, Apr 6, 2012 at 02:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> If anything, we should be changing it to TLSv1 in both client and
>> server, since every client out there now should be using that anyway,
>> given that the client has been specifying it for a long time.
>
> Huh?  libpq isn't every client.

True. I guess I was just assuming that JDBC (and npgsql i think?) were
using TLS - I would assume that to be the default in both Java and
.NET. We'd have to check that before making a change of course - and
I'm not convinced we need to make the change. But if we're making a
change to align those two with each other, that's the direction the
change should be in.


--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/


Re: Question regarding SSL code in backend and frontend

From
Tom Lane
Date:
Magnus Hagander <magnus@hagander.net> writes:
> True. I guess I was just assuming that JDBC (and npgsql i think?) were
> using TLS - I would assume that to be the default in both Java and
> .NET. We'd have to check that before making a change of course - and
> I'm not convinced we need to make the change. But if we're making a
> change to align those two with each other, that's the direction the
> change should be in.

Agreed, but should we align them?  IIUC, changing the server would cause
it to reject connections from old non-TLS-aware clients.  Seems like
that isn't a particularly good idea.
        regards, tom lane


Re: Question regarding SSL code in backend and frontend

From
Magnus Hagander
Date:
On Fri, Apr 6, 2012 at 18:43, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> True. I guess I was just assuming that JDBC (and npgsql i think?) were
>> using TLS - I would assume that to be the default in both Java and
>> .NET. We'd have to check that before making a change of course - and
>> I'm not convinced we need to make the change. But if we're making a
>> change to align those two with each other, that's the direction the
>> change should be in.
>
> Agreed, but should we align them?  IIUC, changing the server would cause
> it to reject connections from old non-TLS-aware clients.  Seems like
> that isn't a particularly good idea.

Well, it would be a good idea for those that want to be sure they're
using TLS for security reasons (tlsv1 is more secure than sslv3 - see
e.g. http://en.wikipedia.org/wiki/Transport_Layer_Security#Security).
We could also add a server parameter saying ssl_tls_only or something
like that which would switch it...


--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/