Обсуждение: Force disable of SSL renegociation in the server

Hi all,

Robert has mentioned https://nvd.nist.gov/vuln/detail/CVE-2021-3449 on
the -security list where a TLS server could crash with some crafted
renegociation message.  We already disable SSL renegociation
initialization for some time now, per 48d23c72, but we don't prevent
the server from complying should the client wish to use renegociation.
In terms of robustness and because SSL renegociation had its set of
flaws and issues for many years, it looks like it would be a good idea
to disable renegociation on the backend (not the client as that may be
used with older access points where renegociation is still used, per
an argument from Andres).

In flavor, this is controlled in a way similar to
SSL_OP_NO_COMPRESSION that we already enforce in the backend to
disable SSL compression.  However, there are a couple of compatibility
tweaks regarding this one:
- SSL_OP_NO_RENEGOTIATION controls that.  It is present in OpenSSL >=
1.1.1 and has been backported in 1.1.0h (it is not present in older
versions of 1.1.0).
- In 1.0.2 and older versions, OpenSSL has an undocumented flag called
SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS, able to do the same as far as I
understand.

Attached is a patch to use SSL_OP_NO_RENEGOTIATION if it exists, and
force that in the backend.  We could go further down, but using
undocumented things looks rather unsafe here, to say the least.  Could
there be a point in backpatching that, in light of what we have done in
48d23c72 in the past, though?

Thoughts?
--
Michael

> On 20 May 2021, at 13:00, Michael Paquier <michael@paquier.xyz> wrote:

> - SSL_OP_NO_RENEGOTIATION controls that.  It is present in OpenSSL >=
> 1.1.1 and has been backported in 1.1.0h (it is not present in older
> versions of 1.1.0).

For OpenSSL 1.1.0 versions < 1.1.0h it will be silently accepted without
actually doing anything, so we might want to combine it with the below.

> - In 1.0.2 and older versions, OpenSSL has an undocumented flag called
> SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS, able to do the same as far as I
> understand.

Well, it's documented in the changelog that it's undocumented (sigh..) along
with a note stating that it works like SSL_OP_NO_RENEGOTIATION.  Skimming the
code it seems to ring true.  For older OpenSSL versions there's also
SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION which controls renegotiation for an
older OpenSSL reneg bug.  That applies to 0.9.8 versions which we don't
support, but a malicious user can craft whatever they feel like so maybe we
should ensure it's off as well?

> Could there be a point in backpatching that, in light of what we have done in
> 48d23c72 in the past, though?

I think there is merit to that idea, especially given the precedent.

> Thoughts?

+    /* disallow SSL renegociation, option available since 1.1.0h */
s/renegociation/renegotiation/

+1 on disabling renegotiation, but I think it's worth considering using
SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS as well.  One could also argue that extending
the comment with a note that it only applies to TLSv1.2 and lower could be
helpful to readers who aren't familiar with TLS protocol versions.  TLSv1.3 did
away with renegotiation.

--
Daniel Gustafsson        https://vmware.com/

On Thu, May 20, 2021 at 02:15:52PM +0200, Daniel Gustafsson wrote:
> On 20 May 2021, at 13:00, Michael Paquier <michael@paquier.xyz> wrote:
>> - SSL_OP_NO_RENEGOTIATION controls that.  It is present in OpenSSL >=
>> 1.1.1 and has been backported in 1.1.0h (it is not present in older
>> versions of 1.1.0).
>
> For OpenSSL 1.1.0 versions < 1.1.0h it will be silently accepted without
> actually doing anything, so we might want to combine it with the below.

Yeah, still that stresses me quite a bit.  OpenSSL does not have a
good history with compatibility, and we are talking about something
that does not officially exist on the map.

>> - In 1.0.2 and older versions, OpenSSL has an undocumented flag called
>> SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS, able to do the same as far as I
>> understand.
>
> Well, it's documented in the changelog that it's undocumented (sigh..) along
> with a note stating that it works like SSL_OP_NO_RENEGOTIATION.

I'd say that this is still part of the definition of undocumented.
There is no mention of it in their online documentation :)

> Skimming the
> code it seems to ring true.  For older OpenSSL versions there's also
> SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION which controls renegotiation for an
> older OpenSSL reneg bug.  That applies to 0.9.8 versions which we don't
> support, but a malicious user can craft whatever they feel like so maybe we
> should ensure it's off as well?

