Thread: Question regarding SSL code in backend and frontend
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
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
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/
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
>> 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
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/
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
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/
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
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/