Protocol Review

[lgrahl]

I have reviewed most of the protocol-related code to get an overview of the Rust client implementation. I identified the following potential issues that may need addressing (some definitely do, some are more of a question).

It may make sense to treat this as an umbrella issue and file separate issues for each when they are being tackled.

Findings

• 1: Accept destination 0x00 indefinitely.
  • Location:

    saltyrtc-client-rs/src/protocol/mod.rs

    Lines 1003 to 1030 in 5f35c42

    	fn validate_nonce_destination(&mut self, nonce: &Nonce) -> Result<(), ValidationError> {
    	// A client MUST check that the destination address targets its
    	// assigned identity (or `0x00` during authentication).
    	if self.identity() == ClientIdentity::Unknown
    	&& !nonce.destination().is_unknown()
    	&& self.server_handshake_state() != ServerHandshakeState::New {
    	// The first message received with a destination address different
    	// to `0x00` SHALL be accepted as the client's assigned identity.
    	// However, the client MUST validate that the identity fits its
    	// role – initiators SHALL ONLY accept `0x01`. The identity MUST
    	// be stored as the client's assigned identity.
    	if nonce.destination().is_initiator() {
    	self.common.identity = ClientIdentity::Initiator;
    	debug!("Assigned identity: {}", self.identity());
    	} else {
    	return Err(ValidationError::Fail(
    	format!("cannot assign address {} to initiator", nonce.destination())
    	));
    	};
    	}
    	if nonce.destination() != self.identity().into() {
    	return Err(ValidationError::Fail(
    	format!("Bad destination: {} (our identity is {})", nonce.destination(), self.identity())
    	));
    	}
    	Ok(())
    	}

    and

    saltyrtc-client-rs/src/protocol/mod.rs

    Lines 1636 to 1664 in 5f35c42

    	fn validate_nonce_destination(&mut self, nonce: &Nonce) -> Result<(), ValidationError> {
    	// A client MUST check that the destination address targets its
    	// assigned identity (or `0x00` during authentication).
    	if self.identity() == ClientIdentity::Unknown
    	&& !nonce.destination().is_unknown()
    	&& self.server_handshake_state() != ServerHandshakeState::New {
    	// The first message received with a destination address different
    	// to `0x00` SHALL be accepted as the client's assigned identity.
    	// However, the client MUST validate that the identity fits its
    	// role – responders SHALL ONLY an identity from the range
    	// `0x02..0xff`. The identity MUST be stored as the client's
    	// assigned identity.
    	if nonce.destination().is_responder() {
    	self.common.identity = ClientIdentity::Responder(nonce.destination().0);
    	debug!("Assigned identity: {}", self.identity());
    	} else {
    	return Err(ValidationError::Fail(
    	format!("cannot assign address {} to a responder", nonce.destination())
    	));
    	};
    	}
    	if nonce.destination() != self.identity().into() {
    	return Err(ValidationError::Fail(
    	format!("Bad destination: {} (our identity is {})", nonce.destination(), self.identity())
    	));
    	}
    	Ok(())
    	}

  • Proposal: None, yet. Unsure if this needs resolving.
• 2: Reacts with protocol error when receiving a message from an unknown responder.
  • Fix in PR Ignore an unknown responder #59
• 3: Does not remove the initiator/responder instance when it has received a 'disconnected' message.
  • Location:

    saltyrtc-client-rs/src/protocol/mod.rs

    Lines 1238 to 1250 in 5f35c42

    	fn handle_disconnected(&mut self, msg: Disconnected) -> SignalingResult<Vec<HandleAction>> {
    	debug!("--> Received disconnected from server");
    	// An initiator who receives a 'disconnected' message SHALL validate
    	// that the id field contains a valid responder address (0x02..0xff).
    	if !msg.id.is_responder() {
    	return Err(SignalingError::Protocol(
    	"Received 'disconnected' message with non-responder id".into()
    	));
    	}
    	Ok(vec![HandleAction::Event(Event::Disconnected(msg.id.0))])
    	}

    and

    saltyrtc-client-rs/src/protocol/mod.rs

    Lines 1833 to 1845 in 5f35c42

    	fn handle_disconnected(&mut self, msg: Disconnected) -> SignalingResult<Vec<HandleAction>> {
    	debug!("--> Received disconnected from server");
    	// A responder who receives a 'disconnected' message SHALL validate
    	// that the id field contains a valid initiator address (0x01).
    	if !msg.id.is_initiator() {
    	return Err(SignalingError::Protocol(
    	"Received 'disconnected' message with non-initiator id".into()
    	));
    	}
    	Ok(vec![HandleAction::Event(Event::Disconnected(msg.id.0))])
    	}

  • Proposal: Remove the instance. Tear down the task if it has already kicked in. (If the implementation cannot cope with further connections after the task kicked in, it may need to close.)
• 4: When a new initiator is connecting via 'new-initiator', a responder simply replaces the initiator instance but does not reset its own state.
  • Location:

    saltyrtc-client-rs/src/protocol/mod.rs

    Lines 1790 to 1826 in 5f35c42

    	fn handle_new_initiator(&mut self, _msg: NewInitiator) -> SignalingResult<Vec<HandleAction>> {
    	debug!("--> Received new-initiator from server");
    	let mut actions: Vec<HandleAction> = vec![];
    	// A responder who receives a 'new-initiator' message MUST proceed by
    	// deleting all currently cached information about and for the previous
    	// initiator (such as cookies and the sequence numbers)...
    	self.initiator = InitiatorContext::new(self.initiator.permanent_key);
    	// ...and continue by sending a 'token' or 'key' client-to-client
    	// message described in the Client-to-Client Messages section.
    	let mut send_token = false;
    	match self.common().auth_provider {
    	Some(AuthProvider::Token(_)) => {
    	send_token = true;
    	},
    	Some(AuthProvider::TrustedKey(_)) => {
    	debug!("Trusted key available, skipping token message");
    	},
    	None => {
    	return Err(SignalingError::Crash("No auth provider set".into()));
    	},
    	}
    	if send_token {
    	let old_auth_provider = mem::replace(&mut self.common_mut().auth_provider, None);
    	if let Some(AuthProvider::Token(token)) = old_auth_provider {
    	actions.push(self.send_token(token)?);
    	} else {
    	return Err(SignalingError::Crash("Auth provider is not a token".into()));
    	}
    	}
    	actions.push(self.send_key()?);
    	self.initiator.set_handshake_state(InitiatorHandshakeState::KeySent);
    	Ok(actions)
    	}

  • Issue: Handle new-initiator/new-responder correctly #61
  • Proposal: Close the connection immediately if the peer handshake has already been completed as this implementation cannot cope with multiple initiators. This must prevent any further interaction with/from the task.
• 5: When a new responder is connecting via 'new-responder' with the same identity, an initiator simply replaces the responder instance but does not reset its own state.
  • Location:

    saltyrtc-client-rs/src/protocol/mod.rs

    Lines 1218 to 1235 in 5f35c42

    	/// Handle an incoming [`NewResponder`](messages/struct.NewResponder.html) message.
    	fn handle_new_responder(&mut self, msg: NewResponder) -> SignalingResult<Vec<HandleAction>> {
    	debug!("--> Received new-responder ({}) from server", msg.id);
    	// An initiator who receives a 'new-responder' message SHALL validate
    	// that the id field contains a valid responder address (0x02..0xff).
    	if !msg.id.is_responder() {
    	return Err(SignalingError::InvalidMessage(
    	"`id` field in new-responder message is not a valid responder address".into()
    	));
    	}
    	// Process responder
    	match self.process_new_responder(msg.id)? {
    	Some(drop_responder) => Ok(vec![drop_responder]),
    	None => Ok(vec![]),
    	}
    	}

  • Issue: Handle new-initiator/new-responder correctly #61
  • Proposal: Drop new responders after the peer handshake has been completed. Do not process them.
• 6: Peer messages are being handled when the server handshake is in progress.
  • Location:

    saltyrtc-client-rs/src/protocol/mod.rs

    Line 314 in 5f35c42

    SignalingState::ServerHandshake => self.handle_handshake_peer_message(bbox),

  • Proposal: Return a protocol error instead.
• 7: When a peer message is being handled, it does a sanity-check to ensure the message is not being sent from a server. But later, it tries to forward server messages.
  • Location:

    saltyrtc-client-rs/src/protocol/mod.rs

    Lines 356 to 357 in 5f35c42

    	SignalingState::PeerHandshake if obox.nonce.source().is_server() =>
    	self.handle_server_message(obox, None),

  • Proposal: Crash instead of handling the message.
• 8: The task_supported_types seems unnecessary to me. The task should filter itself anyway.

The above checkboxes shall be ticked if the issue has been confirmed, resolved and a fix, including a test, has been merged, or alternatively in case it has been identified as a non-issue in which case an explanation needs to be added.

Notes

Having reviewed the JS and Java client as well, Cpt. Hindsight says that we should avoid using separate functions for handling inbound messages. The dispatching logic depending on states has been a weak point in all three clients and made the control flow complicated to follow. Alternative solutions would have to be explored.

[dbrgn]

Regarding 3, the spec says:

A receiving client MUST notify the user application about the incoming 'disconnected' message, along with the id field.

If the client should react to these messages with more than just an event, we should probably put that in the spec. What do you think?

[dbrgn]

Regarding 6:

Proposal: Return a protocol error instead.

I agree. Not sure why I made that mistake.

[dbrgn]

Regarding 7:

Proposal: Crash instead of handling the message.

I agree.

[dbrgn]

Regarding 8:

The task_supported_types seems unnecessary to me.

I believe this was an optimization to avoid having to lock the Arc<Mutex<_>> around the task just for checking whether the message type is supported by the task or not.

The task should filter itself anyway.

That's also an option. Since the message type is a string, the task needs to handle unsupported types anyways.

[lgrahl]

Regarding 3, the spec says:

A receiving client MUST notify the user application about the incoming 'disconnected' message, along with the id field.

If the client should react to these messages with more than just an event, we should probably put that in the spec. What do you think?

Agree it could be more precise. Filed saltyrtc/saltyrtc-meta#158. However, it should be clear that keeping a dangling instance makes no sense.