My Eyes, My Eyes!

I came across this today while redeeming an online coupon.  Let's just get right to it without the preamble (click for larger version):



Oh, how this pains me.  To protect the innocent I blocked off what would otherwise be incriminating, but suffice it to say this came from a large retailer.  And sadly, this is one I've been to -- I don't mean I've been there shopping (though I have), but I mean I've been to their offices, though as far as I know this is outsourced and hosted elsewhere.  Let's look over this...

First and perhaps least important, scrambleId is a bigInt or others long numeric field based on the querystring (not visible in the picture, but take my word for it).  Yet, its data type is a string (inferred based on Request.QueryString) and it gets passed into the query as a string.  But it's not a string.  Now you might be thinking: the database column uniqueID may be a varchar or some other string-esque data field, making this "ok."  It might be -- but then, not a very good choice for either a primary key or, as labeled, a unique identifier.  Design-wise, this is just broken.  At the very least, it's confusing.  Since I'm picking on it, let's get nitty-gritty and say I'm not a big fan of a column named barcode in a table named barcode. 

Second and mildly broken: never output details like this to the client.  (OK, this is something I'm guilty of at times -- but then, there's a world of difference between a personal site on a shared host with no information of value in the database vs. an e-commerce site collecting PII.)  Handle and log errors, but disable this kind of output via the web.config, especially in a production environment.

Last and obviously the "meat" of the problem: HUGE SQL Injection opportunity! There is NO data validation on scambleID, and it's just handed over to the SqlCommand object.  Because another page takes data and presumably writes it to the database, I'm going to make the assumption that this user (based on the connection string -- likely the same one) has write and possibly dbo permissions.  I imagine it would be trivial to cause major havoc by dropping tables, updating values, compromising data, etc. 

So how is this problem eliminated?  Don't execute direct SQL.  Abstract that into a stored procedure, and pass in the scrambleID as a parameter.  Change the permissions to allow this user execute only permissions on the sproc, not direct table access.  If possible, correct the data types.  I suspect there may be reasons in play for it being all-numeric (for example: a cash register that may only take numeric values).  A bigint can handle over 9,000,000,000,000,000,000 values, so that ought to be big enough.  Maybe it is a bigint, and SQL is casting it -- can't really tell.  But the nice thing is, if the value must be numeric, it's super-easy to check.  Frankly, even if you must use dynamic SQL like this, it would be easy to check the parameters.  An Int64.TryParse() or Regex would be easy to implement. 

Anyway, this goes to show that these kinds of problems are very real in the world, and it's critical to evaluate them.  Peer code reviews, threat modeling, and security education can prevent these!

Comments (2) -

Chris Love
Chris Love
4/15/2008 8:55:13 AM #

Brian, great catch and analysis. These types of issues are unaccepable at this point in the lifetime of ASP.NET. The problems you pointed out are far to easy and fundamental for any web programmer to be aware of and just do it the right way the first time.

Pla.NET Southeast!
Pla.NET Southeast!
4/15/2008 3:14:55 PM #

My colleague Brian just posted about an error page he encountered on a public ecommerce site, and the

Comments are closed

My Apps

Dark Skies Astrophotography Journal Vol 1 Explore The Moon
Mars Explorer Moons of Jupiter Messier Object Explorer
Brew Finder Earthquake Explorer Venus Explorer  

My Worldmap

Month List