I reviewed libservicelog commit ebf842c43e0ad16e975db1f4a10b4f83ccbfdf72; this sholdn't be considered a full security audit.
Nearly everything I found is an instance of one of two issues: - Using snprintf() rether than sqlite3_bind_*() and prepared statements -- sometimes the parameters are supplied from static strings elsewhere in the program and can't be supplied via sqlite3_bind_*() functions, but many parameters may come from elsewhere. I think the code would be improved to clearly delineate what comes from within the program -- and can safely be constructed via snprintf() -- and what comes from outside the program and should be handled with prepared statements. This may look like a complication but I think the clear difference between the two types of parameters would improve the program clarity. - snprintf() is never checked for error returns; when populating log->error or slog->error, it doesn't really matter -- a best-effort is all that's called for -- but many times it's being used with potentially untrusted inputs to construct SQL queries. I also found one case where the buffer isn't large enough to handle the query if constructed with fairly large -- yet legal -- ids. (A prepared statement would probably be the better fix.) Here's the functions that seemed likely to suffer the most from lacking prepared statements: - servicelog_event_log() doesn't use sqlite3_bind_*() functions to prevent sql injection - servicelog_event_log() doesn't check snprintf() error returns, sqlite3_bind_*() functions wouldn't require snprintf() steps - servicelog_event_query(), doesn't check snprintf() error returns, doesn't use sqlite3_bind_*() functions to prevent sql injection - servicelog_event_repair() doesn't check snprintf() error returns, buffer isn't long enough (I came up with 86 chars needed) - delete_row() doesn't check snprintf() error return, doesn't use sqlite3_bind_*() functions to prevent sql injection - servicelog_event_delete() doesn't check snprintf() error returns - insert_addl_data_os() doesn't check snprintf() error returns, doesn't use sqlite3_bind_*() functions to prevent sql injection - insert_addl_data_enclosure() doesn't check snprintf() error returns, doesn't use sqlite3_bind_*() functions to prevent sql injection - servicelog_repair_log() doesn't check snprintf() error returns, doesn't use sqlite3_bind_*() functions to prevent sql injection - servicelog_notify_log() doesn't check snprintf() error returns, doesn't use sqlite3_bind_*() functions to prevent sql injection - servicelog_notify_update() doesn't check snprintf() error returns, doesn't use sqlite3_bind_*() functions to prevent sql injection And some small miscellaneous bits: - format_text_to_insert() isn't needed if it can be replaced by sqlite3_bind_*() functions - servicelog_truncate() consider adding VACUUM call I'd like to see sqlite3_bind_*() used whenever string inputs aren't static. Thanks -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1417608 Title: [MIR] ppc64-diag needed in minimal for hotplug capabilities To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/libservicelog/+bug/1417608/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
