From 39cbe5f31fc9c2dfb3d8570767c31a2a5bd1f89b Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 14 Oct 2020 17:34:11 +0200 Subject: [PATCH] Add method verify_activity_domains_valid() (ref #1196) --- docs/src/contributing_apub_api_outline.md | 2 +- lemmy_apub/src/activities/receive/mod.rs | 36 +++++- lemmy_apub/src/extensions/signatures.rs | 2 +- lemmy_apub/src/fetcher.rs | 2 +- lemmy_apub/src/inbox/community_inbox.rs | 30 +++-- lemmy_apub/src/inbox/shared_inbox.rs | 4 +- lemmy_apub/src/inbox/user_inbox.rs | 151 ++++++++++------------ lemmy_apub/src/lib.rs | 22 +--- lemmy_apub/src/objects/comment.rs | 5 +- lemmy_apub/src/objects/community.rs | 5 +- lemmy_apub/src/objects/mod.rs | 28 +++- lemmy_apub/src/objects/post.rs | 5 +- lemmy_apub/src/objects/private_message.rs | 5 +- lemmy_apub/src/objects/user.rs | 4 +- 14 files changed, 162 insertions(+), 139 deletions(-) diff --git a/docs/src/contributing_apub_api_outline.md b/docs/src/contributing_apub_api_outline.md index ae7f21cf..af3eb0f2 100644 --- a/docs/src/contributing_apub_api_outline.md +++ b/docs/src/contributing_apub_api_outline.md @@ -597,7 +597,7 @@ Sent to: User "type": "Delete", "actor": "https://ds9.lemmy.ml/u/sisko", "to": "https://enterprise.lemmy.ml/u/riker/inbox", - "object": "https://enterprise.lemmy.ml/private_message/341" + "object": "https://ds9.lemmy.ml/private_message/341" } ``` diff --git a/lemmy_apub/src/activities/receive/mod.rs b/lemmy_apub/src/activities/receive/mod.rs index 730454b4..ab830ca7 100644 --- a/lemmy_apub/src/activities/receive/mod.rs +++ b/lemmy_apub/src/activities/receive/mod.rs @@ -4,7 +4,8 @@ use crate::{ }; use activitystreams::{ activity::{ActorAndObjectRef, ActorAndObjectRefExt}, - base::{AsBase, Extends, ExtendsExt}, + base::{AsBase, BaseExt, Extends, ExtendsExt}, + error::DomainError, object::{AsObject, ObjectExt}, }; use actix_web::HttpResponse; @@ -121,3 +122,36 @@ pub(crate) async fn find_by_id( return Err(NotFound.into()); } + +pub(crate) fn verify_activity_domains_valid( + activity: &T, + actor_id: Url, + object_domain_must_match: bool, +) -> Result<(), LemmyError> +where + T: AsBase + ActorAndObjectRef, +{ + let expected_domain = actor_id.domain().context(location_info!())?; + + activity.id(expected_domain)?; + + let object_id = match activity.object().to_owned().single_xsd_any_uri() { + // object is just an ID + Some(id) => id, + // object is something like an activity, a comment or a post + None => activity + .object() + .to_owned() + .one() + .context(location_info!())? + .id() + .context(location_info!())? + .to_owned(), + }; + + if object_domain_must_match && object_id.domain() != Some(expected_domain) { + return Err(DomainError.into()); + } + + Ok(()) +} diff --git a/lemmy_apub/src/extensions/signatures.rs b/lemmy_apub/src/extensions/signatures.rs index cdcb707a..8e8c31e4 100644 --- a/lemmy_apub/src/extensions/signatures.rs +++ b/lemmy_apub/src/extensions/signatures.rs @@ -63,7 +63,7 @@ pub async fn sign_and_send( Ok(response) } -pub fn verify(request: &HttpRequest, actor: &dyn ActorType) -> Result<(), LemmyError> { +pub fn verify_signature(request: &HttpRequest, actor: &dyn ActorType) -> Result<(), LemmyError> { let public_key = actor.public_key().context(location_info!())?; let verified = CONFIG2 .begin_verify( diff --git a/lemmy_apub/src/fetcher.rs b/lemmy_apub/src/fetcher.rs index bd85ddc8..986c9dd4 100644 --- a/lemmy_apub/src/fetcher.rs +++ b/lemmy_apub/src/fetcher.rs @@ -93,7 +93,7 @@ pub async fn search_by_apub_id( ) -> Result { // Parse the shorthand query url let query_url = if query.contains('@') { - debug!("{}", query); + debug!("Search for {}", query); let split = query.split('@').collect::>(); // User type will look like ['', username, instance] diff --git a/lemmy_apub/src/inbox/community_inbox.rs b/lemmy_apub/src/inbox/community_inbox.rs index 5def86c2..2c36c684 100644 --- a/lemmy_apub/src/inbox/community_inbox.rs +++ b/lemmy_apub/src/inbox/community_inbox.rs @@ -1,6 +1,7 @@ use crate::{ + activities::receive::verify_activity_domains_valid, check_is_apub_id_valid, - extensions::signatures::verify, + extensions::signatures::verify_signature, fetcher::get_or_fetch_and_upsert_user, insert_activity, ActorType, @@ -48,15 +49,15 @@ pub async fn community_inbox( }) .await??; - if !community.local { - return Err( - anyhow!( - "Received activity is addressed to remote community {}", - &community.actor_id - ) - .into(), - ); + let to = activity + .to() + .context(location_info!())? + .to_owned() + .single_xsd_any_uri(); + if Some(community.actor_id()?) != to { + return Err(anyhow!("Activity delivered to wrong community").into()); } + info!( "Community {} received activity {:?}", &community.name, &activity @@ -75,7 +76,7 @@ pub async fn community_inbox( let user = get_or_fetch_and_upsert_user(&user_uri, &context).await?; - verify(&request, &user)?; + verify_signature(&request, &user)?; let any_base = activity.clone().into_any_base()?; let kind = activity.kind().context(location_info!())?; @@ -98,6 +99,8 @@ async fn handle_follow( context: &LemmyContext, ) -> Result { let follow = Follow::from_any_base(activity)?.context(location_info!())?; + verify_activity_domains_valid(&follow, user.actor_id()?, false)?; + let community_follower_form = CommunityFollowerForm { community_id: community.id, user_id: user.id, @@ -120,7 +123,12 @@ async fn handle_undo_follow( community: Community, context: &LemmyContext, ) -> Result { - let _undo = Undo::from_any_base(activity)?.context(location_info!())?; + let undo = Undo::from_any_base(activity)?.context(location_info!())?; + verify_activity_domains_valid(&undo, user.actor_id()?, true)?; + + let object = undo.object().to_owned().one().context(location_info!())?; + let follow = Follow::from_any_base(object)?.context(location_info!())?; + verify_activity_domains_valid(&follow, user.actor_id()?, false)?; let community_follower_form = CommunityFollowerForm { community_id: community.id, diff --git a/lemmy_apub/src/inbox/shared_inbox.rs b/lemmy_apub/src/inbox/shared_inbox.rs index 5b87e0f8..cdaa0e8f 100644 --- a/lemmy_apub/src/inbox/shared_inbox.rs +++ b/lemmy_apub/src/inbox/shared_inbox.rs @@ -10,7 +10,7 @@ use crate::{ update::receive_update, }, check_is_apub_id_valid, - extensions::signatures::verify, + extensions::signatures::verify_signature, fetcher::get_or_fetch_and_upsert_actor, insert_activity, }; @@ -62,7 +62,7 @@ pub async fn shared_inbox( check_is_apub_id_valid(&actor)?; let actor = get_or_fetch_and_upsert_actor(&actor, &context).await?; - verify(&request, actor.as_ref())?; + verify_signature(&request, actor.as_ref())?; let any_base = activity.clone().into_any_base()?; let kind = activity.kind().context(location_info!())?; diff --git a/lemmy_apub/src/inbox/user_inbox.rs b/lemmy_apub/src/inbox/user_inbox.rs index 7bc36a0f..2ee6d6ed 100644 --- a/lemmy_apub/src/inbox/user_inbox.rs +++ b/lemmy_apub/src/inbox/user_inbox.rs @@ -1,19 +1,20 @@ use crate::{ + activities::receive::verify_activity_domains_valid, check_is_apub_id_valid, - extensions::signatures::verify, + extensions::signatures::verify_signature, fetcher::{get_or_fetch_and_upsert_actor, get_or_fetch_and_upsert_community}, insert_activity, + ActorType, FromApub, }; use activitystreams::{ - activity::{Accept, ActorAndObject, Create, Delete, Undo, Update}, + activity::{Accept, ActorAndObject, Create, Delete, Follow, Undo, Update}, base::AnyBase, - error::DomainError, object::Note, prelude::*, }; use actix_web::{web, HttpRequest, HttpResponse}; -use anyhow::Context; +use anyhow::{anyhow, Context}; use lemmy_db::{ community::{CommunityFollower, CommunityFollowerForm}, private_message::{PrivateMessage, PrivateMessageForm}, @@ -50,6 +51,19 @@ pub async fn user_inbox( ) -> Result { let activity = input.into_inner(); let username = path.into_inner(); + let user = blocking(&context.pool(), move |conn| { + User_::read_from_name(&conn, &username) + }) + .await??; + + let to = activity + .to() + .context(location_info!())? + .to_owned() + .single_xsd_any_uri(); + if Some(user.actor_id()?) != to { + return Err(anyhow!("Activity delivered to wrong user").into()); + } let actor_uri = activity .actor()? @@ -57,7 +71,7 @@ pub async fn user_inbox( .context(location_info!())?; debug!( "User {} inbox received activity {:?} from {}", - username, + user.name, &activity.id_unchecked(), &actor_uri ); @@ -65,16 +79,18 @@ pub async fn user_inbox( check_is_apub_id_valid(actor_uri)?; let actor = get_or_fetch_and_upsert_actor(actor_uri, &context).await?; - verify(&request, actor.as_ref())?; + verify_signature(&request, actor.as_ref())?; let any_base = activity.clone().into_any_base()?; let kind = activity.kind().context(location_info!())?; let res = match kind { - ValidTypes::Accept => receive_accept(any_base, username, &context).await, - ValidTypes::Create => receive_create_private_message(any_base, &context).await, - ValidTypes::Update => receive_update_private_message(any_base, &context).await, - ValidTypes::Delete => receive_delete_private_message(any_base, &context).await, - ValidTypes::Undo => receive_undo_delete_private_message(any_base, &context).await, + ValidTypes::Accept => receive_accept(&context, any_base, actor.as_ref(), user).await, + ValidTypes::Create => receive_create_private_message(&context, any_base, actor.as_ref()).await, + ValidTypes::Update => receive_update_private_message(&context, any_base, actor.as_ref()).await, + ValidTypes::Delete => receive_delete_private_message(&context, any_base, actor.as_ref()).await, + ValidTypes::Undo => { + receive_undo_delete_private_message(&context, any_base, actor.as_ref()).await + } }; insert_activity(actor.user_id(), activity.clone(), false, context.pool()).await?; @@ -83,11 +99,20 @@ pub async fn user_inbox( /// Handle accepted follows. async fn receive_accept( - activity: AnyBase, - username: String, context: &LemmyContext, + activity: AnyBase, + actor: &dyn ActorType, + user: User_, ) -> Result { let accept = Accept::from_any_base(activity)?.context(location_info!())?; + verify_activity_domains_valid(&accept, actor.actor_id()?, false)?; + + // TODO: we should check that we actually sent this activity, because the remote instance + // could just put a fake Follow + let object = accept.object().to_owned().one().context(location_info!())?; + let follow = Follow::from_any_base(object)?.context(location_info!())?; + verify_activity_domains_valid(&follow, user.actor_id()?, false)?; + let community_uri = accept .actor()? .to_owned() @@ -96,11 +121,6 @@ async fn receive_accept( let community = get_or_fetch_and_upsert_community(&community_uri, context).await?; - let user = blocking(&context.pool(), move |conn| { - User_::read_from_name(conn, &username) - }) - .await??; - // Now you need to add this to the community follower let community_follower_form = CommunityFollowerForm { community_id: community.id, @@ -113,15 +133,17 @@ async fn receive_accept( }) .await?; - // TODO: make sure that we actually requested a follow Ok(HttpResponse::Ok().finish()) } async fn receive_create_private_message( - activity: AnyBase, context: &LemmyContext, + activity: AnyBase, + actor: &dyn ActorType, ) -> Result { let create = Create::from_any_base(activity)?.context(location_info!())?; + verify_activity_domains_valid(&create, actor.actor_id()?, true)?; + let note = Note::from_any_base( create .object() @@ -131,18 +153,8 @@ async fn receive_create_private_message( )? .context(location_info!())?; - let actor = create - .actor()? - .to_owned() - .single_xsd_any_uri() - .context(location_info!())?; - let domain = Some( - create - .id(actor.domain().context(location_info!())?)? - .context(location_info!())? - .to_owned(), - ); - let private_message = PrivateMessageForm::from_apub(¬e, context, domain).await?; + let private_message = + PrivateMessageForm::from_apub(¬e, context, Some(actor.actor_id()?)).await?; let inserted_private_message = blocking(&context.pool(), move |conn| { PrivateMessage::create(conn, &private_message) @@ -169,31 +181,22 @@ async fn receive_create_private_message( } async fn receive_update_private_message( - activity: AnyBase, context: &LemmyContext, + activity: AnyBase, + actor: &dyn ActorType, ) -> Result { let update = Update::from_any_base(activity)?.context(location_info!())?; - let note = Note::from_any_base( - update - .object() - .as_one() - .context(location_info!())? - .to_owned(), - )? - .context(location_info!())?; + verify_activity_domains_valid(&update, actor.actor_id()?, true)?; - let actor = update - .actor()? - .to_owned() - .single_xsd_any_uri() - .context(location_info!())?; - let domain = Some( - update - .id(actor.domain().context(location_info!())?)? - .context(location_info!())? - .to_owned(), - ); - let private_message_form = PrivateMessageForm::from_apub(¬e, context, domain).await?; + let object = update + .object() + .as_one() + .context(location_info!())? + .to_owned(); + let note = Note::from_any_base(object)?.context(location_info!())?; + + let private_message_form = + PrivateMessageForm::from_apub(¬e, context, Some(actor.actor_id()?)).await?; let private_message_ap_id = private_message_form .ap_id @@ -232,27 +235,18 @@ async fn receive_update_private_message( } async fn receive_delete_private_message( - activity: AnyBase, context: &LemmyContext, + activity: AnyBase, + actor: &dyn ActorType, ) -> Result { let delete = Delete::from_any_base(activity)?.context(location_info!())?; + verify_activity_domains_valid(&delete, actor.actor_id()?, true)?; + let private_message_id = delete .object() .to_owned() .single_xsd_any_uri() .context(location_info!())?; - let actor = delete - .actor()? - .to_owned() - .single_xsd_any_uri() - .context(location_info!())?; - let delete_id = delete - .id(actor.domain().context(location_info!())?)? - .map(|i| i.domain()) - .flatten(); - if private_message_id.domain() != delete_id { - return Err(DomainError.into()); - } let private_message = blocking(context.pool(), move |conn| { PrivateMessage::read_from_apub_id(conn, private_message_id.as_str()) }) @@ -280,30 +274,21 @@ async fn receive_delete_private_message( } async fn receive_undo_delete_private_message( - activity: AnyBase, context: &LemmyContext, + activity: AnyBase, + actor: &dyn ActorType, ) -> Result { let undo = Undo::from_any_base(activity)?.context(location_info!())?; - let delete = Delete::from_any_base(undo.object().as_one().context(location_info!())?.to_owned())? - .context(location_info!())?; + verify_activity_domains_valid(&undo, actor.actor_id()?, true)?; + let object = undo.object().to_owned().one().context(location_info!())?; + let delete = Delete::from_any_base(object)?.context(location_info!())?; + verify_activity_domains_valid(&delete, actor.actor_id()?, true)?; + let private_message_id = delete .object() .to_owned() .single_xsd_any_uri() .context(location_info!())?; - let actor = undo - .actor()? - .to_owned() - .single_xsd_any_uri() - .context(location_info!())?; - let undo_id = undo - .id(actor.domain().context(location_info!())?)? - .map(|i| i.domain()) - .flatten(); - if private_message_id.domain() != undo_id { - return Err(DomainError.into()); - } - let private_message = blocking(context.pool(), move |conn| { PrivateMessage::read_from_apub_id(conn, private_message_id.as_str()) }) @@ -319,9 +304,7 @@ async fn receive_undo_delete_private_message( .await??; let res = PrivateMessageResponse { message }; - let recipient_id = res.message.recipient_id; - context.chat_server().do_send(SendUserRoomMessage { op: UserOperation::EditPrivateMessage, response: res, diff --git a/lemmy_apub/src/lib.rs b/lemmy_apub/src/lib.rs index d088447e..4c8f9622 100644 --- a/lemmy_apub/src/lib.rs +++ b/lemmy_apub/src/lib.rs @@ -17,10 +17,8 @@ use crate::extensions::{ use activitystreams::{ activity::Follow, actor::{ApActor, Group, Person}, - base::{AnyBase, AsBase}, - markers::Base, + base::AnyBase, object::{Page, Tombstone}, - prelude::*, }; use activitystreams_ext::{Ext1, Ext2}; use anyhow::{anyhow, Context}; @@ -132,24 +130,6 @@ pub trait ApubObjectType { async fn send_undo_remove(&self, mod_: &User_, context: &LemmyContext) -> Result<(), LemmyError>; } -pub(in crate) fn check_actor_domain( - apub: &T, - expected_domain: Option, -) -> Result -where - T: Base + AsBase, -{ - let actor_id = if let Some(url) = expected_domain { - let domain = url.domain().context(location_info!())?; - apub.id(domain)?.context(location_info!())? - } else { - let actor_id = apub.id_unchecked().context(location_info!())?; - check_is_apub_id_valid(&actor_id)?; - actor_id - }; - Ok(actor_id.to_string()) -} - #[async_trait::async_trait(?Send)] pub trait ApubLikeableType { async fn send_like(&self, creator: &User_, context: &LemmyContext) -> Result<(), LemmyError>; diff --git a/lemmy_apub/src/objects/comment.rs b/lemmy_apub/src/objects/comment.rs index 22f22f88..537ab958 100644 --- a/lemmy_apub/src/objects/comment.rs +++ b/lemmy_apub/src/objects/comment.rs @@ -1,11 +1,10 @@ use crate::{ - check_actor_domain, fetcher::{ get_or_fetch_and_insert_comment, get_or_fetch_and_insert_post, get_or_fetch_and_upsert_user, }, - objects::create_tombstone, + objects::{check_object_domain, create_tombstone}, FromApub, ToApub, }; @@ -140,7 +139,7 @@ impl FromApub for CommentForm { published: note.published().map(|u| u.to_owned().naive_local()), updated: note.updated().map(|u| u.to_owned().naive_local()), deleted: None, - ap_id: Some(check_actor_domain(note, expected_domain)?), + ap_id: Some(check_object_domain(note, expected_domain)?), local: false, }) } diff --git a/lemmy_apub/src/objects/community.rs b/lemmy_apub/src/objects/community.rs index 03181718..cf221fa2 100644 --- a/lemmy_apub/src/objects/community.rs +++ b/lemmy_apub/src/objects/community.rs @@ -1,8 +1,7 @@ use crate::{ - check_actor_domain, extensions::group_extensions::GroupExtension, fetcher::get_or_fetch_and_upsert_user, - objects::create_tombstone, + objects::{check_object_domain, create_tombstone}, ActorType, FromApub, GroupExt, @@ -189,7 +188,7 @@ impl FromApub for CommunityForm { updated: group.inner.updated().map(|u| u.to_owned().naive_local()), deleted: None, nsfw: group.ext_one.sensitive, - actor_id: Some(check_actor_domain(group, expected_domain)?), + actor_id: Some(check_object_domain(group, expected_domain)?), local: false, private_key: None, public_key: Some(group.ext_two.to_owned().public_key.public_key_pem), diff --git a/lemmy_apub/src/objects/mod.rs b/lemmy_apub/src/objects/mod.rs index f3a701c2..56d02607 100644 --- a/lemmy_apub/src/objects/mod.rs +++ b/lemmy_apub/src/objects/mod.rs @@ -1,10 +1,13 @@ +use crate::check_is_apub_id_valid; use activitystreams::{ - base::BaseExt, + base::{AsBase, BaseExt}, + markers::Base, object::{Tombstone, TombstoneExt}, }; -use anyhow::anyhow; +use anyhow::{anyhow, Context}; use chrono::NaiveDateTime; -use lemmy_utils::{utils::convert_datetime, LemmyError}; +use lemmy_utils::{location_info, utils::convert_datetime, LemmyError}; +use url::Url; pub mod comment; pub mod community; @@ -36,3 +39,22 @@ where Err(anyhow!("Cant convert object to tombstone if it wasnt deleted").into()) } } + +pub(in crate::objects) fn check_object_domain( + apub: &T, + expected_domain: Option, +) -> Result +where + T: Base + AsBase, +{ + let actor_id = if let Some(url) = expected_domain { + check_is_apub_id_valid(&url)?; + let domain = url.domain().context(location_info!())?; + apub.id(domain)?.context(location_info!())? + } else { + let actor_id = apub.id_unchecked().context(location_info!())?; + check_is_apub_id_valid(&actor_id)?; + actor_id + }; + Ok(actor_id.to_string()) +} diff --git a/lemmy_apub/src/objects/post.rs b/lemmy_apub/src/objects/post.rs index 3a979f5e..0c1aaf29 100644 --- a/lemmy_apub/src/objects/post.rs +++ b/lemmy_apub/src/objects/post.rs @@ -1,8 +1,7 @@ use crate::{ - check_actor_domain, extensions::page_extension::PageExtension, fetcher::{get_or_fetch_and_upsert_community, get_or_fetch_and_upsert_user}, - objects::create_tombstone, + objects::{check_object_domain, create_tombstone}, FromApub, PageExt, ToApub, @@ -193,7 +192,7 @@ impl FromApub for PostForm { embed_description: iframely_description, embed_html: iframely_html, thumbnail_url: pictrs_thumbnail, - ap_id: Some(check_actor_domain(page, expected_domain)?), + ap_id: Some(check_object_domain(page, expected_domain)?), local: false, }) } diff --git a/lemmy_apub/src/objects/private_message.rs b/lemmy_apub/src/objects/private_message.rs index cc01bccb..413ab22f 100644 --- a/lemmy_apub/src/objects/private_message.rs +++ b/lemmy_apub/src/objects/private_message.rs @@ -1,8 +1,7 @@ use crate::{ - check_actor_domain, check_is_apub_id_valid, fetcher::get_or_fetch_and_upsert_user, - objects::create_tombstone, + objects::{check_object_domain, create_tombstone}, FromApub, ToApub, }; @@ -96,7 +95,7 @@ impl FromApub for PrivateMessageForm { updated: note.updated().map(|u| u.to_owned().naive_local()), deleted: None, read: None, - ap_id: Some(check_actor_domain(note, expected_domain)?), + ap_id: Some(check_object_domain(note, expected_domain)?), local: false, }) } diff --git a/lemmy_apub/src/objects/user.rs b/lemmy_apub/src/objects/user.rs index 96ece968..ef314775 100644 --- a/lemmy_apub/src/objects/user.rs +++ b/lemmy_apub/src/objects/user.rs @@ -1,4 +1,4 @@ -use crate::{check_actor_domain, ActorType, FromApub, PersonExt, ToApub}; +use crate::{objects::check_object_domain, ActorType, FromApub, PersonExt, ToApub}; use activitystreams::{ actor::{ApActor, Endpoints, Person}, object::{Image, Tombstone}, @@ -145,7 +145,7 @@ impl FromApub for UserForm { show_avatars: false, send_notifications_to_email: false, matrix_user_id: None, - actor_id: Some(check_actor_domain(person, expected_domain)?), + actor_id: Some(check_object_domain(person, expected_domain)?), bio: Some(bio), local: false, private_key: None, -- 2.44.1