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

JavaScript: Add some new XSS sinks and sources of Next.js (and some extra improvements) #747

Closed
1 of 2 tasks
tyage opened this issue Apr 24, 2023 · 17 comments
Closed
1 of 2 tasks
Labels
All For One Submissions to the All for One, One for All bounty

Comments

@tyage
Copy link

tyage commented Apr 24, 2023

Query PR

github/codeql#10984
github/codeql#12787
github/codeql#12963
github/codeql#12975

Language

JavaScript

CVE(s) ID list

CVE-2022-0087

CWE

CWE-79

Report

  1. What is the vulnerability?
    Reflected XSS

  2. How does the vulnerability work?
    When a victim visits the URL which contains malicious script, it will be evaluated on victim's browser.

  3. What strategy do you use in your query to find the vulnerability?
    I added Next.js router's query and some args in getServerSideProps function, which is commonly used to receive query/path parameter in URL, as sources.
    JS: Add Next.js parameters as source codeql#10984
    I also added Next.js router's query.push as a sink for XSS.
    JS: Add New XSS sink - Next.js router.push/replace codeql#12787
    I also made some improvements to enable CodeQL detect when the application require submodule package and useRouter comes from other modules.
    JS: Track interfile useRouter codeql#12963
    JS: Support sub modules codeql#12975

  4. How have you reduced the number of false positives?
    I think we won't see any false positives.

Are you planning to discuss this vulnerability submission publicly? (Blog Post, social networks, etc).

  • Yes
  • No

Blog post link

No response

@tyage tyage added the All For One Submissions to the All for One, One for All bounty label Apr 24, 2023
@JarLob JarLob assigned JarLob and unassigned JarLob Apr 29, 2023
@tyage tyage changed the title JavaScript: Add some new sinks and sources of Next.js JavaScript: Add some new sinks and sources of Next.js (and some extra improvements) Apr 30, 2023
@tyage
Copy link
Author

tyage commented Apr 30, 2023

here is a database for quicker evaluation

keystone-auth-db.zip

@tyage tyage changed the title JavaScript: Add some new sinks and sources of Next.js (and some extra improvements) JavaScript: Add some new XSS sinks and sources of Next.js (and some extra improvements) Apr 30, 2023
@JarLob
Copy link
Contributor

JarLob commented May 15, 2023

Hi @tyage, which query from the "CWE-79" folder should detect the vulnerability? I have built codeql database from https://github.com/keystonejs/keystone/archive/refs/tags/@keystone-6/[email protected], but it doesn't detect anything.

@tyage
Copy link
Author

tyage commented May 15, 2023

Hi @JarLob , did you tested with #12975 branch?
It is not currently merged.

Xss.ql should detect XSS if it is merged.
https://github.com/github/codeql/blob/main/javascript/ql/src/Security/CWE-079/Xss.ql

@JarLob
Copy link
Contributor

JarLob commented May 15, 2023

I applied it manually, it is just one function if I didn't miss anything.

@tyage
Copy link
Author

tyage commented May 16, 2023

@JarLob
We need to run npm install and npm run build before creating a database since we can't find a XSS source properly without build.
After that, it can detect the XSS flow.

@tyage
Copy link
Author

tyage commented May 16, 2023

this is how the result looks liked to me.
IMG_1645

@JarLob
Copy link
Contributor

JarLob commented May 16, 2023

Thanks, I was able to get the alerts after npm install and npm run build.

@JarLob
Copy link
Contributor

JarLob commented May 16, 2023

Looking at how the CVE was fixed and seeing your query still flags it, do you think it is a false positive or the fix was insufficient?

@tyage
Copy link
Author

tyage commented May 17, 2023

Yes, this is a false positive.
This fix ensures that the query starts from / using a condition router.query.from?.startsWith('/'), but the current XSS query doesn't take care of such custom filters.
I think it is difficult to detect these filters and sanitizers properly since there are so many patterns.

@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Test run.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

@JarLob
Copy link
Contributor

JarLob commented May 17, 2023

I'll try to see how much noise it generates, but it can be a blocker for acceptance.

@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Results analysis.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Query review.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Final decision.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Pay.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

@xcorail
Copy link
Contributor

xcorail commented Jun 8, 2023

Created Hackerone report 2018679 for bounty 487346 : [747] JavaScript: Add some new XSS sinks and sources of Next.js (and some extra improvements)

@xcorail xcorail closed this as completed Jun 8, 2023
@ghsecuritylab
Copy link
Collaborator

Your submission is now in status Closed.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
All For One Submissions to the All for One, One for All bounty
Projects
None yet
Development

No branches or pull requests

4 participants