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

JS: model property projection calls #113

Merged
merged 3 commits into from Aug 31, 2018
Merged

JS: model property projection calls #113

merged 3 commits into from Aug 31, 2018

Conversation

ghost
Copy link

@ghost ghost commented Aug 29, 2018

This PR adds a taint step for property reads done through calls that projects selected properties of an object.

The property projections that have been modeled have slightly different features. So I have adopted @asger-semmle's extensible architecture proposal immediately. We may want to discuss how we generally document the intended usage of the architecture, I have simply added an explanation to the module docstring for now.

This PR only makes use of the object-argument in the models.

For the record, the libraries support various features for the selector-argument:

  • access path strings like 'a.b[42].c'
  • access path arrays like ['a', 'b', 42, 'c']
  • search strings like 'a.\*.c'
  • property predicate functions

We can consider implementing support for these once our general heap treatment is better.

An evaluation with the security suite on big-apps.slugs flags nothing new, and it does not have a significant performance impact (internal link).

@ghost ghost added the JS label Aug 29, 2018
@ghost ghost self-requested a review as a code owner August 29, 2018 10:28
@ghost ghost added this to the 1.18 milestone Aug 29, 2018
(
callee = DataFlow::moduleMember("dotty", "search") and
objectIndex = 0 and
selectorIndex = 1

Choose a reason for hiding this comment

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

Indentation.

import javascript

/**
* A call that projects selected properties of an object.

Choose a reason for hiding this comment

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

Here and below: these comments could do with examples of what is being modelled.

abstract DataFlow::Node getASelector();

/**
* Holds if this call returns the value of a single projected property.

Choose a reason for hiding this comment

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

This is particularly arcane; please explain more carefully and add an example.

@ghost
Copy link
Author

ghost commented Aug 30, 2018

Comments addressed.

@ghost
Copy link
Author

ghost commented Aug 30, 2018

Squashed the fixups.

Copy link

@xiemaisi xiemaisi left a comment

Choose a reason for hiding this comment

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

LGTM

@xiemaisi
Copy link

The tests actually seem to have succeeded, but the status didn't propagate (@pavel). Merging.

@xiemaisi xiemaisi merged commit 7e18426 into github:master Aug 31, 2018
@xiemaisi
Copy link

@esben-semmle, this PR seems to have been left behind when creating https://github.com/Semmle/ql/tree/rc/1.18. Could you open a PR to cherry-pick the commits across? Same for #115.

@ghost ghost mentioned this pull request Sep 3, 2018
@pavel
Copy link

pavel commented Sep 3, 2018

@xiemaisi Thanks for notifying me, but it does not seem that I'm involved in this project in any way.

https://blog.github.com/2011-03-23-mention-somebody-they-re-notified/

@xiemaisi
Copy link

xiemaisi commented Sep 3, 2018

Oops, sorry, wrong Pavel... (It's a great project, though! 😄)

aibaars added a commit that referenced this pull request Oct 14, 2021
smowton pushed a commit to smowton/codeql that referenced this pull request Dec 6, 2021
Kotlin: Allow building only a single way
erik-krogh pushed a commit to erik-krogh/ql that referenced this pull request Dec 15, 2021
erik-krogh pushed a commit to erik-krogh/ql that referenced this pull request Dec 15, 2021
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.

None yet

2 participants