JS: js/reflected-xss through file names#142
Conversation
ghost
self-requested a review
as a 
xiemaisi
left a comment
xiemaisi
left a comment
There was a problem hiding this comment.
LGTM, only one question and a minor suggestion. OOI, why do you think the content-type in the newly identified result is non-HTML?
| } | ||
|
|
||
| /** | ||
| * A data flow node that contains one or more file names from the local file system. |
| } | ||
| } | ||
|
|
||
| /** A source of file name input, considered as a flow source for reflected XSS. */ |
There was a problem hiding this comment.
The phrase "source of file name input" sounds quite awkward. Maybe just "a file name"?
|
Comments addressed.
Several reasons:
|
|
Query LGTM, but I'm having second thoughts about adding this as a source for reflected XSS. The alert message is phrased in terms of user-provided values, and I don't really think local file system paths fit into that category. Is this perhaps a separate query? (Ping @asger-semmle for input.) |
|
I don't understand the vulnerability you want to flag here? As far as I can tell, the commit doesn't actually fix anything. It's a static file server, so attackers have no control over file names. If they could upload files and thereby control file names, I'd categorize it as stored XSS. |
It is a vulnerability, no doubt about that (hackerone agrees).
They do not have to choose the file names through the application with the XSS vulnerability. Imagine an escalation through a compromised FTP server with a directory that happens to be served by
Yes, the source should perhaps be moved to XSS.ql, I had a wrong definition in mind. Reflected XSS is not stored, by definition:
|
|
I like Asger's suggestion of treating this as a stored XSS. We don't have a query for that yet (merging into |
|
OK. Closing for now then. |
ghost
closed this
asger-semmle
left a comment
asger-semmle
left a comment
There was a problem hiding this comment.
Thanks for the clarification.
+1 for moving into a new query. Either one for stored xss, and/or one whose alert message clarifies that it can be used to escalate other vulnerabilities (and thus may not be exploitable as reported).
I still think this is sort of a downwards escalation, as getting write access to the server would almost certainly enable you to XSS its visitors anyway.
| NodeJSFileNameSource() { | ||
| exists (string name | | ||
| name = "readdir" or | ||
| name = "realpath" | |
There was a problem hiding this comment.
Would the FP you found go away if realpath became a taint step instead of a source?
There was a problem hiding this comment.
Yes, I believe so, but I still think it should remain a source.
If we later find that readdir or realpath cause many FPs as sources, we can downgrade them to taint steps.
You escalate from an access to some file server, you do not have access to the application server that displays the XSSed listing of files. |
ghost
mentioned this pull request
Remove as many references to TreeSitter::Generated
Kotlin: Expressions: Improve test
…ldmodes PS: support buildmode none in extractor
2 participants
This PR makes js/reflected-xss treat file names as sources.
Other injection queries, such as js/sql-injection, can perhaps also use this new source, but I would like to introduce this source slowly to prevent too many FPs at once if it turns out that file names in practice are benign.
The PR is inspired by this vulnerability fix: nunnly/m-server@b8613fd, where a file name ultimately is concatenated with some HTML string fragments.
Performance and results are unchanged, except for a result that I suspect is a FP due to a non-HTML response content-type that we fail to identify.
I have started on the 1.19 changes notes. I think we should aim for a less verbose library listing this time. I think a category and an example library is fine WDYT?