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.

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

OpenSslEngine support unwrap plaintext greater than 2^14 and avoid infinite loop #7352

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
2 participants
@Scottmitch
Member

Scottmitch commented Oct 31, 2017

Motivation:
If SslHandler sets jdkCompatibilityMode to false and ReferenceCountedOpenSslEngine sets jdkCompatibilityMode to true there is a chance we will get stuck in an infinite loop if the peer sends a TLS packet with length greater than 2^14 (the maximum length allowed in the TLS 1.2 RFC [1]). However there are legacy implementations which actually send larger TLS payloads than 2^14 (e.g. OpenJDK's SSLSessionImpl [2]) and in this case ReferenceCountedOpenSslEngine will return BUFFER_OVERFLOW in an attempt to notify that a larger buffer is to be used, but if the buffer is already at max size this process will repeat indefinitely.

[1] https://tools.ietf.org/html/rfc5246#section-6.2.1
[2] http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/d5a00b1e8f78/src/share/classes/sun/security/ssl/SSLSessionImpl.java#l793

Modifications:

  • Support TLS payload sizes greater than 2^14 in ReferenceCountedOpenSslEngine
  • ReferenceCountedOpenSslEngine should throw an exception if a
    BUFFER_OVERFLOW is impossible to rectify

Result:
No more infinite loop in ReferenceCountedOpenSslEngine due to
BUFFER_OVERFLOW and large TLS payload lengths.

@Scottmitch Scottmitch added the defect label Oct 31, 2017

@Scottmitch Scottmitch added this to the 4.0.53.Final milestone Oct 31, 2017

@Scottmitch Scottmitch self-assigned this Oct 31, 2017

@Scottmitch Scottmitch requested a review from normanmaurer Oct 31, 2017

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Oct 31, 2017

Member

this depends upon netty/netty-tcnative#318

/cc @rkapsi - This may be related to #7264

Member

Scottmitch commented Oct 31, 2017

this depends upon netty/netty-tcnative#318

/cc @rkapsi - This may be related to #7264

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 31, 2017

Member

@Scottmitch I wonder if we should alsways use jdkCompatible as default with this knowledge just to eliminate any surprises when people deploy their impl and use java on the remote peer as well

Member

normanmaurer commented Oct 31, 2017

@Scottmitch I wonder if we should alsways use jdkCompatible as default with this knowledge just to eliminate any surprises when people deploy their impl and use java on the remote peer as well

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Oct 31, 2017

Member

I wonder if we should alsways use jdkCompatible as default with this knowledge just to eliminate any surprises when people deploy their impl and use java on the remote peer as well

This bug is only visible when jdkCompatible=true so I don't think changing the default would help in this case. I think we should leave the default as jdkCompatible=false when creating from our SslContext. If this is not desired the user should be able to change the default.

Member

Scottmitch commented Oct 31, 2017

I wonder if we should alsways use jdkCompatible as default with this knowledge just to eliminate any surprises when people deploy their impl and use java on the remote peer as well

This bug is only visible when jdkCompatible=true so I don't think changing the default would help in this case. I think we should leave the default as jdkCompatible=false when creating from our SslContext. If this is not desired the user should be able to change the default.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 31, 2017

Member

@Scottmitch ah yeah sorry... brain-fart . Should not review PRs via my phone 😢

Member

normanmaurer commented Oct 31, 2017

@Scottmitch ah yeah sorry... brain-fart . Should not review PRs via my phone 😢

OpenSslEngine support unwrap plaintext greater than 2^14 and avoid
infinite loop

Motivation:
If SslHandler sets jdkCompatibilityMode to false and ReferenceCountedOpenSslEngine sets jdkCompatibilityMode to true there is a chance we will get stuck in an infinite loop if the peer sends a TLS packet with length greater than 2^14 (the maximum length allowed in the TLS 1.2 RFC [1]). However there are legacy implementations which actually send larger TLS payloads than 2^14 (e.g. OpenJDK's SSLSessionImpl [2]) and in this case ReferenceCountedOpenSslEngine will return BUFFER_OVERFLOW in an attempt to notify that a larger buffer is to be used, but if the buffer is already at max size this process will repeat indefinitely.

[1] https://tools.ietf.org/html/rfc5246#section-6.2.1
[2] http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/d5a00b1e8f78/src/share/classes/sun/security/ssl/SSLSessionImpl.java#l793

Modifications:
- Support TLS payload sizes greater than 2^14 in ReferenceCountedOpenSslEngine
- ReferenceCountedOpenSslEngine should throw an exception if a
BUFFER_OVERFLOW is impossible to rectify

Result:
No more infinite loop in ReferenceCountedOpenSslEngine due to
BUFFER_OVERFLOW and large TLS payload lengths.

@Scottmitch Scottmitch requested a review from normanmaurer Nov 2, 2017

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Nov 2, 2017

Member

4.1 (fbe0e35) 4.0 (5c71b47)

Member

Scottmitch commented Nov 2, 2017

4.1 (fbe0e35) 4.0 (5c71b47)

@Scottmitch Scottmitch closed this Nov 2, 2017

@Scottmitch Scottmitch deleted the Scottmitch:ssl_engine_jdk_compat branch Nov 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment