The Daily WTF: Curious Perversions in Information Technology

2011-06-27 Reply Admin

no u:
This has nothing to do with SQL injection. Do all you people who thing it does actually believe that the users are going to be inputting their own homeID/status? Even in the very small chance that they are, who's to say that this isn't sanatized at a much earlier stage.. such as the logical point when it is read in - or would you geniuses parse the input every single time it is used rather than once when it is entered?
The slightest mention of sql on this site and all the kiddies start thinking "I've heard of sql injection.. let me impress everyone by bringing it up over and over again".

Yes the implementation is stupid and smacks of someone fresh out of school, but it isn't "wtf" levels of stupid.

The SQL injection calls are just as valid as your call on it not being vulnerable to them.

The SQL injection calls presume that the parameters are user input. You're absolutely right that there's no way to be sure of this.

It not being vulnerable to SQL injection presume that that homeId isn't a concatenated string of user fields where they fill in their address and, for god knows what reason, that ID is "221BBaker StreetLondon" and status isn't something like: "renting", "owned", "2nd home" or something like that. These all are huuuuge WTFs, but you can't assume they aren't, just because they would be, otherwise. (And that's one nice sentence!)

C-Octothorpe · 2011-06-27 Reply Admin

no u:
This has nothing to do with SQL injection. Do all you people who thing it does actually believe that the users are going to be inputting their own homeID/status? Even in the very small chance that they are, who's to say that this isn't sanatized at a much earlier stage.. such as the logical point when it is read in - or would you geniuses parse the input every single time it is used rather than once when it is entered?
The slightest mention of sql on this site and all the kiddies start thinking "I've heard of sql injection.. let me impress everyone by bringing it up over and over again".

Yes the implementation is stupid and smacks of someone fresh out of school, but it isn't "wtf" levels of stupid.

I refer you to my post with 9 WTFs outlined. Now that's 9 WTFs for every 15 lines of code. According to my calculator, he's fucking up at least 60% of the time. That's pretty bad, IMHO...

Also, SQL injection isn't the only WTF, but it is a biggie, and having a toxic "someone else will do it" attitude about security is why IT is in the state that it's currently in. I call it "security by passing the buck".

Have you ever heard of trust boundaries or canonicalization? Yes, I would parse and sanitize it every time. That's the whole point of stored procs or prepares statements. When the data from the UI passes to the DB, it should be sanitized, period. It's doesn't cost much and the ramifications of getting p0wned via SQL injection to shave off 6 miliseconds is just dumb. Sorry...

2011-06-27 Reply Admin

The Poop... of DOOM:
no u:
This has nothing to do with SQL injection. Do all you people who thing it does actually believe that the users are going to be inputting their own homeID/status? Even in the very small chance that they are, who's to say that this isn't sanatized at a much earlier stage.. such as the logical point when it is read in - or would you geniuses parse the input every single time it is used rather than once when it is entered?
The slightest mention of sql on this site and all the kiddies start thinking "I've heard of sql injection.. let me impress everyone by bringing it up over and over again".

Yes the implementation is stupid and smacks of someone fresh out of school, but it isn't "wtf" levels of stupid.
The SQL injection calls are just as valid as your call on it not being vulnerable to them.

The SQL injection calls presume that the parameters are user input. You're absolutely right that there's no way to be sure of this.

hey, well while we're having fun inventing flaws in this code simply because there is no proof that they don't exist, lets list some more which I've made up:

This application has a horrible GUI
This is a memory hog, it uses 2GB of memory unecessarily
Some of the functionality is no doubt illegal in 95% of most countries.

I'm sure there are many other bugs which people could imagine about this application.

C-Octothorpe · 2011-06-27 Reply Admin

no u:
I always like to ask candidates...

Unlikely...

You're either a manager or a CS grad/student... Isn't it funny how it's hard to tell the difference?

2011-06-27 Reply Admin

no u:
What is an sql query you could use to return (only) the largest value in a particular column without using the max function. (they have an example table in front of them which I use for other simple questions also). I don't think anyone has ever given me a correct answer.. despite it being ridiculously simple.

You could do an descending order-by of the column, and then just pick off the top row ("Select Top 1" in SQL Server, "Where rownum=1" in Oracle).

You could do "Where Not Exists (<row with a higher value>)".

Were you looking for one of those? Either one has the advantage of letting you find a different value from the row than the one you checked was the max; that's a more useful test question than "pretend this feature doesn't exist."

C-Octothorpe · 2011-06-27 Reply Admin

no u:
The Poop... of DOOM:
no u:
This has nothing to do with SQL injection. Do all you people who thing it does actually believe that the users are going to be inputting their own homeID/status? Even in the very small chance that they are, who's to say that this isn't sanatized at a much earlier stage.. such as the logical point when it is read in - or would you geniuses parse the input every single time it is used rather than once when it is entered?
The slightest mention of sql on this site and all the kiddies start thinking "I've heard of sql injection.. let me impress everyone by bringing it up over and over again".

Yes the implementation is stupid and smacks of someone fresh out of school, but it isn't "wtf" levels of stupid.
The SQL injection calls are just as valid as your call on it not being vulnerable to them.

The SQL injection calls presume that the parameters are user input. You're absolutely right that there's no way to be sure of this.

hey, well while we're having fun inventing flaws in this code simply because there is no proof that they don't exist, lets list some more which I've made up:

This application has a horrible GUI

This is a memory hog, it uses 2GB of memory unecessarily

Some of the functionality is no doubt illegal in 95% of most countries.

I'm sure there are many other bugs which people could imagine about this application.

Ok, if that's what you want to do... Personally, I like to base my observations on facts.

2011-06-27 Reply Admin

nonpartisan:
no u:
Canthros:
I'm mostly reminded of a job interview where the technical portion of the interview asked a SQL question about getting the largest value from a given column. Max() being the answer, of course. I asked if it was a trick question or if they were after something obscure involving OLAP or something, it seemed too obvious when the job description specified SQL experience.
Now, I see that my faith in humanity is just too high.

I always like to ask candidates who claim to have problem solving skills and/or sql experience the following question:

What is an sql query you could use to return (only) the largest value in a particular column without using the max function. (they have an example table in front of them which I use for other simple questions also). I don't think anyone has ever given me a correct answer.. despite it being ridiculously simple.

I was once asked this in an interview many years ago and the answer seemed trivial.

Guessing here . . . something like:

SELECT TOP 1 column FROM table ORDER BY column DESC;

?

My SQL is rusty . . .

I would've gone with LIMIT (Different flavor of SQL, same principle), but that's the solution, yeah.

2011-06-27 Reply Admin

airdrik:
To all of the people crying over possible SQL injection attacks, while it should be using prepared/parameterized statements, I would venture to say that unless there is evidence otherwise it is probably safe to assume that there is no possibility of such attacks. First, this looks like inner library code. The values passed in were probably pulled directly from the database (myHomeID) or from some hard-coded enumeration (myStatus). Assuming that this is the case, it would be a big WTF if those values did contain some "extra sql" (though that could explain why they were concatenating strings rather than using prepared statements - oh, how the thought makes my brain hurt). Second, if this were somehow exposed to user input, it is probably safe to assume that it is under a layer or two of (most likely WTF-worthy) sanitization/validation code which makes sure that the values conform to some format. That they are using the values as filter conditions in the where clause leads me to believe that neither is linked to a free-form, user-exposed field.

no u:
This has nothing to do with SQL injection. Do all you people who thing it does actually believe that the users are going to be inputting their own homeID/status? Even in the very small chance that they are, who's to say that this isn't sanatized at a much earlier stage.. such as the logical point when it is read in - or would you geniuses parse the input every single time it is used rather than once when it is entered? The slightest mention of sql on this site and all the kiddies start thinking "I've heard of sql injection.. let me impress everyone by bringing it up over and over again". Yes the implementation is stupid and smacks of someone fresh out of school, but it isn't "wtf" levels of stupid.

So, you prefer using your own sanitization code (or worse yet - sanitization code written by the same person who wrote this code) rather than the sanitization code written by the ADO.NET team?

There's a right way to do sanitization. It's at the layer that talks to the database. It's not hard to do. With tools like Enitiy Framework it's hard not to do sanitization!

If you want to add sanitization at higher levels too, be my guest. But there's a "first line of defense" that should exist at your DAL.

Just because your code isn't going to pass unsanitized data doesn't mean that someone else isn't going to. This is particularly true for ASP.NET services (Fiddler) as well as any program written in .NET (Reflector).

wonkoTheSane · 2011-06-27 Reply Admin

return i;

At first I read that as return 1;

If anything was going to make that code better it would have been THAT!

Besides we all know that the best way to do this would have been to pull the whole table back into a typed dataset then filter it and THEN do a foreach on it...

2011-06-27 Reply Admin

I think that will select the top record and then order the output. Maybe:

SELECT TOP 1 column FROM (SELECT column FROM table ORDER BY column DESC);

C-Octothorpe · 2011-06-27 Reply Admin

Stephen Cleary:
airdrik:
To all of the people crying over possible SQL injection attacks, while it should be using prepared/parameterized statements, I would venture to say that unless there is evidence otherwise it is probably safe to assume that there is no possibility of such attacks. First, this looks like inner library code. The values passed in were probably pulled directly from the database (myHomeID) or from some hard-coded enumeration (myStatus). Assuming that this is the case, it would be a big WTF if those values did contain some "extra sql" (though that could explain why they were concatenating strings rather than using prepared statements - oh, how the thought makes my brain hurt). Second, if this were somehow exposed to user input, it is probably safe to assume that it is under a layer or two of (most likely WTF-worthy) sanitization/validation code which makes sure that the values conform to some format. That they are using the values as filter conditions in the where clause leads me to believe that neither is linked to a free-form, user-exposed field.

no u:
This has nothing to do with SQL injection. Do all you people who thing it does actually believe that the users are going to be inputting their own homeID/status? Even in the very small chance that they are, who's to say that this isn't sanatized at a much earlier stage.. such as the logical point when it is read in - or would you geniuses parse the input every single time it is used rather than once when it is entered? The slightest mention of sql on this site and all the kiddies start thinking "I've heard of sql injection.. let me impress everyone by bringing it up over and over again". Yes the implementation is stupid and smacks of someone fresh out of school, but it isn't "wtf" levels of stupid.

So, you prefer using your own sanitization code (or worse yet - sanitization code written by the same person who wrote this code) rather than the sanitization code written by the ADO.NET team?

There's a right way to do sanitization. It's at the layer that talks to the database. It's not hard to do. With tools like Enitiy Framework it's hard not to do sanitization!

If you want to add sanitization at higher levels too, be my guest. But there's a "first line of defense" that should exist at your DAL.

Just because your code isn't going to pass unsanitized data doesn't mean that someone else isn't going to. This is particularly true for ASP.NET services (Fiddler) as well as any program written in .NET (Reflector).

That's often the problem though. Just because you can't think of a way to pass bad data, doesn't mean someone malicious can't or won't. If you're doing some shite black-list implemenation looking for "javascript" tags in your impout, then I can simply avoid your filter by typing in java script or JavaSCRiPt, or whatever permutation of that. There is a lot to data sanitization, and people much smarter than us wrote and thoroughly tested code (ADO.NET team) so we wouldn't have to write our own half-baked implementation...

2011-06-27 Reply Admin

nonpartisan:

Guessing here . . . something like:

SELECT TOP 1 column FROM table ORDER BY column DESC;

?

My SQL is rusty . . .

Yes, but what if there are more than 1 with the same largest value?

You should do a "SELECT DISTINCT TOP 1 FROM table ORDER BY column DESC;"

Or depending on the flavor of SQL, you might need "WHERE ROWNUM = 1" instead of TOP 1.

--Joe

2011-06-27 Reply Admin

C-Octothorpe:
no u:
The Poop... of DOOM:
no u:
This has nothing to do with SQL injection. Do all you people who thing it does actually believe that the users are going to be inputting their own homeID/status? Even in the very small chance that they are, who's to say that this isn't sanatized at a much earlier stage.. such as the logical point when it is read in - or would you geniuses parse the input every single time it is used rather than once when it is entered?
The slightest mention of sql on this site and all the kiddies start thinking "I've heard of sql injection.. let me impress everyone by bringing it up over and over again".

Yes the implementation is stupid and smacks of someone fresh out of school, but it isn't "wtf" levels of stupid.
The SQL injection calls are just as valid as your call on it not being vulnerable to them.

The SQL injection calls presume that the parameters are user input. You're absolutely right that there's no way to be sure of this.

hey, well while we're having fun inventing flaws in this code simply because there is no proof that they don't exist, lets list some more which I've made up:

This application has a horrible GUI

This is a memory hog, it uses 2GB of memory unecessarily

Some of the functionality is no doubt illegal in 95% of most countries.

I'm sure there are many other bugs which people could imagine about this application.

Ok, if that's what you want to do... Personally, I like to base my observations on facts.

I respect your other 8 points.. but when you and others say that this is vulnerable to sql injection, this is not based on fact. There is absolutely no evidence of this. The closest you can get is to say that you cannot determine whether it is vulnerable or not - which is hardly a compelling argument.

flop · 2011-06-27 Reply Admin

Everything was better at his last assignment. The department's development processes were obsolete and the development tools he was given? Don't even start. His way, no matter how convoluted or obtuse was the best way. Don't question it - just get used to it.

Sounds like Nov 10, 2007 in Dilberts conference room ...

Mason Wheeler · 2011-06-27 Reply Admin

Joe:
nonpartisan:

Guessing here . . . something like:

SELECT TOP 1 column FROM table ORDER BY column DESC;

?

My SQL is rusty . . .

Yes, but what if there are more than 1 with the same largest value?

...then you'll get one of them, and it'll be the correct value. No need to complicate things further.

2011-06-27 Reply Admin

C-Octothorpe:
no u:
I always like to ask candidates...

Unlikely...

You're either a manager or a CS grad/student... Isn't it funny how it's hard to tell the difference?

no idea what this is based on, but whether an internet troll finds it unlikely or not, it is the case.

.. and for all the people offering solutions, you are all more or less coming up with the same answer, which is the one I had in mind. It always suprised me that others couldn't think of this, but maybe that is the pressure of an interview situation.

2011-06-27 Reply Admin

Geoff:
he is using cmdtext rather than calling a procedure with position parameters, usually not ideal.

Not ideal!! I guess so, in the sense that having your database hacked daily so that you look as stupid as Sony and your stock goes flat is "not ideal".

nonpartisan · 2011-06-27 Reply Admin

Joe:
nonpartisan:

Guessing here . . . something like:

SELECT TOP 1 column FROM table ORDER BY column DESC;

?

My SQL is rusty . . .

Yes, but what if there are more than 1 with the same largest value?

You should do a "SELECT DISTINCT TOP 1 FROM table ORDER BY column DESC;"

Or depending on the flavor of SQL, you might need "WHERE ROWNUM = 1" instead of TOP 1.

--Joe

Your solution is indeed cleaner, but in reality it shouldn't matter. The example was looking for the largest value from a single column. If the full return set comes back as

12748 12748 12748 9486 3741 15 4

then TOP 1 will still return the value that I need.

Again, yours is indeed cleaner, but I'll still get the value I need.

EDIT: To clarify, it will return just the first instance of 12748 -- not all three.

2011-06-27 Reply Admin

C-Octothorpe:
JV:
FRIST! FRIST! FRIST!

MORON! MORON! MORON!

You mipsselled "MROON! MROON! MROON!"

@Deprecated · 2011-06-27 Reply Admin

no u:
C-Octothorpe:
no u:
The Poop... of DOOM:
no u:
This has nothing to do with SQL injection. Do all you people who thing it does actually believe that the users are going to be inputting their own homeID/status? Even in the very small chance that they are, who's to say that this isn't sanatized at a much earlier stage.. such as the logical point when it is read in - or would you geniuses parse the input every single time it is used rather than once when it is entered?
The slightest mention of sql on this site and all the kiddies start thinking "I've heard of sql injection.. let me impress everyone by bringing it up over and over again".

Yes the implementation is stupid and smacks of someone fresh out of school, but it isn't "wtf" levels of stupid.
The SQL injection calls are just as valid as your call on it not being vulnerable to them.

The SQL injection calls presume that the parameters are user input. You're absolutely right that there's no way to be sure of this.

hey, well while we're having fun inventing flaws in this code simply because there is no proof that they don't exist, lets list some more which I've made up:

This application has a horrible GUI

This is a memory hog, it uses 2GB of memory unecessarily

Some of the functionality is no doubt illegal in 95% of most countries.

I'm sure there are many other bugs which people could imagine about this application.

Ok, if that's what you want to do... Personally, I like to base my observations on facts.

I respect your other 8 points.. but when you and others say that this is vulnerable to sql injection, this is not based on fact. There is absolutely no evidence of this. The closest you can get is to say that you cannot determine whether it is vulnerable or not - which is hardly a compelling argument.

I think we can conclusively state that this function is vulnerable. The overall program, maybe not at the moment, but somebody adds a SQL keyword to the list of statuses, and the program breaks.

2011-06-27 Reply Admin

airdrik:
... it is probably safe to assume...
... were probably pulled directly...

... Assuming that this is the case ...

... it is probably safe to assume ...

Oh man, I hope you're trolling.a

C-Octothorpe · 2011-06-27 Reply Admin

no u:
I respect your other 8 points.. but when you and others say that this is vulnerable to sql injection, this is not based on fact. There is absolutely no evidence of this. The closest you can get is to say that you cannot determine whether it is vulnerable or not - which is hardly a compelling argument.

What's so difficult to understand? Is this piece of code vulerable to SQLi? Yes, absolutely it is. Is the consumer always going to pass trusted data? Who knows...

The trouble is that you're basing your views on an uncertanty, which is what you claim "everybody else" is doing wrong... Personally, I would rather not trust the caller and be sure that I can handle safe, malicious and garbage data.

I guess you're far more trusting than myself and everybody else, including security experts, are...

You're also failing to think about the junior devs who will maintain this code several years down the road. Do they know about sqli? Probably not. But at least you can be sure you're piece of code is written safely and can handle it.

2011-06-27 Reply Admin

nonpartisan:
EDIT: To clarify, it will return just the first instance of 12748 -- not all three.

Yes, but which one of the 12748s will it return? You can't be sure. It might not even be the same one every time.

2011-06-27 Reply Admin

@Deprecated:
no u:
C-Octothorpe:
no u:
The Poop... of DOOM:
no u:
This has nothing to do with SQL injection. Do all you people who thing it does actually believe that the users are going to be inputting their own homeID/status? Even in the very small chance that they are, who's to say that this isn't sanatized at a much earlier stage.. such as the logical point when it is read in - or would you geniuses parse the input every single time it is used rather than once when it is entered?
The slightest mention of sql on this site and all the kiddies start thinking "I've heard of sql injection.. let me impress everyone by bringing it up over and over again".

Yes the implementation is stupid and smacks of someone fresh out of school, but it isn't "wtf" levels of stupid.
The SQL injection calls are just as valid as your call on it not being vulnerable to them.

The SQL injection calls presume that the parameters are user input. You're absolutely right that there's no way to be sure of this.

hey, well while we're having fun inventing flaws in this code simply because there is no proof that they don't exist, lets list some more which I've made up:

This application has a horrible GUI

This is a memory hog, it uses 2GB of memory unecessarily

Some of the functionality is no doubt illegal in 95% of most countries.

I'm sure there are many other bugs which people could imagine about this application.

Ok, if that's what you want to do... Personally, I like to base my observations on facts.

I respect your other 8 points.. but when you and others say that this is vulnerable to sql injection, this is not based on fact. There is absolutely no evidence of this. The closest you can get is to say that you cannot determine whether it is vulnerable or not - which is hardly a compelling argument.

I think we can conclusively state that this function is vulnerable. The overall program, maybe not at the moment, but somebody adds a SQL keyword to the list of statuses, and the program breaks.

The program is also broken if someone adds ""+(1/0) to the list of statuses.

2011-06-27 Reply Admin

boog:
Geoff:
Oh and he is using cmdtext rather than calling a procedure with position parameters, usually not ideal. I don't see any input validation but depending on where myHomeID and MyStatus are coming from that may or may not be a WTF as well.
Who cares?

I'm sure anybody who could make a profit from taking over the database will care.

frits · 2011-06-27 Reply Admin

Jack:
nonpartisan:
EDIT: To clarify, it will return just the first instance of 12748 -- not all three.
Yes, but which one of the 12748s will it return? You can't be sure. It might not even be the same one every time.

Right. This could screw up any comparisons you need to do. It's probably a good idea to create your own data type in which to wrap the result. Also, you better implement a custom equality operator/method just to make sure...

C-Octothorpe · 2011-06-27 Reply Admin

Hortical:
The program is also broken if someone adds ""+(1/0) to the list of statuses.

I realize I may be lambasted for making assumptions, however based on this code, the values likely come from down-downs in the UI. And if this is web based, then you can easily toss in any value for the drop down using a simple proxy tool (I think someone already mentioned) like fiddler or paros. So a safe value like 'ACTIVE' for myStatus could easily be replaced with '-- drop table whatever', and it'll happily do as you request.

snoofle · 2011-06-27 Reply Admin

no u:
.. and for all the people offering solutions, you are all more or less coming up with the same answer, which is the one I had in mind. It always suprised me that others couldn't think of this, but maybe that is the pressure of an interview situation.

As someone who has interviewed a lot of people. I've found that otherwise qualified folks will sometimes look at something they know how to do and momentarily draw a blank.

When you ask such narrowly focused questions on an interview, all you accomplish is that you determine whether the person can figure out the solution of which you are thinking. Any idiot can google the best algorithm for doing something.

A better approach is to figure out how the person goes about solving a problem with which they're not familiar. If they can think through a problem with a reasonable approach (and don't penalize for missing some subtle detail on a 30 second analysis), and they are personable, they'll probably make a good addition to your team.

nonpartisan · 2011-06-27 Reply Admin

Jack:
nonpartisan:
EDIT: To clarify, it will return just the first instance of 12748 -- not all three.
Yes, but which one of the 12748s will it return? You can't be sure. It might not even be the same one every time.

I think I'm being trolled. My TrollMeter(tm) is pegged.

Nevertheless, in the event that my database experience is that rusty . . .

Why the hell does it matter? I'm looking for a single value returned from a single column. I'm not looking for its associated key value. If I get 12748 from a row that has a keyID of 3748 and then run the same query and get 12748 from a row with a keyID of 159938 what does it matter? It's still the maximum value in the column.

But I'm pretty sure I'm being trolled here . . . so I don't really know why I'm even answering.

2011-06-27 Reply Admin

C-Octothorpe:
What's so difficult to understand? Is this piece of code vulerable to SQLi? Yes, absolutely it is. Is the consumer always going to pass trusted data? Who knows...
The trouble is that you're basing your views on an uncertanty, which is what you claim "everybody else" is doing wrong... Personally, I would rather not trust the caller and be sure that I can handle safe, malicious and garbage data.

I guess you're far more trusting than myself and everybody else, including security experts, are...

You're also failing to think about the junior devs who will maintain this code several years down the road. Do they know about sqli? Probably not. But at least you can be sure you're piece of code is written safely and can handle it.

Everyone knows about SQLi. There was an xkcd about it, so yes, even juniors.

Let's imagine you go ahead and put SQL sanitization at every level of this program, including internal functions like the one shown.

Years later, someone looks at it and thinks "What's with all the redundancy? I'm posting this to TDWTF!"

Perhaps it's more the program's structure that is in question? If you're re-sanitizing several times, I think something is wrong.

2011-06-27 Reply Admin

Hmm, that one was pretty dull. Yes, of couse he should have used count(*) and guarded against SQL injection. It's bad code, but it just doesn't rise to the level of mind-boggling stupid code that would make it funny.

Alex must be getting short on submissions.

Hey, I just came across a line of Java where the programmer wrote "x=x+1;" Ha ha! What a jerk! Didn't he know he could have just written "x++;"? Isn't that hysterical?

C-Octothorpe · 2011-06-27 Reply Admin

Hortical:
C-Octothorpe:
What's so difficult to understand? Is this piece of code vulerable to SQLi? Yes, absolutely it is. Is the consumer always going to pass trusted data? Who knows...
The trouble is that you're basing your views on an uncertanty, which is what you claim "everybody else" is doing wrong... Personally, I would rather not trust the caller and be sure that I can handle safe, malicious and garbage data.

I guess you're far more trusting than myself and everybody else, including security experts, are...

