cve-bin-tool icon indicating copy to clipboard operation
cve-bin-tool copied to clipboard

fix: improve sql table name validation

Open harshittiwariii opened this issue 11 months ago • 8 comments

#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 ;]

harshittiwariii avatar Mar 21 '24 21:03 harshittiwariii

@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.

harshittiwariii avatar Mar 28 '24 20:03 harshittiwariii

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.

harshittiwariii avatar Apr 02 '24 10:04 harshittiwariii

@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

harshittiwariii avatar Apr 04 '24 17:04 harshittiwariii

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.

terriko avatar Apr 10 '24 19:04 terriko

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.

inosmeet avatar Apr 11 '24 03:04 inosmeet

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.

inosmeet avatar Apr 11 '24 10:04 inosmeet

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.

harshittiwariii avatar Apr 11 '24 19:04 harshittiwariii

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:

  1. 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.
  2. Add youris_valid_table_name check as you have it to get_all_records_in_table
  3. 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")

terriko avatar Apr 15 '24 19:04 terriko