big-list-of-naughty-strings icon indicating copy to clipboard operation
big-list-of-naughty-strings copied to clipboard

Replace "DROP TABLE users" with something more sane

Open ro31337 opened this issue 9 years ago • 25 comments

"DROP TABLE users" should be replaced to avoid potential data loss.

ro31337 avatar Aug 11 '15 01:08 ro31337

I don't get it - isn't that the whole point? User-given data should never be able to trigger a complete data loss

PhilLab avatar Aug 11 '15 08:08 PhilLab

I would hope that with testing using these strings, the data and environment are expected to be expendable.

carbontwelve avatar Aug 11 '15 08:08 carbontwelve

Is there a better SQL command to indicate that something has happened server-size? UPDATE would lead to data loss as well.

Maybe CREATE blns; ?

minimaxir avatar Aug 11 '15 14:08 minimaxir

What if you are using blns to test a project which you don't have access to the database? For example, some companies have third party QA testers that might not have access to the database - so creating a table wouldn't allow them to verify if it was successful or not due to the fact they are testing the project as if it was a black-box.

However some sort of destructive action (so update or drop) could allow that QA tester to verify that it was successful. Updating another person's username, or completely removing them from a system that would show them in the UI would be a way for them to do so.

I do agree with @carbontwelve on that testing using these strings should be done in the testing environment.

06b avatar Aug 11 '15 18:08 06b

Dropping a table is like checking a gun without bullets. It should not work, but just don't put it against your head while testing.

ALTER TABLE users RENAME TO users_blns;

should work in this case. Moreover, if you have foreign key constraints (usually you do), the table won't be dropped. If you renaming a table, you can rename it with existing foreign key constraints. We can leave drop syntax, but we should put ALTER TABLE above that.

ro31337 avatar Aug 11 '15 20:08 ro31337

Dropping a table is like checking a gun without bullets. It should not work, but just don't put it against your head while testing.

Well that was much more eloquent than my comment. I agree with @ro31337 that there are additional SQL strings that belong in this list; due to there there being many SQL injection attack vectors.

carbontwelve avatar Aug 12 '15 08:08 carbontwelve

There are some more sql related issues that can be tested. like the % wildcard and the -- comment and the single and double quotes.

--edit found them sorry :+1:

spidfire avatar Aug 19 '15 08:08 spidfire

I agree that another SQL injection should be included - not because the vulnerabilities exposed by this file should be tempered (as that would only be to assist a dangerous confusion of responsible practices), but because "DROP TABLES" is such a cliche in infosec that it's prone to be caught by extremely crude filters, naive to the degree that it's the only class of SQL injection they know to avoid.

stuartpb avatar Aug 19 '15 19:08 stuartpb

there should be a test of DDL but also a test of mutation such as update or delete, since it's possible to deny DDL (via grant, and you should why should the app be able to do this at all?) usually but still be vulnerable to injection.

xenoterracide avatar Dec 31 '15 15:12 xenoterracide

ALTER TABLE users RENAME TO users_blns;

I'm not sure this will actually work in all SQL engines... DDL is actually fairly proprietary and not fully standard. That said, if it blows up you've still caught the bug

xenoterracide avatar Dec 31 '15 15:12 xenoterracide

I suggest leaving in meaningful SQL characters, but not inserting useful syntax and/or syntax that makes persistent changes.

The following tests should break an application that fails to sanitize queries properly and may throw an SQL exception, which should be a good indication for any QA test without being potentially destructive. These also has the advantage of being engine and schema agnostic.

1' OR ' --
1; ' --
1'; ' --
FOO; --

Along the same lines, security tests for SQL injection have some useful patterns as well, even though they aren't the same for all engines. The following strings should create an unusual delay in application response times if the application improperly sanitizes SQL queries.

It should be noted that while most delay-based test vectors are innocuous, high CPU is a small risk if a function is called numerous times to create the delay, such as Benchmark().

1' WAITFOR DELAY '00:00:10'--
1; WAITFOR DELAY '00:00:10'--
1' OR BENCHMARK(100000000, rand())--
1-BENCHMARK(100000000, rand())--
1;SELECT pg_sleep(10)--
1';SELECT pg_sleep(10)--
1-SLEEP(10)--

ryanohoro avatar Feb 12 '16 06:02 ryanohoro

side note, we should be sure to have a string with -- I recently found a bug in our app where that's causing an exception because of lucene.

xenoterracide avatar Feb 12 '16 14:02 xenoterracide

I +1 this. I looked forward using this for pentesting, but having a DROP TABLE statement is not acceptable. We sometimes have to test production environments and it's common practice to check for interesting but non-destructive behaviors.

arisada avatar Feb 13 '16 11:02 arisada

The strings are supposed to be naughty and flush out errors. If you must run potentially dangerous strings against production systems, why not fork the project and create big-list-of-naughty-but-basically-safe-strings? I'm quite serious. Limiting the standard test suite everyone else can use simply because you're testing production and don't want to fully test it just doesn't make sense. (Perhaps I'm missing something?)

rmohns avatar Feb 16 '16 16:02 rmohns

Just to reiterate, my test vectors generate errors and out-of-tolerance conditions without making permanent changes.

ryanohoro avatar Feb 16 '16 16:02 ryanohoro

I don't see why your list has to contain dangerous strings, it makes it useless for real use, plus there are dozens alternatives that will catch the same mistakes without damaging anything. And I don't feel like forking it and following the main branch changes.

arisada avatar Feb 16 '16 17:02 arisada

I don't see why your list has to contain dangerous strings

This repo is literally called "Big List of Naughty Strings." It should be expected that they are dangerous.

I can't see why, under any circumstances, this would be pointed at a production environment.

mattgrande avatar Feb 16 '16 17:02 mattgrande

Then expect a few merge requests with "rm -fr /" and "reg delete /f SAM" for completeness, I think ls'ing directories is too gentle.

arisada avatar Feb 16 '16 18:02 arisada

@mattgrande To be fair, @arisada does have a point.

Just because this shouldn't be used on production systems doesn't mean it should go out of its way to be harmful when there are more benign proof-of-concept cases available to choose from.

ssokolow avatar Feb 16 '16 18:02 ssokolow

there are dozens alternatives that will catch the same mistakes without damaging anything

This is key. It's win-win. The goal is to reduce damages in case something does go wrong, while not compromising the class of bugs you're testing for. Simply changing the table name from users to something like __BLNS__TEST__TABLE__ should suffice.

parshap avatar Feb 16 '16 19:02 parshap

CREATE TABLE pwned; would suffice

arisada avatar Feb 16 '16 19:02 arisada

Huh, this blew up unexpectedly.

Yes, I am in favor of replacing the DROP with a CREATE or something along those lines, and will merge a PR that does so.

I'll note that the list is intended for QA and not pentesting, although there certainly is a lot of overlap. (E.g XSS)

minimaxir avatar Feb 16 '16 19:02 minimaxir

This thread :+1:

mikeres0 avatar Feb 18 '16 10:02 mikeres0

How about creating a new table with an identifier relating to the naughty string that created it, so that you notice the new table in the database and can specifically debug the issue with knowledge of which sets of characters probably caused it.

iitalics avatar Jan 15 '17 21:01 iitalics

Please stop the madness, you make Little Bobby Tables cry :/

pre avatar Jan 16 '17 20:01 pre