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

tyage commented 1 year ago

Query PR

https://github.com/github/codeql/pull/10984 https://github.com/github/codeql/pull/12787 https://github.com/github/codeql/pull/12963 https://github.com/github/codeql/pull/12975

Language

JavaScript

CVE(s) ID list

CVE-2022-0087

CWE

CWE-79

Report

What is the vulnerability? Reflected XSS
How does the vulnerability work? When a victim visits the URL which contains malicious script, it will be evaluated on victim's browser.
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. https://github.com/github/codeql/pull/10984 I also added Next.js router's query.push as a sink for XSS. https://github.com/github/codeql/pull/12787 I also made some improvements to enable CodeQL detect when the application require submodule package and useRouter comes from other modules. https://github.com/github/codeql/pull/12963 https://github.com/github/codeql/pull/12975
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).

[X] Yes
[ ] No

Blog post link

No response

tyage commented 1 year ago

here is a database for quicker evaluation

keystone-auth-db.zip

JarLob commented 1 year ago

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/auth@1.0.1.zip, but it doesn't detect anything.

tyage commented 1 year ago

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 commented 1 year ago

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

tyage commented 1 year ago

@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 commented 1 year ago

this is how the result looks liked to me.

JarLob commented 1 year ago

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

JarLob commented 1 year ago

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 commented 1 year ago

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.