From 313f31589626823584e8d999b801c4874aa83899 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Thu, 6 Aug 2020 21:44:47 +0200 Subject: [PATCH] Various adjustments after review --- .../contributing_federation_development.md | 13 ++++++++++ server/src/api/mod.rs | 5 ++-- server/src/apub/comment.rs | 7 +++--- server/src/apub/inbox/activities/create.rs | 10 -------- server/src/apub/inbox/activities/remove.rs | 13 ++++------ server/src/apub/inbox/activities/undo.rs | 4 ++-- server/src/apub/inbox/activities/update.rs | 24 +++++-------------- server/src/apub/inbox/community_inbox.rs | 24 ++++++++++--------- server/src/apub/inbox/shared_inbox.rs | 10 ++++---- server/src/apub/inbox/user_inbox.rs | 9 +++---- server/src/apub/post.rs | 8 +++---- 11 files changed, 58 insertions(+), 69 deletions(-) diff --git a/docs/src/contributing_federation_development.md b/docs/src/contributing_federation_development.md index 143ae9f8..8af38a07 100644 --- a/docs/src/contributing_federation_development.md +++ b/docs/src/contributing_federation_development.md @@ -68,3 +68,16 @@ cd /lemmy/ sudo docker-compose pull sudo docker-compose up -d ``` + +## Security Model + +- HTTP signature verify: This ensures that activity really comes from the activity that it claims +- check_is_apub_valid : Makes sure its in our allowed instances list +- Lower level checks: To make sure that the user that creates/updates/removes a post is actually on the same instance as that post + +For the last point, note that we are *not* checking whether the actor that sends the create activity for a post is +actually identical to the post's creator, or that the user that removes a post is a mod/admin. These things are checked +by the API code, and its the responsibility of each instance to check user permissions. This does not leave any attack +vector, as a normal instance user cant do actions that violate the API rules. The only one who could do that is the +admin (and the software deployed by the admin). But the admin can do anything on the instance, including send activities +from other user accounts. So we wouldnt actually gain any security by checking mod permissions or similar. \ No newline at end of file diff --git a/server/src/api/mod.rs b/server/src/api/mod.rs index 7c5eeb2f..8124cd4a 100644 --- a/server/src/api/mod.rs +++ b/server/src/api/mod.rs @@ -55,7 +55,7 @@ pub trait Perform { ) -> Result; } -pub async fn is_mod_or_admin( +pub(in crate::api) async fn is_mod_or_admin( pool: &DbPool, user_id: i32, community_id: i32, @@ -65,8 +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()); + return Err(APIError::err("not_a_mod_or_admin").into()); } Ok(()) } diff --git a/server/src/apub/comment.rs b/server/src/apub/comment.rs index 1aa3790c..fbec5905 100644 --- a/server/src/apub/comment.rs +++ b/server/src/apub/comment.rs @@ -1,5 +1,4 @@ use crate::{ - api::check_slurs, apub::{ activities::{generate_activity_id, send_activity_to_community}, check_actor_domain, @@ -50,7 +49,7 @@ use lemmy_db::{ user::User_, Crud, }; -use lemmy_utils::{convert_datetime, scrape_text_for_mentions, MentionData}; +use lemmy_utils::{convert_datetime, remove_slurs, scrape_text_for_mentions, MentionData}; use log::debug; use serde::Deserialize; use serde_json::Error; @@ -174,13 +173,13 @@ impl FromApub for CommentForm { .as_single_xsd_string() .unwrap() .to_string(); - check_slurs(&content)?; + let content_slurs_removed = remove_slurs(&content); Ok(CommentForm { creator_id: creator.id, post_id: post.id, parent_id, - content, + content: content_slurs_removed, removed: None, read: None, published: note.published().map(|u| u.to_owned().naive_local()), diff --git a/server/src/apub/inbox/activities/create.rs b/server/src/apub/inbox/activities/create.rs index 6e201ff3..b45f489a 100644 --- a/server/src/apub/inbox/activities/create.rs +++ b/server/src/apub/inbox/activities/create.rs @@ -24,7 +24,6 @@ use crate::{ }; use activitystreams::{activity::Create, base::AnyBase, object::Note, prelude::*}; use actix_web::{client::Client, HttpResponse}; -use anyhow::anyhow; use lemmy_db::{ comment::{Comment, CommentForm}, comment_view::CommentView, @@ -63,10 +62,6 @@ async fn receive_create_post( let page = PageExt::from_any_base(create.object().to_owned().one().unwrap())?.unwrap(); let post = PostForm::from_apub(&page, client, pool, Some(user.actor_id()?)).await?; - // TODO: not sure if it makes sense to check for the exact user, seeing as we already check the domain - if post.creator_id != user.id { - return Err(anyhow!("Actor for create activity and post creator need to be identical").into()); - } let inserted_post = blocking(pool, move |conn| Post::create(conn, &post)).await??; @@ -99,11 +94,6 @@ async fn receive_create_comment( let note = Note::from_any_base(create.object().to_owned().one().unwrap())?.unwrap(); let comment = CommentForm::from_apub(¬e, client, pool, Some(user.actor_id()?)).await?; - if comment.creator_id != user.id { - return Err( - anyhow!("Actor for create activity and comment creator need to be identical").into(), - ); - } let inserted_comment = blocking(pool, move |conn| Comment::create(conn, &comment)).await??; diff --git a/server/src/apub/inbox/activities/remove.rs b/server/src/apub/inbox/activities/remove.rs index 3db019fe..b81927af 100644 --- a/server/src/apub/inbox/activities/remove.rs +++ b/server/src/apub/inbox/activities/remove.rs @@ -1,10 +1,5 @@ use crate::{ - api::{ - comment::CommentResponse, - community::CommunityResponse, - is_mod_or_admin, - post::PostResponse, - }, + api::{comment::CommentResponse, community::CommunityResponse, post::PostResponse}, apub::{ fetcher::{get_or_fetch_and_insert_comment, get_or_fetch_and_insert_post}, inbox::shared_inbox::{ @@ -29,6 +24,7 @@ use crate::{ }; use activitystreams::{activity::Remove, base::AnyBase, object::Note, prelude::*}; use actix_web::{client::Client, HttpResponse}; +use anyhow::anyhow; use lemmy_db::{ comment::{Comment, CommentForm}, comment_view::CommentView, @@ -49,8 +45,9 @@ pub async fn receive_remove( let remove = Remove::from_any_base(activity)?.unwrap(); let actor = get_user_from_activity(&remove, client, pool).await?; let community = get_community_from_activity(&remove, client, pool).await?; - // TODO: we dont federate remote admins at all, and remote mods arent federated properly - is_mod_or_admin(pool, actor.id, community.id).await?; + if actor.actor_id()?.domain() != community.actor_id()?.domain() { + return Err(anyhow!("Remove activities are only allowed on local objects").into()); + } match remove.object().as_single_kind_str() { Some("Page") => receive_remove_post(remove, client, pool, chat_server).await, diff --git a/server/src/apub/inbox/activities/undo.rs b/server/src/apub/inbox/activities/undo.rs index 238b1bb0..49e70830 100644 --- a/server/src/apub/inbox/activities/undo.rs +++ b/server/src/apub/inbox/activities/undo.rs @@ -67,8 +67,8 @@ where let inner_actor = inner_activity.actor()?; let inner_actor_uri = inner_actor.as_single_xsd_any_uri().unwrap(); - if outer_actor_uri != inner_actor_uri { - Err(anyhow!("An actor can only undo its own activities").into()) + if outer_actor_uri.domain() != inner_actor_uri.domain() { + Err(anyhow!("Cant undo activities from a different instance").into()) } else { Ok(()) } diff --git a/server/src/apub/inbox/activities/update.rs b/server/src/apub/inbox/activities/update.rs index d669e5be..6f2a6d56 100644 --- a/server/src/apub/inbox/activities/update.rs +++ b/server/src/apub/inbox/activities/update.rs @@ -25,7 +25,6 @@ use crate::{ }; use activitystreams::{activity::Update, base::AnyBase, object::Note, prelude::*}; use actix_web::{client::Client, HttpResponse}; -use anyhow::anyhow; use lemmy_db::{ comment::{Comment, CommentForm}, comment_view::CommentView, @@ -64,16 +63,11 @@ async fn receive_update_post( let page = PageExt::from_any_base(update.object().to_owned().one().unwrap())?.unwrap(); let post = PostForm::from_apub(&page, client, pool, Some(user.actor_id()?)).await?; - if post.creator_id != user.id { - return Err(anyhow!("Actor for update activity and post creator need to be identical").into()); - } - let original_post = get_or_fetch_and_insert_post(&post.get_ap_id()?, client, pool).await?; - if post.ap_id != original_post.ap_id { - return Err(anyhow!("Updated post ID needs to be identical to the original ID").into()); - } + let original_post_id = get_or_fetch_and_insert_post(&post.get_ap_id()?, client, pool) + .await? + .id; - let original_post_id = original_post.id; blocking(pool, move |conn| { Post::update(conn, original_post_id, &post) }) @@ -107,17 +101,11 @@ async fn receive_update_comment( let user = get_user_from_activity(&update, client, pool).await?; let comment = CommentForm::from_apub(¬e, client, pool, Some(user.actor_id()?)).await?; - if comment.creator_id != user.id { - return Err(anyhow!("Actor for update activity and post creator need to be identical").into()); - } - let original_comment = - get_or_fetch_and_insert_comment(&comment.get_ap_id()?, client, pool).await?; - if comment.ap_id != original_comment.ap_id { - return Err(anyhow!("Updated post ID needs to be identical to the original ID").into()); - } + let original_comment_id = get_or_fetch_and_insert_comment(&comment.get_ap_id()?, client, pool) + .await? + .id; - let original_comment_id = original_comment.id; let updated_comment = blocking(pool, move |conn| { Comment::update(conn, original_comment_id, &comment) }) diff --git a/server/src/apub/inbox/community_inbox.rs b/server/src/apub/inbox/community_inbox.rs index 37e7c289..8b257d47 100644 --- a/server/src/apub/inbox/community_inbox.rs +++ b/server/src/apub/inbox/community_inbox.rs @@ -69,14 +69,16 @@ pub async fn community_inbox( verify(&request, &user)?; - insert_activity(user.id, activity.clone(), false, &db).await?; - let any_base = activity.clone().into_any_base()?; let kind = activity.kind().unwrap(); - match kind { - ValidTypes::Follow => handle_follow(any_base, user, community, &client, db).await, - ValidTypes::Undo => handle_undo_follow(any_base, user, community, db).await, - } + let user_id = user.id; + let res = match kind { + ValidTypes::Follow => handle_follow(any_base, user, community, &client, &db).await, + ValidTypes::Undo => handle_undo_follow(any_base, user, community, &db).await, + }; + + insert_activity(user_id, activity.clone(), false, &db).await?; + res } /// Handle a follow request from a remote user, adding it to the local database and returning an @@ -86,7 +88,7 @@ async fn handle_follow( user: User_, community: Community, client: &Client, - db: DbPoolParam, + db: &DbPoolParam, ) -> Result { let follow = Follow::from_any_base(activity)?.unwrap(); let community_follower_form = CommunityFollowerForm { @@ -95,12 +97,12 @@ async fn handle_follow( }; // This will fail if they're already a follower, but ignore the error. - blocking(&db, move |conn| { + blocking(db, move |conn| { CommunityFollower::follow(&conn, &community_follower_form).ok() }) .await?; - community.send_accept_follow(follow, &client, &db).await?; + community.send_accept_follow(follow, &client, db).await?; Ok(HttpResponse::Ok().finish()) } @@ -109,7 +111,7 @@ async fn handle_undo_follow( activity: AnyBase, user: User_, community: Community, - db: DbPoolParam, + db: &DbPoolParam, ) -> Result { let _undo = Undo::from_any_base(activity)?.unwrap(); @@ -119,7 +121,7 @@ async fn handle_undo_follow( }; // This will fail if they aren't a follower, but ignore the error. - blocking(&db, move |conn| { + blocking(db, move |conn| { CommunityFollower::unfollow(&conn, &community_follower_form).ok() }) .await?; diff --git a/server/src/apub/inbox/shared_inbox.rs b/server/src/apub/inbox/shared_inbox.rs index 5406d111..d5ed9312 100644 --- a/server/src/apub/inbox/shared_inbox.rs +++ b/server/src/apub/inbox/shared_inbox.rs @@ -77,12 +77,9 @@ pub async fn shared_inbox( check_is_apub_id_valid(sender)?; verify(&request, actor.as_ref())?; - // TODO: probably better to do this after, so we dont store activities that fail a check somewhere - insert_activity(actor.user_id(), activity.clone(), false, &pool).await?; - let any_base = activity.clone().into_any_base()?; let kind = activity.kind().unwrap(); - match kind { + let res = match kind { ValidTypes::Announce => receive_announce(any_base, &client, &pool, chat_server).await, ValidTypes::Create => receive_create(any_base, &client, &pool, chat_server).await, ValidTypes::Update => receive_update(any_base, &client, &pool, chat_server).await, @@ -91,7 +88,10 @@ pub async fn shared_inbox( ValidTypes::Remove => receive_remove(any_base, &client, &pool, chat_server).await, ValidTypes::Delete => receive_delete(any_base, &client, &pool, chat_server).await, ValidTypes::Undo => receive_undo(any_base, &client, &pool, chat_server).await, - } + }; + + insert_activity(actor.user_id(), activity.clone(), false, &pool).await?; + res } pub(in crate::apub::inbox) fn receive_unhandled_activity( diff --git a/server/src/apub/inbox/user_inbox.rs b/server/src/apub/inbox/user_inbox.rs index 53f1fad2..b443b51c 100644 --- a/server/src/apub/inbox/user_inbox.rs +++ b/server/src/apub/inbox/user_inbox.rs @@ -65,11 +65,9 @@ pub async fn user_inbox( let actor = get_or_fetch_and_upsert_actor(actor_uri, &client, &pool).await?; verify(&request, actor.as_ref())?; - insert_activity(actor.user_id(), activity.clone(), false, &pool).await?; - let any_base = activity.clone().into_any_base()?; let kind = activity.kind().unwrap(); - match kind { + let res = match kind { ValidTypes::Accept => receive_accept(any_base, username, &client, &pool).await, ValidTypes::Create => { receive_create_private_message(any_base, &client, &pool, chat_server).await @@ -83,7 +81,10 @@ pub async fn user_inbox( ValidTypes::Undo => { receive_undo_delete_private_message(any_base, &client, &pool, chat_server).await } - } + }; + + insert_activity(actor.user_id(), activity.clone(), false, &pool).await?; + res } /// Handle accepted follows. diff --git a/server/src/apub/post.rs b/server/src/apub/post.rs index a2a83f45..8bd8364f 100644 --- a/server/src/apub/post.rs +++ b/server/src/apub/post.rs @@ -1,5 +1,5 @@ use crate::{ - api::{check_slurs, check_slurs_opt}, + api::check_slurs, apub::{ activities::{generate_activity_id, send_activity_to_community}, check_actor_domain, @@ -44,7 +44,7 @@ use lemmy_db::{ user::User_, Crud, }; -use lemmy_utils::convert_datetime; +use lemmy_utils::{convert_datetime, remove_slurs}; use serde::Deserialize; use url::Url; @@ -225,11 +225,11 @@ impl FromApub for PostForm { .as_ref() .map(|c| c.as_single_xsd_string().unwrap().to_string()); check_slurs(&name)?; - check_slurs_opt(&body)?; + let body_slurs_removed = body.map(|b| remove_slurs(&b)); Ok(PostForm { name, url, - body, + body: body_slurs_removed, creator_id: creator.id, community_id: community.id, removed: None, -- 2.44.1