JS: Add support for threat models #17256
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * Extend this class to model new APIs. If you want to refine existing API models, | ||
| * extend `ThreatModelSource` instead. | ||
| */ | ||
| abstract class Range extends DataFlow::Node { |
Check failure
Code scanning / CodeQL
Bidirectional imports for abstract classes Error
| @@ -11,10 +11,9 @@ | |||
| private module Cached { | |||
| /** A data flow source of remote user input. */ | |||
| cached | |||
| abstract class RemoteFlowSource extends DataFlow::Node { | |||
| /** Gets a human-readable string that describes the type of this remote flow source. */ | |||
| abstract class RemoteFlowSource extends ThreatModelSource::Range { | |||
Check failure
Code scanning / CodeQL
Bidirectional imports for abstract classes Error
asgerf
reviewed
Aug 20, 2024
Integration with RemoteFlowSource is not straightforward, so postponing that for later Naming in other languages: - `SourceNode` (for QL only modeling) - `ThreatModelFlowSource` (for active sources from QL or data-extensions) However, since we use `LocalSourceNode` in Python, and `SourceNode` in JS (for local source nodes), it seems a bit confusing to follow the same naming convention as other languages, and instead I came up with new names.
I didn't want to put the configuration file in `semmle/javascript/frameworks/**/*.model.yml`, so created `ext/` as in other languages
7 cases looks something like this:
```
class RemoteFlowSourceAsSource extends Source instanceof RemoteFlowSource {
RemoteFlowSourceAsSource() { not this instanceof ClientSideRemoteFlowSource }
}
```
(some have variations like `not this.(ClientSideRemoteFlowSource).getKind().isPathOrUrl()`)
javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll
javascript/ql/lib/semmle/javascript/security/dataflow/CommandInjectionCustomizations.qll
javascript/ql/lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll
javascript/ql/lib/semmle/javascript/security/dataflow/RegExpInjectionCustomizations.qll
javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll
javascript/ql/lib/semmle/javascript/security/dataflow/ResourceExhaustionCustomizations.qll
javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
Such that we can reuse the existing modeling, but have it globally applied as a threat-model as well. I Basically just moved the modeling. One important aspect is that this changes is that the previously query-specific `argsParseStep` is now a globally applied taint-step. This seems reasonable, if someone applied the argument parsing to any user-controlled string, it seems correct to propagate that taint for _any_ query.
RasmusWL
force-pushed
the
js-threat-models
branch
from
a465a1b to
1726287
Compare
Mirroring what was done for Python
However, as indicated by the `MISSING` annotations, we could do better.
Makes a difference due to the modeling of NodeJSFileSystemAccessRead depending on these, see https://github.com/github/codeql/blob/412e841d6929c7a4cf6508a01e721db01df7ac49/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll#L479-L488 File copied from https://github.com/github/codeql/blob/7cef4322e70e10c0c62e9ce933ca9f6db44b6ec1/javascript/externs/nodejs/fs.js
You could argue that proper modeling be done in the same way as `NodeJSFileSystemAccessRead` is done for the callback based `fs` API (in NodeJSLib.qll). However, that work is straying from the core goals I'm working towards right now, so I'll argue that "perfect is the enemy of good", and leave this as is for now.
Technically not always true, but my assumption is that +90% of the time that's what it will be used for, so while we could be more precise by adding a taint-step from the `input` part of the construction, I'm not sure it's worth it in this case. Furthermore, doing so would break with the current way we model threat-model sources, and how sources are generally modeled in JS... so for a very pretty setup it would require changing all the other `file` threat-model sources to start at the constructors such as `fs.createReadStream()` and have taint-propagation steps towards the actual use (like we do in Python)... I couldn't see an easy path forwards for doing this while keeping the Concepts integration, so I opted for the simpler solution here.
|
Have added proper docs integration, and have provided some modeling for database/stdin/file (although the latter two could be improved more). Ready for review now, will do DCA soon. |
|
Looks good so far, but will take another look tomorrow when I have more time.
class RemoteFlowSourceAsSource extends Source instanceof RemoteFlowSource {
RemoteFlowSourceAsSource() { not this instanceof ClientSideRemoteFlowSource }
}Just change |
asgerf
reviewed
Oct 31, 2024
javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.model.yml
Outdated
Show resolved
Hide resolved
Co-authored-by: Asger F <[email protected]>
🤯 so obvious after you've seen it 😂 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR mostly follows the structure of the Python PR (#17203)
The most interesting aspect of this PR is that in f733ac1 I was only able to meaningfully add ActiveThreatModelSource as the default source for some queries, since some of them rely on this pattern:
and it's not quite clear how these should be migrated 🤔 This pattern is used in these 7 cases:
I'll also note that while I think the support for
environment,commandargs,databasethreat-models are OK, the ones forstdinandfileare a little simple right now. (see 3 last commit messages for more context)