Skip to content
Snippets Groups Projects
This project is mirrored from https://akkoma.dev/AkkomaGang/akkoma.git. Pull mirroring updated .
  1. Mar 30, 2024
  2. Mar 29, 2024
  3. Mar 27, 2024
  4. Mar 26, 2024
    • Oneric's avatar
      Only allow exact id matches · 8684964c
      Oneric authored
      This protects us from falling for obvious spoofs as from the current
      upload exploit (unfortunately we can’t reasonably do anything about
      spoofs with exact matches as was possible via emoji and proxy).
      
      Such objects being invalid is supported by the spec, sepcifically
      sections 3.1 and 3.2: https://www.w3.org/TR/activitypub/#obj-id
      
      Anonymous objects are not relevant here (they can only exists within
      parent objects iiuc) and neither is client-to-server or transient objects
      (as those cannot be fetched in the first place).
      This leaves us with the requirement for `id` to (a) exist and
      (b) be a publicly dereferencable URI from the originating server.
      This alone does not yet demand strict equivalence, but the spec then
      further explains objects ought to be fetchable _via their ID_.
      Meaning an object not retrievable via its ID, is invalid.
      
      This reading is supported by the fact, e.g. GoToSocial (recently) and
      Mastodon (for 6+ years) do already implement such strict ID checks,
      additionally proving this doesn’t cause federation issues in practice.
      
      However, apart from canonical IDs there can also be additional display
      URLs. *omas first redirect those to their canonical location, but *keys
      and Mastodon directly serve the AP representation without redirects.
      
      Mastodon and GTS deal with this in two different ways,
      but both constitute an effective countermeasure:
       - Mastodon:
         Unless it already is a known AP id, two fetches occur.
         The first fetch just reads the `id` property and then refetches from
         the id. The last fetch requires the returned id to exactly match the
         URL the content was fetched from. (This can be optimised by skipping
         the second fetch if it already matches)
         https://github.com/mastodon/mastodon/blob/05eda8d19330a9c27c0cf07de19a87edff269057/app/helpers/jsonld_helper.rb#L168
         https://github.com/mastodon/mastodon/commit/63f097979990bf5ba9db848b8a253056bad781af
      
       - GTS:
         Only does a single fetch and then checks if _either_ the id
         _or_ url property (which can be an object) match the original fetch
         URL. This relies on implementations always including their display URL
         as "url" if differing from the id. For actors this is true for all
         investigated implementations, for posts only Mastodon includes an
         "url", but it is also the only one with a differing display URL.
         https://github.com/superseriousbusiness/gotosocial/commit/2bafd7daf542d985ee76d9079a30a602cb7be827#diff-943bbb02c8ac74ac5dc5d20807e561dcdfaebdc3b62b10730f643a20ac23c24fR222
      
      Albeit Mastodon’s refetch offers higher compatibility with theoretical
      implmentations using either multiple different display URL or not
      denoting any of them as "url" at all, for now we chose to adopt a
      GTS-like refetch-free approach to avoid additional implementation
      concerns wrt to whether redirects should be allowed when fetching a
      canonical AP id and potential for accidentally loosening some checks
      (e.g. cross-domain refetches) for one of the fetches.
      This may be reconsidered in the future.
      8684964c
    • Oneric's avatar
      Update user reference after fetch · 48b3a357
      Oneric authored
      Since we always followed redirects (and until recently allowed fuzzy id
      matches), the ap_id of the received object might differ from the iniital
      fetch url. This lead to us mistakenly trying to insert a new user with
      the same nickname, ap_id, etc as an existing user (which will fail due
      to uniqueness constraints) instead of updating the existing one.
      48b3a357
    • Oneric's avatar
      Ensure object id doesn’t change on refetch · 9061d148
      Oneric authored
      9061d148
    • Oneric's avatar
      fetcher: return final URL after redirects from get_object · 3e134b07
      Oneric authored
      Since we reject cross-domain redirects, this doesn’t yet
      make a difference, but it’s requried for stricter checking
      subsequent commits will introduce.
      
      To make sure (and in case we ever decide to reallow
      cross-domain redirects) also use the final location
      for containment and reachability checks.
      3e134b07
    • Oneric's avatar
      Sanity check fetched user data · f07eb4cb
      Oneric authored
      In order to properly process incoming notes we need
      to be able to map the key id back to an actor.
      Also, check collections actually belong to the same server.
      
      Key ids of Hubzilla and Bridgy samples were updated to what
      modern versions of those output. If anything still uses the
      old format, we would not be able to verify their posts anyway.
      f07eb4cb
    • Oneric's avatar
      Never fetch resource from ourselves · 59a142e0
      Oneric authored
      If it’s not already in the database,
      it must be counterfeit (or just not exists at all)
      
      Changed test URLs were only ever used from "local: false" users anyway.
      59a142e0
    • Oneric's avatar
      Move actor check into fetch_and_contain_remote_object_from_id · fee57eb3
      Oneric authored
      This brings it in line with its name and closes an,
      in practice harmless, verification hole.
      
      This was/is the only user of contain_origin making it
      safe to change the behaviour on actor-less objects.
      
      Until now refetched objects did not ensure the new actor matches the
      domain of the object. We refetch polls occasionally to retrieve
      up-to-date vote counts. A malicious AP server could have switched out
      the poll after initial posting with a completely different post
      attribute to an actor from another server.
      While we indeed fell for this spoof before the commit,
      it fortunately seems to have had no ill effect in practice,
      since the asociated Create activity is not changed. When exposing the
      actor via our REST API, we read this info from the activity not the
      object.
      
      This at first thought still keeps one avenue for exploit open though:
      the updated actor can be from our own domain and a third server be
      instructed to fetch the object from us. However this is foiled by an
      id mismatch. By necessity of being fetchable and our longstanding
      same-domain check, the id must still be from the attacker’s server.
      Even the most barebone authenticity check is able to sus this out.
      fee57eb3
    • Oneric's avatar
      Reject cross-domain redirects when fetching AP objects · c4cf4d7f
      Oneric authored
      Such redirects on AP queries seem most likely to be a spoofing attempt.
      If the object is legit, the id should match the final domain anyway and
      users can directly use the canonical URL.
      
      The lack of such a check (and use of the initially queried domain’s
      authority instead of the final domain) was enabling the current exploit
      to even affect instances which already migrated away from a same-domain
      upload/proxy setup in the past, but retained a redirect to not break old
      attachments.
      
      (In theory this redirect could, with some effort, have been limited to
       only old files, but common guides employed a catch-all redirect, which
       allows even future uploads to be reachable via an initial query to the
       main domain)
      
      Same-domain redirects are valid and also used by ourselves,
      e.g. for redirecting /notice/XXX to /objects/YYY.
      c4cf4d7f
    • Oneric's avatar
      Update spoofed activity test · baaeffde
      Oneric authored
      Turns out we already had a test for activities spoofed via upload due
      to an exploit several years. Back then *oma did not verify content-type
      at all and doing so was the only adopted countermeasure.
      Even the added test sample though suffered from a mismatching id, yet
      nobody seems to have thought it a good idea to tighten id checks, huh
      
      Since we will add stricter id checks later, make id and URL match
      and also add a testcase for no content type at all. The new section
      will be expanded in subsequent commits.
      baaeffde
    • Oneric's avatar
      Document Pleroma.Object.Fetcher · 2bcf633d
      Oneric authored
      2bcf633d
  5. Mar 19, 2024
    • Oneric's avatar
      mix: fix docs task · 93ab6a01
      Oneric authored
      93ab6a01
    • Oneric's avatar
      Refactor Fetcher.get_object for readability · c806adbf
      Oneric authored
      Apart from slightly different error reasons wrt content-type,
      this does not change functionality in any way.
      c806adbf
    • Oneric's avatar
      Proactively harden emoji pack against path traversal · ddd79ff2
      Oneric authored
      No new path traversal attacks are known. But given the many entrypoints
      and code flow complexity inside pack.ex, it unfortunately seems
      possible a future refactor or addition might reintroduce one.
      Furthermore, some old packs might still contain traversing path entries
      which could trigger undesireable actions on rename or delete.
      
      To ensure this can never happen, assert safety during path construction.
      
      Path.safe_relative was introduced in Elixir 1.14, but
      fortunately, we already require at least 1.14 anyway.
      ddd79ff2
    • Oneric's avatar
      StealEmoji: check remote size before downloading · d6d838cb
      Oneric authored
      To save on bandwith and avoid OOMs with large files.
      Ofc, this relies on the remote server
       (a) sending a content-length header and
       (b) being honest about the size.
      
      Common fedi servers seem to provide the header and (b) at least raises
      the required privilege of an malicious actor to a server infrastructure
      admin of an explicitly allowed host.
      
      A more complete defense which still works when faced with
      a malicious server requires changes in upstream Finch;
      see https://github.com/sneako/finch/issues/224
      d6d838cb
    • Oneric's avatar
      6d003e1a
    • Oneric's avatar
      d1ce5fd9
    • Oneric's avatar
      StealEmoji: make final paths infeasible to predict · a4fa2ec9
      Oneric authored
      Certain attacks rely on predictable paths for their payloads.
      If we weren’t so overly lax in our (id, URL) check, the current
      counterfeit activity exploit would be one of those.
      It seems plausible for future attacks to hinge on
      or being made easier by predictable paths too.
      
      In general, letting remote actors place arbitrary data at
      a path within our domain of their choosing (sans prefix)
      just doesn’t seem like a good idea.
      
      Using fully random filenames would have worked as well, but this
      is less friendly for admins checking emoji dirs.
      The generated suffix should still be more than enough;
      an attacker needs on average 140 trillion attempts to
      correctly guess the final path.
      a4fa2ec9
    • Oneric's avatar
      test: use pack functions to check for emoji · ee5ce878
      Oneric authored
      The hardocded path and filenames assumptions
      will be broken with the next commit.
      ee5ce878
    • Oneric's avatar
      Convert StealEmoji to pack.json · d1c4d074
      Oneric authored
      This will decouple filenames from shortcodes and
      allow more image formats to work instead of only
      those included in the auto-load glob. (Albeit we
      still saved other formats to disk, wasting space)
      
      Furthermore, this will allow us to make
      final URL paths infeasible to predict.
      d1c4d074
    • Oneric's avatar
      Fill out path for newly created packs · fa98b44a
      Oneric authored
      Before this was only filled on loading the pack again,
      preventing the created pack from being used directly.
      fa98b44a
    • Oneric's avatar
      StealEmoji: drop superfluous basename · 5b126567
      Oneric authored
      Since 3 commits ago we restrict shortcodes to a subset of
      the POSIX Portable Filename Character Set, therefore
      this can never have a directory component.
      5b126567
    • Oneric's avatar
      StealEmoji: use Content-Type and reject non-images · a8c6c780
      Oneric authored
      E.g. *key’s emoji URLs typically don’t have file extensions, but
      until now we just slapped ".png" at its end hoping for the best.
      
      Furthermore, this gives us a chance to actually reject non-images,
      which before was not feasible exatly due to those extension-less URLs
      a8c6c780
    • Oneric's avatar
      111cdb0d
    • Norm's avatar
      Limit emoji stealer to alphanum, dash, or underscore characters · af041db6
      Norm authored
      As suggested in b387f4a1, only steal
      emoji with alphanumerc, dash, or underscore characters.
      
      Also consolidate all validation logic into a single function.
      
      ===
      
      Taken from akkoma#703 with cosmetic tweaks
      
      This matches our existing validation logic from Pleroma.Emoji,
      and apart from excluding the dot also POSIX’s Portable Filename
      Character Set making it always safe for use in filenames.
      
      Mastodon is even stricter also disallowing U+002D HYPEN-MINUS
      and requiring at least two characters.
      
      Given both we and Mastodon reject shortcodes excluded
      by this anyway, this doesn’t seem like a loss.
      af041db6
    • Oneric's avatar
      Update example nginx config · fb54c47f
      Oneric authored
      To account for our subdomain recommendations
      fb54c47f
    • Oneric's avatar
      Drop media proxy same-domain default for base_url · fc36b040
      Oneric authored
      Even more than with user uploads, a same-domain proxy setup bears
      significant security risks due to serving untrusted content under
      the main domain space.
      
      A risky setup like that should never be the default.
      fc36b040
    • Oneric's avatar
      Sanitise Content-Type of media proxy URLs · 11ae8344
      Oneric authored
      Just as with uploads and emoji before, this can otherwise be used
      to place counterfeit AP objects or other malicious payloads.
      In this case, even if we never assign a priviliged type to content,
      the remote server can and until now we just mimcked whatever it told us.
      
      Preview URLs already handle only specific, safe content types
      and redirect to the external host for all else; thus no additional
      sanitisiation is needed for them.
      
      Non-previews are all delegated to the modified ReverseProxy module.
      It already has consolidated logic for building response headers
      making it easy to slip in sanitisation.
      
      Although proxy urls are prefixed by a MAC built from a server secret,
      attackers can still achieve a perfect id match when they are able to
      change the contents of the pointed to URL. After sending an posts
      containing an attachment at a controlled destination, the proxy URL can
      be read back and inserted into the payload. After injection of
      counterfeits in the target server the content can again be changed
      to something innocuous lessening chance of detection.
      11ae8344
    • Oneric's avatar
      Never automatically assign privileged content types · bcc528b2
      Oneric authored
      By mapping all extensions related to our custom privileged types
      back to innocuous text/plain, our custom types will never automatically
      be inserted which was one of the factors making impersonation possible.
      
      Note, this does not invalidate the upload and emoji Content-Type
      restrictions from previous commits. Apart from counterfeit AP objects
      there are other payloads with standard types this protects against,
      e.g. *.js Javascript payloads as used in prior frontend injections.
      bcc528b2
    • Oneric's avatar
      Fix Content-Type of our schema · e88d0a28
      Oneric authored
      Strict servers fail to process anything from us otherwise.
      
      Fixes: akkoma#716
      e88d0a28
Loading