After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 673912 - Possible SQL injections
Possible SQL injections
Status: RESOLVED OBSOLETE
Product: grilo
Classification: Other
Component: plugins
git master
Other Linux
: Normal critical
: ---
Assigned To: grilo-maint
grilo-maint
Depends on:
Blocks:
 
 
Reported: 2012-04-11 14:44 UTC by Bastien Nocera
Modified: 2018-09-24 09:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
metadata-store: Fix GET SQL injection (1.68 KB, patch)
2012-04-12 14:58 UTC, Bastien Nocera
committed Details | Review
bookmarks: Use gom to access the database (43.48 KB, patch)
2014-03-21 18:00 UTC, Bastien Nocera
none Details | Review
bookmarks: Use gom to access the database (43.33 KB, patch)
2014-03-25 09:23 UTC, Bastien Nocera
none Details | Review
bookmarks: Remove unused struct member (686 bytes, patch)
2014-03-25 09:23 UTC, Bastien Nocera
committed Details | Review
bookmarks: Don't use g_strconcat() to build paths (1.24 KB, patch)
2014-03-25 09:23 UTC, Bastien Nocera
committed Details | Review
bookmarks: Also save thumbnail URL (5.53 KB, patch)
2014-03-25 09:23 UTC, Bastien Nocera
none Details | Review
core: Don't try to store metadata if already saved (1.09 KB, patch)
2014-03-27 02:42 UTC, Bastien Nocera
committed Details | Review
bookmarks: Use gom to access the database (43.34 KB, patch)
2014-03-29 22:50 UTC, Bastien Nocera
none Details | Review
bookmarks: Also save thumbnail URL (5.54 KB, patch)
2014-03-29 22:50 UTC, Bastien Nocera
none Details | Review
bookmarks: Use gom to access the database (42.42 KB, patch)
2014-04-18 08:34 UTC, Bastien Nocera
committed Details | Review
bookmarks: Also save thumbnail URL (5.46 KB, patch)
2014-04-18 08:36 UTC, Bastien Nocera
committed Details | Review

Description Bastien Nocera 2012-04-11 14:44:17 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"

  • #0 g_logv
    at gmessages.c line 758
  • #1 g_log
    at gmessages.c line 792
  • #2 grl_log_valist
    at grl-log.c line 293
  • #3 grl_log
    at grl-log.c line 309
  • #4 query_metadata_store
    at grl-metadata-store.c line 230
  • #5 grl_metadata_store_source_resolve
    at grl-metadata-store.c line 615
  • #6 resolve_idle
    at grl-metadata-source.c line 398
  • #7 g_idle_dispatch
    at gmain.c line 4634
  • #8 g_main_dispatch
    at gmain.c line 2515
  • #9 g_main_context_dispatch
    at gmain.c line 3052
  • #10 g_main_context_iterate
    at gmain.c line 3123
  • #11 g_main_loop_run
    at gmain.c line 3317
  • #12 gtk_main
    at gtkmain.c line 1161
  • #13 main
    at main.c line 2127

Comment 1 Bastien Nocera 2012-04-12 14:51:28 UTC
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
Comment 2 Bastien Nocera 2012-04-12 14:58:59 UTC
Created attachment 211936 [details] [review]
metadata-store: Fix GET SQL injection
Comment 3 Bastien Nocera 2012-04-12 15:02:14 UTC
Not sure whether that actually works.

More documentation at:
https://sqlite.org/c3ref/stmt.html
and:
https://sqlite.org/c3ref/bind_blob.html
Comment 4 Juan A. Suarez Romero 2012-04-18 09:00:11 UTC
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(-)
Comment 5 Bastien Nocera 2012-04-18 10:22:30 UTC
That's _1_ broken use. There's 2 more in the same plugin, and at least 2 more plugins to fix.
Comment 6 Bastien Nocera 2014-03-21 18:00:53 UTC
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
Comment 7 Bastien Nocera 2014-03-21 18:03:23 UTC
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
Comment 8 Christian Hergert 2014-03-22 19:20:32 UTC
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.
Comment 9 Bastien Nocera 2014-03-24 13:51:24 UTC
(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?
Comment 10 Bastien Nocera 2014-03-24 13:57:18 UTC
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.
Comment 11 Bastien Nocera 2014-03-25 09:23:17 UTC
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
Comment 12 Bastien Nocera 2014-03-25 09:23:23 UTC
Created attachment 272834 [details] [review]
bookmarks: Remove unused struct member
Comment 13 Bastien Nocera 2014-03-25 09:23:30 UTC
Created attachment 272835 [details] [review]
bookmarks: Don't use g_strconcat() to build paths

Use g_build_filename() instead.
Comment 14 Bastien Nocera 2014-03-25 09:23:35 UTC
Created attachment 272836 [details] [review]
bookmarks: Also save thumbnail URL
Comment 15 Bastien Nocera 2014-03-27 02:42:50 UTC
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.
Comment 16 Bastien Nocera 2014-03-27 02:43:45 UTC
(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.
Comment 17 Juan A. Suarez Romero 2014-03-27 09:22:10 UTC
Any idea if GOM will do a release soon?
Comment 18 Bastien Nocera 2014-03-27 17:11:56 UTC
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
Comment 19 Bastien Nocera 2014-03-28 09:37:57 UTC
I've pushed my GOM patches to https://github.com/hadess/gom so that should be ready for testing and review.
Comment 20 Juan A. Suarez Romero 2014-03-29 19:06:17 UTC
(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).
Comment 21 Bastien Nocera 2014-03-29 22:50:36 UTC
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
Comment 22 Bastien Nocera 2014-03-29 22:50:47 UTC
Created attachment 273259 [details] [review]
bookmarks: Also save thumbnail URL
Comment 23 Bastien Nocera 2014-04-18 08:34:38 UTC
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
Comment 24 Bastien Nocera 2014-04-18 08:36:00 UTC
Created attachment 274654 [details] [review]
bookmarks: Also save thumbnail URL
Comment 25 Bastien Nocera 2014-05-07 16:18:34 UTC
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
Comment 26 Juan A. Suarez Romero 2014-05-12 21:46:24 UTC
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?
Comment 27 Bastien Nocera 2014-05-12 22:12:23 UTC
(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.
Comment 28 GNOME Infrastructure Team 2018-09-24 09:19:19 UTC
-- 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.