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

Cookie of c2c handshake is set late. #135

Open
rustonaut opened this issue Sep 14, 2021 · 5 comments
Open

Cookie of c2c handshake is set late. #135

rustonaut opened this issue Sep 14, 2021 · 5 comments

Comments

@rustonaut
Copy link

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

For initiator:

responder.getCookiePair().setTheirs(nonce.getCookie());

For responder:

this.initiator.getCookiePair().setTheirs(nonce.getCookie());

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
Copy link
Member

lgrahl commented Sep 15, 2021

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
Copy link
Member

lgrahl commented Sep 15, 2021

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

@rustonaut
Copy link
Author

rustonaut commented Sep 15, 2021

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

Same for me 👍

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

@rustonaut
Copy link
Author

rustonaut commented Sep 15, 2021

Uhm how 😱

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.

@rustonaut rustonaut reopened this Sep 15, 2021
@lgrahl
Copy link
Member

lgrahl commented Sep 15, 2021

No worries. 🙂

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

No branches or pull requests

2 participants