wxGUI vector digitizer passing unescaped text to database

[marisn]

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 '

[wenzeslaus]

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.

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

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

[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 ?

[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'

[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

[marisn]

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.

[mlennert]

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

[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?')'

[neteler]

Ticket retargeted after milestone closed

[neteler]

Ticket retargeted after 7.0.3 milestone closed

[annakrat]

In 69153:

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

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

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

[annakrat]

In 69174:

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

[annakrat]

In 69175:

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

[annakrat]

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.

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