GNOME Bugzilla – Bug 673912
Possible SQL injections
Last modified: 2018-09-24 09:19:19 UTC
Using git master. Looks like missing escaping. (grilo-test-ui:24869): Grilo-WARNING **: [metadata-store] grl-metadata-store.c:230: Failed to get metadata: unrecognized token: "' LIMIT 1"
+ Trace 230044
The code needs to use sqlite3_bind_*() For example: http://gtkpod.git.sourceforge.net/git/gitweb.cgi?p=gtkpod/libgpod;a=blob;f=src/itdb_sqlite.c;h=b5b29753ece09fc14aec462d3b88bb86882302cd;hb=HEAD#l183 3 plugins will need changes: $ git grep -l sqlite3_open src/media/bookmarks/grl-bookmarks.c src/media/podcasts/grl-podcasts.c src/metadata/metadata-store/grl-metadata-store.c
Created attachment 211936 [details] [review] metadata-store: Fix GET SQL injection
Not sure whether that actually works. More documentation at: https://sqlite.org/c3ref/stmt.html and: https://sqlite.org/c3ref/bind_blob.html
commit 2414933730ca17ffe08be68a5faec8ccec9b4630 Author: Bastien Nocera <hadess@hadess.net> Date: Thu Apr 12 15:58:19 2012 +0100 metadata-store: Fix GET SQL injection https://bugzilla.gnome.org/show_bug.cgi?id=673912 src/metadata/metadata-store/grl-metadata-store.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
That's _1_ broken use. There's 2 more in the same plugin, and at least 2 more plugins to fix.
Created attachment 272586 [details] [review] bookmarks: Use gom to access the database Instead of SQLite directly. gom is currently available at: https://github.com/chergert/gom/ Fixes: - No injection security holes - Easy DB extension Functional changes: - Boxes don't have a childcount anymore - Query has a different syntax which more closely matches the database names (as opposed to the undiscoverable names used in the queries) - Orphans are cleaned up on startup rather than when removing items
2 things to note: - Storing a new bookmark throws an error about non of the keys being writable, but still creates the item - You'll need gom plus a few patches that Christian will commit shortly
Patch looks good. You might want to add a comment in the resource class about it not being okay to mutate local state while an async operation is pending. (thread-safety, etc). Since the save() operation will happen in the sqlite thread. We might want to do something like G{Input,Output}Stream to check if there are pending operations inside of GomResource to make debugging this situation easier.
(In reply to comment #8) > Patch looks good. You might want to add a comment in the resource class about > it not being okay to mutate local state while an async operation is pending. > (thread-safety, etc). Since the save() operation will happen in the sqlite > thread. Actually, more worrying is the sync operations running in the main thread, while async runs in the SQLite thread. I'd rework the sync calls to go through the SQLite thread. The job would be pushed onto the SQLite thread using the queue, and we'd wait until it was finished using a condition (GCond). > We might want to do something like G{Input,Output}Stream to check if there are > pending operations inside of GomResource to make debugging this situation > easier. Isn't locking good enough?
So, this: diff --git a/gom/gom-adapter.c b/gom/gom-adapter.c index 367c466..a02d643 100644 --- a/gom/gom-adapter.c +++ b/gom/gom-adapter.c @@ -50,6 +50,8 @@ gpointer gom_adapter_get_handle (GomAdapter *adapter) { g_return_val_if_fail(GOM_IS_ADAPTER(adapter), NULL); + g_return_val_if_fail(adapter->priv->thread != NULL, NULL); + g_assert (g_thread_self () == adapter->priv->thread); return adapter->priv->db; } And fix the bugs. That would also allow me to implement a sync open() and close() which would be useful for this particular plugin.
Created attachment 272833 [details] [review] bookmarks: Use gom to access the database Instead of SQLite directly. gom is currently available at: https://github.com/chergert/gom/ Fixes: - No injection security holes - Easy DB extension Functional changes: - Boxes don't have a childcount anymore - Query has a different syntax which more closely matches the database names (as opposed to the undiscoverable names used in the queries) - Orphans are cleaned up on startup rather than when removing items
Created attachment 272834 [details] [review] bookmarks: Remove unused struct member
Created attachment 272835 [details] [review] bookmarks: Don't use g_strconcat() to build paths Use g_build_filename() instead.
Created attachment 272836 [details] [review] bookmarks: Also save thumbnail URL
Created attachment 273049 [details] [review] core: Don't try to store metadata if already saved When storing an item, don't try to call store_metadata() if there are no keys left to write. This fixes warnings in grilo-test-ui when trying to create a new bookmark as all the possible metadata we'd add will already be handled.
(In reply to comment #7) > 2 things to note: > - Storing a new bookmark throws an error about non of the keys being writable, > but still creates the item That's fixed by the patch to core above.
Any idea if GOM will do a release soon?
Comment on attachment 273049 [details] [review] core: Don't try to store metadata if already saved Attachment 273049 [details] pushed as 8fb9303 - core: Don't try to store metadata if already saved
I've pushed my GOM patches to https://github.com/hadess/gom so that should be ready for testing and review.
(In reply to comment #19) > I've pushed my GOM patches to https://github.com/hadess/gom so that should be > ready for testing and review. Does your repository contain all the required patches? Or some patch for the plugin that you didn't upload here? I've tried to build it using the above gom repository, and it is failing. For instance, complains about gom_repository_migrate_async() function (expected parameters doesn't fit with the signature).
Created attachment 273258 [details] [review] bookmarks: Use gom to access the database Instead of SQLite directly. gom is currently available at: https://github.com/chergert/gom/ Fixes: - No injection security holes - Easy DB extension Functional changes: - Boxes don't have a childcount anymore - Query has a different syntax which more closely matches the database names (as opposed to the undiscoverable names used in the queries) - Orphans are cleaned up on startup rather than when removing items
Created attachment 273259 [details] [review] bookmarks: Also save thumbnail URL
Created attachment 274653 [details] [review] bookmarks: Use gom to access the database Instead of SQLite directly. gom is currently available at: https://github.com/chergert/gom/ Fixes: - No injection security holes - Easy DB extension Functional changes: - Boxes don't have a childcount anymore - Query has a different syntax which more closely matches the database names (as opposed to the undiscoverable names used in the queries) - Orphans are cleaned up on startup rather than when removing items
Created attachment 274654 [details] [review] bookmarks: Also save thumbnail URL
Attachment 272834 [details] pushed as 2c931a8 - bookmarks: Remove unused struct member Attachment 272835 [details] pushed as 6669ba6 - bookmarks: Don't use g_strconcat() to build paths Attachment 274653 [details] pushed as 06812aa - bookmarks: Use gom to access the database Attachment 274654 [details] pushed as a3898a8 - bookmarks: Also save thumbnail URL
I'm using https://github.com/chergert/gom/, and when building the bookmarks source I get this error: grl-bookmarks.c: In function `grl_bookmarks_source_init': grl-bookmarks.c:251:3: error: implicit declaration of function `gom_repository_automatic_migrate_async' [-Werror=implicit-function-declaration] Seems that function is not available in the current GOM repository. Do I miss something? On the other hand, what remains to close this bug?
(In reply to comment #26) > I'm using https://github.com/chergert/gom/, and when building the bookmarks > source I get this error: > > grl-bookmarks.c: In function `grl_bookmarks_source_init': > grl-bookmarks.c:251:3: error: implicit declaration of function > `gom_repository_automatic_migrate_async' > [-Werror=implicit-function-declaration] > > > Seems that function is not available in the current GOM repository. Do I miss > something? That's not the current gom repository. It lives in GNOME git: http://www.audidude.com/blog/2014/04/12/gom.html and: http://www.hadess.net/2014/04/what-is-gom.html > On the other hand, what remains to close this bug? Fixing all the other plugins (podcasts, and metadata-store from the looks of it), or porting them to gom.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/grilo/issues/22.