- Feature Articles
- CodeSOD
- Error'd
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Seems that his whole life is an "Oh Duh" moment.
Senior Consultant?! The fanciest resume in the world apparently can replace ability and experience. Go figure.
Admin
Well, duh! He is obviously still missing a lot:
I mean seriously, what's the point if I can't specify my where and order by clause."I sense the sarcasm."
"Good because I'm laying it on pretty thick."
Admin
I get it. -1 is a magic number and constants should be used. Geez, what an idiot.
Admin
Uh, why would a mere full-time employee review a consultant's code? That makes no sense. All consultants are super-geniuses. That's why they charge such high hourly-rates. That's the way we do it where we work. For example, we paid contractors two million dollars to write a system. They just stopped working on it after 2 years. Here's the best part. It doesn't work at all. This saves the company a lot of money in the long-run because there's no maintainance cost.
Admin
I think they rejected it because he didn't use sp_executesql.
Admin
Why do I think the next iteration will involve xp_readmail...
Admin
Man, that makes me want to make a consulting company. Think of all the seemingly impossible promises you can make to your clients:
"I guarantee you that you will not need to invest valuable time maintaining the software we produce for you" (your example)
"After we're done producing software for you, you'll never need us for anything ever again!"
"The software we'll produce will not put any kind of strain whatsoever on your servers or network."
"Our software is guaranteed to work as intended."
Admin
Well, they had already produced code that sort of worked or worked with a babysitter and a lot of prodding. But they finally got some kids right out of college and built the ultimate in software. As you say, it uses no resources and costs nothing to maintain. you can understand why we are expanding the use of contractors. We want to see if other companies can produce this type of high-caliber software.
Admin
Man do I love SQL WTFs !!!
Perhaps the next attempt will involve
STEP 1: Inserting the SQL statement into some SQLLibrary table
STEP 2: Present a stored proc that retrieves the SQL statement from SQLLibrary by a passed ID and executes the retrieved SQL string
bye bye SQL Injection .. really ...
CREATE PROCEDURE [DynamicTPSBuilder]
(
@SQLID INT
) AS
BEGIN
SET NOCOUNT ON
SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED
DECLARE @SQL VARCHAR(8000)
--Get SQL
SELECT @SQL = SQLString FROM SQLLibrary WHERE SQLID = @SQLID
--Exec SQL
EXEC (@SQL)
RETURN(-1)
END
Admin
The only thing he did was allow a 16k character sql statement instead of 8k. He/she must be one of those dot com throwbacks who got into it for the money but shouldn't even be out of the house without their hickey helmet...let alone programming. I thought they all went back to their hr/marketing/landscaping/etc. jobs by now.
Admin
One point to consider though with this is that I've been to Security presentations where they said if you used parameterized stored procedures you would protect yourself from SQL Injection attacks. That is obviously not the case here. So make sure you take necessary steps like a regex against the input strings to check for Sql Injection anyways.
Admin
Its for TPS Reports, no-one is gonna call that from outside so its ok right :P
Admin
I worked for a company where that was pretty standard practice. Of course it wasn't for web ASP applications, but rather for SQL batch jobs. And no Return(-1) at the end.
Admin
Any time you execute dynamically-built SQL, you have to look out for a SQL injection possibility.
This applies to client-to-server calls where SQL is passed to a SQL engine... as the parameterized stored procedure sentence above indicates...
But it ALSO applies to dynamically built SQL within a stored procedure. Any time you use EXEC or sp_executesql or any of their cousins, you have to stop and consider the possibility of injection.
Usually all you need is a simple Replace(., '''', '''''') to fix things. And usually you only need that for character-type parameters.
But watch out for things like EXECUTE SearchSomething... @OrderBy = 'ID DESC'
Admin
sign me up for hickey helmet please.
Admin
Hang on.... This is really freaky. A couple of weeks ago, we discovered a veritable dearth of READ UNCOMMITED statements in our stored procedure (that's worth a chortle all on its own), and yesterday I was discussing SQL injection with a colleague of mine, and how it would affect our planned buctracking/knowledge base system if we allowed free text searches.
Spooky, or what?
Admin
Oh, I know what the WTF is. They should have called it sp_SADynamicTPSBuilder, of course.
But, seriously...
That EXEC(sql string) function might possibly be the worst idea ever thought of in the entire history of MS SQL Server. If you really really need your SQL to be that dynamic (and I would contend there's absolutely no reason you should) then at least build your strings in the ASP or VB code.
Admin
Does that really work? (Tries it...) wow, it works...
declare @a varchar(8000)
declare @b varchar(8000)
declare @prefix varchar(500)
declare @postfix varchar(500)
select @prefix = 'select datalength('''
select @postfix = ''')'
select @a = @prefix + replicate('a', 8000 - len(@prefix))
select @b = replicate('b', 8000 - len(@postfix)) + @postfix
exec (@a + @b)
-- result: 15979
Admin
doh! I forgot to put mine on this AM.
Admin
Probably his parents', too.
Admin
Home Security Guy has a meetiing with this SQL Consultant:
HSG: You know, if you leave your door open like that someone will steal your car.
SQLC: .......
HSG: Ummm... someone can get in the door, and steal the car.
SQLC: ..... oh, I get it.
(5 minutes later)
SQLCL: Right, I went back to my office and closed the doo.... huh, where's my car gone?
Admin
He didn't get the memo about the TPS reports... [:D]
Admin
Wtf? Code reviews? Worried about injection attacks? What universe does Louis work in? I want to switch. Don't they know that code review is something to pay lip service to, but is too hard to actually do? The few times I've seen it, I've had to force it down their throats.
Here, fresh out of school, and I wrote this code, will you review it and tell me if I'm doing ok? "Looks like code written in our language and it seems like it will compile - commit it." Come to think of it, those guys quit a few months latter, and I ended up maintaining their code - it would have been sent here if this site existed back then.
Admin
Allowing only stored procedure access to a database is a huge WTF and should only be favored by psycho DBAs. It's like the cops fighting speeders by making everyone take the bus. Then only the bus drivers can speed. Just try and implement a sensible caching or object relational mapping strategy when you are in this scenario...there are lots of ways to prevent SQL injection attacks. Allowing only stored procedures is not one on its own, as this case proves. Who is to say that the consultant wasn't filtering for SQL injection attacks on the code that invoked the stored procedure?
Admin
Yeah, I mean, it's just like have a controlled production environment. WTF, why shouldn't anybody be able to go and do whatever they please. Encapsulation, that's stupid too.
Huh? Why would that be a problem? Stored procedures are just as relational as any other type of SQL.
Again, huh? The WTF is that the person was executing a statement outside of a stored procedure. If the developer only called a stored procedure, he wouldn't be able to create one. How do you create a SQL injection attack if all you do is call stored procedures?
That the consultant wasn't aware of them is a big clue. And why reinvent the wheel?
A stored procdure is a great way to create clean interfaces between code and the DB. It's a lot easier to refactor code to call the same stored procedure in a new DB than to refactor all the SQL in your code. In Java, for example, it's just a matter of changing drivers to switch DB vendors if you only use stored procedures. It also insulates the code from table structure changes.
Admin
Oh man. One typo away from butcracking.
Admin
Obviously the WTF is that he included no exception handling in the stored procedure.
This code needs to be added to the end:
Admin
"Buy a car from us and you'll never go anywhere else again!"
Admin
This is not a rhetorical question.
How could anyone possible think that this would fix sql injection problems?
Admin
Sean Connery: "I'll take Anal Bum Cover for 7000."
Trebek: "That's An Album Cover."
Admin
<FONT face="Courier New">
</FONT><FONT face="Courier New"></FONT>
<FONT face="Courier New">Actually, not even that. If the database in question has an 8000 character limit on the varchars, as is quite likely, then concatenating two big strings that add up to over 8000 characters will truncate. So this is *even dumber* than it first appears.</FONT>
Admin
I've seen that scenario in the double-digit millions a couple times now.
Maybe I shouldn't feel so guilty for my consulting rates...
Admin
Portability is a good point but I've never been able to change a schema and NOT change the SPs. Plus the poster is just saying that a lot of ORM systems have a hard time with SPs. I agree.
Here:
http://www.hibernate.org/hib_docs/v3/reference/en/html_single/#sp_query
Admin
Not in T-SQL, no. Even though there is a definite limit to the size of CHARs/VARCHARs, you can concatenate them and it won't truncate.
For instance (for Sybase, the limit is 255),
would return "AABB".
Admin
Ok, I'll admit to writing a sproc similar to this, but there is a perfectly good reason for this. My former employer, who will go unnamed (but basically Germany's largest bank), put a procedure in place where all the procs had to be reviewed and approved by at least 2 different groups (one of which didn't have a foggiest idea of what SQL was and the other one merely knew how to put together a basic SELECT) before being commited to the database. This had to be accompanied by a multitude online forms, all of which had expiration dates. If one of the groups didn't approve it in time, you had to resubmit. So having gone through a couple of these sproc submissions, I had enough and simply wrote a proc like the one above. As I expected, the sproc got approved and never had to submit another sproc again.
Chances of SQL Injection attack on my apps? All of them were internal, the likelyhood is small. And if it happens, that's just the cost of putting stupid policies in place. Would serve them right.
Admin
WTF
Admin
We do use EXEC in one of our stored procedures. We use it to flip a table so that rows become columns and columns become rows. This is not possible in SQL 2000 in another way as far as I (and I'm not a databaser, actually) and my boss know...
Ofcourse if there is another way, now would be the rihgt time to mention it [;)]
Drak
Admin
One nasty side effect of ExecuteSql is that it runs with the Public permission set.
This means all database objects (tables, other sprocs, user-defined functions, jobs, extended stored procedures, etc.) that the dynamic SQL refers to must have the permissions opened up to Public. The same fools that allow direct table access, are typically too lazy, or too uninformed, to do other than grant ALL to Public.
It's fairly trivial to then take over the server by using xp_CmdShell [1], or a number of other methods.
The WTF here is that there isn't a massive amount of these attacks being published, given how many systems are vulnerable to it.
[1] For those of you unfamiliar with this gem, it can be invoked by anybody with perms to invoke it, yet when it runs, it can execute any operating-system command that the account running Sql Server has the privileges to execute. The typical account used for the SQL Server process is the "System" account. (READ: God Account).
Admin
Probably just a typo. It should be senile consultant, not senior consultant.
Admin
I bet the params are passed to application via HTTP GET query params and WWW is public accessed. Didn't?
Seriously, this is one of my favourite WTFs. It hurts my eyes and my mind hard.
Admin
Can you forward him a copy of the memo about the new TPS report cover sheets? [;)]
Admin
After seeing all the SQL-related WTFs here, would any sane DBA (I'll leave proof of existence as an exercise) allow senior conslutants to write and execute ad-hoc queries from the code?
Admin
Possibly those who are r00ting such machines are not doing so for the 'zombie bot' aspect but rather for industrial espionage purposes. In which case, victims might be somewhat reluctant to come forward - after all, if your sex toy customer database has just been pwned, you're probably not going to be over-eager to let everyone know. Just a thought.
Otherwise, yeah, hacking into a box with SQL Server on it is not terribly difficult by all accounts. To be honest, most of the web app installations I've come across using SQL Server use 'sa' as the account to connect as, for fuck's sake.
Simon
Admin
It depends, actually. In a lot of my systems, it's much easier to go through the code and refactor embedded SQL than it is to change stored procedures. This is mostly because it's easier to write and change Ruby code than it is to write and change SQL code.
Admin
How about this?
dZ.
Admin
Oh noooo! The Brillant Paula Bean (tm) got a job as an DBA consultant... Gawd help us all.
dZ.
Admin
Ewww. Take your filthy programming fetishes outta here!
dZ.
Admin
But then you lose the performance benefit stored procedures have (cached query plans etc)
Admin
Hmm, which RDBMS is that for then?
Admin
I worked on a project for a very large uk company performing support of their main processing systems. Due to one nonce doing an update to a table, but somehow losing the 'where' clause, we were all subsequently made to have an 'sql buddy' watching over us whenever we needed to fire sql against the tables. I don't think it mattered whether they knew sql or not. The managers told us it was to 'protect us'....