minimaxir / big-list-of-naughty-strings

The Big List of Naughty Strings is a list of strings which have a high probability of causing issues when used as user-input data.
MIT License
46.18k stars 2.13k forks source link

Replace "DROP TABLE users" with something more sane #16

Open ro31337 opened 9 years ago

ro31337 commented 9 years ago

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

PhilLab commented 9 years ago

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

carbontwelve commented 9 years ago

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

minimaxir commented 9 years ago

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

06b commented 9 years ago

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.

ro31337 commented 9 years ago

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.

carbontwelve commented 9 years ago

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.

spidfire commented 9 years ago

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:

stuartpb commented 9 years ago

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.

xenoterracide commented 8 years ago

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 commented 8 years ago

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

ryanohoro commented 8 years ago

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)--
xenoterracide commented 8 years ago

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.

arisada commented 8 years ago

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.

rmohns commented 8 years ago

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

ryanohoro commented 8 years ago

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

arisada commented 8 years ago

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.

mattgrande commented 8 years ago

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.

arisada commented 8 years ago

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

ssokolow commented 8 years ago

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

parshap commented 8 years ago

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.

arisada commented 8 years ago

CREATE TABLE pwned; would suffice

minimaxir commented 8 years ago

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)

mikeres0 commented 8 years ago

This thread :+1:

iitalics commented 7 years ago

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.

pre commented 7 years ago

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