Python: CWE-943 - Add NoSQL injection query #5612

mrthankyou · 2021-04-06T18:13:43Z

This PR introduces a new query that searches for NoSQL injections. This query specifically looks for vulnerabilities that utilize any one of these three major Python MongoDB libraries:

The most common scenario this query looks for is for a Python application to accept user input from a HTTP request, convert that HTTP request to a Python object, and then inject that object directly into a sink method provided by one of the three libraries above.

This query is also only currently testing for find related methods as well as MongoEngine's objects method.

PS: Please note that both @mrthankyou and @jorgectf wrote this query. If accepted as a bug bounty we would like to split the bounty 50/50. This will be re-iterated in the bug bounty submission but I thought it would be a good idea to mention here. Any review edits will be done by @mrthankyou.

This __raw__ keyword is required for the actual mongoengine vulnerability. More info can be found below: http://docs.mongoengine.org/guide/querying.html?highlight=inc__#raw-queries

After initial research on our end, we believe that the only vulnerability within the objects() method is passing a query into the __raw__ keyword argument. More info can be found below: http://docs.mongoengine.org/guide/querying.html?highlight=inc__#raw-queries

jorgectf · 2021-04-06T18:16:54Z

🎉

Although it is for `json.loads` and the like.

RasmusWL

I did a few minor fixes to this PR in jty-team#5, but overall it looks ok 👍 (so requesting changes until that is merged)

A few small things to consider for the future:

I would have merged all the tests files for mongoengine into one file, since they all contain most of the same setup code anyway, so it would be easier to just have all the different variants one after each other, instead of having them in separate files. All the _good.py variants are also a bit trivial... maybe just having one or two of these would be enough.
it would have been nice with QLDoc for all predicates/classes, even the ones that are private.

This PR is going through a bit of internal process, and if everything goes according to plan, it should be merged within the next few days +1

For our own future work on this query (promoting to standard set of queries), I still think parts of these comments about manually adding $eq to the NoSQL query to make it safe would be worthwhile to investigate a bit further. The part about $where also needs more investigation, I still didn't try out whether it would be possible to use it with a string containing a JS function body, such as db.users.find({'$where': 'this.name.first === "John"'}).

This PR also highlights some work that we need to do to make writing queries easier. Right now this query only looks at decoding from JSON, so in the unlikely scenario that the input is encoded as YAML, this query would not be able to catch it. If we provided an easy way to know when a remote user input had been turned into a dictionary, that would probably make this query a bit easier to write. (we did make an internal note about this, but also wanted to recognize it on this PR).

RasmusWL · 2021-06-28T11:17:01Z

python/ql/src/experimental/semmle/python/frameworks/NoSQL.qll

+
+  /** Gets a reference to a `Mongo` collection method. */
+  private DataFlow::Node mongoCollectionMethod() {
+    mongoCollection() in [result.(DataFlow::AttrRead), result.(DataFlow::AttrRead).getObject()] and


This is a continuation of #5612 (comment)

I still don't see why we would flag such method calls on databases and not just only on collections.

client = pymongo.MongoClient(...) client.mydb.find(...) # <-- I'm surprised if this one is allowed :O client.mydb.mycollection.find(...)

So I think this code should be

Suggested change

mongoCollection() in [result.(DataFlow::AttrRead), result.(DataFlow::AttrRead).getObject()] and

mongoCollection() = result.(DataFlow::AttrRead).getObject() and

I wasn't sure, but asked @mrthankyou and you are right, we don't think it's possible to execute a query without having previously specified a collection. Fixed in 68c6831#diff-ca1baaa83ce64bcb0c8f0dca86c8be11cef6a77a6bc422ee1584f1eadf17cc12R141

RasmusWL

Just spotted 2 more minor things related to library modeling

python/ql/src/experimental/semmle/python/frameworks/Stdlib.qll

Small NoSQL fixes

Update `xmltodict` format and delete `ujson` modeling. Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

jorgectf · 2021-06-28T19:27:31Z

Thank you so much for the review @RasmusWL 😊

I would have merged all the tests files for mongoengine into one file, since they all contain most of the same setup code anyway, so it would be easier to just have all the different variants one after each other, instead of having them in separate files.

Done in 0ca4f24. They are now divided by frameworks.

All the _good.py variants are also a bit trivial... maybe just having one or two of these would be enough.

I've deleted some redundant tests that, although they weren't the same, they would work if the rest do too. 3fd1129

it would have been nice with QLDoc for all predicates/classes, even the ones that are private.

Feel free to suggest any rephrasing needed 😃 68c6831

Would be great if this modeling of xmltodict could be moved to it's own file.

Done in 51395d1.

For our own future work on this query (promoting to standard set of queries), I still think parts of these comments about manually adding $eq to the NoSQL query to make it safe would be worthwhile to investigate a bit further. The part about $where also needs more investigation, I still didn't try out whether it would be possible to use it with a string containing a JS function body, such as db.users.find({'$where': 'this.name.first === "John"'}).

This is an interesting issue. For the first one, I guess using a Sanitizer or SanitizerGuard checking that the lhs (of a dict whose rhs is our dict-RFS) equals '$eq' would work, and regarding $where... perhaps we would need to create another configuration, I can't think of a way to do it using current's chained config.

RasmusWL

Thanks for making these updates 💪 I think maybe you were a bit too eager to delete tests though? 😄

I'm not in a rush for this one, so happy to get the last minor bits and pieces sorted out before we merge this 👍

RasmusWL · 2021-06-29T09:28:42Z

python/ql/test/experimental/query-tests/Security/CWE-943/mongoengine_bad.py

-@app.route("/connection_connect_find")
-def connection_connect_find():
-    unsafe_search = request.args['search']
-    json_search = json.loads(unsafe_search)
-
-    db = connect('mydb')
-    return db.movie.find({'name': json_search})
-
-@app.route("/get_db_find")
-def get_db_find():
-    unsafe_search = request.args['search']
-    json_search = json.loads(unsafe_search)
-
-    db = me.get_db()
-    return db.movie.find({'name': json_search})
-


Can you highlight why these tests are now trivial? As far as I can tell, none of the other tests highlight that mongoengine.connection.connect() and mongoengine.get_db() are supported. So as far as I can tell, these tests are still useful 😊

Sure! My approach was that connect('mydb') is the same function as me.connect('mydb') (although they come from a different class (mongoengine.connection/mongoengine)). If me.connect('mydb') works, connect('mydb') should also get flagged (if it is specified in the codeql query). And the same happens with get_db(). I believed you meant something about that regarding trivial tests, but I will put them back :)

