python-opcua icon indicating copy to clipboard operation
python-opcua copied to clipboard

security vulnerability: history_sql.py isn't injection safe!

Open AndreasHeine opened this issue 5 years ago • 2 comments

found some unsafe sql-querys in our codebase! (also in asyncua -> f-strings are not injection safe!)

# BAD EXAMPLES. DON'T DO THIS!
cursor.execute("SELECT admin FROM users WHERE username = '" + username + '");
cursor.execute("SELECT admin FROM users WHERE username = '%s' % username);
cursor.execute("SELECT admin FROM users WHERE username = '{}'".format(username));
cursor.execute(f"SELECT admin FROM users WHERE username = '{username}'");

# SAFE EXAMPLES. DO THIS!
cursor.execute("SELECT admin FROM users WHERE username = %s'", (username, ));
cursor.execute("SELECT admin FROM users WHERE username = %(username)s", {'username': username});

AndreasHeine avatar Aug 28 '20 09:08 AndreasHeine

    def save_event(self, event):
        with self._lock:
            _c_sub = self._conn.cursor()

            table = self._get_table_name(event.emitting_node)
            columns, placeholders, evtup = self._format_event(event)
            event_type = event.EventType  # useful for troubleshooting database

            # insert the event into the database
            try:
                _c_sub.execute(
                    'INSERT INTO "{tn}" ("_Id", "_Timestamp", "_EventTypeName", {co}) VALUES (NULL, "{ts}", "{et}", {pl})'
                    .format(tn=table, co=columns, ts=event.Time, et=event_type, pl=placeholders), evtup)

"tn=table" cant be escaped by the parametriezes query so it has to be done with a own function.

maybe like:

_c_sub.execute(
'''INSERT INTO "{tn}" (:id, :ts, :et, :co) VALUES (NULL, :ts, :et, :pl)'''.format(tn=escape(table)) , 
{"id": NULL, "co": columns, "ts": event.Time, "et": event_type, "pl": placeholders, "evtup": evtup}
)

or

sql = '''INSERT INTO "{tn}" (:id, :ts, :et, :co) VALUES (NULL, :ts, :et, :pl)'''.format(tn=escape(table))
params = {"id": NULL, "co": columns, "ts": event.Time, "et": event_type, "pl": placeholders, "evtup": evtup})
_c_sub.execute(sql, params)

AndreasHeine avatar Aug 28 '20 10:08 AndreasHeine

Feel free to change. This history SQL was not thoroughly developed. It was mostly as an example for real persistence instead of the memory dict.

zerox1212 avatar Aug 28 '20 13:08 zerox1212