-
Notifications
You must be signed in to change notification settings - Fork 455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: improve sql table name validation #3965
fix: improve sql table name validation #3965
Conversation
@terriko |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is failing a number of test jobs because it can't run at all:
│ ❱ 242 │ │ │ │ not self.latest_schema("cve_severity", severity_schem │
│ 243 │ │ │ │ or not self.latest_schema("cve_range", range_schema) │
│ 244 │ │ │ │ or not self.latest_schema("cve_exploited", exploit_sc │
│ 245 │ │ │ │ # or not self.latest_schema("cve_metrics",cve_metrics │
│ │
│ /home/runner/work/cve-bin-tool/cve-bin-tool/cve_bin_tool/cvedb.py:288 in │
│ latest_schema │
│ │
│ 285 │ │ table_schema.pop() │
│ 286 │ │ │
│ 287 │ │ # getting current schema from cve_severity │
│ ❱ 288 │ │ current_schema = [x[0] for x in result.description] │
│ 289 │ │ │
│ 290 │ │ if table_schema == current_schema: │
│ 291 │ │ │ schema_latest = True │
╰──────────────────────────────────────────────────────────────────────────────╯
AttributeError: 'list' object has no attribute 'description'
Thank you for the review @terriko |
@terriko Thanks for reviews. From past 2 days I m not well. I will get back to it as soon as i feel little better. |
added dictionary to be used for better table name validation. This will help resolve bandit issues in intel#3965. Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
added dictionary to be used for better table name validation. This will help resolve bandit issues in #3965. --------- Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
I think maybe the bandit issue has been resolved with the refactoring of the schema checks in #3968. I'll leave this open so you can decide if there's more to do in terms of improving validation, though. |
Actually, I've just implemented the structure so that @harshittiwariii can go forward with this PR using that. |
Hey @harshittiwariii! |
I am really sorry for all the delays and everything on this @terriko |
As @inosmeet says, their changes to fix another issue should make your code easier in the end. You could Either way, here's what you need:
if table_name in EMPTY_SELECT_QUERIES.keys():
query = EMPTY_SELECT_QUERIES [table_name]
cursor.execute(query)
else:
# Handle invalid table names
raise ValueError("Invalid table name") |
In the course of some other refactoring in cvedb.py, we've got another way to handle schema valiation such that bandit won't complain. * fixes intel#3933 * closes intel#3965 Signed-off-by: Terri Oda <terri.oda@intel.com>
#3933 fix: improve sql table name validation
issue: #3933
Change 1 - A validation function to ensure that only valid table names are used when constructing queries dynamically in the codebase. ✅
Change 2 - In our
latest_schema
method, replaced the manual table name checks with a call to this function✅:Change 3 - Bandit issue addressed in line 269 and 888 i.e; Possible SQL injection vector through string-based query construction.✅
I think bandit is happy now for
cvedb.py
file :)Note: I have divided problem in chunks so right now I cleared out cvedb.py file now I will move towards other files also.
Once everything done, I will squash commits no worries ;]