[Security] StealEmojiPolicy: Sanitize shortcodes #701

Merged

floatingghost merged 2 commits from erincandescent/akkoma:stealemojipolicy-sanitize into develop

2024-02-20 15:08:55 +00:00

erincandescent commented

2024-02-20 10:22:46 +00:00

Contributor

Pleroma announced this one as a security vuln and our code is identical here. It sems to allow directory traversal, which would allow a stolen emoji to smash over one from a pack.

I wonder if we should also hardcode a list of emoji names which are always banned such as "." and "..". Then again, are dots even allowed in shortcodes? Can we just ban shortcodes with dots?

Pleroma announced this one as a security vuln and our code is identical here. It sems to allow directory traversal, which would allow a stolen emoji to smash over one from a pack. I wonder if we should also hardcode a list of emoji names which are always banned such as `"."` and `".."`. Then again, are dots even allowed in shortcodes? Can we just ban shortcodes with dots?

erincandescent added 1 commit 2024-02-20 10:22:46 +00:00

ci/woodpecker/pr/build-amd64 Pipeline is pending Details

ci/woodpecker/pr/build-arm64 Pipeline is pending Details

ci/woodpecker/pr/docs Pipeline is pending Details

ci/woodpecker/pr/lint Pipeline is pending Details

ci/woodpecker/pr/test Pipeline is pending Details

7d94476dd6 StealEmojiPolicy: Sanitize shortcodes

Closes: https://git.pleroma.social/pleroma/pleroma/-/issues/3245

erincandescent added 1 commit 2024-02-20 10:36:21 +00:00

ci/woodpecker/pr/lint Pipeline failed Details

ci/woodpecker/pr/test unknown status Details

ci/woodpecker/pr/build-arm64 unknown status Details

ci/woodpecker/pr/build-amd64 unknown status Details

ci/woodpecker/pr/docs unknown status Details

b387f4a1c1 Don't steal emoji who's shortcodes have dots or colons in their name

Mastodon at the very least seems to prevent the creation of emoji with
dots in their name (and refuses to accept them in federation). It feels
like being cautious in what we accept is reasonable here.

Colons are the emoji separator and so obviously should be blocked.

Perhaps instead of filtering out things like this we should just
do a regex match on `[a-zA-Z0-9_-]`? But that's plausibly a decision
for another day

    Perhaps we should also have a centralised "is this a valid emoji shortcode?"
    function

erincandescent commented

2024-02-20 10:37:07 +00:00

Author

Contributor

You know what, I don't see a good reason to allow dots or colons, so we should probably block those too

You know what, I don't see a _good_ reason to allow dots or colons, so we should probably block those too

erincandescent commented

2024-02-20 10:45:16 +00:00

Author

Contributor

The fact that this just takes the extension from the origin seems potentially smelly to me (emoji.activitystreams2 anyone?) but at least this leaves things in a safer state than we found it

The fact that this just takes the extension from the origin seems potentially smelly to me (`emoji.activitystreams2` anyone?) but at least this leaves things in a safer state than we found it