If I am getting it right by reading upstream, SSL_OP_NO_RENEGOTIATION
takes priority over that.  Hence, if we force SSL_OP_NO_RENEGOTIATION,
then SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION has no effect anyway.

> +    /* disallow SSL renegociation, option available since 1.1.0h */
> s/renegociation/renegotiation/

Argh, French-ism here.

> +1 on disabling renegotiation, but I think it's worth considering using
> SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS as well.

This one can be set within ssl->s3->flags in the port information.
Still that's not completely feasable either as some versions of
OpenSSL hide the internals of a bunch of internal structures, and some
distributions patch the upstream code?  At the end of the day, I think
that I would stick with simplicity and use SSL_OP_NO_RENEGOTIATION.
It is not our job to go around any decision OpenSSL has poorly done
either over the years.  At least this part is officially documented :)

> One could also argue that extending
> the comment with a note that it only applies to TLSv1.2 and lower could be
> helpful to readers who aren't familiar with TLS protocol versions.  TLSv1.3 did
> away with renegotiation.

Good idea to mention that.
--
Michael

On Fri, May 21, 2021 at 10:41:34AM +0900, Michael Paquier wrote:
> This one can be set within ssl->s3->flags in the port information.
> Still that's not completely feasable either as some versions of
> OpenSSL hide the internals of a bunch of internal structures, and some
> distributions patch the upstream code?  At the end of the day, I think
> that I would stick with simplicity and use SSL_OP_NO_RENEGOTIATION.
> It is not our job to go around any decision OpenSSL has poorly done
> either over the years.  At least this part is officially documented :)

I got to look at that in details, and the attached would be able to do
the job with OpenSSL 1.0.2 and older versions.  The main idea is to
set up SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS once the SSL object is
created when opening the TLS connection to business.  I have tested
that down to 0.9.8 on all supported branches with the protocols we
support (heads up to ssl_min_protocol_version here), and that looks to
work as I'd expect.

It is not a good idea to rely on OPENSSL_VERSION_NUMBER for such
version checks as I am doing here, as we've been bitten with
compatibility with LibreSSL in the past.  So this had better use a
check based on HAVE_OPENSSL_INIT_SSL to make sure that 1.1.0 is the
version of OpenSSL used.  Anyway, I really don't like using this
undocumented option, and there is nothing that can be done with
OpenSSL < 1.1.0h in the 1.1.0 series as the s3 part of the *SSL object
gets hidden to the application, so it is not possible to set
SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS there.  And so, I would like to
stick with a backpatch here, only for the part of the patch involving
be_tls_init().  Full patch is attached for reference.

While on it, I have added a comment about TLSv1.2 being the last
protocol supporting renegotiation.

Any objections?
--
Michael

> On 24 May 2021, at 03:29, Michael Paquier <michael@paquier.xyz> wrote:

> I got to look at that in details, and the attached would be able to do
> the job with OpenSSL 1.0.2 and older versions.  The main idea is to
> set up SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS once the SSL object is
> created when opening the TLS connection to business.  I have tested
> that down to 0.9.8 on all supported branches with the protocols we
> support (heads up to ssl_min_protocol_version here), and that looks to
> work as I'd expect.
>
> It is not a good idea to rely on OPENSSL_VERSION_NUMBER for such
> version checks as I am doing here, as we've been bitten with
> compatibility with LibreSSL in the past.  So this had better use a
> check based on HAVE_OPENSSL_INIT_SSL to make sure that 1.1.0 is the
> version of OpenSSL used.

I agree that a capability based check is better than using the version numbers
as their is a collision risk between distributions (and even within OpenSSL as
NetBSD for example invented their own version etc).

> Anyway, I really don't like using this undocumented option, and there is
> nothing that can be done with OpenSSL < 1.1.0h in the 1.1.0 series as the s3
> part of the *SSL object gets hidden to the application, so it is not possible
> to set SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS there.


1.1.0d killed what was left of SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS while keeping
it defined, so there is also very little value in even attempting it there.

+1 on the patch, LGTM.

--
Daniel Gustafsson        https://vmware.com/

On Mon, May 24, 2021 at 11:09:38AM +0200, Daniel Gustafsson wrote:
> 1.1.0d killed what was left of SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS while keeping
> it defined, so there is also very little value in even attempting it there.
>
> +1 on the patch, LGTM.

Thanks, applied.

I was having a very hard time putting a T instead of a C to
renegotiation..
--
Michael