cve-bin-tool
cve-bin-tool copied to clipboard
fix: improve sql table name validation
#3933 fix: improve sql table name validation
issue: https://github.com/intel/cve-bin-tool/issues/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✅:
# Use the validation function to check the table name
if not self.is_valid_table_name(table_name):
raise ValueError(f"Invalid table name: {table_name}")
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 ;]
@terriko
Update: I wasn't well from the past few days.
Here are the few changes I did-
Since, we are using predefined table_names
, thus eliminating the possibility of taking user input. Therefore, I have defined table names and based on that I created a validation function to validate table name and then I constructed the query directly using the table names.
Any suggestions and queries are welcomed.
Thank you.
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 I am already working on it.
@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.
Apologies for delay.
Regards
Harshit Tiwari
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.
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! Continuing after gitter discussion, for rebase you can do interactive rebase. And then implement something like this comment I made in #3968. Doing so will resolve bandit issues.
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.
I am really sorry for all the delays and everything on this @terriko Currently I am feeling little bit good. @terriko I wanted to ask what needs to be done here at this point and what else U want me to do. I will proceed with that. Since, A lot happened when I was away coz of infection and all. It would be great if @terriko point me further. Thank you. Ps: I want to continue with improvement part.
As @inosmeet says, their changes to fix another issue should make your code easier in the end.
You could git rebase origin/main
but since you don't have a huge number of changes, you might find it easier to just make a new branch based on origin/main
.
Either way, here's what you need:
- Add the function
is_valid_table_name
, but use the new EMPTY_SELECT_QUERIES instead of your VALID_TABLE_NAMES. Since EMPTY_SELECT_QUERIES is a dictionary, you can use EMPTY_SELECT_QUERIES.keys() to get the list of table names. - Add your
is_valid_table_name
check as you have it toget_all_records_in_table
- make the changes to
latest_schema
so it also uses EMPTY_SELECT_QUERIES (as @inosmeet described). Translated to use the new variable name it'll look something like this.
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")