thanks 👍

My main point was that all the tests in the mongoengine_good.py file is just copies of the mongoengine_bad.py file, with a single sanitizer step added. So it would really be enough with a single test that this sanitizer works. (just imagine how many lines of code it would take if there were 15 different sanitizers, and we had to use it for all the different ways to do an exploit)

Oh, I get what you mean. I will leave only 1 test in *_good.py then :)

RasmusWL

as I see it, all of my required changes have been addressed 👍

mrthankyou · 2021-07-11T14:06:38Z

Some of the CI checks are unsuccessful. Is this something I need to look into? Thanks!

RasmusWL

Some of the CI checks are unsuccessful. Is this something I need to look into? Thanks!

Yes, please do. I think it is just the expected files that needs to be updated after the cleanup in e02a63a 😊 also noticed what I guess is a small typo in the test code

python/ql/test/experimental/query-tests/Security/CWE-943/mongoengine_good.py

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

RasmusWL · 2021-07-14T09:10:28Z

Nice, I just think we need the .expected files to be updated then 😉

mrthankyou · 2021-07-19T13:08:24Z

Is Python3 Language Tests (GitHub Actions) caused by out-of-date tests? I can take a look when a free moment appears in my schedule this week. Now that I can finally run tests on my local machine I will be able to handle this.

RasmusWL · 2021-07-19T15:13:19Z

We've made some changes on how to do type-tracking very recently, so since we run tests on the commit that would arise if this was merged into the main branch, the failure is due to the fact that the code in this PR has not been updated. No need to worry about it for this PR, I can just fix it once it's merged 👍

mrthankyou · 2021-07-19T15:21:24Z

Interesting. Would like to see those new changes sometime ;)

Thanks!

mrthankyou · 2021-08-06T19:41:09Z

@RasmusSemmle There is a conflict with Concepts.qll. I assume you want this fixed? It seems simple and am happy to do so.

yoff · 2021-08-09T20:43:02Z

@RasmusSemmle There is a conflict with Concepts.qll. I assume you want this fixed? It seems simple and am happy to do so.

Indeed, if you can then please do :-)

…tion

RasmusWL

Barring in mind my comment here, this PR LGTM.

jorgectf and others added 25 commits March 30, 2021 17:51

PR init

517a920

Add Concepts

aea7546

PyMongo and Mongoengine sinks

bd5ff01

Adapt query configs and custom classes

d856f16

Add left tests

4579132

Fix imports

ccd57be

Fix MongoEngine Sink

01f9d4a

Fix Sinks

7a4dc46

Fix Mongoengine test

5a1dc48

Remove unused class variables

017a826

Polish up configs

f0a50eb

Attempt to apply TaintTracking2

3a47a45

Update naming

c8740a2

Fix taint configs

f980d06

Polish query select

15e176a

Update qhelp file

9072d19

Add relevant PyMongo sink methods

be9a3a9

Rename classes

80216f6

Add required __raw__ keyword

3f0c758

This __raw__ keyword is required for the actual mongoengine vulnerability. More info can be found below: http://docs.mongoengine.org/guide/querying.html?highlight=inc__#raw-queries

Update query to search for more pymongo sink methods

759fa2c

Made grammar changes

ac31260

Remove unnecessary example code

520e65e

Improve sentence structure and grammar

dc274ec

Remove comment

4e98348

mrthankyou requested a review from a team as a code owner April 6, 2021 18:13

