Skip to content

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Feb 5, 2020

As a general improvement, we now recognise imports through lazy-cache:

let lazy = require('lazy-cache')(require)
lazy('mypackage')

lazy.mypackage() // actual use of 'mypackage'

The prototype pollution utility query has been improved to recognize property enumeration through a few NPM packages, such as for-own and for-in, allowing us to flag another CVE.

This currently includes more general-purpose enumerators like _.each which is possibly a bit risky, as we might pick up on places where it's always enumerating an array, but we'll see from the evaluation how it works.

@asgerf asgerf added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Feb 5, 2020
@asgerf asgerf requested a review from a team as a code owner February 5, 2020 13:04
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

Looks good.

I think we should have a test case or two for the lazy-cache+require combo.

We also need to update the change notes with all the libraries we end up modelling something for in this PR.

result = moduleImport("for-own") or
result = moduleImport("for-in") or
result = moduleMember("ramda", "forEachObjIndexed") or
result = LodashUnderscore::member("forEach") or
Copy link
Contributor

Choose a reason for hiding this comment

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

If the evaluation turns out fine, we should consider adding jQuery's forEach..

This particular predicate can probably be extended with a thousand more cases though...

}

/**
* Property enumeration through the `for-own` or `for-in` package.
Copy link
Contributor

Choose a reason for hiding this comment

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

This docstring needs to be more general now that propertyEnumerator includes many more cases than just for-own and for-in

@asgerf
Copy link
Contributor Author

asgerf commented Feb 6, 2020

The evaluation shows that this PR actually fixes four timeouts that had snuck in due to a bad join ordering from a refactoring in #2731.

I'll run a quick evaluation against a commit that specifically fixes the join ordering, to see if this PR introduces any overhead of its own, possibly hidden behind the unrelated speed-up.

@asgerf
Copy link
Contributor Author

asgerf commented Feb 6, 2020

Smoke-test evaluation against the commit that just fixes the join-ordering looks fine. Taken together with the original evaluation I'd say both this PR and the join order fix look safe.

I'll put up a separate PR for the join order fix in case a cherry-picked and/or revert is needed.

@asgerf asgerf removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Feb 6, 2020
@asgerf asgerf force-pushed the js/protopol-packages branch from b21ae68 to 91a5385 Compare February 6, 2020 15:00
@asgerf
Copy link
Contributor Author

asgerf commented Feb 6, 2020

Rebased on top of the fix commit. Should be ready to merge after the PR checks.

Edit: never mind, there are still outstanding review comments

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.

3 participants