Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BoringSSL + TLSv1.2 client side SSL renegociation fails #11529

Closed
slandelle opened this issue Jul 29, 2021 · 11 comments · Fixed by #11601
Closed

BoringSSL + TLSv1.2 client side SSL renegociation fails #11529

slandelle opened this issue Jul 29, 2021 · 11 comments · Fixed by #11601
Assignees
@normanmaurer
Milestone
4.1.68.Final

Comments

@slandelle
Copy link
Contributor

Expected behavior

BoringSSL based SSLEngine should support SSL renegociation out of the box like the JDK one.

Actual behavior

SSLEngine and connection crashes during renegociation.

This issue was originally reported against Gatling, see gatling/gatling#4120 for more details.

Steps to reproduce

See pure Netty reproducer below.

Minimal yet complete reproducer code (or URL to code)

https://github.com/slandelle/netty-ssl-renegociation

Netty version

4.1.66.Final

JVM version (e.g. java -version)

openjdk version "1.8.0_292"
OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_292-b10)
OpenJDK 64-Bit Server VM (AdoptOpenJDK)(build 25.292-b10, mixed mode)

OS version (e.g. uname -a)

Darwin Kernel Version 20.5.0: Sat May 8 05:10:33 PDT 2021; root:xnu-7195.121.3~9/RELEASE_X86_64 x86_64
@slandelle slandelle changed the title BoringSSL + TLSv1.2 client side SSL renegociation doesn't work BoringSSL + TLSv1.2 client side SSL renegociation crash Jul 29, 2021
@normanmaurer
Copy link
Member

As far as I remember we don't support renegotiation when using the OpenSSL / BoringSSL based engines because these are considered not secure anymore.

https://boringssl.googlesource.com/boringssl/+/refs/heads/master/include/openssl/ssl.h#4056

@slandelle
Copy link
Contributor Author

If |ssl| is a server, peer-initiated renegotiations are always rejected and this function does nothing.

There is no support in BoringSSL for initiating renegotiations as a client or server.

Here, Netty is client side and the initiator is the server. Also, this request works fine with Chrome, whose TLS implementation is BoringSSL.

Doesn't renegotiation only bring security issues when the initiator is the client, not the server?

@normanmaurer normanmaurer changed the title BoringSSL + TLSv1.2 client side SSL renegociation crash BoringSSL + TLSv1.2 client side SSL renegociation falis Jul 30, 2021
@normanmaurer normanmaurer changed the title BoringSSL + TLSv1.2 client side SSL renegociation falis BoringSSL + TLSv1.2 client side SSL renegociation fails Jul 30, 2021
@normanmaurer
Copy link
Member

I suspect we would need to enable it explicit for BoringSSL:

https://boringssl.googlesource.com/boringssl/+/6b6e0b20893e2be0e68af605a60ffa2cbb0ffa64/PORTING.md#tls-renegotiation

@slandelle
Copy link
Contributor Author

Indeed.

Does it looks feasible to you to introduce a new option to be passed through SslContextOption to configure the underlying BoringSSL?

@normanmaurer
Copy link
Member

Yeah I guess something like this. That said I think we will also need to add some changes to ReferenceCountedOpenSslEngine

@slandelle
Copy link
Contributor Author

I'm off on vacation tonight, so I'll only be able to have a look in a few weeks. Thanks for your insights!

@normanmaurer
Copy link
Member

@slandelle no worries... I will also be on vacation soon 😆

@slandelle
Copy link
Contributor Author

Again??? You're just back! 😆

@normanmaurer normanmaurer mentioned this issue Aug 1, 2021
Merged
@normanmaurer
Copy link
Member

Let me pick this up this week again

@normanmaurer normanmaurer self-assigned this Aug 17, 2021
@normanmaurer
Copy link
Member

This is also related to apple/swift-nio-ssl#111

normanmaurer added a commit to netty/netty-tcnative that referenced this issue Aug 17, 2021
@normanmaurer
Allow to set renegiotion mode when BoringSSL is used
2c64d41 
Motivation:

renegiotion is not supported by BoringSSL by default but sometimes when using on the client-side you might want to support it for CLIENT-AUTH.
See also apple/swift-nio-ssl#111

Modifications:

Add native / java code to be able to enable support

Result:

Part of netty/netty#11529
@normanmaurer normanmaurer mentioned this issue Aug 17, 2021
Merged
@normanmaurer
Copy link
Member

tcnative change that is needed: netty/netty-tcnative#654

@normanmaurer normanmaurer added this to the 4.1.68.Final milestone Aug 17, 2021
normanmaurer added a commit to netty/netty-tcnative that referenced this issue Aug 18, 2021
@normanmaurer
Allow to set renegiotion mode when BoringSSL is used (#654)
0bc3255 
Motivation:

renegiotion is not supported by BoringSSL by default but sometimes when using on the client-side you might want to support it for CLIENT-AUTH.
See also apple/swift-nio-ssl#111

Modifications:

Add native / java code to be able to enable support

Result:

Part of netty/netty#11529
normanmaurer added a commit that referenced this issue Aug 19, 2021
@normanmaurer
Allow server initiated renegotiate when using OpenSSL / BoringSSL bas…
823c991 
…ed SSLEngine

Motivation:

We should allow server initiated renegotiation when OpenSSL / BoringSSL bases SSLEngine is used as it might be used for client auth.

Modifications:

- Upgrade netty-tcnative version to be able to allow renegotiate once
- Adjust code

Result
Fixes #11529
@normanmaurer normanmaurer mentioned this issue Aug 19, 2021
Merged
normanmaurer added a commit that referenced this issue Aug 20, 2021
@normanmaurer
Allow server initiated renegotiate when using OpenSSL / BoringSSL bas…
09c404d 
…ed SSLEngine

Motivation:

We should allow server initiated renegotiation when OpenSSL / BoringSSL bases SSLEngine is used as it might be used for client auth.

Modifications:

- Upgrade netty-tcnative version to be able to allow renegotiate once
- Adjust code

Result
Fixes #11529
normanmaurer added a commit that referenced this issue Aug 20, 2021
@normanmaurer
Allow server initiated renegotiate when using OpenSSL / BoringSSL bas…
33b63c3 
…ed SSLEngine (#11601)


Motivation:

We should allow server initiated renegotiation when OpenSSL / BoringSSL bases SSLEngine is used as it might be used for client auth.

Modifications:

- Upgrade netty-tcnative version to be able to allow renegotiate once
- Adjust code

Result
Fixes #11529
normanmaurer added a commit that referenced this issue Aug 20, 2021
@normanmaurer
Allow server initiated renegotiate when using OpenSSL / BoringSSL bas…
3c5b6c3 
…ed SSLEngine (#11601)


Motivation:

We should allow server initiated renegotiation when OpenSSL / BoringSSL bases SSLEngine is used as it might be used for client auth.

Modifications:

- Upgrade netty-tcnative version to be able to allow renegotiate once
- Adjust code

Result
Fixes #11529
laosijikaichele pushed a commit to laosijikaichele/netty that referenced this issue Dec 16, 2021
@normanmaurer @laosijikaichele
Allow server initiated renegotiate when using OpenSSL / BoringSSL bas…
364282f 
…ed SSLEngine (netty#11601)


Motivation:

We should allow server initiated renegotiation when OpenSSL / BoringSSL bases SSLEngine is used as it might be used for client auth.

Modifications:

- Upgrade netty-tcnative version to be able to allow renegotiate once
- Adjust code

Result
Fixes netty#11529
laosijikaichele pushed a commit to laosijikaichele/netty that referenced this issue Dec 16, 2021
@normanmaurer @laosijikaichele
Allow server initiated renegotiate when using OpenSSL / BoringSSL bas…
5698b26 
…ed SSLEngine (netty#11601)


Motivation:

We should allow server initiated renegotiation when OpenSSL / BoringSSL bases SSLEngine is used as it might be used for client auth.

Modifications:

- Upgrade netty-tcnative version to be able to allow renegotiate once
- Adjust code

Result
Fixes netty#11529
raidyue pushed a commit to raidyue/netty that referenced this issue Jul 8, 2022
@normanmaurer
Allow server initiated renegotiate when using OpenSSL / BoringSSL bas…
7749a22 
…ed SSLEngine (netty#11601)


Motivation:

We should allow server initiated renegotiation when OpenSSL / BoringSSL bases SSLEngine is used as it might be used for client auth.

Modifications:

- Upgrade netty-tcnative version to be able to allow renegotiate once
- Adjust code

Result
Fixes netty#11529
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@normanmaurer normanmaurer

Labels
None yet
Projects
None yet
Milestone
4.1.68.Final
Development

Successfully merging a pull request may close this issue.

Allow server initiated renegotiate when using OpenSSL / BoringSSL bas… netty/netty
2 participants
@normanmaurer @slandelle