You're also failing to think about the junior devs who will maintain this code several years down the road. Do they know about sqli? Probably not. But at least you can be sure you're piece of code is written safely and can handle it.

Everyone knows about SQLi. There was an xkcd about it, so yes, even juniors.

Let's imagine you go ahead and put SQL sanitization at every level of this program, including internal functions like the one shown.

Years later, someone looks at it and thinks "What's with all the redundancy? I'm posting this to TDWTF!"

Perhaps it's more the program's structure that is in question? If you're re-sanitizing several times, I think something is wrong.

[image]

2011-06-27 Reply Admin

C-Octothorpe:
I realize I may be lambasted for making assumptions, however based on this code, the values likely come from down-downs in the UI.

Yes, and that would be a problem if this code was used for that purpose.

And SQL-injection is always something to keep in mind.

But we have no idea if it applies or not.

I don't know, man... Sometimes I wonder what the point of this is.

2011-06-27 Reply Admin

no u:
I always like to ask candidates who claim to have problem solving skills and/or sql experience the following question:
What is an sql query you could use to return (only) the largest value in a particular column without using the max function. (they have an example table in front of them which I use for other simple questions also). I don't think anyone has ever given me a correct answer.. despite it being ridiculously simple.

I was once asked this in an interview many years ago and the answer seemed trivial.

