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 added 2 commits 2024-07-13 05:41:42 +00:00

Refetch on AP ID mismatch 3db8199420

As hinted at in the commit message when strict checking
was added in 8684964c5d,
refetching is more robust than display URL comparison
but in exchange is harder to implement correctly.

A similar refetch approach is also employed by
e.g. Mastodon, IceShrimp and FireFish.

To make sure no checks can be bypassed by forcing
a refetch, id checking is placed at the very end.

This will fix:
 - Peertube display URL arrays our transmogrifier fails to normalise
 - non-canonical display URLs from alternative frontends
   (theoretical; we didnt’t get any actual reports about this)

It will also be helpful in the planned key handling overhaul.

The modified user collision test was introduced in
https://git.pleroma.social/pleroma/pleroma/-/merge_requests/461
and unfortunately the issues this fixes aren’t public.
Afaict it was just meant to guard against someone serving
faked data belonging to an unrelated domain. Since we now
refetch and the id actually is mocked, lookup now succeeds
but will use the real data from the authorative server
making it unproblematic. Instead modify the fake data further
and make sure we don’t end up using the spoofed version.

Allow cross-domain redirects on AP requests

ci/woodpecker/pr/lint Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

ci/woodpecker/pr/build-amd64 Pipeline was successful

Details

ci/woodpecker/pr/build-arm64 Pipeline was successful

Details

ci/woodpecker/pr/docs Pipeline was successful

Details

bc2b776109

Since we now remember the final location redirects lead to
and use it for all further checks since
3e134b07fa, these redirects
can no longer be exploited to serve counterfeit objects.

This fixes:
 - display URLs from independent webapp clients
   redirecting to the canonical domain
 - Peertube display URLs for remote content
   (acting like the above)

Oneric referenced this pull request

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

WIP: 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