Skip to content

Dynamic: add Fuzzy token#13737

Merged
asgerf merged 8 commits intogithub:mainfrom
asgerf:dynamic/fuzzy-models
Aug 3, 2023
Merged

Dynamic: add Fuzzy token#13737
asgerf merged 8 commits intogithub:mainfrom
asgerf:dynamic/fuzzy-models

Conversation

@asgerf
Copy link
Copy Markdown
Contributor

@asgerf asgerf commented Jul 13, 2023

Adds a Fuzzy token to the identifying access path for all dynamic languages, which will match, loosely speaking, an arbitrary series of operations.

For example, suppose we wanted to add a SQL injection sink for the query method in the mysql package, but we don't have a detailed enough knowledge of the library to do this precisely. So we just write the model like this:

mysql; Fuzzy.Member[query].Argument[0]; sql-injection

This would match all query calls whose receiver is an object that appears to come from the mysql package. The latter criterion comes from "tracking through an arbitrary series of operations".

This can serve as a foundation for a dedicated extension point for fuzzy models. An entry in such a fuzzy model might simply look like:

mysql; query; Argument[0]; sql-injection

asgerf added 2 commits July 13, 2023 13:41
The API graph entry point depended on API::Node.

This was due to depending on the the TComponent newtype which has a branch that depends on API::Node
Comment thread ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll Fixed
@asgerf asgerf force-pushed the dynamic/fuzzy-models branch from 43fbd08 to eb5c600 Compare July 13, 2023 13:42
@asgerf asgerf added the no-change-note-required This PR does not need a change note label Jul 14, 2023
@asgerf asgerf marked this pull request as ready for review July 14, 2023 10:50
@asgerf asgerf requested review from a team as code owners July 14, 2023 10:50
@calumgrant calumgrant requested review from alexrford and yoff July 17, 2023 08:20
yoff
yoff previously approved these changes Jul 17, 2023
Copy link
Copy Markdown
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

Python 👍
And thanks for implementing this for Python, also! 💪
I suppose that in getAFuzzySuccessor we should make sure to avoid loops and that is why self parameters are avoided in Ruby. The Python implementation does look loop-free as far as I can tell. Do the DCA runs actually test the new feature, if we have no fuzzy specifications yet?

Comment thread docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst Outdated
Comment thread docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst Outdated
Comment thread docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst Outdated
Comment thread docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst Outdated
alexrford
alexrford previously approved these changes Jul 27, 2023
Copy link
Copy Markdown
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

Ruby LGTM - I've played around with this a bit and it seems potentially very useful for modeling automation.

or
result = node.getAnElement()
or
result = node.getInstance()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess that this means that "FuzzyLib!;Fuzzy.<path>;test-sink" will find strictly more results than "FuzzyLib;Fuzzy.<path>;test-sink"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, slightly unfortunate and probably not a user experience for beginners.

I'm thinking if we add a dedicated extensible predicate for fuzzy models, it would translate into regular models that use both FuzzyLib and FuzzyLib! as the initial type. Or perhaps we could add FuzzyLib! as a fuzzy successor of FuzzyLib.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The current behaviour does make sense under the mindset of "FuzzyLib.new is just another method", but it was initially unintuitive for me because I was thinking of object instantiation as something separate from a regular class method call.

@asgerf asgerf dismissed stale reviews from alexrford and yoff via da3eb28 July 31, 2023 11:51
Co-authored-by: Jorge <46056498+jorgectf@users.noreply.github.com>
@asgerf asgerf merged commit c38cbe8 into github:main Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants