Coding Horrors: You can gather all your technical debt into one easy framework!

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1501 »

Atom: Apr 6, 2005; by Y Kant Ozma Post

Ryouga Inverse posted:

The flaw is in your thought process. "Well, I don't know of any attacks that this doesn't prevent against, so this level of security is okay, I'll just use that." Why would you even want to think about that? Just code it the way that's not vulnerable to that class of attack AT ALL and go about your day.

I challenge you to construct for me a sample query where prepared statements are any more secure than properly-used mysql_real_escape_string().

# ? Jan 8, 2009 01:13

Adbot: ADBOT LOVES YOU

# ? Apr 26, 2024 04:57

Dessert Rose: May 17, 2004; awoken in control of a lucid deep dream...

Atom posted:

I challenge you to construct for me a sample query where prepared statements are any more secure than properly-used mysql_real_escape_string().

I challenge you to prove to me that mysql_real_escape_string() takes care of absolutely every possible SQL injection attack, including ones that may be discovered later.

# ? Jan 8, 2009 01:21

minato: Jun 7, 2004; cutty cain't hang, say 7-up.; Taco Defender

By "properly-used" I guess you mean that they're used comprehensively, in which case there is no difference - but that's not what I think Ryouga Inverse was getting at. If you always use escape_string() then there's a risk you might forget to, and that risk isn't possible with prepared statements. However that's beside the point, because I think Ryouga Inverse was referring to mysql_escape_string() vs mysql_real_escape_string(), not escape_string() vs prepared statements.

# ? Jan 8, 2009 01:22

Atom: Apr 6, 2005; by Y Kant Ozma Post

Ryouga Inverse posted:

I challenge you to prove to me that mysql_real_escape_string() takes care of absolutely every possible SQL injection attack, including ones that may be discovered later.

mysql_real_escape_string() is guaranteed by the MySQL API documentation to be 100% correct. The same level of dignity it gives to its prepared statements, no more, no less.

That's why the burden of proof is on you to prove that one method is any more secure than the other.

# ? Jan 8, 2009 01:28