SELECT -MIN(-theColumn) FROM theTable;

2011-06-27 Reply Admin

While we're unsure if the code needs to do sql injection prevention, how sure are we that this isn't the best way to count the rows? There could be some context-specific issue that resulted in this being necessary.

And the unused string reference: It may have been necessary in order to trigger some obscure side-effect, who knows?

2011-06-27 Reply Admin

simple test of response time of dbreader

2011-06-27 Reply Admin

nonpartisan:
Jack:
nonpartisan:
EDIT: To clarify, it will return just the first instance of 12748 -- not all three.
Yes, but which one of the 12748s will it return? You can't be sure. It might not even be the same one every time.

I think I'm being trolled. My TrollMeter(tm) is pegged.

Nevertheless, in the event that my database experience is that rusty . . .

Why the hell does it matter? I'm looking for a single value returned from a single column. I'm not looking for its associated key value. If I get 12748 from a row that has a keyID of 3748 and then run the same query and get 12748 from a row with a keyID of 159938 what does it matter? It's still the maximum value in the column.

But I'm pretty sure I'm being trolled here . . . so I don't really know why I'm even answering.

That's highly likely.

2011-06-27 Reply Admin

boog:
boog:
boog:
boog:
boog:
boog:
This has nothing to do with SQL injection. Do all you people who thing it does actually believe that the users are going to be inputting their own homeID/status? Even in the very small chance that they are, who's to say that this isn't sanatized at a much earlier stage.. such as the logical point when it is read in - or would you geniuses parse the input every single time it is used rather than once when it is entered?
The slightest mention of sql on this site and all the kiddies start thinking "I've heard of sql injection.. let me impress everyone by bringing it up over and over again".

