FreeOpcUa / python-opcua

LGPL Pure Python OPC-UA Client and Server
http://freeopcua.github.io/
GNU Lesser General Public License v3.0
1.33k stars 661 forks source link

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

Open AndreasHeine opened 3 years ago

AndreasHeine commented 3 years ago

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 commented 3 years ago
    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)
zerox1212 commented 3 years ago

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.