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

Implement threat models as extension packs #14548

Merged
merged 6 commits into from
Oct 27, 2023
Merged

Conversation

dbartol
Copy link
Contributor

@dbartol dbartol commented Oct 19, 2023

This PR allows users to configure the threat models for their analysis by representing each threat model as an extension pack that can be added to the analysis. For example, if a user wanted to enable the "local flow sources" threat model, they would run codeql database analyze --extensionPack codeql/threat-local, or the Actions or VS Code equivalent. This avoids having to add a new configuration option in many different places.

Implementation notes

I moved the existing ExternalFlowConfiguration.qll file from Java into its own library pack, codeql/threat-models. This pack is language-agnostic. Any language that wants to consume threat models will depend on this pack, and every threat model extension pack will target this pack for extension.

For each of the high-level threat models currently supported, I added a new extension pack codeql/threat-<name>, where <name> is the name of the threat model. This extension pack just adds a single row to the supportedThreatModels predicate from codeql/threat-models, specifying the name of the threat model.

The threat model system in QL supports a hierarchy of threat models (for example, "files" and "environment" are both sub-threat-models of "local"). I haven't added extension packs for the low-level threat models yet, but we can add those later easily enough.

@github-actions github-actions bot added the Java label Oct 19, 2023
@dbartol dbartol added the no-change-note-required This PR does not need a change note label Oct 19, 2023
@dbartol dbartol marked this pull request as ready for review October 20, 2023 18:05
@dbartol dbartol requested a review from a team as a code owner October 20, 2023 18:05
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!
IMO it is great idea to share this between the different languages as it will help drive the re-use of existing threat model kinds/groups.

As a part of this PR we probably need to add an extension pack for the threat model "all" (it is not explicitly listed in the hierarcy, but if this is used then all sources will be included).

You also mention in the description, we probably need to add extension packs for the "low level" kinds as well (if these are not needed it might be an indication that the threat model kind/grouping are to granular).

It requires a bit more boiler plate code to introduce a new source or group with this design - I don't think that is a problem as we don't have that many source kinds as it is (at least not for java or C#) - but maybe it is worth checking with language teams in general whether this is a problem (either now or in the known future). However, if we are to do something similar for sink kinds then it will look differently as they are more of those and it appears that new ones are more frequently introduced.

Furthermore, what happens if we introduce tens or hundreds of extension packs in this way?

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I think this is generally the best approach.

@@ -1,7 +1,7 @@
extensions:

- addsTo:
pack: codeql/java-all
pack: codeql/threat-models
extensible: threatModelGrouping
data:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we just need to assume that having one threat model grouping will not have any conflicts across languages. At some point, we may want to move some of these lines into library packs themselves.

@dbartol dbartol marked this pull request as draft October 24, 2023 15:34
@dbartol dbartol merged commit 76a9b71 into main Oct 27, 2023
48 checks passed
@dbartol dbartol deleted the dbartol/threat-models branch October 27, 2023 13:33
@dbartol
Copy link
Contributor Author

dbartol commented Oct 27, 2023

To be clear, this PR was marked as "merged" because #14582 was merged, and that PR was based on the commits from this PR. The actual extension packs were removed before that PR was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants