Skip to content

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Mar 1, 2021

Adds a model of ajv along with some general concepts for JSON schema validation.

It has the following impact on our security queries:

  • Validating an object against a schema blocks flow of the TaintedObject label, preventing FPs from queries like prototype pollution and NoSQL injection.
  • Validating a deeply user-controlled object with allErrors: true is flagged as potential a denial-of-service attack. For this I introduced the somewhat generically-named query 'Resource exhaustion from deep object traversal'. It's only used for ajv at the moment, but I avoided using ajv or JSON schema validation as part of its name, so it's easier to extend in the future.
  • The error message from ajv is treated as a source for ExceptionXss. This query has been rephrased to include errors in a more broad sense, which have not necessarily been through a throw/catch.
  • Regular expressions in a JSON schema are now analyzed by our RegExp queries, if the JSON schema is written in a JS file. (See internal issue)

Evaluations (internal links):

  • Security on default slugs looks ok.
    • I've confirmed that the minor regression found in the slowest repo goes away after rebasing on main. In particular, the caching of getAType fixed it.
  • Security on ajv slugs shows a couple new results.

@asgerf asgerf added the JS label Mar 1, 2021
@asgerf asgerf requested a review from a team as a code owner March 1, 2021 11:59
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

Looks pretty good, I just have some minor comments.

Do we flag CVE-2020-8192 with the new query?

asgerf and others added 2 commits March 2, 2021 14:01
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
…ctResourceExhaustionCustomizations.qll

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
@asgerf
Copy link
Contributor Author

asgerf commented Mar 2, 2021

Thanks @erik-krogh!

Addressed review comments. I also noticed I forgot to add the tests for the new query, so there's a new commit with tests as well.

Do we flag CVE-2020-8192 with the new query?

Ah, forgot to mention this, we don't flag this yet.

@codeql-ci codeql-ci merged commit 342c7ab into github:main Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants