LemmyNet / lemmy-ui

The official web app for lemmy.
https://join-lemmy.org/
GNU Affero General Public License v3.0
890 stars 334 forks source link

Possible XSS attack #1895

Closed NomNuggetNom closed 1 year ago

NomNuggetNom commented 1 year ago

Requirements

Summary

The sidebar dangerously sets HTML but does not configure the Markdown render to strip HTML codes. This enables simple XSS attacks like <img onload="maliciousCodeHere()" />. It seems like an attempt is made to create a markdown renderer with HTML disabled, however.

It now seems that this attack might be done via custom emojis.

Steps to Reproduce

Technical Details

markdown-it does some extremely simple guarding, but they don't claim to prevent XSS. Custom HTML should be removed in favor of plugins.

Lemmy Instance Version

0.18.1

Lemmy Instance URL

No response

geneccx commented 1 year ago

The issue is not with the sidebar, but the markdown renderer. Anywhere that markdown can be used is a vector.

I've emailed details to the devs, but @makotech222 has already identified a fix.

tgxn commented 1 year ago

Also, this needs to be fixed on the backend, client-side validation is never enough. @geneccx - Where can I find the fix that you mention @makotech222 contributed on?

geneccx commented 1 year ago

Also, this needs to be fixed on the backend, client-side validation is never enough. @geneccx - Where can I find the fix that you mention @makotech222 contributed on?

1897

calculuschild commented 1 year ago

@NomNuggetNom please keep the issue open until it has been fixed. We want to be sure the Devs see this.

geneccx commented 1 year ago

After implementing this fix, you will likely also need to invalidate all existing sessions (logging everyone out) due to https://github.com/LemmyNet/lemmy/issues/3499

In the meantime,

From the matrix chat:

Current mitigations

  1. Remove custom emoji

    DELETE FROM custom_emoji_keyword;
    DELETE FROM custom_emoji;
  2. Overwrite content with the exploit

    UPDATE comment SET content = '<REMOVED BY ADMIN>' WHERE content LIKE '%![" onload%';
    UPDATE private_message SET content = '<REMOVED BY ADMIN>' WHERE content LIKE '%![" onload%';
    UPDATE post SET body = '<REMOVED BY ADMIN>' WHERE body LIKE '%![" onload%';
    UPDATE post SET name = '<REMOVED BY ADMIN>' WHERE name LIKE '%![" onload%';
  3. Rotate your JWT secret (invalidates all current login sessions)

    -- back up your secret first, just in case
    SELECT * FROM secret;
    -- generate a new secret
    UPDATE secret SET jwt_secret = gen_random_uuid();
  4. Restart lemmy to make use of the new secret.

AlmightySnoo commented 1 year ago

Both the sidebar and the legal information field don't seem to escape strings? The "Legal" page in lemmy.world is allowing malicious JavaScript to run right now.

geneccx commented 1 year ago

Both the sidebar and the legal information field don't seem to escape strings? The "Legal" page in lemmy.world is allowing malicious JavaScript to run right now.

It's custom emoji rendered anywhere. The mitigation is above, deleting your custom emojis until the fix is in place, and invalidate all sessions.

AlmightySnoo commented 1 year ago

Both the sidebar and the legal information field don't seem to escape strings? The "Legal" page in lemmy.world is allowing malicious JavaScript to run right now.

It's custom emoji rendered anywhere.

EDIT: I'm wrong, it's indeed the emoji.

arifwn commented 1 year ago

Rotate your JWT secret (invalidates all current login sessions)

Looks like you'll need to restart lemmy to make it uses the new secret.

geneccx commented 1 year ago

Rotate your JWT secret (invalidates all current login sessions)

Looks like you'll need to restart lemmy to make it uses the new secret.

Thanks, I'll add that to the list of steps.

geneccx commented 1 year ago

@tgxn teaching more people how to exploit it in the wild probably isn't the best idea...

tgxn commented 1 year ago

@tgxn teaching people how to exploit it in the wild probably isn't the best idea...

Sure, I'll delete the post; but it's not like malicious actors care, we know that it's specifically:

  1. Custom Emoijs are being exploited.
  2. Site admins removing custom emojis should remove this attack vector.
abhibeckert commented 1 year ago

@tgxn teaching people how to exploit it in the wild probably isn't the best idea...

It was already being executed in the wild on some of the largest instances before this issue was created.

geneccx commented 1 year ago

It was already being executed in the wild on some of the largest instances before this issue was created.

I am aware. The fix and mitigations came after discovering it being used in the wild. The point still stands, until more servers have had the chance to patch this up, and the Lemmy devs have had a chance to release the fix, sharing the specifics of how to execute the exploit can only cause more harm than good.

StephenWetzel commented 1 year ago
UPDATE comment SET content = '<REMOVED BY ADMIN>' WHERE content LIKE '%![" onload%';
UPDATE private_message SET content = '<REMOVED BY ADMIN>' WHERE content LIKE '%![" onload%';
UPDATE post SET body = '<REMOVED BY ADMIN>' WHERE body LIKE '%![" onload%';
UPDATE post SET name = '<REMOVED BY ADMIN>' WHERE name LIKE '%![" onload%';

Should these be ILIKE so the attackers can't just change the case and escape detection?

stirante commented 1 year ago

Maybe quick and easy way to fix it would be to either use https://www.npmjs.com/package/safe-marked or adapt current solution to use https://www.npmjs.com/package/dompurify EDIT: Also found this plugin for existing markdown-it library: https://www.npmjs.com/package/markdown-it-dompurify

As always no user input can be trusted.

Nutomic commented 1 year ago

What makes you think that custom emojis are used as the attack vector? The fact is that emojis can only be set by local admins, and they dont federate. So I fail to see how an attack could set emojis, unless he already gained admin privileges through another attack vector.

dcx commented 1 year ago

What makes you think that custom emojis are used as the attack vector? The fact is that emojis can only be set by local admins, and they dont federate. So I fail to see how an attack could set emojis, unless he already gained admin privileges through another attack vector.

I'm doing some triage right now. I'm not sure if I fully understand this yet, but this is in my instance's database. I think this means it will be injected if our users click into the infected discussion?

252295989-66f0236f-7eb6-4862-8c67-24e36150d5a3

(Edit: Scrubbed to minimise proliferation)

abhibeckert commented 1 year ago

I think this means it will be injected if our users click into the infected discussion?

Anyone has viewed a page where that onload code executes has had their login token sent to a third party server. Allowing whoever runs that server to impersonate them.

If you haven't already, you should take your instance offline until this has been sorted out.

sunaurus commented 1 year ago

Those comments from the screenshot are all from lemmy.world, so they would not have any effect on the poster's instance. Or @abhibeckert are you aware of an additional vulnerability other than what has been discussed here?

abhibeckert commented 1 year ago

Those comments from the screenshot are all from lemmy.world, so they would not have any effect on the poster's instance. Or @abhibeckert are you aware of an additional vulnerability other than what has been discussed here?

No if I was aware of anything you guys don't know I would've offered it.

I was just answering their question, since they seemed to be unclear what the code does (and what the risk is).

SheriffJake commented 1 year ago

I'm doing some triage right now. I'm not sure if I fully understand this yet, but this is in my instance's database. I think this means it will be injected if our users click into the infected discussion?

image

That code resolves to a url: https://zelensky[.zip]/save/navAdmin Virustotal has so far only one detection with it, but that might change soon: https://www.virustotal.com/gui/url/c2bb82f2bd01786c16f4425544e9d5d080fe3302fa139eb67ad1744890f95e4f?nocache=1

dcx commented 1 year ago

As a precaution, I've run the measures suggested by @geneccx above (thanks!) and disabled federation for now. I've provided the full list of ten below for convenience. Removed to minimise proliferation, DM me if you're a lemmy dev and would like the code.

Hmm. This is not important for me personally now, but I still don't yet understand how this doesn't affect users on my local instance, who view the "infected" comment (which came from the lemmy.world federated post, but was living in my db). It seems to run on every browser onLoad and send the user's cookie.

I'm not a frontend guy. I assume by the developers' comments that these must be escaped in some way.

phiresky commented 1 year ago

Mentioning #1900 which should prevent this and most other XSS attacks without modifying the db or other code (barely tested)

finnim commented 1 year ago

Can someone please confirm if instances without custom emoji are unaffected?

DarkIrata commented 1 year ago

How does blacklisted words get handled when federated? It could be a workaround to blacklist 'onload' as a word. Tested on my own instance with private messages, it did filtere it and prevented execution.

NettoHikari commented 1 year ago

Can someone please confirm if instances without custom emoji are unaffected?

I never had any custom emjos. I reacted pretty quickly right after I woke up, took my instance offline and patched it with the aforementioned patch in the PR by Makotech222.

