search

saltyrtc / saltyrtc-client-java

SaltyRTC Java implementation.
Apache License 2.0
3 stars 4 forks source link

The "No shared task" case isn't handled correctly ("close" msg & channel termination). #134

Open rustonaut opened 2 years ago

rustonaut commented 2 years ago

https://github.com/saltyrtc/saltyrtc-client-java/blob/d01e553c06a05c74a532104608c0dc57cb9a23f2/src/main/java/org/saltyrtc/client/signaling/InitiatorSignaling.java#L491-L493

The protocol specifies a close message should be send which we do not do. (The spec is kinda in conflict with itself here, as it in the section of the close message state it must only be send after a successful client handshake).

Furthermore the spec states we should close the connection to the server afterwards, as far as I understand this is because at this point in time we already did new where where "correctly" connected with the "right" client (responder) we tried to connect to, but we are fundamentally incompatible. (So no point in still trying to connect to it again or to other responders.)

Details

Relevant Parts of Spec

From auth message

In case no common task could be found, the initiator SHALL send a 'close' message to the responder containing the close code 3006 (No Shared Task Found) as reason and raise an error event indicating that no common signalling task could be found². The initiator SHALL then proceed with the termination of the connection as described in the section 'close' Message.

From close message

The client SHALL also terminate the connection to the server with a close code of 1001 (Going Away) if the connection is still open.

Code:

https://github.com/saltyrtc/saltyrtc-client-java/blob/d01e553c06a05c74a532104608c0dc57cb9a23f2/src/main/java/org/saltyrtc/client/signaling/Signaling.java#L449-L452

https://github.com/saltyrtc/saltyrtc-client-java/blob/d01e553c06a05c74a532104608c0dc57cb9a23f2/src/main/java/org/saltyrtc/client/signaling/InitiatorSignaling.java#L86-L97

rustonaut commented 2 years ago

Hm, I think (maybe?, speculate?) the reason why we need to send a close message is so that the client can (somewhat) verify it was incompatible with the "right" server (through your_cookie wasn't verified). :thinking:

lgrahl commented 2 years ago

The protocol specifies a close message should be send which we do not do.

Yep, would probably have to do be done explicitly in this case instead of letting it bubble upwards. It's also a minor but still unintended information leak towards the server.

Security impact: Minor, leaks information that no task could be found to the server (although the server is most likely able to extract that information by just counting messages as well).

(The spec is kinda in conflict with itself here, as it in the section of the close message state it must only be send after a successful client handshake).

Agree that there is a slight bug in the spec. On crypto level, authentication is complete once the cookie has been validated, so it's safe to send any messages now, and therefore 'close' messages are no exception. However, the spec does not clearly state that validating the cookie must take place before doing the rest of the list. But it was meant as a strictly ordered list of things to do and in that order. I hope this makes it clearer.

Furthermore the spec states we should close the connection to the server afterwards, as far as I understand this is because at this point in time we already did new where where "correctly" connected with the "right" client (responder) we tried to connect to, but we are fundamentally incompatible. (So no point in still trying to connect to it again or to other responders.)

Correct, there is no point in continuing.