Sql injection

Question

Sql injection

sandro.venturoni

SSC Journeyman

Points: 83
More actions
November 30, 2018 at 2:40 am

#382378

Hi all,
I'm working in an existing C# - Sql Server project where all query was like this:
"SELECT Field FROM Table WHERE Filter='"+ userinput.Replace("'", "''") + "'"
Is this code sql injection safe? If not can someone provide a real example on injection?
I'm aware that this is not the best way to do it, but this is an obsolete software so optimize this code is not an option.

Viewing 15 posts - 1 through 15 (of 21 total)

You must be logged in to reply to this topic. Login to reply

Thom A SSC Guru Points: 99129 More actions · Answer 1

No, the only way to to make your query safe is to parametrise it. The documentation example covers how to parametrise your query.

Thom~

Excuse my typos and sometimes awful grammar. My fingers work faster than my brain does.
Larnu.uk

sandro.venturoni SSC Journeyman Points: 83 More actions · Answer 2

Hi Thom and thanks for your post.
Your position was exactly my position, but I was answered:
"That's true in case of multiple target database, but we works only with Sql Server, so this code works fine. If you can provide an example that brokes this code, we will be happy to consider"
So I'm still looking for.

Thom A SSC Guru Points: 99129 More actions · Answer 3

sandro.venturoni - Friday, November 30, 2018 3:19 AM
Hi Thom and thanks for your post.
Your position was exactly my position, but I was answered:
"That's true in case of multiple target database, but we works only with Sql Server, so this code works fine. If you can provide an example that brokes this code, we will be happy to consider"
So I'm still looking for.

Still looking for what? The attitude you were given is awful. There is no good reason to not parametrise the query; clearly the people you are working with are just lazy and probably don't care. Are you saying you want an example that would inject?

Thom~

Excuse my typos and sometimes awful grammar. My fingers work faster than my brain does.
Larnu.uk

Jonathan AC Roberts SSCoach Points: 18347 More actions · Answer 4

sandro.venturoni - Friday, November 30, 2018 2:40 AM
Hi all,
I'm working in an existing C# - Sql Server project where all query was like this:
"SELECT Field FROM Table WHERE Filter='"+ userinput.Replace("'", "''") + "'"
Is this code sql injection safe? If not can someone provide a real example on injection?
I'm aware that this is not the best way to do it, but this is an obsolete software so optimize this code is not an option.

It is possible to have SQL injection with Replace("'", "''"). The string delimiter can be changed in SQL Server to be another character other than single quote.
What's also bad about that SQL is that it doesn't allow reuse of the query plan as the SQL will be different on every call due to having different values to compare the Filter column against.

Jeff Moden SSC Guru Points: 1004312 More actions · Answer 5

Jonathan AC Roberts - Friday, November 30, 2018 6:52 AM
sandro.venturoni - Friday, November 30, 2018 2:40 AM
Hi all,
I'm working in an existing C# - Sql Server project where all query was like this:
"SELECT Field FROM Table WHERE Filter='"+ userinput.Replace("'", "''") + "'"
Is this code sql injection safe? If not can someone provide a real example on injection?
I'm aware that this is not the best way to do it, but this is an obsolete software so optimize this code is not an option.
It is possible to have SQL injection with Replace("'", "''"). The string delimiter can be changed in SQL Server to be another character other than single quote.
What's also bad about that SQL is that it doesn't allow reuse of the query plan as the SQL will be different on every call due to having different values to compare the Filter column against.

Thom A - Friday, November 30, 2018 3:23 AM
sandro.venturoni - Friday, November 30, 2018 3:19 AM
Hi Thom and thanks for your post.
Your position was exactly my position, but I was answered:
"That's true in case of multiple target database, but we works only with Sql Server, so this code works fine. If you can provide an example that brokes this code, we will be happy to consider"
So I'm still looking for.
Still looking for what? The attitude you were given is awful. There is no good reason to not parametrise the query; clearly the people you are working with are just lazy and probably don't care. Are you saying you want an example that would inject?

With the idea that one good test is worth a thousand "expert" opinions, he's looking for a demonstrable test that he can use to clearly demonstrate the error of their thinking.

--Jeff Moden

RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.

Change is inevitable... Change for the better is not.

Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally)

Jonathan AC Roberts SSCoach Points: 18347 More actions · Answer 6

Jeff Moden - Friday, November 30, 2018 8:40 AM
With the idea that one good test is worth a thousand "expert" opinions, he's looking for a demonstrable test that he can use to clearly demonstrate the error of their thinking.

I had a quick go with no success.
I was thinking if someone had SET QUOTED_IDENTIFIER OFF it might be possible. But couldn't get it to inject anything.

autoexcrement SSCertifiable Points: 5956 More actions · Answer 7

I'd love to see an example too. But it seems the issue is somewhat complicated by the use of C# and SQL. Hacking this system would require passing a value that tricks both the C# code and then the SQL code. So it's not something we can just test using SQL alone is it?

"If I had been drinking out of that toilet, I might have been killed." -Ace Ventura

Matt Miller (4) SSC Guru Points: 124210 More actions · Answer 8

autoexcrement - Friday, November 30, 2018 2:37 PM
I'd love to see an example too. But it seems the issue is somewhat complicated by the use of C# and SQL. Hacking this system would require passing a value that tricks both the C# code and then the SQL code. So it's not something we can just test using SQL alone is it?

Actually - C# is the thing that gives you an opening, since it allows escape sequences.

Try passing this in:

userinput="Idontwantanyofyourdata\u2019 union select name from sys.tables;--";

Which would then concatenate into the following once it passes through C# into SQL.

SELECT Field FROM Table WHERE Filter='Idontwantanyofyourdata' union select name from sys.tables;--'

And then again - you could replace the UNION with just about anything. Delete/insert/select from system tables.

----------------------------------------------------------------------------------
Your lack of planning does not constitute an emergency on my part...unless you're my manager...or a director and above...or a really loud-spoken end-user..All right - what was my emergency again?

Matt Miller (4) SSC Guru Points: 124210 More actions · Answer 9

Matt Miller (4) - Friday, November 30, 2018 5:14 PM
autoexcrement - Friday, November 30, 2018 2:37 PM
I'd love to see an example too. But it seems the issue is somewhat complicated by the use of C# and SQL. Hacking this system would require passing a value that tricks both the C# code and then the SQL code. So it's not something we can just test using SQL alone is it?
Actually - C# is the thing that gives you an opening, since it allows escape sequences.
Try passing this in:
userinput="Idontwantanyofyourdata\u2019 union select name from sys.tables;--";
Which would then concatenate into the following once it passes through C# into SQL.
SELECT Field FROM Table WHERE Filter='Idontwantanyofyourdata' union select name from sys.tables;--'
And then again - you could replace the UNION with just about anything. Delete/insert/select from system tables.

little Bobby tables is real, and trying the scrub the input yourself is a lost cause. Just say no.

----------------------------------------------------------------------------------
Your lack of planning does not constitute an emergency on my part...unless you're my manager...or a director and above...or a really loud-spoken end-user..All right - what was my emergency again?

Jeff Moden SSC Guru Points: 1004312 More actions · Answer 10

autoexcrement - Friday, November 30, 2018 2:37 PM
I'd love to see an example too. But it seems the issue is somewhat complicated by the use of C# and SQL. Hacking this system would require passing a value that tricks both the C# code and then the SQL code. So it's not something we can just test using SQL alone is it?

Looks like Matt Miller did just exactly that. And, his implicit warning should be heeded.... just because you can't break in using just an SQL example (in this case), the combination of C# + T-SQL proves deadly.

Just say "NO" to ANY form of concatenated SQL coming from the front end. Use parameters as they were intended. If there's something that you can't use a parameter for from the front end, then you're trying to do something that probably shouldn't be done from the front end. 😉

--Jeff Moden

RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.

Change is inevitable... Change for the better is not.

Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally)

autoexcrement SSCertifiable Points: 5956 More actions · Answer 11

Amen! Glad someone found a hack. I knew it was just a matter of time! 🙂 Now we just await the OP's coworkers' defensive and dismissive replies... lol

"If I had been drinking out of that toilet, I might have been killed." -Ace Ventura

Jeff Moden SSC Guru Points: 1004312 More actions · Answer 12

Matt Miller (4) - Friday, November 30, 2018 5:21 PM
Matt Miller (4) - Friday, November 30, 2018 5:14 PM
autoexcrement - Friday, November 30, 2018 2:37 PM
I'd love to see an example too. But it seems the issue is somewhat complicated by the use of C# and SQL. Hacking this system would require passing a value that tricks both the C# code and then the SQL code. So it's not something we can just test using SQL alone is it?
Actually - C# is the thing that gives you an opening, since it allows escape sequences.
Try passing this in:
userinput="Idontwantanyofyourdata\u2019 union select name from sys.tables;--";
Which would then concatenate into the following once it passes through C# into SQL.
SELECT Field FROM Table WHERE Filter='Idontwantanyofyourdata' union select name from sys.tables;--'
And then again - you could replace the UNION with just about anything. Delete/insert/select from system tables.
little Bobby tables is real, and trying the scrub the input yourself is a lost cause. Just say no.

Nice job, Matt. I don't even know how to spell C#.

--Jeff Moden

RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.

Change is inevitable... Change for the better is not.

Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally)

Matt Miller (4) SSC Guru Points: 124210 More actions · Answer 13

Jeff Moden - Friday, November 30, 2018 6:41 PM
Matt Miller (4) - Friday, November 30, 2018 5:21 PM
Matt Miller (4) - Friday, November 30, 2018 5:14 PM
autoexcrement - Friday, November 30, 2018 2:37 PM
I'd love to see an example too. But it seems the issue is somewhat complicated by the use of C# and SQL. Hacking this system would require passing a value that tricks both the C# code and then the SQL code. So it's not something we can just test using SQL alone is it?
Actually - C# is the thing that gives you an opening, since it allows escape sequences.
Try passing this in:
userinput="Idontwantanyofyourdata\u2019 union select name from sys.tables;--";
Which would then concatenate into the following once it passes through C# into SQL.
SELECT Field FROM Table WHERE Filter='Idontwantanyofyourdata' union select name from sys.tables;--'
And then again - you could replace the UNION with just about anything. Delete/insert/select from system tables.
little Bobby tables is real, and trying the scrub the input yourself is a lost cause. Just say no.
Nice job, Matt. I don't even know how to spell C#.

I barely do anymore as well. The truly scary part is that I did the check with the "hello world" demo version of a C# project, in about 2 hours (until today I haven't been actively coding much outside of SQL for quite a while). Just had to remember how to get around the single quote replacement piece, which is that odd-looking code in the middle of the string (i.e. \u2019 = return Unicode character 2019 aka single quote). If I can do that with my C# as rusty as it is - there have to be lots more ways.

That's why using the parameters help a bunch, but frankly Gail's approach of inspecting the parameters using IF statements but keeping the user provided text out of direct interaction with queries is even better. There are just too darn many ways to trip up the query call built up via concatenation, ultimately to get outside of the "quotes" and all of a sudden the typed text is actually being executed. Pretty sure we could find a way to use the binary text translation trick as well, or finding the XML, HTML or JSON escaping equivalents of what I pulled.

----------------------------------------------------------------------------------
Your lack of planning does not constitute an emergency on my part...unless you're my manager...or a director and above...or a really loud-spoken end-user..All right - what was my emergency again?

Thom A SSC Guru Points: 99129 More actions · Answer 14

Great example Matt. I think what the OP has experienced here with the attitude from their(?) development team is just further evidence as to why things like injection are still so ever present. In the world we live in now, where everything is attached to the internet, and can probably be accessed from it, we need to take every opportunity to make sure that what we right is safe and secure.

Sure, string concatenation is "quick", but it's far from safe and if you get used to writing parametrised code then you'll find you can type it (almost) as fast! 😀

Thom~

Excuse my typos and sometimes awful grammar. My fingers work faster than my brain does.
Larnu.uk