Python: promote nosql query

[yoff]

Promotes the NoSQL injection query and updates it to modern style. Also addresses the points raised in the original review.

The approach is similar to JavaScript but, rather than having a state for just strings and a state for "control of the entire object", we have a state for just strings and a state for a tainted dictionary. This requires us to add a sanitizer for the $eq-pattern.

[github-actions]

QHelp previews:

python/ql/src/Security/CWE-943/NoSqlInjection.qhelp

NoSQL Injection

Passing user-controlled sources into NoSQL queries can result in a NoSQL injection flaw. This tainted NoSQL query containing a user-controlled source can then execute a malicious query in a NoSQL database such as MongoDB. In order for the user-controlled source to taint the NoSQL query, the user-controller source must be converted into a Python object using something like json.loads or xmltodict.parse.

Because a user-controlled source is passed into the query, the malicious user can have complete control over the query itself. When the tainted query is executed, the malicious user can commit malicious actions such as bypassing role restrictions or accessing and modifying restricted data in the NoSQL database.

Recommendation

NoSQL injections can be prevented by escaping user-input's special characters that are passed into the NoSQL query from the user-supplied source. Alternatively, using a sanitize library such as MongoSanitizer will ensure that user-supplied sources can not act as a malicious query.

Example

In the example below, the user-supplied source is passed to a MongoDB function that queries the MongoDB database.

from flask import Flask, request
from flask_pymongo import PyMongo
import json

mongo = PyMongo(app)


@app.route("/")
def home_page():
    unsanitized_search = request.args['search']
    json_search = json.loads(unsanitized_search)

    result = mongo.db.user.find({'name': json_search})

This can be fixed by using a sanitizer library like MongoSanitizer as shown in this annotated code version below.

from flask import Flask, request
from flask_pymongo import PyMongo
from mongosanitizer.sanitizer import sanitize
import json

mongo = PyMongo(app)


@app.route("/")
def home_page():
    unsafe_search = request.args['search']
    json_search = json.loads(unsafe_search)
    safe_search = sanitize(unsanitized_search)

    result = client.db.collection.find_one({'data': safe_search})

References

• Mongoengine: Documentation.
• Flask-Mongoengine: Documentation.
• PyMongo: Documentation.
• Flask-PyMongo: Documentation.
• OWASP: NoSQL Injection.
• Security Stack Exchange Discussion: Question 83231.
• Common Weakness Enumeration: CWE-943.

[RasmusWL]

I've looked superficially at tests, and will do a proper review of the QL code a bit later on. But here are a few things I noticed:

Can you please add tests of
$group and mapReduce? 🙏 I think these should be easy enough to model, but we should at the very least have tests to highlight how they work.

the query tests use a pattern like our examples _good.py and _bad.py. Since the _good.py version is mostly copied boilerplate setup code, I think it would be nice to merge the files into ONE file 😊

Lastly, we need a change-note saying a new query was added 👍

[RasmusWL]

Did you consider naming this NoSqlExecution to match with our current SqlExecution concept? I don't disagree with the naming per-se, but I'm thinking consistency might be better here 😊

[yoff]

I just kept the name from the submission, but I agree that I should probably change it 👍

[yoff]

Can you please add tests of
$group and mapReduce?

Yes, although it appears to not be $group but rather $accumulator that is interesting, since that is the one that can contain JavaScript code (and it can appear inside other things than $group).

[RasmusWL]

I still think we could end up with a very long jumpstep due to the way it's written right now, which I don't think is a good solution.

[RasmusWL]

I'm falling into the trap of bike-shedding InterpretedString, since it instantly makes me think of string-interpolation (f-strings). As I've understood this problem, we are looking at two scenarios: Where a user controls a dictionary (bad), and where user only controls a string (ok in something like db.find_one({'data': user_string}). I think the original distinction made that very clear, which was really good 👍 (If I had to choose between the two, I honestly liked the old one better.. it just had some other minor problems)

I forgot to mention in the written review that you should add the new sinks to

codeql/python/ql/src/meta/alerts/TaintSinks.ql

Line 35 in d7aea22

DataFlow::Node relevantTaintSink(string kind) {

(I only briefly mentioned this in the meeting monday)

[yoff]

As I've understood this problem, we are looking at two scenarios: Where a user controls a dictionary (bad), and where user only controls a string (ok in something like db.find_one({'data': user_string}).

I think that is actually a simplification. There is the scenario where the string gets interpreted as a dictionary (and you can write something like { $neq: 1 }), but there is also the scenario where the string is interpreted as JavaScript code. Perhaps, instead of InterpretedString something like Payload would be better?

[yoff]

Renamed flow states again, as discussed offline.

[RasmusWL]

should this ensure the sink interprets dicts?

[yoff]

Yes, that is an oversight, thanks!

[RasmusWL]

I think we're good to go for now 👍

I think we might want to look into supporting more formats than JSON (and a general approach), and might want to look into how taint should propagate through the dictionaries, but this could be done as future work 👍

[yoff]

Should modeling more conversions to dictionaries? Like YAML parsing?

We should at least investigate if other formats get interpreted. I will leave this as a future improvement.