geetee: Feb 2, 2004; >;[

The point is that with using prepared statements you don't need to remember to wrap everything with mysql_real_escape_string(). It also makes your code easier to read when it's not filled with the same stupidly_long_function_name_over_and_over().

If you choose to escape manually, you're going to forget eventually. Will it be exploitable? Depends on how often you forget and where... There's no reason not to use prepared statements.

geetee fucked around with this message at 01:38 on Jan 8, 2009

# ? Jan 8, 2009 01:34

Atom: Apr 6, 2005; by Y Kant Ozma Post

geetee posted:

The point is that with using prepared statements you don't need to remember to wrap everything with mysql_real_escape_string().

This is not the argument. Especially when prepared statements have an additional overhead of an additional statement sent to MySQL, there is no reason it should be preferred over the traditional method when the traditional method is used properly.

Edit: Situations that benefit from being able to use the same prepared statement twice and skip the parse/stats phase of the query the second time are excluded.

# ? Jan 8, 2009 01:49

minato: Jun 7, 2004; cutty cain't hang, say 7-up.; Taco Defender

What is the argument then? Is it mysql_escape_string() vs mysql_real_escape_string(), which is what zergstain was asking about? To reiterate what Ryouga Inverse said, a flaw was found with mes() whereas none has been found with mres(), so use mres(). Just because zergstain couldn't find any exploit code that used mes() isn't a good excuse to keep using it.

If the argument is prepared vs thorough use of mres(), then I don't think the choice is so clear cut. Prepared statements are free of injection risks and may be processed faster since the app doesn't have to plod through a potentially large string to copy and escape it, and the DB server doesn't have to spend time parsing the string to unescape it.

But the downside is that prepared statements can be a little harder to read (since it's necessary to read 2 areas of code simultaneously to ensure that each replacement token is correctly matched up with the variable that will be replacing it), and can they can be difficult to use in some dynamic SQL situations. Use of mres() can be easier and more legible in these situations, but runs the risk of the coder forgetting to use it.

# ? Jan 8, 2009 02:21

zergstain: Dec 15, 2005

Hell, if it were my personal code I'd have switched over to mres() sometime in 2003 or whenever it was introduced. Assuming I'd been doing PHP for that long. But this is company code, so I don't really want to just go around loving with poo poo. I'll go ahead and mention it tomorrow, though the work they actually want me to do will be a priority, and I'm fairly sure everything is latin 1 anyway. I will admit that I was beginning to wonder about the real world risks of using addslashes() since mysql_[real_]escape_string() takes much longer to type.

# ? Jan 8, 2009 02:41

minato: Jun 7, 2004; cutty cain't hang, say 7-up.; Taco Defender

zergstain posted:

and I'm fairly sure everything is latin 1 anyway.

If you're using mes() or mres() then you're probably escaping user input. How can you be sure all user input is Latin 1?

# ? Jan 8, 2009 02:53

zergstain: Dec 15, 2005

minato posted:

If you're using mes() or mres() then you're probably escaping user input. How can you be sure all user input is Latin 1?

Well, what happens if somebody submits unicode, but the connection charset is latin 1?

Edit: mres() escapes based on the connection charset I'm fairly sure, and I assume that means if somebody submitted some Shift-JIS or something it would just be interpreted as Latin 1, so explain how that is relevant.

zergstain fucked around with this message at 03:20 on Jan 8, 2009

# ? Jan 8, 2009 03:11

minato: Jun 7, 2004; cutty cain't hang, say 7-up.; Taco Defender

Well, you're asking whether you should get your bosses to approve moving from mes() to mres(). And you're right that mres() does respect the connection's character set. But mes() doesn't:

The PHP manual for mes() posted:

This function is identical to mysql_real_escape_string() except that mysql_real_escape_string() takes a connection handler and escapes the string according to the current character set. mysql_escape_string() does not take a connection argument and does not respect the current charset setting.

so that sounds to me like using mes() may be introducing at least some fragility into your code. Hey, your mes() code may be 100% safe, but rather than ponder the risks I'd rule them out altogether and use mres().

# ? Jan 8, 2009 03:38

Steampunk Mario: Aug 12, 2004; DIAGNOSIS ACQUIRED

No Safe Word posted:

*whoooosh*

edit: or "look at you, look how stupid you are" I guess

I guess I would have been more suspicious of a fakepost if it was funnier? I.D.K.

# ? Jan 8, 2009 05:36

zergstain: Dec 15, 2005

minato posted:

Well, you're asking whether you should get your bosses to approve moving from mes() to mres(). And you're right that mres() does respect the connection's character set. But mes() doesn't:

so that sounds to me like using mes() may be introducing at least some fragility into your code. Hey, your mes() code may be 100% safe, but rather than ponder the risks I'd rule them out altogether and use mres().

While I'm at it, anybody think I should ask about going through all 3.5 million or so rows in the users table replacing the plaintext passwords with a salted sha1 hash and dealing with the load on the slave servers as they have to deal with all the new data? Any of those people seriously think anyone at work would go for it? AFAIK, they want to be able to see passwords anyway. BTW, this is probably a good example for why you should be careful about reusing your passwords.

I suppose at the very least thanks to the insane password requirements on these forums I'd have little chance of doing anything if I ever found a goon in our database.

At least the dev environment is PHP 5.2.5 and the live is 5.2.6, unlike that other guy's company. And I get to look at porn.

Oh hey: http://ilia.ws/archives/103-mysql_real_escape_string-versus-Prepared-Statements.html

mes() would also interpret the sequence wrong and be vulnerable. Assuming I was coding for a Chinese site or something I guess.

This gives me the idea to make a wrapper function literally called mres() to reduce typing.

# ? Jan 8, 2009 06:32

geetee: Feb 2, 2004; >;[

Steampunk Mario posted:

I guess I would have been more suspicious of a fakepost if it was funnier? I.D.K.

Don't get too upset. I know sometimes it's hard to tell the blatantly obvious fake posts apart from the hexadecimal posts.

zergstain posted:

While I'm at it, anybody think I should ask about going through all 3.5 million or so rows in the users table replacing the plaintext passwords with a salted sha1 hash and dealing with the load on the slave servers as they have to deal with all the new data?

I think you should definitely propose it, but expect to be defined. I always feel so :smith:

when I maintain an application someone else developed and I see all the user's passwords in plain text. It feels awfully dirty.

geetee fucked around with this message at 06:59 on Jan 8, 2009

# ? Jan 8, 2009 06:56

Victor: Jun 18, 2004

Ryouga Inverse posted:

I challenge you to prove to me that mysql_real_escape_string() takes care of absolutely every possible SQL injection attack, including ones that may be discovered later.

This discussion fails because PHP and MySQL are so hosed up that if you are not paranoid enough, that if you don't pay absolute attention to security, you will lose and be hax0red. If you do "abc...'...wut".Replace("'", "''") in .NET, you are guaranteed that will work well. No encoding poo poo (everything is UTF-16 in .NET strings), nothing. There is no way you can fail unless you don't do that .Replace religiously. Now, few people are rigorous enough to do this, so they should suck it up and use parameterized queries. Whether you use prepared queries is immaterial -- that's only one way to parameterize in a safe way. I cannot believe there is so much loving bullshit on this issue. I can only attribute it to PHP and MySQL being lovely pieces of software. If you aren't anal about doing stuff right in the core layer, the layer upon which millions of people build, then you are worthless. You deserve to languish in the hell of bad code and ugly frameworks. I'm not sure I would work on PHP and MySQL if you paid me $1 million a year. They suck that much. The fact that plenty of people use these is irrelevant and you know it (or you are a retarded loser who never leaves his bubble).

I was just visiting this insults page and teapot's quote really resonated with me:

teapot posted:

No, it merely poisons minds, making programmers incapable of writing high-quality software and developing new ideas. It cripples their model of thinking by pushing them toward the optimal way of programming for severely mis-designed Microsoft products. Destruction of human minds' ability to improve is something that I don't take lightly.

I used to doubt this, but now I could not agree more. Except that Microsoft does not hold the patent on poo poo generation.

Victor fucked around with this message at 07:58 on Jan 8, 2009

# ? Jan 8, 2009 07:28

minato: Jun 7, 2004; cutty cain't hang, say 7-up.; Taco Defender

The mes()/mres() decision shouldn't really be a issue anyway - most well-designed apps would use some form of DB abstraction layer.

zergstain posted:

AFAIK, they want to be able to see passwords anyway.

If that's true, then your bosses are idiots and you're hosed.

# ? Jan 8, 2009 08:17

rotor: Jun 11, 2001; classic case of pineapple derangement syndrome

Victor posted:

I was just visiting this insults page

I'm really upset that I don't have a section there :mad:

# ? Jan 8, 2009 08:34

Victor: Jun 18, 2004

I guess it's just hard for old people to be truly offensive other than, you know, being old and crufty and smelly with bad breath and only knowing ancient languages that stunt brand new minds. Since that's the kind of person who probably wrote up that site, well, you're not special enough to make it in.

# ? Jan 8, 2009 08:36

Zombywuf: Mar 29, 2008

Victor posted:

Except that Microsoft does not hold the patent on poo poo generation.

I would be very surprised if they didn't. Making it illegal to to create mind poisoning software on any Linux except SUSE.

# ? Jan 8, 2009 09:02

Blotto Skorzany: Nov 7, 2008; He's a PSoC, loose and runnin'
came the whisper from each lip
And he's here to do some business with
the bad ADC on his chip
bad ADC on his chiiiiip

Btw mysql_real_escape_string has a long history of vulnerabilities (as does every function ever coded by MySQL AB, funny that...), and the PHP folks are nuts for using a really loving thin wrapper over it generally

If you are going to code a database-backed php app just use parameterized queries like every other language/framework out there

# ? Jan 8, 2009 09:05

Victor: Jun 18, 2004

How do I parameterize on the table from which I am selecting, using this wonderful parameterized functionality like every other language/framework out there?

# ? Jan 8, 2009 09:27

Blotto Skorzany: Nov 7, 2008; He's a PSoC, loose and runnin'
came the whisper from each lip
And he's here to do some business with
the bad ADC on his chip
bad ADC on his chiiiiip

Victor posted:

How do I parameterize on the table from which I am selecting, using this wonderful parameterized functionality like every other language/framework out there?

http://tinyurl.com/7ex6d8

# ? Jan 8, 2009 09:34

Victor: Jun 18, 2004

You must have missed that post of passion of mine above. It's ok, it's shorter than what I used to post long ago, so maybe you missed it. Maybe longs posts were good...

# ? Jan 8, 2009 10:43

Avenging Dentist: Oct 1, 2005; oh my god is that a circular saw that does not go in my mouth aaaaagh

Victor posted:

You must have missed that post of passion of mine above. It's ok, it's shorter than what I used to post long ago, so maybe you missed it. Maybe longs posts were good...

Your posts are just as long as they were before, now you just hit "Submit Reply" every few sentences. :pwn:

# ? Jan 8, 2009 10:53

PnP Bios: Oct 24, 2005; optional; no images are allowed, only text

Apparently PHP 5 supports prepared statements.

http://tinyurl.com/7jce4o

This mitigates all your strip slashing and quote adding.

There is no reason to send a raw query anymore, ever.

# ? Jan 8, 2009 11:02

Steampunk Mario: Aug 12, 2004; DIAGNOSIS ACQUIRED

code:

bufferIndex++ %= 2;

(C++)
Just saw this at work, someone should tell this chucklefuck that there's only one sequence point here. :argh:

# ? Jan 8, 2009 16:11

zergstain: Dec 15, 2005

PnP Bios posted:

Apparently PHP 5 supports prepared statements.

http://tinyurl.com/7jce4o

This mitigates all your strip slashing and quote adding.

There is no reason to send a raw query anymore, ever.

Unless your PHP was compiled without mysqli. And does that poo poo work with a 4.0.27 server?

Otto Skorzeny posted:

Btw mysql_real_escape_string has a long history of vulnerabilities (as does every function ever coded by MySQL AB, funny that...), and the PHP folks are nuts for using a really loving thin wrapper over it generally

I'd like to read more about that as I'm curious as to how a function with a fairly simple job can have a "long history of vulnerabilities". A quick google only revealed a multibyte handling issue.

minato posted:

The mes()/mres() decision shouldn't really be a issue anyway - most well-designed apps would use some form of DB abstraction layer.

I did mention that we had a quote method, so all we'd have to do is change that probably.

# ? Jan 8, 2009 17:18

hexadecimal: Nov 23, 2008; by Fragmaster

I found this code I wrote a while ago for comparing 2 revisions. This class I made also represents local changes as well as CVS revision, so my idea was to always have local revision to be higher, and then compare by time committed, and if that is equal compare author names.

But wtf, that is one big rear end return statement.

code:

public int compareTo(Object o)
{
	if (o instanceof CVSRevision)
	{
		CVSRevision another_revision = (CVSRevision) o;

		Long a = new Long( time.getTime() );
		Long b = new Long( ((CVSRevision) o).time.getTime() );
		int c = a.compareTo( b );
				
                // vvvvv WTF?!
	        return (!(local ^ another_revision.local)) ? (author.equals( another_revision.author )) ? c : (c == 0 ? author.compareTo( another_revision.author ) : c) : (local) ? 1 : -1;
	}
	return 0;
}

# ? Jan 8, 2009 17:21

Axel Rhodes Scholar: May 12, 2001; Courage Reactor

I enjoy that your `wtf' is some trivial syntactic who-cares, along the lines of the people fuming about "return x == true", while you ignore the enjoyable semantic wtf of every instance of that class being `equal' to everything that isn't a CVSRevision

(hint: Finally, the implementer must ensure that x.compareTo(y)==0 implies that sgn(x.compareTo(z)) == sgn(y.compareTo(z)), for all z )

# ? Jan 8, 2009 17:38

hexadecimal: Nov 23, 2008; by Fragmaster

dazjw posted:

I enjoy that your `wtf' is some trivial syntactic who-cares, along the lines of the people fuming about "return x == true", while you ignore the enjoyable semantic wtf of every instance of that class being `equal' to everything that isn't a CVSRevision

(hint: Finally, the implementer must ensure that x.compareTo(y)==0 implies that sgn(x.compareTo(z)) == sgn(y.compareTo(z)), for all z )

There will never be any other objects other than CVSRevision coming in there. If an object of other types comes through, this will be an error in my program, and will be caught in another place, since the data structure I use is Vector<CVSRevision>

Also what should it return if its not of CVSRevision type? Since its an error anyway, I think 0 is as good as any other return value.

My WTF is really because I have trouble reading my own code, which is not good. Probably should never use 3 ternary operators on same line.

EDIT: Whoops noticed another horror in same function. Good thing I went back to clean up my project.
EDIT2: Lol even another horror in same goddamn function. drat I suck.

vvvv I already take care of this in other functions, but its a good suggestion nonetheless.

hexadecimal fucked around with this message at 18:33 on Jan 8, 2009

# ? Jan 8, 2009 18:21

TSDK: Nov 24, 2003; I got a wooden uploading this one

hexadecimal posted:

There will never be any other objects other than CVSRevision coming in there. If an object of other types comes through, this will be an error in my program, and will be caught in another place,

Why not throw in an exception or an assert, just to be sure?

Then when the thing that should never happen actually does happen, you'll have a head start on tracking it down and fixing the bug.

# ? Jan 8, 2009 18:31

Dessert Rose: May 17, 2004; awoken in control of a lucid deep dream...

hexadecimal posted:

Also what should it return if its not of CVSRevision type? Since its an error anyway, I think 0 is as good as any other return value.

Yeah, "it's equal" is definitely as good as throwing a ComparisonException or something.

# ? Jan 8, 2009 20:21

hexadecimal: Nov 23, 2008; by Fragmaster

Ryouga Inverse posted:

Yeah, "it's equal" is definitely as good as throwing a ComparisonException or something.

Well, like I said, currently I check for this error in another part of same class. However I agree, that its probably a good idea to generate an exception or some other notification instead of just returning 0.

# ? Jan 8, 2009 20:32

Presto: Nov 22, 2002; Keep calm and Harry on.

Painless posted:

It won't "bitch", but it's quite possible that it will randomly fail! Just use unions drat it. Jeet chirst this is making me so angry. What nubbery..

Strictly speaking, this:

code:

union {
    int i;
    float f;
} butts;

float x;

butts.i = 666;
x = butts.f;

is not guaranteed to work either. You can't assign to one union member and then read another union member of a different type. The exception is that if the union members are structures with the same initial sequence of members and the union currently holds one of those structures.

# ? Jan 8, 2009 20:43

TSDK: Nov 24, 2003; I got a wooden uploading this one

Presto posted:

Strictly speaking, this:
code:
union {
    int i;
    float f;
} butts;

float x;

butts.i = 666;
x = butts.f;
is not guaranteed to work either. You can't assign to one union member and then read another union member of a different type. The exception is that if the union members are structures with the same initial sequence of members and the union currently holds one of those structures.

Actually, union casting is guaranteed to 'work' in the presence of strict aliasing (spit), it's just that the values returned by reinterpretting the bit pattern from one type to another aren't defined.

# ? Jan 8, 2009 20:47

floWenoL: Oct 23, 2002

hexadecimal posted:

Well, like I said, currently I check for this error in another part of same class. However I agree, that its probably a good idea to generate an exception or some other notification instead of just returning 0.

Why do you even bother checking "o instanceof CVSRevision" if you know only objects of that type will be passed in? The cast would just throw an exception (which is what you want) if you did pass in an object of another type.

I also enjoy !(x ^ y), which is equivalent to x == y.

quote:

Well, like I said, currently I check for this error in another part of same class. However I agree, that its probably a good idea to generate an exception or some other notification instead of just returning 0.

Even if you were absolutely required to return some value, 0 is pretty much the worst value you can return. Really, what were you thinking?

# ? Jan 8, 2009 21:01

Mustach: Mar 2, 2003; In this long line, there's been some real strange genes. You've got 'em all, with some extras thrown in.

floWenoL posted:

I also enjoy !(x ^ y), which is equivalent to x == y.

Reminds me of a thread from a few years ago in which somebody called '!=' bloated.

# ? Jan 8, 2009 21:25

dancavallaro: Sep 10, 2006; My title sucks

Can we get a mod to change the thread title to "Coding horrors: hexadecimal's personal pastebin"?

# ? Jan 8, 2009 21:33

Avenging Dentist: Oct 1, 2005; oh my god is that a circular saw that does not go in my mouth aaaaagh

Steampunk Mario posted:

code:
bufferIndex++ %= 2;
(C++)
Just saw this at work, someone should tell this chucklefuck that there's only one sequence point here.

Does that even compile? Post-increment returns an rvalue and you can't assign to rvalues.

# ? Jan 8, 2009 21:35

Adbot: ADBOT LOVES YOU

# ? Apr 26, 2024 04:57

hexadecimal: Nov 23, 2008; by Fragmaster

floWenoL posted:

Why do you even bother checking "o instanceof CVSRevision" if you know only objects of that type will be passed in? The cast would just throw an exception (which is what you want) if you did pass in an object of another type.

I also enjoy !(x ^ y), which is equivalent to x == y.

Even if you were absolutely required to return some value, 0 is pretty much the worst value you can return. Really, what were you thinking?

Lol yea I know, right? I coded this as fast as I could at the time to meet the dead line and and didn't give it much thought.

# ? Jan 8, 2009 21:37

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1501 »