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

[rustonaut]

saltyrtc-client-java/src/main/java/org/saltyrtc/client/signaling/InitiatorSignaling.java

Lines 491 to 493 in d01e553

	if (task == null) {
	throw new SignalingException(CloseCode.NO_SHARED_TASK, "No shared task could be found");
	}

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:

saltyrtc-client-java/src/main/java/org/saltyrtc/client/signaling/Signaling.java

Lines 449 to 452 in d01e553

	case PEER_HANDSHAKE:
	// Handle error depending on role
	Signaling.this.handlePeerHandshakeSignalingError(e, nonce.getSource());
	break;

saltyrtc-client-java/src/main/java/org/saltyrtc/client/signaling/InitiatorSignaling.java

Lines 86 to 97 in d01e553

	synchronized void handlePeerHandshakeSignalingError(@NonNull SignalingException e, short source) {
	// Simply drop the responder
	Responder responder = this.responders.get(source);
	if (responder != null) {
	try {
	this.dropResponder(responder.getId(), e.getCloseCode());
	} catch (SignalingException | ConnectionException ee) {
	ee.printStackTrace();
	// Ignore, we're handling these errors already
	}
	}
	}

[rustonaut]

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). 🤔

[lgrahl]

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.