feat: add header integrity check and replay protection to control messages#1740
feat: add header integrity check and replay protection to control messages#1740markpmarton wants to merge 19 commits into
Conversation
…l messages Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
…l messages Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
…l messages Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
Signed-off-by: Mark Marton <30534230+markpmarton@users.noreply.github.com>
Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
Signed-off-by: Mark Marton <30534230+markpmarton@users.noreply.github.com>
Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
62f81dc to
9c2ec9e
Compare
Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
Signed-off-by: Mark Marton <mark.p.marton@gmail.com>
| session_ctx, | ||
| ))) => return session_ctx, | ||
| Some(Ok(_)) => continue, | ||
| Some(Err(e)) => { |
| ); | ||
| // Require E2E verification only when the sender included a signature (matches pre-session path). | ||
| let e2e_required = | ||
| is_post_session_control && msg.get_slim_header().e2e_header_sig.is_some(); |
There was a problem hiding this comment.
this should be always mandatory otherwise we can always send fake packets with e2e_header_sig set to None
| optional uint64 incoming_conn = 9; | ||
| optional bool error = 10; | ||
| optional bytes header_mac = 11; | ||
| optional uint64 sequence_number = 12; |
There was a problem hiding this comment.
why do we need a new sequence_number here? isn't enough to use the message_id in session header?
There was a problem hiding this comment.
AFAIK message_id is getting a random u32 for control messages, that is not viable for replay protection. Maybe it can be changed to a monotonic counter, I'm going to check it.
| let seen = seen_control_seqs.entry(sender.clone()).or_default(); | ||
| if !seen.insert(s) { | ||
| // Duplicate delivery (network retransmission or fanout). Handlers are idempotent. | ||
| return Ok(()); |
There was a problem hiding this comment.
I am not sure here, but should we drop the message here instead of return ok? If it is in seen it means that the message was already processed. Notice that if we use the message_id in the session header RTX message will have a different id with respect to the original message so the two messages will be handled as two different messages.
| } | ||
| Err(e) => { | ||
| debug!( | ||
| tracing::error!( |
| custom_claims: Option<CustomClaims>, | ||
| } | ||
|
|
||
| let claims_res = verifier.get_claims(&identity).await; |
There was a problem hiding this comment.
here I think we can reuse the IdentityClaims in Auth. This part can be replaced with something like
let claims_json = verifier.get_claims(&identity).await?;
let identity_claims = slim_auth::identity_claims::IdentityClaims::from_json(&claims_json)?;
let pubkey = identity_claims.public_key;
|
|
||
| let aad = crate::mls_state::build_aad(m); | ||
| let private_key = identity_provider.get_signature_secret_key()?; | ||
| let public_key = identity_provider.get_signature_public_key()?; |
There was a problem hiding this comment.
This can be moved out of the loop
let private_key = identity_provider.get_signature_secret_key()?;
let public_key = identity_provider.get_signature_public_key()?;
|
|
||
| if let Err(e) = | ||
| crate::session_controller::verify_identity(&message, &layer.identity_verifier).await | ||
| let e2e_required = message.get_slim_header().e2e_header_sig.is_some(); |
There was a problem hiding this comment.
here this also should always be mandatory
closes #1721
Description
In the current implementation only publish message headers are e2e validated because it is built around the MLS payload encryption. To prevent malicious control message replay attacks, we need to add manual integrity check for pre- and post-session messages.
Changes
sequence_numberadded for replay protectione2e_header_sigadded for header fields (signed by MLS key)Type of Change
Checklist