search

saltyrtc / saltyrtc-client-java

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

Cookie of c2c handshake is set late. #135

Open rustonaut opened 2 years ago

rustonaut commented 2 years ago

The line(s) below sets the cookie of the c2c peer when receiving the auth message.

For initiator:

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

For responder:

https://github.com/saltyrtc/saltyrtc-client-java/blob/d01e553c06a05c74a532104608c0dc57cb9a23f2/src/main/java/org/saltyrtc/client/signaling/ResponderSignaling.java#L310

But as far as I understand the cookie should be set when first receiving a message from a client (independent of weather it's from/as the Server, Responder or Initiator). But before receiving the auth the Initiator/Responder do receive a Key and potential Token message, so I think the cookie should be set when receiving Token (or Key if no Token message is send/expected).

Details

Spec

In case this is the first message received from the sender, the peer: [...]

  • MUST store cookie, overflow number and sequence number (or the combined sequence number) for checks on further messages.
lgrahl commented 2 years ago

This is clearly a bug and the security impact is actually quite hard to estimate and I'll have to think hard about that. I'm not sure if the cookie is also checked elsewhere in the code.

It also clearly shows that always sending the cookie is in hindsight a mistake in the spec, encouraging such errors.

lgrahl commented 2 years ago

I couldn't really think of a plausible attack scenario so far but due to its complexity, we should fix this ASAP nevertheless.

rustonaut commented 2 years ago

I couldn't really think of a plausible attack scenario so far but due to its complexity, [..]

Same for me :+1:

I'm not a security expert so I didn't add any speculations about security implications to the issue when I opened it :wink:

rustonaut commented 2 years ago

Uhm how :scream:

Sorry, I'm not sure what happened.

I just wanted to comment that for the s2c handshake the cookie is set correctly, but then though it's unnecessary and deleted the comment. I have no idea how this closed the thread.

lgrahl commented 2 years ago

No worries. :slightly_smiling_face: