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

Double CloseNotify in response to incoming CloseNotify #261

Closed
roelfdutoit opened this issue Dec 12, 2017 · 3 comments · Fixed by #263
Closed

Double CloseNotify in response to incoming CloseNotify #261

roelfdutoit opened this issue Dec 12, 2017 · 3 comments · Fixed by #263

Comments

@roelfdutoit
Copy link

roelfdutoit commented Dec 12, 2017

A hs-tls server instance will send 2 x CloseNotify alerts in response to receiving a CloseNotify alert:
debug: << Alert [(AlertLevel_Warning,CloseNotify)]
debug: >> Alert [(AlertLevel_Warning,CloseNotify)]
debug: >> Alert [(AlertLevel_Warning,CloseNotify)]

This can be reproduced with tls-simpleserver:
tls-simpleserver -d --tls12 --certificate=server.pem --key=server.pem 4433
.. and the following openssl command running on the same machine:
echo Q | openssl s_client

The TLS 1.2 RFC states the following:

close_notify This message notifies the recipient that the sender will not send any more messages on this connection.

One can argue that the RFC also states:

Any data received after a closure alert is ignored.

.. but 'data' in that context would be AppData. In the strict sense it would be an error to send 2 x CloseNotify.

Either way, I think some client TLS stacks will actually view the double CloseNotify as an error, and it would be good to improve interoperability by only sending 1 x CloseNotify.

@kazu-yamamoto
Copy link
Collaborator

I can reproduce this.

@kazu-yamamoto
Copy link
Collaborator

This is because:

  • CloseNofity is sent when CloseNotify is received in Core
  • tls-simpleserver uses bye to send CloseNotify

I guess that bye should check the status of EOF.

kazu-yamamoto added a commit to kazu-yamamoto/hs-tls that referenced this issue Dec 13, 2017
@kazu-yamamoto
Copy link
Collaborator

The patch above should fix this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants