4

Is this statement prone to injection? I've tried converting it to a prepared statement but have run into a lot of trouble.

import cgi
import sqlite3

form = cgi.FieldStorage()
user = form['username'].value
passw = form['password'].value

conn = sqlite3.connect('class.db')
c = conn.cursor()

c.execute('select * from users where username="' + user + '" and password="' + passw + '";')
results = c.fetchall()
conn.close()

The aim is to secure this statement so it cannot be intercepted.

Thanks

Robert Harvey
  • 178,213
  • 47
  • 333
  • 501
saturnstar
  • 41
  • 3
  • 2
    Yes absolutely it is. Also you can't call a variable `pass`. – khelwood Nov 20 '19 at 09:35
  • yes. check out bind and placeholder syntax for sqllite https://pythonprogramming.net/sqlite-part-2-dynamically-inserting-database-timestamps/ and https://xkcd.com/327/ – JL Peyret Nov 21 '19 at 06:45
  • What is this dupe target? That's a completely different question. – ruohola Nov 22 '19 at 11:11
  • This answer for PHP, but the same holds in every language. Use prepared statements and parameterized queries. https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php – jnovack Oct 12 '20 at 17:41

1 Answers1

7

Yes it is prone to injection. If someone for example enters foo";-- as the login username, they could login to the account named foo without any password checking:

user = 'foo";--'
passw = 'anything here'
sql = 'select * from users where username="' + user + '" and password="' + passw + '";'
print(sql)

Output (with SQL syntax highlighting):

select * from users where username="foo";--" and password="anything here";

There are also a million other ways that your original code is vulnerable to injection, the point is that you should never ever do simple string building for SQL statements.

You can should simply use parametrized queries to avoid this. On top of being 100% secure against injection, they make complicated queries much more simple to write and read:

import cgi
import sqlite3

form = cgi.FieldStorage()
user = form['username'].value
passw = form['password'].value

conn = sqlite3.connect('class.db')
c = conn.cursor()

c.execute('select * from users where username = ? and password = ?;', (user, passw))
results = c.fetchall()
conn.close()
ruohola
  • 21,987
  • 6
  • 62
  • 97
  • So simply including _?_ and including _, (username, password)_ at the end of the statement makes the statement safe? – saturnstar Nov 20 '19 at 09:36
  • 1
    Yes, then the parameter values will *never* be interpreted as sql code. – ruohola Nov 20 '19 at 09:37
  • but doesn't the _c.execute_ python command only execute one statement? So dropping the table wouldn't occur? – saturnstar Nov 20 '19 at 09:38
  • is there any way to drop table using my statement? – saturnstar Nov 20 '19 at 09:53
  • Even if they couldn't drop a table, they could use something like `" or true; --` as a password and bypass your authentication. – khelwood Nov 20 '19 at 09:54
  • @saturnstar See my edited threat example. – ruohola Nov 20 '19 at 09:56
  • 1
    @saturnstar, Executing multiple statements isn't the only harmful thing that can be done with SQL injection. The example shows a way that an attacker can log into _any_ account without knowing the password. So they could act as any user. What if this code were for your bank's online web interface? Anyone could log in as you, and deduct money from your account. – Bill Karwin Nov 20 '19 at 16:53
  • 1
    @saturnstar There’s an extra reason to prefer binds/parametrization over query string concatenation : mapping `None` to `NULL` is a proper pain using string building. Been there, done that. Edge cases all over with chars and dates. Btw you can **still** build queries dynamically, but just keep the variable placeholders, not their values. – JL Peyret Nov 21 '19 at 08:19