Yes the implementation is stupid and smacks of someone fresh out of school, but it isn't "wtf" levels of stupid.
The SQL injection calls are just as valid as your call on it not being vulnerable to them.

The SQL injection calls presume that the parameters are user input. You're absolutely right that there's no way to be sure of this.

hey, well while we're having fun inventing flaws in this code simply because there is no proof that they don't exist, lets list some more which I've made up:

This application has a horrible GUI

This is a memory hog, it uses 2GB of memory unecessarily

Some of the functionality is no doubt illegal in 95% of most countries.

I'm sure there are many other bugs which people could imagine about this application.

Ok, if that's what you want to do... Personally, I like to base my observations on facts.

I respect your other 8 points.. but when you and others say that this is vulnerable to sql injection, this is not based on fact. There is absolutely no evidence of this. The closest you can get is to say that you cannot determine whether it is vulnerable or not - which is hardly a compelling argument.

I think we can conclusively state that this function is vulnerable. The overall program, maybe not at the moment, but somebody adds a SQL keyword to the list of statuses, and the program breaks.

And while we're at it, we can check a make sure the incoming string doesn't contain any arithmetic problems like "1/0". Or that the incoming table name isn't misspelled, 'cause later devs (damn juniors!) might misspell a table name and then everything's fucked!

2011-06-27 Reply Admin

Righteous indignation? Raucous indigestion? WTF?

Captcha: tego - for here to tego?

2011-06-27 Reply Admin

C-Octothorpe:
You're also failing to think about the junior devs who will maintain this code several years down the road. Do they know about sqli? Probably not. But at least you can be sure you're piece of code is written safely and can handle it.

I'll have to say: websites like this are fairly popular amongst students. If you've seen the problem on here, there's a good chance they've heard of it.

operagost · 2011-06-27 Reply Admin

veggen:
Ok, skipping the typos on "richeous indigation" for now... but isn't indignation righteous by definition?

Well, your indignation could be unwarranted, which would be unrighteous.

TheCPUWizard · 2011-06-27 Reply Admin

mmc:
Does he get points for using Dispose() and Close()? That's far more than we can expect around here on a regular basis.

No he does ot...becuse is usage is completely wrong.

First you do not need Close and Dispose. Second the implementation does not ensure that Dispose (or Close) is called in the event of an exception.

Using something WRONG is typically worse than not using it at all from a "grading/points" perspective.

2011-06-27 Reply Admin

So fuckin' predicable...

sQL injection! sql InJectiOn! sql 'n' jerkoFf!

Oh really, "SQL injection", what's that? No one else here has ever heard of it before! How smart you are for being aware of that!

pat on the head

Have a cookie, geniuses.

2011-06-27 Reply Admin

Clearly this is because embedded databases don't implement the Count function.

Captcha ideo ...where the code shows was clearly written by a richeos ideot

TheCPUWizard · 2011-06-27 Reply Admin

no u:
. but when you and others say that this is vulnerable to sql injection, this is not based on fact. There is absolutely no evidence of this. The closest you can get is to say that you cannot determine whether it is vulnerable or not - which is hardly a compelling argument.

As others pointed out, the METHOD itself if vulerable. A caller of the method can subvert the functionallity.

This is what defense in depth is all about. You code based on the presumption that the parameters to each method can be "anything" from correct, to silly to downright attacks.

2011-06-27 Reply Admin

TheCPUWizard:
no u:
. but when you and others say that this is vulnerable to sql injection, this is not based on fact. There is absolutely no evidence of this. The closest you can get is to say that you cannot determine whether it is vulnerable or not - which is hardly a compelling argument.

As others pointed out, the METHOD itself if vulerable. A caller of the method can subvert the functionallity.

This is what defense in depth is all about. You code based on the presumption that the parameters to each method can be "anything" from correct, to silly to downright attacks.

In every method/function on every level? For every type of parameter? Does anyone actually do that?

2011-06-27 Reply Admin

This guy deserve a MCM from Microsoft. Genius!

2011-06-27 Reply Admin

snoofle:
When you ask such narrowly focused questions on an interview, all you accomplish is that you determine whether the person can figure out the solution of which you are thinking. Any idiot can google the best algorithm for doing something.
A better approach is to figure out how the person goes about solving a problem with which they're not familiar. If they can think through a problem with a reasonable approach (and don't penalize for missing some subtle detail on a 30 second analysis), and they are personable, they'll probably make a good addition to your team.

I don't see it as a narrow question. There are presumably many different methods of getting the correct result. This also does challenge problem solving skills. Anyone who could break it down into a few easier questions such as "how can I sort", "how can I select just 1 element", "how do I get the correct column" etc would get just about full marks as far as I would be concerned, giving the actual syntax is the icing on the cake.

2011-06-27 Reply Admin

C-Octothorpe:
Hortical:
The program is also broken if someone adds ""+(1/0) to the list of statuses.

I realize I may be lambasted for making assumptions, however based on this code, the values likely come from down-downs in the UI. And if this is web based, then you can easily toss in any value for the drop down using a simple proxy tool (I think someone already mentioned) like fiddler or paros. So a safe value like 'ACTIVE' for myStatus could easily be replaced with '-- drop table whatever', and it'll happily do as you request.

yes, far too many assumptions. Such as the one where this application has a UI.

C-Octothorpe · 2011-06-27 Reply Admin

Lucent:
C-Octothorpe:
You're also failing to think about the junior devs who will maintain this code several years down the road. Do they know about sqli? Probably not. But at least you can be sure you're piece of code is written safely and can handle it.

I'll have to say: websites like this are fairly popular amongst students. If you've seen the problem on here, there's a good chance they've heard of it.

I have one thing to say to this: TDWTF.

A More Better Way to Count

Leave a comment on “A More Better Way to Count”