Skip to content

Conversation

sfc-gh-mkeller
Copy link
Collaborator

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-693548

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    This fixes the CVE https://nvd.nist.gov/vuln/detail/CVE-2022-42965
    The issue comes from the greediness of the original regex. With this reworked function we have roughly linear performance. This is achieved by starting from the beginning to lazily strip away comments one-by-one instead of matching the whole SQL command with one regex. The new SQL is also better by matching more than multi-line comments.

@sfc-gh-mkeller
Copy link
Collaborator Author

sfc-gh-mkeller commented Nov 15, 2022

So as it turns out the new Regex is now able to find more comments and this has broken this old test.
Which makes this definitely a behavior change 😞 I've modified the old test in 2faa971.
edit: This was split into a new follow-up PR.

Comment on lines +13 to +15
^\s*(?:
/\*[\w\W]*?\*/
)""",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need these new lines and spaces in this regex?

Copy link
Collaborator Author

@sfc-gh-mkeller sfc-gh-mkeller Nov 16, 2022

Choose a reason for hiding this comment

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

It's easier to read. Here's the doc on re.VERBOSE, it's not a requirement, just wanted it to be readable.

Copy link

@sfc-gh-ssarangpur sfc-gh-ssarangpur left a comment

Choose a reason for hiding this comment

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

LGTM

@sfc-gh-hpathak
Copy link
Collaborator

@sfc-gh-mkeller Looks good overall for the CVE. However, I wonder if there is a potential to exploit this again via some other means. We may want to think about making this robust at a design level.

@codecov-commenter
Copy link

Codecov Report

Merging #1327 (9653dca) into main (aa6c88f) will increase coverage by 0.07%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##             main    #1327      +/-   ##
==========================================
+ Coverage   82.15%   82.22%   +0.07%     
==========================================
  Files          59       60       +1     
  Lines        8382     8421      +39     
  Branches     1240     1247       +7     
==========================================
+ Hits         6886     6924      +38     
  Misses       1187     1187              
- Partials      309      310       +1     
Impacted Files Coverage Δ
src/snowflake/connector/_sql_util.py 95.83% <95.83%> (ø)
src/snowflake/connector/cursor.py 95.59% <100.00%> (-0.04%) ⬇️
src/snowflake/connector/gcs_storage_client.py 78.16% <100.00%> (+4.75%) ⬆️
src/snowflake/connector/errors.py 89.78% <0.00%> (-0.54%) ⬇️
src/snowflake/connector/ocsp_snowflake.py 76.65% <0.00%> (-0.48%) ⬇️
src/snowflake/connector/cache.py 88.85% <0.00%> (-0.40%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sfc-gh-mkeller sfc-gh-mkeller linked an issue Nov 18, 2022 that may be closed by this pull request
@sfc-gh-mkeller sfc-gh-mkeller merged commit b9d2fc7 into main Nov 18, 2022
@sfc-gh-mkeller sfc-gh-mkeller deleted the mkeller/SNOW-693548-fix_redos branch November 18, 2022 23:30
@github-actions github-actions bot locked and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SNOW-697064: CVE-2022-42965
7 participants