So far, nothing has happened to my instance (before and after the patch).

BxOxSxS commented 1 year ago

I'm doing some triage right now. I'm not sure if I fully understand this yet, but this is in my instance's database. I think this means it will be injected if our users click into the infected discussion? image

That code resolves to a url: https://zelensky[.zip]/save/navAdmin Virustotal has so far only one detection with it, but that might change soon: https://www.virustotal.com/gui/url/c2bb82f2bd01786c16f4425544e9d5d080fe3302fa139eb67ad1744890f95e4f?nocache=1

You are wrong about the url. The code fetch url https://zelensky.[zip]/save/{x}, where x is encoded base64 string of cookies (document.cookie) and content of html element with id navAdmin (content is not actually sent as it changes to [object HTMLScriptElement] right for encoding to base64). Only admins have such element (on the navigation bar) so for regular users it will be null. This way attacker quickly gets information if the stolen cookie belongs to an admin

rcmaehl commented 1 year ago

I've created #1902 although I should probably copy the ticket to the lemmy backend as well

AlmightySnoo commented 1 year ago

How does blacklisted words get handled when federated? It could be a workaround to blacklist 'onload' as a word. Tested on my own instance with private messages, it did filtere it and prevented execution.

I don't think that's a good idea, what if people simply want to discuss "onload" in a Javascript or HTML community?

AlmightySnoo commented 1 year ago

Those comments from the screenshot are all from lemmy.world, so they would not have any effect on the poster's instance. Or @abhibeckert are you aware of an additional vulnerability other than what has been discussed here?

Other instances like beehaw.org were also affected. Also, the comments are not only from lemmy.world, check (disable Javascript to be safe and load in a private browsing window) https://lemmy.blahaj.zone/search?q=onload%3D&type=All&listingType=All&page=1&sort=TopAll

There's in particular a malicious comment by 2pac@forum.basedcount.com over there. And the JavaScript that they're trying to inject is trying to steal admin cookies, and given how Lemmy.world's admin MichelleG got her account stolen, and it also happened to beehaw.org and lemmy.blahaj.zone, that means that those custom emojis in the federated comments are likely to be the XSS vectors. It remains to see where exactly in lemmy-ui those comments aren't escaped properly.

db0 commented 1 year ago

Beehaw wasn't affected from what I know. They just went dark out of an abundance of caution

AlmightySnoo commented 1 year ago

I just found where the XSS is still live outside lemmy.world: https://forum.basedcount.com/post/65325

Please load only with JavaScript disabled, it has a cookie stealer.

rcmaehl commented 1 year ago

I just found where the XSS is still live outside lemmy.world: https://forum.basedcount.com/post/65325

Please load only with JavaScript disabled, it has a cookie stealer.

Please escape the URL by wrapping it in ` s for people that click before they read

DarkIrata commented 1 year ago

AlmightySnoo commented Jul 10, 2023

i know, thats why i said workaround. It would be more than just waiting for an update and gettting users cookies snatched in the time. If everyone disbles federation for the time, the whole point of lemmy is more or less lost for the time.

geneccx commented 1 year ago

that means that those custom emojis in the federated comments are likely to be the XSS vectors. It remains to see where exactly in lemmy-ui those comments aren't escaped properly.

No, malicious comments (and other activities) need to be specific to the instance's custom emoji for the exploit to work.

From my testing and observations, federated activities referencing custom emoji from those other instances do not trigger the issue.

FireMasterK commented 1 year ago

The current fix in #1897 doesn't really do anything when the emoji itself has a malicious image URL or alt text. This was also pointed out by https://github.com/LemmyNet/lemmy-ui/pull/1897#issuecomment-1628300373

My fix sanitizes everything in #1906, just to be safe.

kevincox commented 1 year ago

I agree that the code can still be improved. I filed https://github.com/LemmyNet/lemmy-ui/issues/1904 to track proper HTML generation. Even your solution seems a bit strange as it generates possibly invalid or malicious HTML then tries to clean it, I think it would be better to just avoid generating invalid or malicious HTML in the first place.

AlmightySnoo commented 1 year ago
2. Overwrite content with the exploit
UPDATE comment SET content = '<REMOVED BY ADMIN>' WHERE content LIKE '%![" onload%';
UPDATE private_message SET content = '<REMOVED BY ADMIN>' WHERE content LIKE '%![" onload%';
UPDATE post SET body = '<REMOVED BY ADMIN>' WHERE body LIKE '%![" onload%';
UPDATE post SET name = '<REMOVED BY ADMIN>' WHERE name LIKE '%![" onload%';

Just to be cautious, I'd also do the same with onerror with something like '%![%](" onerror%' as the following is also a possible XSS:

<img src="" onerror="something()">

as onerror will be triggered because the src attribute is empty.

I'd also do the same without a space between onload/onerror and the first quotation mark.

kevincox commented 1 year ago

This is still incomplete. You could add two spaces instead of one and bypass that pattern. It is also one-time only. The only true fix is to push out an updated UI.

geneccx commented 1 year ago

Yep, there's no way to identify all possible cases where this is done.

Update the frontend - the fix is available in 0.18.2-rc.1.

Deleting the comments/posts is not technically necessary, it's more for cleanup afterwards anyway.

FireMasterK commented 1 year ago

Just to be cautious, I'd also do the same with onerror with something like '%![%](" onerror%' as the following is also a possible XSS:

<img src="" onerror="something()">

as onerror will be triggered because the src attribute is empty.

I'd also do the same without a space between onload/onerror and the first quotation mark.

That wouldn't be sufficient in my opinion, you could potentially inject a <script> tag altogether. There are so many ways to perform an XSS attack with the current attack vector.

AlmightySnoo commented 1 year ago

Just to be cautious, I'd also do the same with onerror with something like '%![%](" onerror%' as the following is also a possible XSS: <img src="" onerror="something()"> as onerror will be triggered because the src attribute is empty. I'd also do the same without a space between onload/onerror and the first quotation mark.

That wouldn't be sufficient in my opinion, you could potentially inject a <script> tag altogether. There are so many ways to perform an XSS attack with the current attack vector.

Which means, as long as there isn't a proper fix to this, we still don't know whether instances currently host other malicious JavaScript code that is still active and discretely stealing cookies. We only know that we got rid of the onload's.

sunaurus commented 1 year ago

Just so we're all on the same page: the current fix wasn't getting rid of onloads, it was actually disabling the custom rendering logic for emojis.

For now, the only known way to actually run this code was through custom emojis (before 0.18.2-rc.1). We can speculate that there are other XSS vulnerabilities, but there are no known ones at the moment.

AlmightySnoo commented 1 year ago

I just checked again in the forum.basedcount.com instance which is running the 0.18.1 backend, and Markdown in the comments isn't properly escaped.

It is very curious however. The exploit requires a title that is exactly the name of any custom emoji (not necessarily a compromised one, it just needs to be valid!) in the Markdown image tag to work and the image URL can be anything, but without a title it doesn't work. For instance, putting this in a comment successfully injects the JavaScript:

![" onload="alert('pwned')"](any_image_url "name of a custom emoji, like tradwife or emily in the instance above") 

but this won't work:

![" onload="alert('pwned')"](any_image_url) 

Thus, a simple user without any admin rights can perfectly perform the exploit and attempt to steal admin cookies, so it's wrong to assume that it's exclusively a compromised custom emoji that is allowing the hack.

geneccx commented 1 year ago

@AlmightySnoo https://github.com/LemmyNet/lemmy-ui/issues/1895#issuecomment-1628356382

It's usage of a local custom emoji by any user that triggers the exploit, and the PR fixes the usage piece such that normal users cannot inject arbitrary JS by using local custom emoji. Admins however, can still place arbitrary JS when creating an emoji.

The existing PR fixes the current issue that is being exploited in the wild.

sunaurus commented 1 year ago

it's wrong to assume that it's exclusively a compromised custom emoji that is allowing the hack.

This is not being assumed, it is known that any emoji will trigger the vulnerable emoji rendering code. This is exactly what is fixed in 0.18.2-rc.1, by the way.

Previous comment said the same thing, I'll hide this one 😅

calculuschild commented 1 year ago

I think their point is it's wrong to assume the icon is the only vulnerability, just because that's the approach the hackers used this time.

geneccx commented 1 year ago

Sure, but who's assuming that?

There's movement now in other areas, including adding a CSP, potentially revisiting the JWT implementation, among others.

There are going to be other exploits, that is inevitable. Focus is better directed towards layers of defence rather than speculation.

abhibeckert commented 1 year ago

@sunaurus I think it would be good to learn from this and add some automated method to quickly deploy urgent security patches. There are thousands of lemmy instances and it will take forever for all of them to update.

I'm not suggesting fully automated software updates, just some way to quickly patch a critical vulnerability like this one.