codeql icon indicating copy to clipboard operation
codeql copied to clipboard

[Python] Add Unicode DoS (qhelp, tests and the query)

Open Sim4n6 opened this issue 2 years ago • 1 comments

Please, find in this pull request a new query "Unicode DoS" (CWE-770).

Sim4n6 avatar Jan 13 '24 09:01 Sim4n6

Hello Sim4n6 👋 You have submitted this pull request as a bug bounty report in the github/securitylab repository and therefore this pull request has been put into draft state to give time for the GitHub Security Lab to assess the PR. When GitHub Security Lab has finished assessing your pull request, it will be marked automatically as Ready for review. Until then, please don't change the draft state.

In the meantime, feel free to make changes to the pull request. If you'd like to maximize payout for your this and future submissions, here are a few general guidelines, that we might take into consideration when reviewing a submission.

  • the submission models widely-used frameworks/libraries
  • the vulnerability modeled in the submission is impactful
  • the submission finds new true positive vulnerabilities
  • the submission finds very few false positives
  • code in the submission is easy to read and will be easy to maintain
  • documentation is written clearly, highlighting the impact of the issue it finds and is written without grammatical or other errors. The code samples clearly show the vulnerability
  • the submission includes tests, change note etc.

Please note that these are guidelines, not rules. Since we have a lot of different types of submissions, the guidelines might vary for each submission.

Happy hacking!

ghsecuritylab avatar Jan 13 '24 16:01 ghsecuritylab

Basically a solid contribution :-)

yoff avatar Mar 11 '24 14:03 yoff

QHelp previews:

python/ql/src/experimental/Security/CWE-770/UnicodeDoS.qhelp

Denial of Service using Unicode Characters

When a remote user-controlled data can reach a costly Unicode normalization with either form, NFKC or NFKD, an attack such as the One Million Unicode Characters, could lead to a denial of service on Windows OS.

And, with the use of special Unicode characters, like U+2100 (℀) or U+2105 (℅), the payload size could be tripled after the compatibility normalization.

Recommendation

Ensure limiting the size of any incoming data that would go through a costly operations, including a Windows Unicode normalization with NFKC or NFKD. Such a recommandation would avoid a potential denial of service.

Example

In this example a simple user-controlled data reaches a Unicode normalization with the form "NFKC".

from flask import Flask, jsonify, request
import unicodedata

app = Flask(__name__)


@app.route("/bad_1")
def bad_1():
    # User controlled data
    file_path = request.args.get("file_path", "")

    # Normalize the file path using NFKC Unicode normalization
    return (
        unicodedata.normalize("NFKC", file_path),
        200,
        {"Content-Type": "application/octet-stream"},
    )

To fix this vulnerability, we need restrain the size of the user input.

For example, we can use the len() builtin function to limit the size of the user input.

from flask import Flask, jsonify, request
import unicodedata

app = Flask(__name__)


@app.route("/good_1")
def good_1():
    r = request.args.get("file_path", "")

    if len(r) <= 1_000:
        # Normalize the r using NFKD Unicode normalization
        r = unicodedata.normalize("NFKD", r)
        return r, 200, {"Content-Type": "application/octet-stream"}
    else:
        return jsonify({"error": "File not found"}), 404

References

github-actions[bot] avatar Mar 13 '24 10:03 github-actions[bot]

The qhelp file has an issue, see: https://github.com/github/codeql/actions/runs/8255777397/job/22602805894?pr=15319#step:7:25.

yoff avatar Mar 13 '24 10:03 yoff

One detail please, I've got this unusual error :

Could not evaluate queries in /home/sim4n6/Desktop/GhSec/codeql-fork-final/python/ql/test/experimental/query-tests/Security/CWE-770: com.semmle.util.concurrent.UnhandledAsyncException: 1 asynchronous exceptions caught
A fatal error occurred: Cant write tuple pool file

when I run codeql test run --show-extractor-output --threads=0 . on python/ql/test/experimental/query-tests/Security/CWE-770 directory.

Something is broken right ?

Sim4n6 avatar Mar 13 '24 11:03 Sim4n6

One detail please, I've got this unusual error :

Could not evaluate queries in /home/sim4n6/Desktop/GhSec/codeql-fork-final/python/ql/test/experimental/query-tests/Security/CWE-770: com.semmle.util.concurrent.UnhandledAsyncException: 1 asynchronous exceptions caught
A fatal error occurred: Cant write tuple pool file

when I run codeql test run --show-extractor-output --threads=0 . on python/ql/test/experimental/query-tests/Security/CWE-770 directory.

Something is broken right ?

Uh, that does sound broken. Two possibilities come to mind: It cannot write the file because the disk is full or it cannot write the file because of some issue with path length. I guess it could also be a permissions thing. Did it work before? What changed? It might make sense to report it, if there is no obvious solution..

yoff avatar Mar 13 '24 12:03 yoff

Test failures look good...in that you now catch two more bad results :-) So just update the expected file.

yoff avatar Mar 13 '24 14:03 yoff

The path limitation issue due to the encrypted disk was the issue indeed.

Sim4n6 avatar Mar 13 '24 15:03 Sim4n6

There is a new column in the results, related to provenance. It does not have anything to do with your query, I have rebased on main.

yoff avatar Mar 15 '24 13:03 yoff

Hi @yoff , I see that some checks are still failing and blocking the merge. Is there something else that needs to be done, please?

by the way, I did git diff 26a16b7857f6e8bb81e52fb9e4628fbec0a63a36 3acdd3382c85ad63c578d9afe41049a6882d3013 and not noticed the column you are referring to ?

Sim4n6 avatar Mar 15 '24 13:03 Sim4n6

Hi @yoff , I see that some checks are still failing and blocking the merge. Is there something else that needs to be done, please?

Yes, the tests should fail locally for you if you pull now, just update the expectations and all should be well :-)

by the way, I did git diff 26a16b7857f6e8bb81e52fb9e4628fbec0a63a36 3acdd3382c85ad63c578d9afe41049a6882d3013 and not noticed the column you are referring to ?

You can see the column here: https://github.com/github/semmle-code/actions/runs/8296908167/job/22706866832#step:26:5222

yoff avatar Mar 18 '24 12:03 yoff

.expected file is updated via https://github.com/github/codeql/pull/15319/commits/1af81673543e87b2ce7492d776fd0b0f40d9ded8 . The URL returned a 404 maybe the repo github/semmle-code is private. But I understand :-)

Sim4n6 avatar Mar 18 '24 13:03 Sim4n6

The URL returned a 404

Ah, I was hoping you had access to the test results..

yoff avatar Mar 18 '24 19:03 yoff