Skip to content

Commit

Permalink
avoid SQL injection exploits
Browse files Browse the repository at this point in the history
  • Loading branch information
@edeutsch
edeutsch committed Apr 20, 2022
1 parent 12b9fa7 commit fa2797e
Showing 1 changed file with 10 additions and 2 deletions.
12 changes: 10 additions & 2 deletions code/autocomplete/rtxcomplete.py
View file
Expand Up @@ -23,6 +23,7 @@ def load():
database_name = f"{autocomplete_filepath}{os.path.sep}{RTXConfig.autocomplete_path.split('/')[-1]}"
conn = sqlite3.connect(database_name)
cursor = conn.cursor()
#print(f"INFO: Connected to {database_name}",file=sys.stderr)
return True


Expand All @@ -39,6 +40,9 @@ def get_nodes_like(word,requested_limit):
if len(word) < 2:
return values

#### Try to avoid SQL injection exploits by sanitizing input #1823
word = word.replace('"','')

floor = word[:-1]
ceiling = floor + 'zz'

Expand Down Expand Up @@ -103,8 +107,12 @@ def get_nodes_like(word,requested_limit):
if found_fragment is None:

#### Cache this fragment in the database
cursor.execute("INSERT INTO cached_fragments(fragment) VALUES(?)", (word,))
fragment_id = cursor.lastrowid
try:
cursor.execute("INSERT INTO cached_fragments(fragment) VALUES(?)", (word,))
fragment_id = cursor.lastrowid
except:
print(f"ERROR: Unable to INSERT into cached_fragments(fragment)",file=sys.stderr)
fragment_id = 0
if debug:
print(f"fragment_id = {fragment_id}")

Expand Down

4 comments on commit fa2797e

@dkoslicki
Copy link
Member

Choose a reason for hiding this comment

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

Should we use query parameters/substitution variables instead? Since otherwise we would need to anticipate all special characters, right? (disclaimer: Not my area of expertise)

@dkoslicki
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like changing:

cursor.execute(f"SELECT term FROM terms WHERE term > \"{floor}\" AND term < \"{ceiling}\" AND term LIKE \"{word}%%\" ORDER BY length(term),term LIMIT {requested_limit}")

to

cursor.execute("SELECT term FROM terms WHERE term > %(floor)s AND term < %(ceiling)s AND term LIKE %(word)s ORDER BY length(term),term LIMIT %(requested_limit)d", {'floor': floor, 'ceiling': ceiling, 'word': word, 'requested_limit': requested_limit)

that way types are respected. My VM is borked atm, otherwise I would test this out

@edeutsch
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't really know for sure, but don't think your proposal solves the problem. I think it would be just as vulnerable to the exploit. But I am not certain. Using prepared statements is likely the safest way to do this. But I think stripping out all " characters renders it safe, too. I should do some more testing.

@saramsey
Copy link
Member

Choose a reason for hiding this comment

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

Good discussion. Yes, prepared statements are generally described as protecting against SQL injection but of course, would need to be tested in the context of a specific library and query.

Please sign in to comment.