Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authorization checks on incoming activities #566

Closed
8 tasks done
snarfed opened this issue Jun 27, 2023 · 22 comments
Closed
8 tasks done

Authorization checks on incoming activities #566

snarfed opened this issue Jun 27, 2023 · 22 comments
Labels

Comments

@snarfed
Copy link
Owner

snarfed commented Jun 27, 2023

We currently do some authentication - verify HTTP sigs on incoming AP activities, require SSL and check certificates on web fetches - but we don't really do any authorization. We currently accept any activity from any actor and blindly apply it, without checking that the actor is authorized to perform the given activity. We should check any object that they're updating or deleting, that they're the follower on stop-following activities, etc.

===

TODO:

@snarfed
Copy link
Owner Author

snarfed commented Jul 21, 2023

AP mentions this very lightly for Update:

The receiving server MUST take care to be sure that the Update is authorized to modify its object. At minimum, this may be done by ensuring that the Update and its object are of same origin.

...and Delete:

The side effect of receiving this is that (assuming the object is owned by the sending actor / server) ...

@snarfed
Copy link
Owner Author

snarfed commented Oct 14, 2023

...but sadly AP doesn't specify any authorization/permission model more comprehensive than those bits.

Supposedly the most common one is "same origin," which says that the actor of any activity that modifies an object must be in the object's attributedTo. That's confusing re the much more well know browser same origin policy, which is about domains/hostnames, not full URLs like AP actor ids. Google finds some of both, and it's hard to distinguish: https://www.google.com/search?q=activitypub+"single+origin" .

(I've also seen hints of a more relaxed model that only requires that the actor is on the same instance as the object's attributedTo, and maybe only warn if it's the same instance but a different user.)

Creates have a similar question too, right? Should we require that the inbox delivery POST for a Create be signed by the object's attributedTo and/or the Create's actor?

Background:

@snarfed
Copy link
Owner Author

snarfed commented Oct 14, 2023

Another point to check: when we fetch an actor, we should check that its id is the same (final) URL we fetched. If it's not, we should override it with the fetched URL...right? Eg not doing that enabled this attack: https://hackerone.com/reports/461308

@snarfed
Copy link
Owner Author

snarfed commented Oct 15, 2023

For a second, I worried that this started to re-introduce the req't from some implementations like Mastodon that object ids are on the same domain as their author/actor's id, which made BF itself difficult back in the day, eg #16 (comment) .

Fortunately I don't think that's the case here. This is all about comparing AP actor and author/attributedTo ids themselves; it doesn't care about object/activity ids or WebFinger lookups at all. So in a bridge's case, all of these already have to be on the bridge's domain (eg fed.brid.gy), so we're fine.

snarfed added a commit that referenced this issue Oct 16, 2023
for #566. just logging for now, want to see if we're already hitting this at all.
@snarfed
Copy link
Owner Author

snarfed commented Oct 16, 2023

TODO: make task queue handlers admin only, pass authed_as to receive task.

@snarfed
Copy link
Owner Author

snarfed commented Oct 16, 2023

snarfed added a commit that referenced this issue Oct 16, 2023
…ctor

for #566. just logging for now, want to see if we're already hitting this at all.
@snarfed
Copy link
Owner Author

snarfed commented Oct 16, 2023

I implemented these, log-only to start, and got some interesting results.

First up: AP inbox forwarding makes this tricky. For example, we got this Create of a reply by @hamlin81@mastodon.social to a post by @NanoRaptor@bitbang.social. It was sent to us by bitbang.social and signed by @NanoRaptor, not by mastodon.social and signed by @hamlin81. Presumably an inbox forward.

OK, so we can't require that the signing user is always the activity's actor/author. Looks like the alternatives are:

  1. If the sig user doesn't match, fetch the activity by its id and use that instead. (This only works if the activity and actor are on the same domain, right?)
  2. If the activity has an LD sig, like Mastodon generates, check that instead. (Sigh.)
{
  "id": "https://mastodon.social/users/hamlin81/statuses/111246155046597039/activity",
  "type": "Create",
  "actor": "https://mastodon.social/users/hamlin81",
  "object": {
    "id": "https://mastodon.social/users/hamlin81/statuses/111246155046597039",
    "type": "Note",
    "inReplyTo": "https://bitbang.social/users/NanoRaptor/statuses/111244176519913170",
    "url": "https://mastodon.social/@hamlin81/111246155046597039",
    "attributedTo": "https://mastodon.social/users/hamlin81",
    "..."
  },
  "signature": {
    "type": "RsaSignature2017",
    "creator": "https://mastodon.social/users/hamlin81#main-key",
    "signatureValue": "eq8DBc2FZFwttF7VgkvRa+1Xwop1q98yj/GjhWbERq8o27i0BBRMMKIJg1sYI/wWdbN2ryw5aGxKCsaeoqJrILZ7SaQ0h1cX6RcSlhexCmRuXqyW7Jbc0bCv12XATJ8s0OlN3tD8wGpG/OxU/iE++MLtF6NsrcYXcZZKhOiUKRu7h02aI3fnRdwBPZmZAZNqVRXp9kUfITv8rV5VoMaTyIrae4V0+V9qyKK+4epT8vTuW70aFD4ScWIbmM9TogMetqhEpy/m3Cv+i9j17wopfdDky2PaYpzSkfaxUvoxMhXyQ0kLllwHHxKUwnAA8e8Va/pDlWPjFlEPDUz/wp6N6g=="
  }
}
@snarfed
Copy link
Owner Author

snarfed commented Oct 16, 2023

Interesting data point, we get a substantial number of inbox forwards, roughly 2 per min over the last 45m.

@snarfed
Copy link
Owner Author

snarfed commented Oct 19, 2023

@snarfed
Copy link
Owner Author

snarfed commented Oct 25, 2023

Got the ok on that writeup! Next step is to review the logs and implement these checks. After that, ideally I should abstract them across protocols, since this applies to at least some others too, eg web.

@snarfed snarfed added the now label Nov 29, 2023
@snarfed
Copy link
Owner Author

snarfed commented Feb 9, 2024

Current status: planning to implement LD Sig verification, but first I need to know how Mastodon canonicalizes the activity JSON before it signs it.

Complete example activity from Mastodon with an LD Sig:

{
  "@context": [
    "https://www.w3.org/ns/activitystreams",
    "https://w3id.org/security/v1",
    {
      "manuallyApprovesFollowers": "as:manuallyApprovesFollowers",
      "sensitive": "as:sensitive",
      "Hashtag": "as:Hashtag",
      "movedTo": {
        "@id": "as:movedTo",
        "@type": "@id"
      },
      "alsoKnownAs": {
        "@id": "as:alsoKnownAs",
        "@type": "@id"
      },
      "toot": "http://joinmastodon.org/ns#",
      "Emoji": "toot:Emoji",
      "featured": {
        "@id": "toot:featured",
        "@type": "@id"
      },
      "featuredTags": {
        "@id": "toot:featuredTags",
        "@type": "@id"
      },
      "schema": "http://schema.org#",
      "PropertyValue": "schema:PropertyValue",
      "value": "schema:value",
      "ostatus": "http://ostatus.org#",
      "atomUri": "ostatus:atomUri",
      "inReplyToAtomUri": "ostatus:inReplyToAtomUri",
      "conversation": "ostatus:conversation",
      "focalPoint": {
        "@container": "@list",
        "@id": "toot:focalPoint"
      },
      "blurhash": "toot:blurhash",
      "discoverable": "toot:discoverable",
      "indexable": "toot:indexable",
      "memorial": "toot:memorial",
      "votersCount": "toot:votersCount",
      "Device": "toot:Device",
      "Ed25519Signature": "toot:Ed25519Signature",
      "Ed25519Key": "toot:Ed25519Key",
      "Curve25519Key": "toot:Curve25519Key",
      "EncryptedMessage": "toot:EncryptedMessage",
      "publicKeyBase64": "toot:publicKeyBase64",
      "deviceId": "toot:deviceId",
      "claim": {
        "@type": "@id",
        "@id": "toot:claim"
      },
      "fingerprintKey": {
        "@type": "@id",
        "@id": "toot:fingerprintKey"
      },
      "identityKey": {
        "@type": "@id",
        "@id": "toot:identityKey"
      },
      "devices": {
        "@type": "@id",
        "@id": "toot:devices"
      },
      "messageFranking": "toot:messageFranking",
      "messageType": "toot:messageType",
      "cipherText": "toot:cipherText",
      "suspended": "toot:suspended"
    }
  ],
  "id": "https://libretooth.gr/users/chartrandsaintlouis/statuses/111902659083835796/activity",
  "type": "Create",
  "actor": "https://libretooth.gr/users/chartrandsaintlouis",
  "published": "2024-02-09T17:17:50Z",
  "to": [
    "https://www.w3.org/ns/activitystreams#Public"
  ],
  "cc": [
    "https://libretooth.gr/users/chartrandsaintlouis/followers",
    "https://jasette.facil.services/users/hs0ucy"
  ],
  "object": {
    "id": "https://libretooth.gr/users/chartrandsaintlouis/statuses/111902659083835796",
    "type": "Note",
    "inReplyTo": "https://jasette.facil.services/users/hs0ucy/statuses/111902446198482548",
    "published": "2024-02-09T17:17:50Z",
    "url": "https://libretooth.gr/@chartrandsaintlouis/111902659083835796",
    "attributedTo": "https://libretooth.gr/users/chartrandsaintlouis",
    "to": [
      "https://www.w3.org/ns/activitystreams#Public"
    ],
    "cc": [
      "https://libretooth.gr/users/chartrandsaintlouis/followers",
      "https://jasette.facil.services/users/hs0ucy"
    ],
    "sensitive": false,
    "atomUri": "https://libretooth.gr/users/chartrandsaintlouis/statuses/111902659083835796",
    "inReplyToAtomUri": "https://jasette.facil.services/users/hs0ucy/statuses/111902446198482548",
    "conversation": "tag:libretooth.gr,2024-02-04:objectId=48182059:objectType=Conversation",
    "content": "<p><span class=\"h-card\" translate=\"no\"><a href=\"https://jasette.facil.services/@hs0ucy\" class=\"u-url mention\">@<span>hs0ucy</span></a></span> </p><p>Oui, c&#39;est un livre int\u00e9ressant.</p><p>Bonne lecture !</p>",
    "contentMap": {
      "fr": "<p><span class=\"h-card\" translate=\"no\"><a href=\"https://jasette.facil.services/@hs0ucy\" class=\"u-url mention\">@<span>hs0ucy</span></a></span> </p><p>Oui, c&#39;est un livre int\u00e9ressant.</p><p>Bonne lecture !</p>"
    },
    "attachment": [],
    "tag": [
      {
        "type": "Mention",
        "href": "https://jasette.facil.services/users/hs0ucy",
        "name": "@hs0ucy@jasette.facil.services"
      }
    ],
    "replies": {
      "id": "https://libretooth.gr/users/chartrandsaintlouis/statuses/111902659083835796/replies",
      "type": "Collection",
      "first": {
        "type": "CollectionPage",
        "next": "https://libretooth.gr/users/chartrandsaintlouis/statuses/111902659083835796/replies?only_other_accounts=true&page=true",
        "partOf": "https://libretooth.gr/users/chartrandsaintlouis/statuses/111902659083835796/replies",
        "items": []
      }
    }
  },
  "signature": {
    "type": "RsaSignature2017",
    "creator": "https://libretooth.gr/users/chartrandsaintlouis#main-key",
    "created": "2024-02-09T17:17:50Z",
    "signatureValue": "iz9eLOyliXRazD6++l3VEOaCYHjtWFcsdXXmxOki4DdCMZ0Z1ZYGaCymrjKcgnDJoxlwfc16Y4bIfzx0QI9MnDzumzbb1RHutsVQycFMUPrCkqO2JpZu/fJ2rigdmMNUtAUijPst4sEJOM79ejcyoD4vMrv5qHdFDQmiqTm6fA7whveyFVvHmyW59MgDiF9CfGgddmw/8zCu8k3x7fhEOJjOWg5xMO2obaD4trOrBGfIm5Ize0tHL1PuEFiTEEhf1sOxryeMPUUzouCA17KRqaqglhxwUgsSWb27M2ZW9kiq5qfKN4fZq0CPbwEXIy1IiMnASMV9abv5PxZDCk4pXQ=="
  }
}
@snarfed
Copy link
Owner Author

snarfed commented Feb 9, 2024

Aha, Claire says

this is defined in app/lib/activitypub/linked_data_signature.rb and app/helpers/jsonld_helper.rb (canonicalize)

@snarfed
Copy link
Owner Author

snarfed commented Feb 9, 2024

Code is:

graph = RDF::Graph.new << JSON::LD::API.toRdf(json, documentLoader: method(:load_jsonld_context))
graph.dump(:normalize)

The second line comes from https://github.com/ruby-rdf/rdf-normalize, docs at https://ruby-rdf.github.io/rdf-normalize/, JSON ser/de from https://github.com/ruby-rdf/json-ld . I can't find an actual description of the normalization algorithm anywhere there though, so I'm getting set up to run it myself and see.

@snarfed
Copy link
Owner Author

snarfed commented May 24, 2024

Finally getting back to looking at this. I'm now inclined to just skip LD Sigs for now and drop those activities instead of handling them. Need to look at a sample first though to confirm that I'm ok missing them.

@snarfed
Copy link
Owner Author

snarfed commented May 25, 2024

OK! Apart from inbox forwarding, one source of activities we're getting that don't pass authz is Guppe Groups. Looks like they similarly forward activities, with a new HTTP Sig from the group's actor, but there's no LD Sig from the original actor, so we can't verify the activities.

This example included HTTP Sig by the group AP actor https://a.gup.pe/u/allstartrek:

{
  "type": "Create",
  "id": "https://mindly.social/users/joewynne/statuses/112499304245297194/activity",
  "actor": "https://mindly.social/users/joewynne",
  "cc": [
    "https://mindly.social/users/joewynne/followers",
    "https://a.gup.pe/u/allstartrek",
    "https://a.gup.pe/u/allstartrek/followers"
  ],
  "object": {
    "type": "Note",
    "id": "https://mindly.social/users/joewynne/statuses/112499304245297194",
    "url": "https://mindly.social/@joewynne/112499304245297194"
    "attributedTo": "https://mindly.social/users/joewynne",
    "to": "as:Public",
    "cc": [
      "https://mindly.social/users/joewynne/followers",
      "https://a.gup.pe/u/allstartrek",
      "https://a.gup.pe/u/allstartrek/followers"
    ],
    "content": "...",
    "published": "2024-05-25T02:12:33Z",
  },
  "published": "2024-05-25T02:12:33Z",
  "to": "as:Public"
}
@snarfed
Copy link
Owner Author

snarfed commented May 25, 2024

@snarfed
Copy link
Owner Author

snarfed commented May 25, 2024

Next up! GoToSocial. Its actors' publicKey.ids are a separate URL, on a sub-path of the actor id, that serve a minimal version of the actor (without requiring a signed GET) that only include the key. Totally fine, we just need to handle this in our sig verification.

Example: actor https://social.chriswb.dev/users/chrisw_b :

{
  "type": "Person",
  "id": "https://social.chriswb.dev/users/chrisw_b",
  "url": "https://social.chriswb.dev/@chrisw_b"
  "alsoKnownAs": ["https://teal.social/users/chrisw_b"],
  "name": "chris b 💖",
  "preferredUsername": "chrisw_b",
  "publicKey": {
    "id": "https://social.chriswb.dev/users/chrisw_b/main-key",
    "owner": "https://social.chriswb.dev/users/chrisw_b",
    "publicKeyPem": "..."
  },
  "..."
}

...and publicKey.id https://social.chriswb.dev/users/chrisw_b/main-key :

{
   "type" : "Person"
   "id" : "https://social.chriswb.dev/users/chrisw_b",
   "preferredUsername" : "chrisw_b",
   "publicKey" : {
      "id" : "https://social.chriswb.dev/users/chrisw_b/main-key",
      "owner" : "https://social.chriswb.dev/users/chrisw_b",
      "publicKeyPem" : "..."
   },
}
@qazmlp
Copy link

qazmlp commented May 25, 2024

^ filed immers-space/guppe#106

IIrc this won't work in all cases though, since as far as I've heard mentioned it's possible to switch some servers into something called "secure mode" so that they don't serve forwardable JSON-LD signatures on content. But I very much haven't read up on this myself.

I think a good fallback for this case would be to re-fetch from the id to authenticate, either throwing away the forwarded data or (ideally) checking that it all matches so that there can be no disagreement about what was boosted.

(I really hope either that or at least the fresh fetch is what Guppe does in this situation.)

@snarfed
Copy link
Owner Author

snarfed commented May 28, 2024

Next: Bluesky app.bsky.feed.generator records. They have their own DIDs, which aren't (necessarily) unique or the same as the repo that they get published in, eg all feeds from SkyFeed get the same DID, did:web:skyfeed.me. Example from the did:plc:ffklbxnlk3kpwkyr4oxngp5q repo:

{
  "$type": "app.bsky.feed.generator",
  "did": "did:web:skyfeed.me",
  "avatar": "...",
  "createdAt": "2024-05-28T13:14:10.044Z",
  "description": "...",
  "displayName": "\u304a\u3046\u3061\u306e\u9ce5\u90e8",
  "skyfeedBuilder": "...",
}
@snarfed
Copy link
Owner Author

snarfed commented May 30, 2024

Getting close! I've done a ton of work on this ^ over the last few days. I've covered over all of the cases seen in the wild over the last two weeks, except for one in RSS/Atom ingest that I'll fix later in #829. Otherwise, I'll watch it log-only for a couple more days, see if there's anything new, then turn it on.

@snarfed
Copy link
Owner Author

snarfed commented Jun 3, 2024

^ Got a few more hits over the last four days, besides RSS/Atom ingest #829, but not many. Details below, interestingly they all involve momostr.pink. In any case, I think I'm ready to turn this on, as soon as I can update the tests to handle it.

Auth: https://momostr.pink/users/npub1dww6jgxykmkt7tqjqx985tg58dxlm7v83sa743578xa4j7zpe3hql6pdnf isn't https://momostr.pink/notes/note1rlsjyulj9x39s6q3q82n00suvfcjcyyrnky04v0fymu9cwkr2c2stueajd 's author or actor: ['https://momostr.pink/users/npub1r0rs5q2gk0e3dk3nlc7gnu378ec6cnlenqp8a3cjhyzu6f8k5sgs4sq9ac', 'https://momostr.pink/notes/note1rlsjyulj9x39s6q3q82n00suvfcjcyyrnky04v0fymu9cwkr2c2stueajd']
Auth: would cowardly refuse to overwrite bsky.brid.gy/followers#accept-https://momostr.pink/follow/npub1qnmamgyup683z9ehn40jrdgryjhn8qlpntwzqsrk8r80n3xspdrq4r245g/https%3A%2F%2Fbsky%2Ebrid%2Egy%2Fbsky%2Ebrid%2Egy without checking actor
Auth: would cowardly refuse to overwrite did:plc:p2cp5gopk7mgjegy6wadk3ep/followers#accept-https://momostr.pink/follow/npub1k979np6dcpwh7mkfwk7wq3msezml48fh7wksp9hakakf8pwk3y5qhdz7te/https%3A%2F%2Fbsky%2Ebrid%2Egy%2Fap%2Fdid%3Aplc%3Ap2cp5gopk7mgjegy6wadk3ep without checking actor
Auth: would cowardly refuse to overwrite did:plc:ak6xsotudhfibusxxtiqan6b/followers#accept-https://momostr.pink/follow/npub1qnmamgyup683z9ehn40jrdgryjhn8qlpntwzqsrk8r80n3xspdrq4r245g/https%3A%2F%2Fbsky%2Ebrid%2Egy%2Fap%2Fdid%3Aplc%3Aak6xsotudhfibusxxtiqan6b without checking actor
Auth: would cowardly refuse to overwrite bsky.brid.gy/followers#accept-https://momostr.pink/follow/npub1qnmamgyup683z9ehn40jrdgryjhn8qlpntwzqsrk8r80n3xspdrq4r245g/https%3A%2F%2Fbsky%2Ebrid%2Egy%2Fbsky%2Ebrid%2Egy without checking actor
Auth: would cowardly refuse to overwrite bsky.brid.gy/followers#accept-https://momostr.pink/follow/npub1uf9a0mvyvx7c449476h7e5zy5xd5yfcl7vpxcsz5g0udas2nht8qd55400/https%3A%2F%2Fbsky%2Ebrid%2Egy%2Fbsky%2Ebrid%2Egy without checking actor
snarfed added a commit that referenced this issue Jun 3, 2024
snarfed added a commit that referenced this issue Jun 3, 2024
snarfed added a commit that referenced this issue Jun 4, 2024
trying to collect them into two top-level Error Reporting errors, with details for each one in the user field

for #566
@snarfed
Copy link
Owner Author

snarfed commented Jun 5, 2024

It's alive, it's alive!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants