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

Fix XSS vuln #1897

Merged
merged 1 commit into from
Jul 10, 2023
Merged

Conversation

makotech222
Copy link
Contributor

Description

Screenshots

Before

After

@rystaf
Copy link

rystaf commented Jul 10, 2023

With this fix, XSS is still possible in the emoji creation form.
image

@jamesmanes
Copy link

To be clear, is this the vulnerability assumed to actively being exploited in the wild? i.e., on lemmy dot world?

@geneccx
Copy link

geneccx commented Jul 10, 2023

To be clear, is this the vulnerability assumed to actively being exploited in the wild? i.e., on lemmy dot world?

Yes.

@DraconicNEO
Copy link

To be clear, is this the vulnerability assumed to actively being exploited in the wild? i.e., on lemmy dot world?

It has been used both on Lemmy.world and lemmy.blahaj.zone

@Nutomic
Copy link
Member

Nutomic commented Jul 10, 2023

I can confirm that this fixes the vulnerability.

@rystaf That is an unrelated problem and has nothing to do with the hack.

@SleeplessOne1917 SleeplessOne1917 merged commit e80bcf5 into LemmyNet:main Jul 10, 2023
dessalines pushed a commit that referenced this pull request Jul 10, 2023
@Daniel15
Copy link
Contributor

Is there a plan to audit the codebase to ensure that there's no other places that build HTML using string concatenation, and to rewrite this call site so it doesn't do that? Would you like help with that?

@ubergeek77
Copy link
Contributor

ubergeek77 commented Jul 11, 2023

I very much doubt an audit is going to be affordable, but there are some free SAST/SCA scanners that may be beneficial to use in this project. The number of tests per month may not cover every commit Lemmy goes through, but Snyk would be a very good option as a baseline, and it's much better than nothing:

https://snyk.io/plans/

I haven't scanned Lemmy with Snyk, but I have worked with it before, and I'm pretty confident it would have caught this.

It can be set up to scan on PRs and block builds if a new vulnerability is about to be introduced, so this could be a good way to manage security over time without it being completely overwhelming.

@Daniel15
Copy link
Contributor

@ubergeek77

I very much doubt an audit is going to be affordable,

Maybe not an official audit, but at least have some volunteers familiar with web security look through the code (both backend and frontend) and see if they can find any common issues.

@0mWh
Copy link

0mWh commented Jul 11, 2023

Building HTML strings can be error prone.
Perhaps directly using the DOM should be prioritized?

function emoji(src: string, title: string, alt_text: string) {
    const node = document.createElement('img');
    node.classList.add('icon', 'icon-emoji');
    node.src = src;
    node.title = title;
    node.alt = alt_text;
    return node;
}

//example usage
const src: string = '<><><><><><>?,./,/\\\t|>>>\\,<\'';
const title: string = '<script>alert("xss?")</script>';
const alt_text: string = '%*(!&(%@#&!%!!%!))';

const x: any = emoji(src, title, alt_text);

//directly use document element node
document.body.appendChild( x );

//convert to html string via browser
console.log( x.outerHTML );

@Nutomic
Copy link
Member

Nutomic commented Jul 11, 2023

@Daniel15 All the code is open source, everyone is welcome to look through it for potential problems and report/fix them. But like ubergeek said we dont have any money to pay for a professional audit. Maybe there are some organizations which would do audits of open source projects for free, might be worth searching for.

@Daniel15
Copy link
Contributor

@Nutomic

everyone is welcome to look through it for potential problems and report/fix them.

I'll try to take a look if I get some free time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants