Attempt to fix race in securejoin handling

The ongoing process of dc_join_securejoin() was stopped before the
corresponding chat was created.  This resulted in a race-condition
between the sqlite threads executing the creation and query
statements, thus sometimes the query would not find the group and
mysteriously fail.

Tripple-programming with hpk & r10s.
This commit is contained in:
Floris Bruynooghe
2019-11-30 00:01:20 +01:00
committed by holger krekel
parent e4b2fd87de
commit 642276c90c
2 changed files with 83 additions and 44 deletions

View File

@@ -392,6 +392,8 @@ unsafe fn add_parts(
MessageState::InFresh MessageState::InFresh
}; };
*to_id = DC_CONTACT_ID_SELF; *to_id = DC_CONTACT_ID_SELF;
let mut needs_stop_ongoing_process = false;
// handshake messages must be processed _before_ chats are created // handshake messages must be processed _before_ chats are created
// (eg. contacs may be marked as verified) // (eg. contacs may be marked as verified)
if mime_parser.lookup_field("Secure-Join").is_some() { if mime_parser.lookup_field("Secure-Join").is_some() {
@@ -399,11 +401,24 @@ unsafe fn add_parts(
msgrmsg = 1; msgrmsg = 1;
*chat_id = 0; *chat_id = 0;
allow_creation = 1; allow_creation = 1;
let handshake = handle_securejoin_handshake(context, mime_parser, *from_id); match handle_securejoin_handshake(context, mime_parser, *from_id) {
if 0 != handshake & DC_HANDSHAKE_STOP_NORMAL_PROCESSING { Ok(ret) => {
*hidden = 1; if ret.hide_this_msg {
*needs_delete_job = 0 != handshake & DC_HANDSHAKE_ADD_DELETE_JOB; *hidden = 1;
state = MessageState::InSeen; *needs_delete_job = ret.delete_this_msg;
state = MessageState::InSeen;
}
if let Some(status) = ret.bob_securejoin_success {
context.bob.write().unwrap().status = status as i32;
}
needs_stop_ongoing_process = ret.stop_ongoing_process;
}
Err(err) => {
warn!(
context,
"Unexpected messaged passed to Secure-Join handshake protocol: {}", err
);
}
} }
} }
@@ -501,6 +516,13 @@ unsafe fn add_parts(
{ {
state = MessageState::InNoticed; state = MessageState::InNoticed;
} }
if needs_stop_ongoing_process {
// The Secure-Join protocol finished and the group
// creation handling is done. Stopping the ongoing
// process will let dc_join_securejoin() return.
context.stop_ongoing();
}
} else { } else {
// Outgoing // Outgoing

View File

@@ -318,21 +318,40 @@ fn fingerprint_equals_sender(
false false
} }
pub(crate) struct HandshakeMessageStatus {
pub(crate) hide_this_msg: bool,
pub(crate) delete_this_msg: bool,
pub(crate) stop_ongoing_process: bool,
pub(crate) bob_securejoin_success: Option<bool>,
}
impl Default for HandshakeMessageStatus {
fn default() -> Self {
Self {
hide_this_msg: true,
delete_this_msg: false,
stop_ongoing_process: false,
bob_securejoin_success: None,
}
}
}
/* library private: secure-join */ /* library private: secure-join */
pub fn handle_securejoin_handshake( pub(crate) fn handle_securejoin_handshake(
context: &Context, context: &Context,
mimeparser: &MimeParser, mimeparser: &MimeParser,
contact_id: u32, contact_id: u32,
) -> libc::c_int { ) -> Result<HandshakeMessageStatus, Error> {
let own_fingerprint: String; let own_fingerprint: String;
if contact_id <= DC_CONTACT_ID_LAST_SPECIAL { ensure!(
return 0; contact_id > DC_CONTACT_ID_LAST_SPECIAL,
} "handle_securejoin_handshake(): called with special contact id"
);
let step = match mimeparser.lookup_optional_field("Secure-Join") { let step = match mimeparser.lookup_optional_field("Secure-Join") {
Some(s) => s, Some(s) => s,
None => { None => {
return 0; bail!("This message is not a Secure-Join message");
} }
}; };
info!( info!(
@@ -345,8 +364,8 @@ pub fn handle_securejoin_handshake(
if contact_chat_id_blocked != Blocked::Not { if contact_chat_id_blocked != Blocked::Not {
chat::unblock(context, contact_chat_id); chat::unblock(context, contact_chat_id);
} }
let mut ret: libc::c_int = DC_HANDSHAKE_STOP_NORMAL_PROCESSING;
let join_vg = step.starts_with("vg-"); let join_vg = step.starts_with("vg-");
let mut ret = HandshakeMessageStatus::default();
match step.as_str() { match step.as_str() {
"vg-request" | "vc-request" => { "vg-request" | "vc-request" => {
@@ -362,12 +381,12 @@ pub fn handle_securejoin_handshake(
Some(n) => n, Some(n) => n,
None => { None => {
warn!(context, "Secure-join denied (invitenumber missing).",); warn!(context, "Secure-join denied (invitenumber missing).",);
return ret; return Ok(ret);
} }
}; };
if !token::exists(context, token::Namespace::InviteNumber, &invitenumber) { if !token::exists(context, token::Namespace::InviteNumber, &invitenumber) {
warn!(context, "Secure-join denied (bad invitenumber).",); warn!(context, "Secure-join denied (bad invitenumber).",);
return ret; return Ok(ret);
} }
info!(context, "Secure-join requested.",); info!(context, "Secure-join requested.",);
@@ -393,7 +412,7 @@ pub fn handle_securejoin_handshake(
if cond { if cond {
warn!(context, "auth-required message out of sync.",); warn!(context, "auth-required message out of sync.",);
// no error, just aborted somehow or a mail from another handshake // no error, just aborted somehow or a mail from another handshake
return ret; return Ok(ret);
} }
let scanned_fingerprint_of_alice = get_qr_attr!(context, fingerprint).to_string(); let scanned_fingerprint_of_alice = get_qr_attr!(context, fingerprint).to_string();
let auth = get_qr_attr!(context, auth).to_string(); let auth = get_qr_attr!(context, auth).to_string();
@@ -408,8 +427,9 @@ pub fn handle_securejoin_handshake(
"Not encrypted." "Not encrypted."
}, },
); );
end_bobs_joining(context, DC_BOB_ERROR); ret.stop_ongoing_process = true;
return ret; ret.bob_securejoin_success = Some(false);
return Ok(ret);
} }
if !fingerprint_equals_sender(context, &scanned_fingerprint_of_alice, contact_chat_id) { if !fingerprint_equals_sender(context, &scanned_fingerprint_of_alice, contact_chat_id) {
could_not_establish_secure_connection( could_not_establish_secure_connection(
@@ -417,8 +437,9 @@ pub fn handle_securejoin_handshake(
contact_chat_id, contact_chat_id,
"Fingerprint mismatch on joiner-side.", "Fingerprint mismatch on joiner-side.",
); );
end_bobs_joining(context, DC_BOB_ERROR); ret.stop_ongoing_process = true;
return ret; ret.bob_securejoin_success = Some(false);
return Ok(ret);
} }
info!(context, "Fingerprint verified.",); info!(context, "Fingerprint verified.",);
own_fingerprint = get_self_fingerprint(context).unwrap(); own_fingerprint = get_self_fingerprint(context).unwrap();
@@ -453,7 +474,7 @@ pub fn handle_securejoin_handshake(
contact_chat_id, contact_chat_id,
"Fingerprint not provided.", "Fingerprint not provided.",
); );
return ret; return Ok(ret);
} }
}; };
if !encrypted_and_signed(mimeparser, &fingerprint) { if !encrypted_and_signed(mimeparser, &fingerprint) {
@@ -462,7 +483,7 @@ pub fn handle_securejoin_handshake(
contact_chat_id, contact_chat_id,
"Auth not encrypted.", "Auth not encrypted.",
); );
return ret; return Ok(ret);
} }
if !fingerprint_equals_sender(context, &fingerprint, contact_chat_id) { if !fingerprint_equals_sender(context, &fingerprint, contact_chat_id) {
could_not_establish_secure_connection( could_not_establish_secure_connection(
@@ -470,7 +491,7 @@ pub fn handle_securejoin_handshake(
contact_chat_id, contact_chat_id,
"Fingerprint mismatch on inviter-side.", "Fingerprint mismatch on inviter-side.",
); );
return ret; return Ok(ret);
} }
info!(context, "Fingerprint verified.",); info!(context, "Fingerprint verified.",);
// verify that the `Secure-Join-Auth:`-header matches the secret written to the QR code // verify that the `Secure-Join-Auth:`-header matches the secret written to the QR code
@@ -482,12 +503,12 @@ pub fn handle_securejoin_handshake(
contact_chat_id, contact_chat_id,
"Auth not provided.", "Auth not provided.",
); );
return ret; return Ok(ret);
} }
}; };
if !token::exists(context, token::Namespace::Auth, &auth_0) { if !token::exists(context, token::Namespace::Auth, &auth_0) {
could_not_establish_secure_connection(context, contact_chat_id, "Auth invalid."); could_not_establish_secure_connection(context, contact_chat_id, "Auth invalid.");
return ret; return Ok(ret);
} }
if mark_peer_as_verified(context, fingerprint).is_err() { if mark_peer_as_verified(context, fingerprint).is_err() {
could_not_establish_secure_connection( could_not_establish_secure_connection(
@@ -495,7 +516,7 @@ pub fn handle_securejoin_handshake(
contact_chat_id, contact_chat_id,
"Fingerprint mismatch on inviter-side.", "Fingerprint mismatch on inviter-side.",
); );
return ret; return Ok(ret);
} }
Contact::scaleup_origin_by_id(context, contact_id, Origin::SecurejoinInvited); Contact::scaleup_origin_by_id(context, contact_id, Origin::SecurejoinInvited);
info!(context, "Auth verified.",); info!(context, "Auth verified.",);
@@ -509,7 +530,7 @@ pub fn handle_securejoin_handshake(
let (group_chat_id, _, _) = chat::get_chat_id_by_grpid(context, &field_grpid); let (group_chat_id, _, _) = chat::get_chat_id_by_grpid(context, &field_grpid);
if group_chat_id == 0 { if group_chat_id == 0 {
error!(context, "Chat {} not found.", &field_grpid); error!(context, "Chat {} not found.", &field_grpid);
return ret; return Ok(ret);
} else { } else {
if let Err(err) = if let Err(err) =
chat::add_contact_to_chat_ex(context, group_chat_id, contact_id, true) chat::add_contact_to_chat_ex(context, group_chat_id, contact_id, true)
@@ -524,11 +545,11 @@ pub fn handle_securejoin_handshake(
} }
"vg-member-added" | "vc-contact-confirm" => { "vg-member-added" | "vc-contact-confirm" => {
if join_vg { if join_vg {
ret = DC_HANDSHAKE_CONTINUE_NORMAL_PROCESSING; ret.hide_this_msg = false;
} }
if context.bob.read().unwrap().expects != DC_VC_CONTACT_CONFIRM { if context.bob.read().unwrap().expects != DC_VC_CONTACT_CONFIRM {
info!(context, "Message belongs to a different handshake.",); info!(context, "Message belongs to a different handshake.",);
return ret; return Ok(ret);
} }
let cond = { let cond = {
let bob = context.bob.read().unwrap(); let bob = context.bob.read().unwrap();
@@ -540,7 +561,7 @@ pub fn handle_securejoin_handshake(
context, context,
"Message out of sync or belongs to a different handshake.", "Message out of sync or belongs to a different handshake.",
); );
return ret; return Ok(ret);
} }
let scanned_fingerprint_of_alice = get_qr_attr!(context, fingerprint).to_string(); let scanned_fingerprint_of_alice = get_qr_attr!(context, fingerprint).to_string();
@@ -564,8 +585,8 @@ pub fn handle_securejoin_handshake(
contact_chat_id, contact_chat_id,
"Contact confirm message not encrypted.", "Contact confirm message not encrypted.",
); );
end_bobs_joining(context, DC_BOB_ERROR); ret.bob_securejoin_success = Some(false);
return ret; return Ok(ret);
} }
if mark_peer_as_verified(context, &scanned_fingerprint_of_alice).is_err() { if mark_peer_as_verified(context, &scanned_fingerprint_of_alice).is_err() {
@@ -574,7 +595,7 @@ pub fn handle_securejoin_handshake(
contact_chat_id, contact_chat_id,
"Fingerprint mismatch on joiner-side.", "Fingerprint mismatch on joiner-side.",
); );
return ret; return Ok(ret);
} }
Contact::scaleup_origin_by_id(context, contact_id, Origin::SecurejoinJoined); Contact::scaleup_origin_by_id(context, contact_id, Origin::SecurejoinJoined);
emit_event!(context, Event::ContactsChanged(None)); emit_event!(context, Event::ContactsChanged(None));
@@ -583,7 +604,7 @@ pub fn handle_securejoin_handshake(
.unwrap_or_default(); .unwrap_or_default();
if join_vg && !addr_equals_self(context, cg_member_added) { if join_vg && !addr_equals_self(context, cg_member_added) {
info!(context, "Message belongs to a different handshake (scaled up contact anyway to allow creation of group)."); info!(context, "Message belongs to a different handshake (scaled up contact anyway to allow creation of group).");
return ret; return Ok(ret);
} }
secure_connection_established(context, contact_chat_id); secure_connection_established(context, contact_chat_id);
context.bob.write().unwrap().expects = 0; context.bob.write().unwrap().expects = 0;
@@ -597,7 +618,8 @@ pub fn handle_securejoin_handshake(
"", "",
); );
} }
end_bobs_joining(context, DC_BOB_SUCCESS); ret.stop_ongoing_process = true;
ret.bob_securejoin_success = Some(true);
} }
"vg-member-added-received" => { "vg-member-added-received" => {
/* ============================================================ /* ============================================================
@@ -607,7 +629,7 @@ pub fn handle_securejoin_handshake(
if let Ok(contact) = Contact::get_by_id(context, contact_id) { if let Ok(contact) = Contact::get_by_id(context, contact_id) {
if contact.is_verified(context) == VerifiedStatus::Unverified { if contact.is_verified(context) == VerifiedStatus::Unverified {
warn!(context, "vg-member-added-received invalid.",); warn!(context, "vg-member-added-received invalid.",);
return ret; return Ok(ret);
} }
inviter_progress!(context, contact_id, 800); inviter_progress!(context, contact_id, 800);
inviter_progress!(context, contact_id, 1000); inviter_progress!(context, contact_id, 1000);
@@ -621,22 +643,17 @@ pub fn handle_securejoin_handshake(
}); });
} else { } else {
warn!(context, "vg-member-added-received invalid.",); warn!(context, "vg-member-added-received invalid.",);
return ret; return Ok(ret);
} }
} }
_ => { _ => {
warn!(context, "invalid step: {}", step); warn!(context, "invalid step: {}", step);
} }
} }
if ret == DC_HANDSHAKE_STOP_NORMAL_PROCESSING { if ret.hide_this_msg {
ret |= DC_HANDSHAKE_ADD_DELETE_JOB; ret.delete_this_msg = true;
} }
ret Ok(ret)
}
fn end_bobs_joining(context: &Context, status: libc::c_int) {
context.bob.write().unwrap().status = status;
context.stop_ongoing();
} }
fn secure_connection_established(context: &Context, contact_chat_id: u32) { fn secure_connection_established(context: &Context, contact_chat_id: u32) {