Обсуждение: PostgreSQL12 and older versions of OpenSSL
Dear hackers,
PostgreSQL 12 documentation states, that minimum required version of
OpenSSL is 0.9.8. However, I was unable to сompile current
PGPRO_12_STABLE with OpenSSL 0.9.8j (from SLES 11sp4).
-fno-strict-aliasing -fwrapv -g -O2 -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o
be-secure-openssl.obe-secure-openssl.c
be-secure-openssl.c: In function ‘SSL_CTX_set_min_proto_version’:
be-secure-openssl.c:1340: error: ‘SSL_OP_NO_TLSv1_1’ undeclared (first use in this function)
be-secure-openssl.c:1340: error: (Each undeclared identifier is reported only once
be-secure-openssl.c:1340: error: for each function it appears in.)
be-secure-openssl.c:1344: error: ‘SSL_OP_NO_TLSv1_2’ undeclared (first use in this function)
be-secure-openssl.c: In function ‘SSL_CTX_set_max_proto_version’:
be-secure-openssl.c:1361: error: ‘SSL_OP_NO_TLSv1_1’ undeclared (first use in this function)
be-secure-openssl.c:1365: error: ‘SSL_OP_NO_TLSv1_2’ undeclared (first use in this function)
make: *** [be-secure-openssl.o] Error 1
Problem is that some code in src/backend/libpq/be-secure-openssl.c
assumes that if preprocessor symbols TLS1_1_VERSION and TLS1_2_VERSION
are defined in the openssl headers, corresponding versions of TLS are
supported by the library.
It is not so. Here is exempt from tls1.h header file from the openssl
0.9.8j
#define TLS1_VERSION 0x0301
#define TLS1_1_VERSION 0x0302
#define TLS1_2_VERSION 0x0303
/* TLS 1.1 and 1.2 are not supported by this version of OpenSSL, so
* TLS_MAX_VERSION indicates TLS 1.0 regardless of the above
* definitions. (s23_clnt.c and s23_srvr.c have an OPENSSL_assert()
* check that would catch the error if TLS_MAX_VERSION was too low.)
*/
#define TLS_MAX_VERSION TLS1_VERSION
Replacing all
#ifdef TLS1_1_VERSION
with
#if defined(TLS1_1_VERSION) && TLS1_1_VERSION <= TLS_MAX_VERSION
and analogue for TLS1_2_VERSION fixes the problem.
Really, problem is that symbol SSL_OP_NO_TLSv1_1 (and 1_2 accordingly)
might be undefined even if TLS1_1_VERSION defined.
Replacing
#ifdef TLS1_1_VERSION
with
#ifdef SSL_OP_NO_TLSv1_1
seems to be correct solution for two of three #ifdef TLS1_1_VERSION
statements in be-secure-openssl.c, because this symbol is used inside
#ifdef block.
But there is third (first from start of file) one.
...
case PG_TLS1_1_VERSION:
#ifdef TLS1_1_VERSION
return TLS1_1_VERSION;
#else
break;
#endif
...
(line 1290). In this case check for TLS1_1_VERSION <= TLS_MAX_VERSION
seems to be more self-explanatory, than check for somewhat unrelated
symbol SSL_OP_NO_TLSv1_1
--
On Tue, Sep 24, 2019 at 10:18:59AM +0300, Victor Wagner wrote: > PostgreSQL 12 documentation states, that minimum required version of > OpenSSL is 0.9.8. However, I was unable to сompile current > PGPRO_12_STABLE with OpenSSL 0.9.8j (from SLES 11sp4). I can reproduce that with REL_12_STABLE and the top of OpenSSL_0_9_8-stable fromx OpenSSL's git. > It is not so. Here is exempt from tls1.h header file from the openssl > 0.9.8j > > #define TLS1_VERSION 0x0301 > #define TLS1_1_VERSION 0x0302 > #define TLS1_2_VERSION 0x0303 > /* TLS 1.1 and 1.2 are not supported by this version of OpenSSL, so > * TLS_MAX_VERSION indicates TLS 1.0 regardless of the above > * definitions. (s23_clnt.c and s23_srvr.c have an OPENSSL_assert() > * check that would catch the error if TLS_MAX_VERSION was too low.) > */ > #define TLS_MAX_VERSION TLS1_VERSION Indeed, we rely currently on a false assumption that the version is supported if the object is defined. That's clearly wrong. > Replacing all > > #ifdef TLS1_1_VERSION > > with > > #if defined(TLS1_1_VERSION) && TLS1_1_VERSION <= TLS_MAX_VERSION > > and analogue for TLS1_2_VERSION fixes the problem. That sounds like a plan. > Really, problem is that symbol SSL_OP_NO_TLSv1_1 (and 1_2 accordingly) > might be undefined even if TLS1_1_VERSION defined. > > Replacing > > #ifdef TLS1_1_VERSION > > with > > #ifdef SSL_OP_NO_TLSv1_1 Hmm. Wouldn't it be better to check if the maximum version of TLS is supported and if SSL_OP_NO_TLSv1_1 is defined (same for 1.2)? > But there is third (first from start of file) one. > ... > case PG_TLS1_1_VERSION: > #ifdef TLS1_1_VERSION > return TLS1_1_VERSION; > #else > break; > #endif > ... > (line 1290). In this case check for TLS1_1_VERSION <= TLS_MAX_VERSION > seems to be more self-explanatory, than check for somewhat unrelated > symbol SSL_OP_NO_TLSv1_1 That sounds right. Victor, would you like to write a patch? -- Michael
Вложения
On Tue, 24 Sep 2019 18:49:17 +0900 Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Sep 24, 2019 at 10:18:59AM +0300, Victor Wagner wrote: > > PostgreSQL 12 documentation states, that minimum required version of > > OpenSSL is 0.9.8. However, I was unable to сompile current > > PGPRO_12_STABLE with OpenSSL 0.9.8j (from SLES 11sp4). > > I can reproduce that with REL_12_STABLE and the top of > OpenSSL_0_9_8-stable fromx OpenSSL's git. > > > Replacing all > > > > #ifdef TLS1_1_VERSION > > > > with > > > > #if defined(TLS1_1_VERSION) && TLS1_1_VERSION <= TLS_MAX_VERSION > > > > and analogue for TLS1_2_VERSION fixes the problem. > > That sounds like a plan. [skip] > > ... > > (line 1290). In this case check for TLS1_1_VERSION <= > > TLS_MAX_VERSION seems to be more self-explanatory, than check for > > somewhat unrelated symbol SSL_OP_NO_TLSv1_1 > > That sounds right. Victor, would you like to write a patch? I'm attaching patch which uses solution mentioned above. It seems that chedk for SSL_OP_NO_TLSvX_Y is redundant if we are checking for TLS_MAX_VERSION. --
Вложения
On 2019-Sep-24, Victor Wagner wrote: > Dear hackers, > > PostgreSQL 12 documentation states, that minimum required version of > OpenSSL is 0.9.8. However, I was unable to сompile current > PGPRO_12_STABLE with OpenSSL 0.9.8j (from SLES 11sp4). (Nice branch name.) I wonder if we should really continue to support OpenSSL 0.9.8. That branch was abandoned by the OpenSSL dev group in 2015 ... and I wouldn't want to assume that there are no security problems fixed in the meantime. Why shouldn't we drop support for that going forward, raising our minimum required OpenSSL version to be at least something in the 1.0 branch? (I'm not entirely sure about minor version numbers in OpenSSL -- it seems 1.0.2 is still being maintained, but 1.0.0 itself was also abandoned in 2016, as was 1.0.1. As far as I understand they use the alphabetical sequence *after* the three-part version number in the way we use minor number; so 1.0.1u (2016) is the last there, and 1.0.2t is a recent one in the maintained branch. Along the same lines, 0.9.8j was released in Jan 2009. The last in 0.9.8 was 0.9.8zi in December 2015.) Anyway I suppose it's not impossible that third parties are still maintaining their 1.0.0 branch, but I doubt anyone cares for 0.9.8 with Postgres 12 ... particularly since SUSE themselves suggest not to use the packaged OpenSSL for their stuff but rather stick to NSS. That said, in 2014 (!!) SUSE released OpenSSL 1.0.1 separately, for use with SLES 11: https://www.suse.com/c/introducing-the-suse-linux-enterprise-11-security-module/ Who would use the already obsolete SLES 11 (general support ended in March 2019, though extended support ends in 2022) with Postgres 12? That seems insane. All that being said, I don't oppose to this patch, since it seems a quick way to get out of the immediate trouble. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> ... I wonder if we should really continue to support
> OpenSSL 0.9.8.
Fair question, but post-rc1 is no time to be moving that goalpost
for the v12 branch.
> Anyway I suppose it's not impossible that third parties are still
> maintaining their 1.0.0 branch,
Another data point on that is that Red Hat is still supporting
1.0.1e in RHEL6. I don't think we should assume that just because
OpenSSL upstream has dropped support for a branch, it no longer
exists in the wild.
Having said that, if it makes our lives noticeably easier to
drop support for 0.9.8 in HEAD, I won't stand in the way.
(We should survey the buildfarm and see what the older critters
are running, perhaps.)
regards, tom lane
Victor Wagner <vitus@wagner.pp.ru> writes:
> I'm attaching patch which uses solution mentioned above.
> It seems that chedk for SSL_OP_NO_TLSvX_Y is redundant if
> we are checking for TLS_MAX_VERSION.
One thing I'm wondering is if it's safe to assume that TLS_MAX_VERSION
will be defined whenever these other symbols are. Looking in an
0.9.8x install tree, that doesn't seem to define any of them; while
in 1.0.1e I see
./tls1.h:#define TLS1_1_VERSION 0x0302
./tls1.h:#define TLS1_2_VERSION 0x0303
./tls1.h:#define TLS_MAX_VERSION TLS1_2_VERSION
So the patch seems okay for these two versions, but I have no data about
intermediate OpenSSL versions.
BTW, the spacing in this patch seems rather random.
regards, tom lane
On 2019-09-24 09:18, Victor Wagner wrote: > Problem is that some code in src/backend/libpq/be-secure-openssl.c > assumes that if preprocessor symbols TLS1_1_VERSION and TLS1_2_VERSION > are defined in the openssl headers, corresponding versions of TLS are > supported by the library. > > It is not so. Here is exempt from tls1.h header file from the openssl > 0.9.8j > > #define TLS1_VERSION 0x0301 > #define TLS1_1_VERSION 0x0302 > #define TLS1_2_VERSION 0x0303 > /* TLS 1.1 and 1.2 are not supported by this version of OpenSSL, so > * TLS_MAX_VERSION indicates TLS 1.0 regardless of the above > * definitions. (s23_clnt.c and s23_srvr.c have an OPENSSL_assert() > * check that would catch the error if TLS_MAX_VERSION was too low.) > */ > #define TLS_MAX_VERSION TLS1_VERSION That's not actually what this file looks like in the upstream release. It looks like the packagers must have patched in the protocol codes for TLS 1.1 and 1.2 themselves. Then they should also add the corresponding SSL_OP_NO_* flags. AFAICT, these pairs of symbols are always added together in upstream commits. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Sep 24, 2019 at 11:25:30AM -0400, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> ... I wonder if we should really continue to support >> OpenSSL 0.9.8. > > Fair question, but post-rc1 is no time to be moving that goalpost > for the v12 branch. Yeah. I worked in the past with SUSE-based appliances, and I recall that those folks have been maintaining their own patched version of OpenSSL 0.9.8 with a bunch of custom patches, some of them coming from newer versions of upstream to take care of security issues with 0.9.8. So even if they call their version 0.9.8j, I think that they include much more security-related fixes than their version string suggests. I don't know at which extent though. >> Anyway I suppose it's not impossible that third parties are still >> maintaining their 1.0.0 branch, > > Another data point on that is that Red Hat is still supporting > 1.0.1e in RHEL6. I don't think we should assume that just because > OpenSSL upstream has dropped support for a branch, it no longer > exists in the wild. > > Having said that, if it makes our lives noticeably easier to > drop support for 0.9.8 in HEAD, I won't stand in the way. Agreed. There is an argument for dropping support for OpenSSL 0.9.8 in 13~, but I don't agree of doing that in 12. Let's just fix the issue. -- Michael
Вложения
On Tue, Sep 24, 2019 at 11:52:48PM +0200, Peter Eisentraut wrote: > That's not actually what this file looks like in the upstream release. > It looks like the packagers must have patched in the protocol codes for > TLS 1.1 and 1.2 themselves. Then they should also add the corresponding > SSL_OP_NO_* flags. AFAICT, these pairs of symbols are always added > together in upstream commits. Yes, they did so. I see those three fields as of 6287fa5 from upstream which is the release tag for 0.9.8j: #define TLS1_VERSION 0x0301 #define TLS1_VERSION_MAJOR 0x03 #define TLS1_VERSION_MINOR 0x01 However if you look at the top branch OpenSSL_0_9_8-stable (7474341), then you would notice that ssl/tls1.h does something completely different and defines TLS_MAX_VERSION. So everything is in line to say that the version of OpenSSL in SUSE labelled 0.9.8j is using something compatible with the latest version of upstream 0.9.8zi. I think that we should just stick for simplicity with the top of their branch instead of trying to be compatible with 0.9.8j because Postgres 12 has not been released yet, hence if one tries to compile Postgres 12 with OpenSSL 0.9.8j then they would get a compilation failure, and we could just tell them to switch to the latest version of upstream for 0.9.8. That's something they should really do anyway to take care of various security issues on this branch. Well, if that happens they should rather upgrade to at least 1.0.2 anyway :) So I agree with the proposal to rely on the presence of TLS_MAX_VERSION, and base our decision-making on that. -- Michael
Вложения
On Tue, Sep 24, 2019 at 12:43:17PM -0400, Tom Lane wrote: > One thing I'm wondering is if it's safe to assume that TLS_MAX_VERSION > will be defined whenever these other symbols are. Looking in an > 0.9.8x install tree, that doesn't seem to define any of them; while > in 1.0.1e I see Yeah, I could personally live with the argument of simplicity and just say that trying to compile v12 with any version older than 0.9.8zc or any version that does not have those symbols just does not work, and that one needs to use the top of the released versions. > ./tls1.h:#define TLS1_1_VERSION 0x0302 > ./tls1.h:#define TLS1_2_VERSION 0x0303 > ./tls1.h:#define TLS_MAX_VERSION TLS1_2_VERSION > > So the patch seems okay for these two versions, but I have no data about > intermediate OpenSSL versions. More precisely, all those fields have been added by this upstream commit, so the fields are present since 0.9.8zc: commit: c6a876473cbff0fd323c8abcaace98ee2d21863d author: Bodo Moeller <bodo@openssl.org> date: Wed, 15 Oct 2014 04:18:29 +0200 Support TLS_FALLBACK_SCSV. > BTW, the spacing in this patch seems rather random. Indeed. Now that I think about it, another method would be to rely on the fact that a given version of OpenSSL does not support TLS 1.1 and 1.2. So we could also just add checks based on OPENSSL_VERSION_NUMBER and be done with it. And actually, looking at their tree TLS 1.1 and 2.2 are not supported in 1.0.0 either. 1.0.1, 1.0.2, 1.1.0 and HEAD do support them, but not TLS 1.3. I would still prefer relying on TLS_MAX_VERSION though, as that's more portable for future decisions, like the introduction of TLS1_3_VERSION for which we have already some logic in be-secure-openssl.c. And updating this stuff would very likely get forgotten once OpenSSL adds support for TLS 1.3... There is another issue in the patch: -#ifdef TLS1_3_VERSION +#if defined(TLS1_3_VERSION) && TLS1_2_VERSION <= TLS_MAX_VERSION The second part of the if needs to use TLS1_3_VERSION. I would also add more brackets around the extra conditions for readability. -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes:
> Now that I think about it, another method would be to rely on the fact
> that a given version of OpenSSL does not support TLS 1.1 and 1.2. So
> we could also just add checks based on OPENSSL_VERSION_NUMBER and be
> done with it.
No, that way madness lies. We *know* that there are lots of
vendor-patched versions of OpenSSL out there, so that the nominal
version number isn't really going to tell us what the package can do.
What I'm concerned about at the moment is Peter's comment upthread
that what we seem to be dealing with here is a broken vendor patch,
not any officially-released OpenSSL version at all. Is it our job
to work around that situation, rather than pushing the vendor to
fix their patch?
regards, tom lane
On Thu, Sep 26, 2019 at 02:03:12AM -0400, Tom Lane wrote: > What I'm concerned about at the moment is Peter's comment upthread > that what we seem to be dealing with here is a broken vendor patch, > not any officially-released OpenSSL version at all. Is it our job > to work around that situation, rather than pushing the vendor to > fix their patch? Yes, rather broken. SUSE got the header visibly right, at least the version string is not. The best solution in our favor would be that they actually fix their stuff :) And OpenSSL is also to blame by not handling those flags consistently in a stable branch.. -- Michael
Вложения
On 2019-09-26 06:53, Michael Paquier wrote:
> So I agree with the proposal to rely on the presence of
> TLS_MAX_VERSION, and base our decision-making on that.
But then there is this:
commit 04cd70c6899c6b36517b2b07d7a12b2cceba1bef
Author: Kurt Roeckx <kurt@roeckx.be>
Date: Tue Sep 18 22:17:14 2018
Deprecate TLS_MAX_VERSION, DTLS_MAX_VERSION and DTLS_MIN_VERSION
Fixes: #7183
Reviewed-by: Matt Caswell <matt@openssl.org>
GH: #7260
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Here is my proposed patch, currently completely untested. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Thu, Sep 26, 2019 at 04:43:33PM +0200, Peter Eisentraut wrote: > On 2019-09-26 06:53, Michael Paquier wrote: > > So I agree with the proposal to rely on the presence of > > TLS_MAX_VERSION, and base our decision-making on that. > > But then there is this: > > commit 04cd70c6899c6b36517b2b07d7a12b2cceba1bef > Author: Kurt Roeckx <kurt@roeckx.be> > Date: Tue Sep 18 22:17:14 2018 > > Deprecate TLS_MAX_VERSION, DTLS_MAX_VERSION and DTLS_MIN_VERSION > > Fixes: #7183 > > Reviewed-by: Matt Caswell <matt@openssl.org> > GH: #7260 Ouch. I missed that part, thanks! That's included as part of current HEAD, so even 1.1.1 still has the flags. -- Michael
Вложения
On Thu, Sep 26, 2019 at 06:24:22PM +0200, Peter Eisentraut wrote: > Here is my proposed patch, currently completely untested. I have tested compilation of REL_12_STABLE with the top of OpenSSL 0.9.8, 1.0.0, 1.0.1, 1.0.2, 1.1.0 and 1.1.1. Our SSL tests also pass in all the setups I have tested. Your patch does not issue a ereport(LOG/FATAL) in the event of a failure with SSL_CTX_set_max_proto_version(), which is something done when ssl_protocol_version_to_openssl()'s result is -1. Wouldn't it be better to report that properly to the user? Some more nits about the patch I have. Would it be worth copying the comment from min_proto_version() to SSL_CTX_set_max_proto_version()? I would add a newline before the comment block as well. Note: We have a failure with ssl/t/002_scram.pl because of the introduction of the recent channel_binding parameter if you try to run the SSL tests on HEAD with at least 0.9.8 as we forgot to add a conditional check for HAVE_X509_GET_SIGNATURE_NID as c3d41cc did. I'll send a patch for that separately. That's why I have checked the patch only with REL_12_STABLE. -- Michael
Вложения
On 2019-09-27 03:51, Michael Paquier wrote: > I have tested compilation of REL_12_STABLE with the top of OpenSSL > 0.9.8, 1.0.0, 1.0.1, 1.0.2, 1.1.0 and 1.1.1. Our SSL tests also pass > in all the setups I have tested. great > Your patch does not issue a ereport(LOG/FATAL) in the event of a > failure with SSL_CTX_set_max_proto_version(), which is something done > when ssl_protocol_version_to_openssl()'s result is -1. Wouldn't it be > better to report that properly to the user? Our SSL_CTX_set_max_proto_version() is a reimplementation of a function that exists in newer versions of OpenSSL, so it has a specific error behavior. Our implementation should probably not diverge from it too much. > Some more nits about the patch I have. Would it be worth copying the > comment from min_proto_version() to SSL_CTX_set_max_proto_version()? > I would add a newline before the comment block as well. ok -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Sep 27, 2019 at 03:50:57PM +0200, Peter Eisentraut wrote: > On 2019-09-27 03:51, Michael Paquier wrote: >> Your patch does not issue a ereport(LOG/FATAL) in the event of a >> failure with SSL_CTX_set_max_proto_version(), which is something done >> when ssl_protocol_version_to_openssl()'s result is -1. Wouldn't it be >> better to report that properly to the user? > > Our SSL_CTX_set_max_proto_version() is a reimplementation of a function > that exists in newer versions of OpenSSL, so it has a specific error > behavior. Our implementation should probably not diverge from it too much. I agree with this point. Now my argument is about logging LOG or FATAL within be_tls_init() after the two OpenSSL functions (or our wrappers) SSL_CTX_set_min/max_proto_version are called. -- Michael
Вложения
On 2019-09-27 16:20, Michael Paquier wrote: > On Fri, Sep 27, 2019 at 03:50:57PM +0200, Peter Eisentraut wrote: >> On 2019-09-27 03:51, Michael Paquier wrote: >>> Your patch does not issue a ereport(LOG/FATAL) in the event of a >>> failure with SSL_CTX_set_max_proto_version(), which is something done >>> when ssl_protocol_version_to_openssl()'s result is -1. Wouldn't it be >>> better to report that properly to the user? >> >> Our SSL_CTX_set_max_proto_version() is a reimplementation of a function >> that exists in newer versions of OpenSSL, so it has a specific error >> behavior. Our implementation should probably not diverge from it too much. > > I agree with this point. Now my argument is about logging LOG or > FATAL within be_tls_init() after the two OpenSSL functions (or our > wrappers) SSL_CTX_set_min/max_proto_version are called. committed with that -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Sep 28, 2019 at 10:52:18PM +0200, Peter Eisentraut wrote: > committed with that Thanks, LGTM. -- Michael