Tweak fetch security checks #819

Oneric · 2024-07-13T05:41:42Z

Oneric commented

2024-07-13 05:41:42 +00:00

As discussed when id checking was initially introduced, refetch can be more robust, but if done improperly also create exploits of its own. I believe this one should now work, but best apply an extra dose of scrutiny during review
Refetching will fix some theoretical cases, the Peertube display-url array brought up on IRC and should allow using a properly cehcked fetch_and_contain for keys over in #816

Things worth highlighting:

i’m not sure whether to disregard fragments when checking IDs
This would allow us to skip refetches for fragment keys, but might be confusing in other instances. E.g. Mastodon’s intentionally (albeit spec-breaking) unfetchable fragment activities might end up getting "fetched" as the object they act on
for future key fetching, falling back to a refetch might in theory introduce a federation bootstrapping problem if both servers
a) use non-fragment keys (which will still use the actor’s id in the top-level)
b) refetch on id mismatch
c) require signed fetch for actors
This way they’ll be able to confirm signatures, since neither can retrieve they key of the other.
However, for one current Akkoma already has this problem if auth fetch is enabled, but future Akkoma with #816 won’t have it due to using fragment keys and serving a minimal, key-only actor on unsigned requests even if auth fetch is enabled, so this shouldn’t be a concern here

Additionally, this also reallows cross-domain redirects instead enforcing authenticity by using the final location for containment checks (preparation for this already happened in 3e134b07fa4e382f1f4cfdbe90e74f8e73336a4e,). This fixes e.g. lookups via Peertube display URLs for content from a remote instance (also brought up on IRC before)

As discussed when id checking was initially introduced, refetch can be more robust, but if done improperly also create exploits of its own. I believe this one should now work, but best apply an extra dose of scrutiny during review Refetching will fix some theoretical cases, the Peertube display-url array brought up on IRC and should allow using a properly cehcked `fetch_and_contain` for keys over in #816 Things worth highlighting: - i’m not sure whether to disregard fragments when checking IDs This would allow us to skip refetches for fragment keys, but might be confusing in other instances. E.g. Mastodon’s intentionally (albeit spec-breaking) unfetchable fragment activities might end up getting "fetched" as the object they act on - for future key fetching, falling back to a refetch might in theory introduce a federation bootstrapping problem if both servers a) use non-fragment keys (which will still use the actor’s id in the top-level) b) refetch on id mismatch c) require signed fetch for actors This way they’ll be able to confirm signatures, since neither can retrieve they key of the other. However, for one current Akkoma already has this problem if auth fetch is enabled, but future Akkoma with #816 won’t have it due to using fragment keys and serving a minimal, key-only actor on unsigned requests even if auth fetch is enabled, so this shouldn’t be a concern here Additionally, this also reallows cross-domain redirects instead enforcing authenticity by using the final location for containment checks (preparation for this already happened in 3e134b07fa4e382f1f4cfdbe90e74f8e73336a4e,). This fixes e.g. lookups via Peertube display URLs for content from a remote instance (also brought up on IRC before)

Oneric referenced this pull request

2024-07-13 05:42:46 +00:00

Extract keys to their own table, match keyID #816

floatingghost reviewed 2024-09-24 23:47:36 +00:00

floatingghost left a comment

i think this is fine, nothing jumps out at me as being off - wrt the fragment keys, we can prooooobably ignore fragments? i think?

lib/pleroma/object/containment.ex Outdated

					
				@ -109,2 +99,2 @@

				      _ -> :error

				    end

				  def contain_id_to_fetch(url, %{"id" => id}) when is_binary(id) do

				    # XXX: should we also ignore fragments?

floatingghost commented

2024-09-24 23:42:44 +00:00

probably? fragments aren't technically part of the URL as far as the pure URL spec is concerned and really only matter to a browser

Oneric marked this conversation as resolved

Oneric commented

2024-10-05 16:27:29 +00:00

I’ve been meaning to adjust this for fragments but didn’t get to it yet, sorry

While grepping through logs though i found one more odd case this will fix and seems worth documenting for future reference: case mismatches in the domain part
While paths are generally case sensitive, domain names are explicitly case insensitive but i got an id rejection for https://QuarteredCircle.net/users/LexPendragon/collections/featured whose AP object id is https://quarteredcircle.net/users/LexPendragon/collections/featured

The associated user’s AP id though actually is https://QuarteredCircle.net/users/LexPendragon and refers to owned collections with CamelCased URI.

I’m not sure why casing differs; the newest posts also use CamelCasing in their id, so it doesn't seem like they changed casing in their config (but while odd and almost certainly a bad idea, technically those URIs are equivalent)

While we could manually normalise all domain parts prior to comparison, some servers might also allow for case insensitive paths, but we can’t generally assume this and thus shouldn't alter the path part of an URI

I’ve been meaning to adjust this for fragments but didn’t get to it yet, sorry While grepping through logs though i found one more odd case this will fix and seems worth documenting for future reference: case mismatches in the domain part While paths are generally case sensitive, domain names are explicitly case insensitive but i got an id rejection for `https://QuarteredCircle.net/users/LexPendragon/collections/featured` whose AP object id is `https://quarteredcircle.net/users/LexPendragon/collections/featured` The associated user’s AP id though actually is `https://QuarteredCircle.net/users/LexPendragon` and refers to owned collections with CamelCased URI. I’m not sure why casing differs; the newest posts also use CamelCasing in their id, so it doesn't seem like they changed casing in their config *(but while odd and almost certainly a bad idea, technically those URIs are equivalent)* While we could manually normalise all domain parts prior to comparison, some servers might also allow for case insensitive paths, but we can’t generally assume this and thus shouldn't alter the path part of an URI

Oneric force-pushed id-refetch from bc2b776109 to d5b0720596

2024-10-13 23:51:56 +00:00

Compare

Oneric commented

2024-10-13 23:52:54 +00:00

rebased and allowed non-matching/non-existent fragments and different domain casing

floatingghost commented

2024-10-16 14:15:48 +00:00

all good i thunk, thanks~

floatingghost merged commit 09fa7227f6 into develop

2024-10-16 14:16:15 +00:00

floatingghost deleted branch id-refetch

2024-10-16 14:16:15 +00:00

floatingghost referenced this pull request from a commit

2024-10-16 14:16:17 +00:00

Merge pull request 'Tweak fetch security checks' (#819) from Oneric/akkoma:id-refetch into develop

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: AkkomaGang/akkoma#819