From 233aa34d54697c366a23a31a34990616e4231ec7 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 5 Aug 2020 14:18:08 +0200 Subject: [PATCH 1/7] Verify ID of received apub objects against domain allowlist etc --- server/src/apub/comment.rs | 6 +++++- server/src/apub/community.rs | 5 ++++- server/src/apub/post.rs | 6 +++++- server/src/apub/private_message.rs | 7 ++++--- server/src/apub/user.rs | 8 +++++++- 5 files changed, 25 insertions(+), 7 deletions(-) diff --git a/server/src/apub/comment.rs b/server/src/apub/comment.rs index 05b40dbe5..8bd79b799 100644 --- a/server/src/apub/comment.rs +++ b/server/src/apub/comment.rs @@ -1,6 +1,7 @@ use crate::{ apub::{ activities::{generate_activity_id, send_activity_to_community}, + check_is_apub_id_valid, create_apub_response, create_apub_tombstone_response, create_tombstone, @@ -166,6 +167,9 @@ impl FromApub for CommentForm { None => None, }; + let ap_id = note.id_unchecked().unwrap().to_string(); + check_is_apub_id_valid(&Url::parse(&ap_id)?)?; + Ok(CommentForm { creator_id: creator.id, post_id: post.id, @@ -181,7 +185,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: note.id_unchecked().unwrap().to_string(), + ap_id, local: false, }) } diff --git a/server/src/apub/community.rs b/server/src/apub/community.rs index 96f0f84c3..b35c47bbc 100644 --- a/server/src/apub/community.rs +++ b/server/src/apub/community.rs @@ -1,6 +1,7 @@ use crate::{ apub::{ activities::{generate_activity_id, send_activity}, + check_is_apub_id_valid, create_apub_response, create_apub_tombstone_response, create_tombstone, @@ -334,6 +335,8 @@ impl FromApub for CommunityForm { .unwrap(); let creator = get_or_fetch_and_upsert_user(creator_uri, client, pool).await?; + let actor_id = group.inner.id_unchecked().unwrap().to_string(); + check_is_apub_id_valid(&Url::parse(&actor_id)?)?; Ok(CommunityForm { name: group @@ -359,7 +362,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: group.inner.id_unchecked().unwrap().to_string(), + actor_id, local: false, private_key: None, public_key: Some(group.ext_two.to_owned().public_key.public_key_pem), diff --git a/server/src/apub/post.rs b/server/src/apub/post.rs index 4b687b0ae..ed4dfe0f9 100644 --- a/server/src/apub/post.rs +++ b/server/src/apub/post.rs @@ -1,6 +1,7 @@ use crate::{ apub::{ activities::{generate_activity_id, send_activity_to_community}, + check_is_apub_id_valid, create_apub_response, create_apub_tombstone_response, create_tombstone, @@ -203,6 +204,9 @@ impl FromApub for PostForm { None => (None, None, None), }; + let ap_id = page.inner.id_unchecked().unwrap().to_string(); + check_is_apub_id_valid(&Url::parse(&ap_id)?)?; + let url = page .inner .url() @@ -245,7 +249,7 @@ impl FromApub for PostForm { embed_description, embed_html, thumbnail_url, - ap_id: page.inner.id_unchecked().unwrap().to_string(), + ap_id, local: false, }) } diff --git a/server/src/apub/private_message.rs b/server/src/apub/private_message.rs index 69f552d3b..af0f56107 100644 --- a/server/src/apub/private_message.rs +++ b/server/src/apub/private_message.rs @@ -1,6 +1,7 @@ use crate::{ apub::{ activities::{generate_activity_id, send_activity}, + check_is_apub_id_valid, create_tombstone, fetcher::get_or_fetch_and_upsert_user, insert_activity, @@ -84,10 +85,10 @@ impl FromApub for PrivateMessageForm { .unwrap(); let creator = get_or_fetch_and_upsert_user(&creator_actor_id, client, pool).await?; - let recipient_actor_id = note.to().unwrap().clone().single_xsd_any_uri().unwrap(); - let recipient = get_or_fetch_and_upsert_user(&recipient_actor_id, client, pool).await?; + let ap_id = note.id_unchecked().unwrap().to_string(); + check_is_apub_id_valid(&Url::parse(&ap_id)?)?; Ok(PrivateMessageForm { creator_id: creator.id, @@ -102,7 +103,7 @@ impl FromApub for PrivateMessageForm { updated: note.updated().map(|u| u.to_owned().naive_local()), deleted: None, read: None, - ap_id: note.id_unchecked().unwrap().to_string(), + ap_id, local: false, }) } diff --git a/server/src/apub/user.rs b/server/src/apub/user.rs index 2922006d5..80b91ddbb 100644 --- a/server/src/apub/user.rs +++ b/server/src/apub/user.rs @@ -1,6 +1,7 @@ use crate::{ apub::{ activities::{generate_activity_id, send_activity}, + check_is_apub_id_valid, create_apub_response, insert_activity, ActorType, @@ -217,6 +218,11 @@ impl FromApub for UserForm { None => None, }; + // TODO: here and in community we could actually check against the exact domain where we fetched + // the actor from, if we can pass it in somehow + let actor_id = person.id_unchecked().unwrap().to_string(); + check_is_apub_id_valid(&Url::parse(&actor_id)?)?; + Ok(UserForm { name: person .name() @@ -241,7 +247,7 @@ impl FromApub for UserForm { show_avatars: false, send_notifications_to_email: false, matrix_user_id: None, - actor_id: person.id_unchecked().unwrap().to_string(), + actor_id, bio: person .inner .summary() From 81d4922740d390e0e92bc6f8ce6668d8d6735363 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 5 Aug 2020 17:41:35 +0200 Subject: [PATCH 2/7] Instance shouldnt send Announce activities to itself --- server/src/apub/community.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/apub/community.rs b/server/src/apub/community.rs index b35c47bbc..44e3ad4e6 100644 --- a/server/src/apub/community.rs +++ b/server/src/apub/community.rs @@ -461,12 +461,13 @@ pub async fn do_announce( insert_activity(community.creator_id, announce.clone(), true, pool).await?; - // dont send to the instance where the activity originally came from, because that would result - // in a database error (same data inserted twice) let mut to = community.get_follower_inboxes(pool).await?; + // dont send to the local instance, nor to the instance where the activity originally came from, + // because that would result in a database error (same data inserted twice) // this seems to be the "easiest" stable alternative for remove_item() to.retain(|x| *x != sender.get_shared_inbox_url()); + to.retain(|x| *x != community.get_shared_inbox_url()); send_activity(client, &announce.into_any_base()?, community, to).await?; From cc2c7db9fec3cf86b5dd488f3c48d556d80ed529 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Thu, 6 Aug 2020 14:53:58 +0200 Subject: [PATCH 3/7] Add security checks and slur checks for activitypub inbox --- server/lemmy_db/src/comment.rs | 10 +--- server/lemmy_db/src/community.rs | 10 +--- server/lemmy_db/src/post.rs | 10 +--- server/src/api/mod.rs | 5 +- server/src/apub/comment.rs | 23 ++++----- server/src/apub/community.rs | 52 ++++++++++++-------- server/src/apub/fetcher.rs | 21 ++++---- server/src/apub/inbox/activities/announce.rs | 34 ++++++++----- server/src/apub/inbox/activities/create.rs | 20 +++++++- server/src/apub/inbox/activities/delete.rs | 7 +-- server/src/apub/inbox/activities/dislike.rs | 4 +- server/src/apub/inbox/activities/like.rs | 4 +- server/src/apub/inbox/activities/remove.rs | 20 ++++++-- server/src/apub/inbox/activities/undo.rs | 48 ++++++++++++++---- server/src/apub/inbox/activities/update.rs | 52 +++++++++++++++----- server/src/apub/inbox/shared_inbox.rs | 16 ++++-- server/src/apub/inbox/user_inbox.rs | 12 +++-- server/src/apub/mod.rs | 29 +++++++++++ server/src/apub/post.rs | 28 ++++++----- server/src/apub/private_message.rs | 4 +- server/src/apub/user.rs | 48 ++++++++++-------- 21 files changed, 299 insertions(+), 158 deletions(-) diff --git a/server/lemmy_db/src/comment.rs b/server/lemmy_db/src/comment.rs index 99efde8d7..cdb5a1b64 100644 --- a/server/lemmy_db/src/comment.rs +++ b/server/lemmy_db/src/comment.rs @@ -116,10 +116,7 @@ impl Comment { ) -> Result { use crate::schema::comment::dsl::*; diesel::update(comment.find(comment_id)) - .set(( - deleted.eq(new_deleted), - updated.eq(naive_now()) - )) + .set((deleted.eq(new_deleted), updated.eq(naive_now()))) .get_result::(conn) } @@ -130,10 +127,7 @@ impl Comment { ) -> Result { use crate::schema::comment::dsl::*; diesel::update(comment.find(comment_id)) - .set(( - removed.eq(new_removed), - updated.eq(naive_now()) - )) + .set((removed.eq(new_removed), updated.eq(naive_now()))) .get_result::(conn) } diff --git a/server/lemmy_db/src/community.rs b/server/lemmy_db/src/community.rs index 2c86f1e75..14e8f9849 100644 --- a/server/lemmy_db/src/community.rs +++ b/server/lemmy_db/src/community.rs @@ -107,10 +107,7 @@ impl Community { ) -> Result { use crate::schema::community::dsl::*; diesel::update(community.find(community_id)) - .set(( - deleted.eq(new_deleted), - updated.eq(naive_now()) - )) + .set((deleted.eq(new_deleted), updated.eq(naive_now()))) .get_result::(conn) } @@ -121,10 +118,7 @@ impl Community { ) -> Result { use crate::schema::community::dsl::*; diesel::update(community.find(community_id)) - .set(( - removed.eq(new_removed), - updated.eq(naive_now()) - )) + .set((removed.eq(new_removed), updated.eq(naive_now()))) .get_result::(conn) } diff --git a/server/lemmy_db/src/post.rs b/server/lemmy_db/src/post.rs index 56ff7474b..43e002113 100644 --- a/server/lemmy_db/src/post.rs +++ b/server/lemmy_db/src/post.rs @@ -119,10 +119,7 @@ impl Post { ) -> Result { use crate::schema::post::dsl::*; diesel::update(post.find(post_id)) - .set(( - deleted.eq(new_deleted), - updated.eq(naive_now()) - )) + .set((deleted.eq(new_deleted), updated.eq(naive_now()))) .get_result::(conn) } @@ -133,10 +130,7 @@ impl Post { ) -> Result { use crate::schema::post::dsl::*; diesel::update(post.find(post_id)) - .set(( - removed.eq(new_removed), - updated.eq(naive_now()) - )) + .set((removed.eq(new_removed), updated.eq(naive_now()))) .get_result::(conn) } diff --git a/server/src/api/mod.rs b/server/src/api/mod.rs index a9aae823a..7c5eeb2fa 100644 --- a/server/src/api/mod.rs +++ b/server/src/api/mod.rs @@ -65,6 +65,7 @@ pub async fn is_mod_or_admin( }) .await?; if !is_mod_or_admin { + // TODO: more accurately, not_a_mod_or_admin? return Err(APIError::err("not_an_admin").into()); } Ok(()) @@ -104,14 +105,14 @@ pub(in crate::api) async fn get_user_from_jwt_opt( } } -pub(in crate::api) fn check_slurs(text: &str) -> Result<(), APIError> { +pub(in crate) fn check_slurs(text: &str) -> Result<(), APIError> { if let Err(slurs) = slur_check(text) { Err(APIError::err(&slurs_vec_to_str(slurs))) } else { Ok(()) } } -pub(in crate::api) fn check_slurs_opt(text: &Option) -> Result<(), APIError> { +pub(in crate) fn check_slurs_opt(text: &Option) -> Result<(), APIError> { match text { Some(t) => check_slurs(t), None => Ok(()), diff --git a/server/src/apub/comment.rs b/server/src/apub/comment.rs index 8bd79b799..1aa3790cc 100644 --- a/server/src/apub/comment.rs +++ b/server/src/apub/comment.rs @@ -1,7 +1,8 @@ use crate::{ + api::check_slurs, apub::{ activities::{generate_activity_id, send_activity_to_community}, - check_is_apub_id_valid, + check_actor_domain, create_apub_response, create_apub_tombstone_response, create_tombstone, @@ -132,6 +133,7 @@ impl FromApub for CommentForm { note: &Note, client: &Client, pool: &DbPool, + expected_domain: Option, ) -> Result { let creator_actor_id = ¬e .attributed_to() @@ -166,26 +168,25 @@ impl FromApub for CommentForm { } None => None, }; - - let ap_id = note.id_unchecked().unwrap().to_string(); - check_is_apub_id_valid(&Url::parse(&ap_id)?)?; + let content = note + .content() + .unwrap() + .as_single_xsd_string() + .unwrap() + .to_string(); + check_slurs(&content)?; Ok(CommentForm { creator_id: creator.id, post_id: post.id, parent_id, - content: note - .content() - .unwrap() - .as_single_xsd_string() - .unwrap() - .to_string(), + content, removed: None, read: None, published: note.published().map(|u| u.to_owned().naive_local()), updated: note.updated().map(|u| u.to_owned().naive_local()), deleted: None, - ap_id, + ap_id: check_actor_domain(note, expected_domain)?, local: false, }) } diff --git a/server/src/apub/community.rs b/server/src/apub/community.rs index 44e3ad4e6..3773b8fb4 100644 --- a/server/src/apub/community.rs +++ b/server/src/apub/community.rs @@ -1,7 +1,8 @@ use crate::{ + api::{check_slurs, check_slurs_opt}, apub::{ activities::{generate_activity_id, send_activity}, - check_is_apub_id_valid, + check_actor_domain, create_apub_response, create_apub_tombstone_response, create_tombstone, @@ -323,7 +324,12 @@ impl FromApub for CommunityForm { type ApubType = GroupExt; /// Parse an ActivityPub group received from another instance into a Lemmy community. - async fn from_apub(group: &GroupExt, client: &Client, pool: &DbPool) -> Result { + async fn from_apub( + group: &GroupExt, + client: &Client, + pool: &DbPool, + expected_domain: Option, + ) -> Result { let creator_and_moderator_uris = group.inner.attributed_to().unwrap(); let creator_uri = creator_and_moderator_uris .as_many() @@ -335,26 +341,30 @@ impl FromApub for CommunityForm { .unwrap(); let creator = get_or_fetch_and_upsert_user(creator_uri, client, pool).await?; - let actor_id = group.inner.id_unchecked().unwrap().to_string(); - check_is_apub_id_valid(&Url::parse(&actor_id)?)?; + let name = group + .inner + .name() + .unwrap() + .as_one() + .unwrap() + .as_xsd_string() + .unwrap() + .to_string(); + let title = group.inner.preferred_username().unwrap().to_string(); + // TODO: should be parsed as html and tags like