Various adjustments after review

This commit is contained in:
Felix Ableitner 2020-08-06 21:44:47 +02:00
parent cc2c7db9fe
commit 313f315896
11 changed files with 58 additions and 69 deletions

View file

@ -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.

View file

@ -55,7 +55,7 @@ pub trait Perform {
) -> Result<Self::Response, LemmyError>;
}
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(())
}

View file

@ -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()),

View file

@ -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(&note, 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??;

View file

@ -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,

View file

@ -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(())
}

View file

@ -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(&note, 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)
})

View file

@ -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<HttpResponse, LemmyError> {
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<HttpResponse, LemmyError> {
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?;

View file

@ -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<A>(

View file

@ -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.

View file

@ -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,