Opened 5 years ago

Closed 3 years ago

Last modified 2 years ago

#2252 closed defect (fixed)

wxGUI vector digitizer passing unescaped text to database

Reported by: marisn Owned by: grass-dev@…
Priority: critical Milestone: 7.0.5
Component: wxGUI Version: svn-trunk
Keywords: security, code injection, SQL injection, data loss, v.db.update Cc:
CPU: Unspecified Platform: Unspecified

Description

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 '

Change History (20)

comment:1 Changed 5 years ago by wenzeslaus

Keywords: security code injection SQL injection data loss v.db.update added

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.

comment:2 Changed 5 years ago by wenzeslaus

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.

v.db.update also does not attempt to avoid SQL injection. And by the way, db.execute even cannot.

In this case, 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.

comment:3 Changed 5 years ago 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 tried something like this and there is even a Gist dedicated to it.

Here is the code from Gist:

    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 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).

comment:4 Changed 4 years ago 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 ?

comment:5 in reply to:  4 ; Changed 4 years ago by marisn

Replying to 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 ;)

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'

comment:6 in reply to:  5 ; Changed 4 years ago by mlennert

Replying to marisn:

Replying to 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 ;)

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

comment:7 in reply to:  6 Changed 4 years ago by marisn

Priority: blockercritical

Replying to 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.

comment:8 Changed 4 years ago by mlennert

I would propose to close this bug, as I don't see any real issue at hand, here.

comment:9 in reply to:  8 Changed 4 years ago by marisn

Replying to mlennert:

I would propose to close this bug, as I don't see any real issue at hand, here.

I'll paste my answer to your comment as an error message I got from wxgui vector digitizer with current trunk:

DBMI-DBF driver error:
SQL parser error (syntax error, unexpected NAME, expecting ')' or ',' processing 'ing') in statement:
INSERT INTO rm_me (cat,comments) VALUES (1,'you must be f'ing kidding; right?')
Unable to execute statement.

DBMI-DBF driver error:
SQL parser error (syntax error, unexpected NAME, expecting ')' or ',' processing 'ing') in statement:
INSERT INTO rm_me (cat,comments) VALUES (1,'you must be f'ing kidding; right?')
Unable to execute statement.

KĻŪDA: Error while executing: 'INSERT INTO rm_me (cat,comments) VALUES
         (1,'you must be f'ing kidding; right?')'

comment:10 Changed 3 years ago by neteler

Milestone: 7.0.07.0.3

comment:11 Changed 3 years ago by neteler

Milestone: 7.0.3

Ticket retargeted after milestone closed

comment:12 Changed 3 years ago by neteler

Milestone: 7.0.4

Ticket retargeted after 7.0.3 milestone closed

comment:13 Changed 3 years ago by martinl

Milestone: 7.0.47.0.5

comment:14 Changed 3 years ago by annakrat

In 69153:

wxGUI: escape single quotes when editing attributes from GUI, see #2252

comment:15 in reply to:  14 Changed 3 years ago by annakrat

Replying to annakrat:

In 69153:

wxGUI: escape single quotes when editing attributes from GUI, see #2252

This deals with single quotes only, no security issue is solved by this. So please test, I can backport it and decide what else to do with this ticket. We should at least downgrade the priority if not close it at all.

comment:16 Changed 3 years ago by marisn

After backporting, this ticket should be closed, as original issue has been solved. Parameter binding is a more complicated issue and thus should be addressed in a separate bug.

comment:17 Changed 3 years ago by annakrat

In 69174:

wxGUI: escape single quotes when editing attributes from GUI, see #2252 (merge from trunk, r69153, r69173)

comment:18 Changed 3 years ago by annakrat

In 69175:

wxGUI: escape single quotes when editing attributes from GUI, see #2252 (merge from trunk, r69153, r69173)

comment:19 in reply to:  16 Changed 3 years ago by annakrat

Resolution: fixed
Status: newclosed

Replying to marisn:

After backporting, this ticket should be closed, as original issue has been solved. Parameter binding is a more complicated issue and thus should be addressed in a separate bug.

comment:20 Changed 2 years ago by mlennert

I don't think we have to reopen this ticket, but an interesting effort was just done for QGIS Server:

http://oslandia.com/en/2017/06/14/qgis-server-security-aspect/

If anyone with SQL skills wants to try to wreck havoc on some GRASS GISDBASE data, the feedback would obviously be more than welcome.

Note: See TracTickets for help on using tickets.