Are any of these SQL Queries open to SQL injection attacks?

Question

I have re-written my code after great help from some friendly stack overflow members (big thanks to Martin B and Kev Chadders especially). I would now like to check if my code is still open to SQL Injections after this work. I believe the code is now working as it should, but any blinding errors that you see i'd love to hear about too. My code is now looking like:

   -code removed-

The Response.Write(result) was just added during the edits to check what was in result after the first query — Phil
– Phil, Commented Jan 29, 2010 at 10:26
I am also doing this: Dim querystringvar As String = Request.QueryString.ToString If InStr(querystringvar, "%20union") Then Response.Redirect("/errors/504.aspx?" & querystringvar) ElseIf InStr(querystringvar, "%20select") Then Response.Redirect("/errors/504.aspx?" & "error= " & querystringvar) With a long list of banned words which will redirect to the error page (before the code in my question is loaded) — Phil
– Phil, Commented Jan 29, 2010 at 10:34

Mark Byers · Accepted Answer · 2010-01-29 10:37:55Z

5

It seems you are safe from SQL injection attacks, but code like this:

Response.Write(result);

and:

Response.Write("<b><u> --- Begin SQL Exception Message ---</u></b><br />")
Response.Write(ex)
Response.Write("<br /><b><u> --- End SQL Exception Message ---</u></b>")

could leave you open for other forms of attack such as XSS. You should set the text element of an ASP.NET control, not directly write to the page.

answered Jan 29, 2010 at 10:37

Mark Byers

844k202 gold badges1.6k silver badges1.5k bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

Wim Over a year ago

Regardless of it being bad practice to perform Response.Write in ASP.NET, how is this open to XSS? None of that what's being written to the page is user input. XSS (if it was there) doesn't go away just because you set the Text property on say an ASP.NET label. You should use the AntiXSS library if it is user input that you are displaying on the page.

Phil Over a year ago

Thanks for the extra information Wim. For best practice sake would; Catch ex As SqlException SQLErrLabel.Text = ex.ToString be more appropriate?

Wim Over a year ago

Yeah that would be fine, although you may not want to show the complete stacktrace to the end user, but simply display the exception message like SQLErrLabel.Text = ex.Message; should suffice. Or alternatively wrap it with a generic error message and log the specifics of the exception appropriately for you to inspect.

bobince Over a year ago

Yes, sadly the Label.Text property takes HTML markup not text as the name might lead you to believe — making it just as dangerous as Response.Write. To use either of these methods you must use HTMLEncode on the text value first. It is highly unfortunate that ASP.NET bungles this one, especially given that other classes like TextBox have an apparently-identical Text property that doesn't take HTML.

bobince Over a year ago

Worse, the warning about it on the MSDN doc implies it isn't a problem by saying that “by default, ASP.NET Web pages validate that user input does not include script or HTML elements”. This is totally disingenuous, a dangerous misrepresentation of how much the utterly bogus “XSS protection” in ASP.NET protects you (tip: not at all, and it messes up your web pages in the process too).

|

Rubens Farias · Accepted Answer · 2010-01-29 10:29:44Z

2

Seems fine to me.

Basically, if you don't concatenate SQL string and uses parametrized queries, you're safe against SQL injection attacks.

answered Jan 29, 2010 at 10:29

Rubens Farias

58k8 gold badges136 silver badges165 bronze badges

Comments

Chris S · Accepted Answer · 2010-01-29 10:30:04Z

2

You're using SqlParameters which effectively removes all SQL injection issues.

answered Jan 29, 2010 at 10:30

Chris S

65.6k53 gold badges225 silver badges240 bronze badges

Comments

Matt · Accepted Answer · 2010-01-29 11:08:28Z

2

You can run the static code analysis tool CAT.NET to identify all XSS and SQL injection vectors accross a project, including referenced assemblies.

http://www.microsoft.com/downloads/details.aspx?FamilyId=0178e2ef-9da8-445e-9348-c93f24cc9f9d&displaylang=en

Reports usually make for some interesting reading.

answered Jan 29, 2010 at 11:08

Matt

1,2678 silver badges16 bronze badges

Comments

Daniel Coffman · Accepted Answer · 2010-01-29 19:14:16Z

1

You should run a scanner to check for potential SQL injection vulnerabilities. I have had some luck with http://sqlmap.sourceforge.net/

answered Jan 29, 2010 at 19:14

Daniel Coffman

2,0054 gold badges27 silver badges34 bronze badges

Collectives™ on Stack Overflow

Are any of these SQL Queries open to SQL injection attacks?

5 Answers 5

7 Comments

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

7 Comments

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related