[GRASS-dev] [GRASS GIS] #2252: wxGUI vector digitizer passing unescaped text to database

GRASS GIS GRASS GIS developer
1

#2252: wxGUI vector digitizer passing unescaped text to database
-------------------------+--------------------------------------------------
Reporter: marisn | Owner: grass-dev@…
     Type: defect | Status: new
Priority: blocker | Milestone: 7.0.0
Component: wxGUI | Version: svn-trunk
Keywords: | Platform: Unspecified
      Cpu: Unspecified |
-------------------------+--------------------------------------------------
It seems that it is not possible to enter attribute data for a new vector
feature that is not valid SQL due to code being unable to pass user text
to the database as text.

Steps to reproduce:
  * Create a new vector data set;
  * Create a new text attribute column for it;
  * Digitize a new feature;
  * Provide following text as the attribute value: '; drop database
important_data; '
  * Observe kaBOOM! as text is parsed by database instead of being properly
escaped/passed as prepared statement to the DB.

{{{
DBMI-SQLite driver error:
Error in sqlite3_prepare():
near ";": syntax error

DBMI-SQLite driver error:
Error in sqlite3_prepare():
near ";": syntax error

KĻŪDA: Error while executing: 'INSERT INTO remove_me (cat,nosaukums)
          VALUES (3,''; drop database important_data; '')'
}}}

The issue will work also with more harmless examples like: It's fun

For better effect enter value as: '); delete from MYVECTORMAP; select '

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2252&gt;
GRASS GIS <http://grass.osgeo.org>

2

#2252: wxGUI vector digitizer passing unescaped text to database
-----------------------------------------------------------------------------+
Reporter: marisn | Owner: grass-dev@…
     Type: defect | Status: new
Priority: blocker | Milestone: 7.0.0
Component: wxGUI | Version: svn-trunk
Keywords: security, code injection, SQL injection, data loss, v.db.update | Platform: Unspecified
      Cpu: Unspecified |
-----------------------------------------------------------------------------+
Changes (by wenzeslaus):

  * keywords: => security, code injection, SQL injection, data loss,
               v.db.update

Comment:

I don't know (and quick look into source code haven't told me) what is
used in digitizer as a backend. Library, Python SQLite API or modules?

I've tried `v.db.update` with map `bridges` copied from `PERMANENT` and
this was OK:
{{{
v.db.update map=bridges column=LOCATION value="; drop database
important_data;" where=cat=1
}}}
String "; drop database important_data;" saved to the database.

But this:
{{{
v.db.update map=bridges column=LOCATION value="'; drop database
important_data; SELECT 1='1" where=cat=1
}}}
removed all the values from the column `LOCATION`. I'm not getting any
error messages.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2252#comment:1&gt;
GRASS GIS <http://grass.osgeo.org>

3

#2252: wxGUI vector digitizer passing unescaped text to database
-----------------------------------------------------------------------------+
Reporter: marisn | Owner: grass-dev@…
     Type: defect | Status: new
Priority: blocker | Milestone: 7.0.0
Component: wxGUI | Version: svn-trunk
Keywords: security, code injection, SQL injection, data loss, v.db.update | Platform: Unspecified
      Cpu: Unspecified |
-----------------------------------------------------------------------------+

Comment(by wenzeslaus):

[source:grass/trunk/gui/wxpython/vdigit/wxdigit.py?rev=60817
vdigit/wxdigit] is calling `db_set_string()` (or other db related
functions) at several places but I'm not able to spot quickly where are
the vulnerable places.

[source:grass/trunk/scripts/v.db.update/v.db.update.py?rev=59236#L92
v.db.update] also does not attempt to avoid SQL injection. And by the way,
`db.execute` even cannot.

In this case,
[source:grass/trunk/lib/python/pygrass/vector/table.py?rev=60969 PyGRASS]
has the best potential to handle these things correctly, since it is using
Python `sqlite3` package directly (although it should use GRASS library to
get all drivers).

Perhaps the library itself should provide a mechanism to handle user input
in a correct way using the proper database API for it or some custom code
(or nothing) if it is not available for the given database.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2252#comment:2&gt;
GRASS GIS <http://grass.osgeo.org>

4

#2252: wxGUI vector digitizer passing unescaped text to database
-----------------------------------------------------------------------------+
Reporter: marisn | Owner: grass-dev@…
     Type: defect | Status: new
Priority: blocker | Milestone: 7.0.0
Component: wxGUI | Version: svn-trunk
Keywords: security, code injection, SQL injection, data loss, v.db.update | Platform: Unspecified
      Cpu: Unspecified |
-----------------------------------------------------------------------------+

Comment(by wenzeslaus):

If we decide to do this separately in each usage of database (places in
GUI and each module for which it makes sense), there some people already
[http://stackoverflow.com/questions/6514274/how-do-you-escape-strings-for-
sqlite-table-column-names-in-python tried something] like this and there
is even a
[https://gist.github.com/1083518/584008c38a363c45acb84e4067b5188bb36e20f4
Gist] dedicated to it.

Here is the code from Gist:
{{{
#!python
     import codecs

     def quote_identifier(s, errors="strict"):
         encodable = s.encode("utf-8", errors).decode("utf-8")

         nul_index = encodable.find("\x00")

         if nul_index >= 0:
             error = UnicodeEncodeError("utf-8", encodable, nul_index,
nul_index + 1, "NUL not allowed")
             error_handler = codecs.lookup_error(errors)
             replacement, _ = error_handler(error)
             encodable = encodable.replace("\x00", replacement)

         return "\"" + encodable.replace("\"", "\"\"") + "\""
}}}

{{{
Given a string single argument, it will escape and quote it correctly or
raise an exception. The second argument can be used to specify any error
handler registered in [the `codecs` module][8]. The built-in ones are:

  - `'strict'`: raise an exception in case of an encoding error
  - `'replace'`: replace malformed data with a suitable replacement marker,
such as `'?'` or `'\ufffd'`
  - `'ignore'`: ignore malformed data and continue without further notice
  - `'xmlcharrefreplace'`: replace with the appropriate XML character
reference (for encoding only)
  - `'backslashreplace'`: replace with backslashed escape sequences (for
encoding only)
This doesn't check for reserved identifiers, so if you try to create a new
`SQLITE_MASTER` table it won't stop you.
}}}

It is using Python package [https://docs.python.org/2/library/codecs.html
codecs].

But still it would be probably ideal to use database API for it and expose
this through GRASS C API. Additionally, it would be interesting to do
something PyGRASS tried to do: use Python API directly but use some GRASS
functions to obtain the right database (backend).

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2252#comment:3&gt;
GRASS GIS <http://grass.osgeo.org>

5

#2252: wxGUI vector digitizer passing unescaped text to database
-----------------------------------------------------------------------------+
Reporter: marisn | Owner: grass-dev@…
     Type: defect | Status: new
Priority: blocker | Milestone: 7.0.0
Component: wxGUI | Version: svn-trunk
Keywords: security, code injection, SQL injection, data loss, v.db.update | Platform: Unspecified
      Cpu: Unspecified |
-----------------------------------------------------------------------------+

Comment(by mlennert):

I can't reproduce this bug. I've tried with different SQL texts and they
all are just put into the text field in the attribute table.

Maris, can you still confirm this bug ?

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2252#comment:4&gt;
GRASS GIS <http://grass.osgeo.org>

6

#2252: wxGUI vector digitizer passing unescaped text to database
-----------------------------------------------------------------------------+
Reporter: marisn | Owner: grass-dev@…
     Type: defect | Status: new
Priority: blocker | Milestone: 7.0.0
Component: wxGUI | Version: svn-trunk
Keywords: security, code injection, SQL injection, data loss, v.db.update | Platform: Unspecified
      Cpu: Unspecified |
-----------------------------------------------------------------------------+

Comment(by marisn):

Replying to [comment:4 mlennert]:
> I can't reproduce this bug. I've tried with different SQL texts and they
all are just put into the text field in the attribute table.
>
> Maris, can you still confirm this bug ?
Nothing has changed. Still text fields fail if a single apostrophe is
entered. Deleting whole database via text attribute entry field has been
left as an excise for reader :wink:

GRASS version: 7.1.svn
GRASS SVN revision: 64597M
Build date: 2015-01-13
Build platform: x86_64-unknown-linux-gnu

{{{
SQL parser error (syntax error, unexpected SELECT, expecting $end or ';'
processing 'select') in statement:
UPDATE lid_vor SET nosaukums=''; select * from lid_vor; '' WHERE cat=61
Unable to execute statement.

DBMI-DBF driver error:
SQL parser error (syntax error, unexpected SELECT, expecting $end or ';'
processing 'select') in statement:
UPDATE lid_vor SET nosaukums=''; select * from lid_vor; '' WHERE cat=61
Unable to execute statement.

KĻŪDA: Error while executing: 'UPDATE lid_vor SET nosaukums=''; select *
          from lid_vor; '' WHERE cat=61'
}}}

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2252#comment:5&gt;
GRASS GIS <http://grass.osgeo.org>

7

#2252: wxGUI vector digitizer passing unescaped text to database
-----------------------------------------------------------------------------+
Reporter: marisn | Owner: grass-dev@…
     Type: defect | Status: new
Priority: blocker | Milestone: 7.0.0
Component: wxGUI | Version: svn-trunk
Keywords: security, code injection, SQL injection, data loss, v.db.update | Platform: Unspecified
      Cpu: Unspecified |
-----------------------------------------------------------------------------+

Comment(by mlennert):

Replying to [comment:5 marisn]:
> Replying to [comment:4 mlennert]:
> > I can't reproduce this bug. I've tried with different SQL texts and
they all are just put into the text field in the attribute table.
> >
> > Maris, can you still confirm this bug ?
> Nothing has changed. Still text fields fail if a single apostrophe is
entered. Deleting whole database via text attribute entry field has been
left as an excise for reader :wink:

Ok, I forgot the apostrophes.

However, I tried deleting a table and haven't been able to do so:

{{{
db.execute sql="CREATE TABLE test_db_bug (id int)"
v.db.update test_digit_new col=test_text val="';drop table test_db_bug;'"
}}}

Table test_db_bug is still in the database. Same when I put the same value
in a text field in the digitizer: I get a similar error message to yours
above, but the table is not dropped.

Apparently any apostrophe in the update value causes an error message. I
agree that the error message is not clear, but I cannot reproduce the
danger you see for database integrity.

So my question remains, is this really a blocker ?

Moritz

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2252#comment:6&gt;
GRASS GIS <http://grass.osgeo.org>

8

#2252: wxGUI vector digitizer passing unescaped text to database
-----------------------------------------------------------------------------+
Reporter: marisn | Owner: grass-dev@…
     Type: defect | Status: new
Priority: critical | Milestone: 7.0.0
Component: wxGUI | Version: svn-trunk
Keywords: security, code injection, SQL injection, data loss, v.db.update | Platform: Unspecified
      Cpu: Unspecified |
-----------------------------------------------------------------------------+
Changes (by marisn):

  * priority: blocker => critical

Comment:

Replying to [comment:6 mlennert]:
>
> Ok, I forgot the apostrophes.
>
> However, I tried deleting a table and haven't been able to do so:
>
> {{{
> db.execute sql="CREATE TABLE test_db_bug (id int)"
> v.db.update test_digit_new col=test_text val="';drop table
test_db_bug;'"
> }}}
>
> Table test_db_bug is still in the database. Same when I put the same
value in a text field in the digitizer: I get a similar error message to
yours above, but the table is not dropped.
>
> Apparently any apostrophe in the update value causes an error message. I
agree that the error message is not clear, but I cannot reproduce the
danger you see for database integrity.
>
> So my question remains, is this really a blocker ?
>
> Moritz

OK, OK. I'm downgrading priority. My attempts to execute drop table
somehow failed. I blame my poor SQL skills for it. Probably it is really
just a "no apostrophes in attribute text" rule without any (so far
confirmed) security issue.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2252#comment:7&gt;
GRASS GIS <http://grass.osgeo.org>