github-actions bot added documentation Python labels Apr 6, 2021

RasmusWL self-assigned this Apr 7, 2021

Python: Don't rely on d = d.getOutput() for Decoding

318694c

Although it is for `json.loads` and the like.

RasmusWL requested changes Jun 28, 2021

View reviewed changes

python/ql/src/experimental/semmle/python/frameworks/Stdlib.qll Outdated Show resolved Hide resolved

python/ql/src/experimental/semmle/python/frameworks/Stdlib.qll Outdated Show resolved Hide resolved

jorgectf and others added 6 commits June 28, 2021 14:23

Merge pull request #5 from RasmusWL/nosql-fixes

a5009ef

Small NoSQL fixes

Merge tests and update .expected

0ca4f24

Delete trivial tests

3fd1129

Polish documentation, mongoCollectionMethod() and update .expected

68c6831

Apply suggestions from code review

3504408

Update `xmltodict` format and delete `ujson` modeling. Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Move xmltodict to its own file under frameworks/

51395d1

RasmusWL reviewed Jun 29, 2021

View reviewed changes

jorgectf added 4 commits June 29, 2021 16:53

Fix qldocs typo

0819090

Take back non-trivial tests

9a8d1f8

Update .expected

621a810

Delete trivial *_good.py tests

e02a63a

RasmusWL approved these changes Jun 30, 2021

View reviewed changes

RasmusWL requested changes Jul 12, 2021

View reviewed changes

python/ql/test/experimental/query-tests/Security/CWE-943/mongoengine_good.py Outdated Show resolved Hide resolved

Change variable name to correct sanitized input variable

51a6140

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Update .expected

6f09b95

Merge remote-tracking branch 'origin/main' into jty/python/nosqlInjec…

e6ce10b

…tion

RasmusWL approved these changes Aug 24, 2021

View reviewed changes

RasmusWL merged commit ca341bd into github:main Aug 24, 2021

yoff mentioned this pull request Sep 11, 2023

Python: promote nosql query #14070

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: CWE-943 - Add NoSQL injection query #5612

Python: CWE-943 - Add NoSQL injection query #5612

mrthankyou commented Apr 6, 2021 •

edited

Loading

jorgectf commented Apr 6, 2021

RasmusWL left a comment •

edited

Loading

RasmusWL Jun 28, 2021

jorgectf Jun 28, 2021

RasmusWL left a comment

jorgectf commented Jun 28, 2021 •

edited

Loading

RasmusWL left a comment

RasmusWL Jun 29, 2021

jorgectf Jun 29, 2021 •

edited

Loading

RasmusWL Jun 29, 2021

jorgectf Jun 29, 2021 •

edited

Loading

RasmusWL left a comment

mrthankyou commented Jul 11, 2021

RasmusWL left a comment

RasmusWL commented Jul 14, 2021

mrthankyou commented Jul 19, 2021

RasmusWL commented Jul 19, 2021

mrthankyou commented Jul 19, 2021

mrthankyou commented Aug 6, 2021 •

edited

Loading

yoff commented Aug 9, 2021

RasmusWL left a comment

	mongoCollection() in [result.(DataFlow::AttrRead), result.(DataFlow::AttrRead).getObject()] and
	mongoCollection() = result.(DataFlow::AttrRead).getObject() and

Python: CWE-943 - Add NoSQL injection query #5612

Python: CWE-943 - Add NoSQL injection query #5612

Conversation

mrthankyou commented Apr 6, 2021 • edited Loading

jorgectf commented Apr 6, 2021

RasmusWL left a comment • edited Loading

Choose a reason for hiding this comment

RasmusWL Jun 28, 2021

Choose a reason for hiding this comment

jorgectf Jun 28, 2021

Choose a reason for hiding this comment

RasmusWL left a comment

Choose a reason for hiding this comment

jorgectf commented Jun 28, 2021 • edited Loading

RasmusWL left a comment

Choose a reason for hiding this comment

RasmusWL Jun 29, 2021

Choose a reason for hiding this comment

jorgectf Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

RasmusWL Jun 29, 2021

Choose a reason for hiding this comment

jorgectf Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

RasmusWL left a comment

Choose a reason for hiding this comment

mrthankyou commented Jul 11, 2021

RasmusWL left a comment

Choose a reason for hiding this comment

RasmusWL commented Jul 14, 2021

mrthankyou commented Jul 19, 2021

RasmusWL commented Jul 19, 2021

mrthankyou commented Jul 19, 2021

mrthankyou commented Aug 6, 2021 • edited Loading

yoff commented Aug 9, 2021

RasmusWL left a comment

Choose a reason for hiding this comment

mrthankyou commented Apr 6, 2021 •

edited

Loading

RasmusWL left a comment •

edited

Loading

jorgectf commented Jun 28, 2021 •

edited

Loading

jorgectf Jun 29, 2021 •

edited

Loading

jorgectf Jun 29, 2021 •

edited

Loading

mrthankyou commented Aug 6, 2021 •

edited

Loading