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.

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

SSLEngine.closeInbound() should throw SSLException if no close notification was received #839

Open
normanmaurer opened this issue Apr 24, 2020 · 1 comment · May be fixed by #845
Open
Assignees
Labels
Milestone

Comments

@normanmaurer
Copy link

During testing conscrypt in the netty testsuite we encountered that Conscrypt does not correct implement SSLEngine.closeInbound().

See https://github.com/netty/netty/pull/10211/files#diff-08da41869dfc1d842df441440b1c605bR2184

If you call closeInbound() and you did not receive a proper SSL/TLS close notification before it must throw an SSLException.

See also https://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLEngine.html#closeInbound()

@prbprbprb prbprbprb self-assigned this Apr 24, 2020
@prbprbprb prbprbprb added the bug label Apr 24, 2020
@prbprbprb
Copy link
Collaborator

Thanks!

prbprbprb added a commit to prbprbprb/conscrypt that referenced this issue May 1, 2020
Fixes google#839.

This should be _mostly_ uncontroversial as it is already documented
to do so[1] but could cause app compat issues.  A quick scan of
AOSP suggests no major issues however there is a CTS test for the
old behaviour[2] which will need changing.

The bulk of this change is regression tests for the correct behaviour
for the various possible orderings of close calls and TLS close
alerts. The behaviour change test is
closingInboundBeforeClosingOutboundShouldFail() in place of
closingInboundShouldOnlyCloseInbound().  Changes outside
ConscryptEngineTest are minimal.

Close behaviour before handshaking starts is undefined and we differ
from the RI, but I don't think that's problematic.

Obviously also needs documenting in Conscrypt and Android release
notes.

This also means that STATE_CLOSED_INBOUND is never reached, which
means it can be eliminated in a future CL allowing some minor
simplifications.

[1] https://developer.android.com/reference/javax/net/ssl/SSLEngine#closeInbound()
[2] https://cs.android.com/android/platform/superproject/+/master:libcore/harmony-tests/src/test/java/org/apache/harmony/tests/javax/net/ssl/SSLEngineTest.java;l=611
prbprbprb added a commit to prbprbprb/conscrypt that referenced this issue May 1, 2020
Fixes google#839.

This should be _mostly_ uncontroversial as it is already documented
to do so[1] but could cause app compat issues.  A quick scan of
AOSP suggests no major issues however there is a CTS test for the
old behaviour[2] which will need changing.

The bulk of this change is regression tests for the correct behaviour
for the various possible orderings of close calls and TLS close
alerts. The behaviour change test is
closingInboundBeforeClosingOutboundShouldFail() in place of
closingInboundShouldOnlyCloseInbound().  Changes outside
ConscryptEngineTest are minimal.

Close behaviour before handshaking starts is undefined and we differ
from the RI, but I don't think that's problematic.

Obviously also needs documenting in Conscrypt and Android release
notes.

This also means that STATE_CLOSED_INBOUND is never reached, which
means it can be eliminated in a future CL allowing some minor
simplifications.

NB This can be merged independently of google#844 and I'll rebase that
change on top of it.

[1] https://developer.android.com/reference/javax/net/ssl/SSLEngine#closeInbound()
[2] https://cs.android.com/android/platform/superproject/+/master:libcore/harmony-tests/src/test/java/org/apache/harmony/tests/javax/net/ssl/SSLEngineTest.java;l=611
prbprbprb added a commit to prbprbprb/conscrypt that referenced this issue May 1, 2020
Fixes google#839.

This should be _mostly_ uncontroversial as it is already documented
to do so[1] but could cause app compat issues.  A quick scan of
AOSP suggests no major issues however there is a CTS test for the
old behaviour[2] which will need changing.

The bulk of this change is regression tests for the correct behaviour
for the various possible orderings of close calls and TLS close
alerts. The behaviour change test is
closingInboundBeforeClosingOutboundShouldFail() in place of
closingInboundShouldOnlyCloseInbound().  Changes outside
ConscryptEngineTest are minimal.

Close behaviour before handshaking starts is undefined and we differ
from the RI, but I don't think that's problematic.

Obviously also needs documenting in Conscrypt and Android release
notes.

This also means that STATE_CLOSED_INBOUND is never reached, which
means it can be eliminated in a future CL allowing some minor
simplifications.

NB This can be merged independently of google#844 and I'll rebase that
change on top of it.

[1] https://developer.android.com/reference/javax/net/ssl/SSLEngine#closeInbound()
[2] https://cs.android.com/android/platform/superproject/+/master:libcore/harmony-tests/src/test/java/org/apache/harmony/tests/javax/net/ssl/SSLEngineTest.java;l=611
prbprbprb added a commit to prbprbprb/conscrypt that referenced this issue May 1, 2020
Fixes google#839.

This should be _mostly_ uncontroversial as it is already documented
to do so[1] but could cause app compat issues.  A quick scan of
AOSP suggests no major issues however there is a CTS test for the
old behaviour[2] which will need changing.

The bulk of this change is regression tests for the correct behaviour
for the various possible orderings of close calls and TLS close
alerts. The behaviour change test is
closingInboundBeforeClosingOutboundShouldFail() in place of
closingInboundShouldOnlyCloseInbound().  Changes outside
ConscryptEngineTest are minimal.

Close behaviour before handshaking starts is undefined and we differ
from the RI, but I don't think that's problematic.

Obviously also needs documenting in Conscrypt and Android release
notes.

This also means that STATE_CLOSED_INBOUND is never reached, which
means it can be eliminated in a future CL allowing some minor
simplifications.

NB This can be merged independently of google#844 and I'll rebase that
change on top of it.

[1] https://developer.android.com/reference/javax/net/ssl/SSLEngine#closeInbound()
[2] https://cs.android.com/android/platform/superproject/+/master:libcore/harmony-tests/src/test/java/org/apache/harmony/tests/javax/net/ssl/SSLEngineTest.java;l=611
@daulet daulet added this to the 2.7.0 milestone Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants