OpenSslEngine wrap may generate bad data if multiple src buffers #6715
Conversation
| @@ -638,9 +638,7 @@ public final SSLEngineResult wrap( | |||
| bytesProduced += bioLengthBefore - pendingNow; | |||
| bioLengthBefore = pendingNow; | |||
|
|
|||
| if (bytesConsumed == MAX_PLAINTEXT_LENGTH || bytesProduced == dst.remaining()) { | |||
| return newResultMayFinishHandshake(status, bytesConsumed, bytesProduced); | |||
| } | |||
There was a problem hiding this comment.
@Scottmitch why is this not needed anymore ?
There was a problem hiding this comment.
This is explained in the commit message:
OpenSslEngine#wrap should only produce a single TLS packet according to the SSLEngine API specificaiton [1].
[1] https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLEngine.html#wrap-java.nio.ByteBuffer:A-int-int-java.nio.ByteBuffer-
There was a problem hiding this comment.
If we consume multiple packets we must be more careful about how much overhead we account for. This is done in PR #6365
There was a problem hiding this comment.
Got it... Sorry I missed this before.
|
@Scottmitch LGTM... please merge once the CI passes |
Scottmitch
force-pushed
the
openssl_multi_src_overhead
branch
from
5005ab5
to
14701eb
Compare
|
3 minutes isn't enough on the CI servers ... bumping to 5 minutes. |
Motivation: SSL_write requires a fixed amount of bytes for overhead related to the encryption process for each call. OpenSslEngine#wrap(..) will attempt to encrypt multiple input buffers until MAX_PLAINTEXT_LENGTH are consumed, but the size estimation provided by calculateOutNetBufSize may not leave enough room for each call to SSL_write. If SSL_write is not able to completely write results to the destination buffer it will keep state and attempt to write it later. Netty doesn't account for SSL_write keeping state and assumes all writes will complete synchronously (by attempting to allocate enough space to account for the overhead) and feeds the same data to SSL_write again later which results in corrupted data being generated. Modifications: - OpenSslEngine#wrap should only produce a single TLS packet according to the SSLEngine API specificaiton [1]. [1] https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLEngine.html#wrap-java.nio.ByteBuffer:A-int-int-java.nio.ByteBuffer- - OpenSslEngine#wrap should only consider a single buffer when determining if there is enough space to write, because only a single buffer will ever be consumed. Result: OpenSslEngine#wrap will no longer produce corrupted data due to incorrect accounting of space required in the destination buffers.
Scottmitch
force-pushed
the
openssl_multi_src_overhead
branch
from
14701eb
to
5e05dbf
Compare
|
Note that performance may be impacted as a result of this PR (due to strictly limiting the amount of data encrypted per wrap call), but PR #6365 will improve upon the ability to wrap (and unwrap) multiple TLS packets per call. |
|
grpc-java was failing when upgraded to For posterity, if you're app is failing with weird |
|
@lukaszx0 thanks for reporting back. |
|
@lukaszx0 - Thanks for confirming. We are aware of the implications of this issue and are in the process of generating a new release now. Standby. |
Motivation:
SSL_write requires a fixed amount of bytes for overhead related to the encryption process for each call. OpenSslEngine#wrap(..) will attempt to encrypt multiple input buffers until MAX_PLAINTEXT_LENGTH are consumed, but the size estimation provided by calculateOutNetBufSize may not leave enough room for each call to SSL_write. If SSL_write is not able to completely write results to the destination buffer it will keep state and attempt to write it later. Netty doesn't account for SSL_write keeping state and assumes all writes will complete synchronously (by attempting to allocate enough space to account for the overhead) and feeds the same data to SSL_write again later which results in corrupted data being generated.
Modifications:
[1] https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLEngine.html#wrap-java.nio.ByteBuffer:A-int-int-java.nio.ByteBuffer-
Result:
OpenSslEngine#wrap will no longer produce corrupted data due to incorrect accounting of space required in the destination buffers.