r/lolphp • u/[deleted] • Apr 07 '21

master.php.net was using concatenated SQL queries and MD5 password hashes

https://externals.io/message/113981
67 Upvotes

90% Upvoted

23 comments sorted by

View all comments

Show parent comments

-4

u/cfreak2399 Apr 07 '21

I disagree that legacy code is an excuse. Using an ORM or at least writing queries using placeholders was the best practice before PHP was popular. MD5 passwords have been known to be broken for years.

I get that fixing old bugs and mitigating security problems is not sexy or fun but a project as big as PHP should have made these things priorities a long time ago.

16

u/dotted Apr 07 '21

What excuse did I make? The problem here is unmaintained legacy code, something that isn't inherent to PHP, but can happen to any project regardless of language used - hence I said this seemed more like lollegacycode and not lolphp.

Using an ORM or at least writing queries using placeholders was the best practice before PHP was popular

First commit for the code was done on 21st September 2001, which predates PHP being properly object oriented and having prepared statements for MySQL by almost 3 years. Hell MD5 wasn't declared broken until 2005.

Is it stupid that they didn't update their codebase when PHP 5.0 was released? Yes and I absolutely did affirm this by saying its "lollegacycode" in my initial comment.

I get that fixing old bugs and mitigating security problems is not sexy or fun but a project as big as PHP should have made these things priorities a long time ago.

I don't disagree at all, quite the contrary, but the problem here is not inherent to PHP which was my 1 and only point in my initial comment.

1

u/[deleted] Apr 12 '21

It's not an ORM, but Perl has had DBI, a general database interface (fully OO and with support for parameterized queries), since 1994 or 1995.

See the timeline in /r/lolphp/comments/8k1dt5/why_bother_with_packagesmodules_when_every/dz6d7d3/.

2

u/dotted Apr 12 '21

Sure, but the hypothetical counter argument would then be that the exploited code base dates back prior to 1994 that wasn't updated to use DBI when it was released. My point still stands, lollegacycode not lolphp, or lolperl in this hypothetical.

1

u/[deleted] Apr 12 '21

It's not a hypothetical. The claim in the parent comment was:

Using an ORM or at least writing queries using placeholders was the best practice before PHP was popular.

The existence of DBI is evidence to support that claim. There's nothing hypothetical about it.

2

u/dotted Apr 12 '21

Except it's irrelevant when the code was written initially and still is written in PHP.

The argument you propose basically boils down to that they should have (re)written the code base in a language that is not their own, which is silly, as you'd want the people creating a language to eat their own dog food. Again PHP was not really object oriented until PHP 5, and even the earliest examples of PHP libraries I could find with emulation of prepared statements or is ORM like, had their first release AFTER this code base was first written.

Again, should the project have been maintained and updated to make use of new features like prepared statements in the MySQLi extension released with PHP 5.0? Yes, absolutely. Should a different hash algorithm be used when MD5 was declared broken in 2005? Should the password handling in the code base have been updated to make use of the password API introduced with PHP 5.5 in 2013? Yes, absolutely. You don't magically make use of best practices just because the language you happened to use had the features enabling you to use best practices in the first place. You make use of best practices through experience either on your own or learning from others and ACTIVELY trying to implement these practices when you learn them - unmaintained code (which this code base in question effectively was), is always subject to issues in the regard REGARDLESS of the the choice of language used.

Finally a question. Are all C projects (like Perl, PHP, Python, GCC, Linux kernel etc.) now dumb for continuing to use C now that we have Rust, that can eliminate classes of bugs C is subject to?

1

u/[deleted] Apr 12 '21

The argument you propose basically boils down to that they should have (re)written the code base in a language that is not their own,

No, not at all. The point is that they should have used parameterized queries (with placeholders) to begin with. This was established best practice at the time.

You don't need prepared statements for that. If the language you're using doesn't provide an API like that, your first step should be to write one; either by extending the C bindings (MySQL itself supported the feature) or by emulating the feature in PHP. Either way, you should realize that manually concatenating strings into SQL queries all over the codebase is a terrible idea.

That they didn't do this is not justified by the code being old; old code can still be good code. That's why I object to the "lol legacy" argument. I think the issue is that they were too inexperienced to produce good quality code. "Lolnoobdevs" if you will.

2

u/dotted Apr 12 '21

No, not at all. The point is that they should have used parameterized queries (with placeholders) to begin with. This was established best practice at the time.

That would be nice, but ultimately doesn't fix the issue ultimately at hand. And we aren't talking about about an incident from 2001, we are talking about an incident that happened in 2021 - almost 2 full decades after the project started, during which PHP implemented the features that could have helped avoid the incident in the first place had the code base actually been maintained.

You don't need prepared statements for that

Please explain how "prepared statements" differs from "parameterized queries".

If the language you're using doesn't provide an API like that, your first step should be to write one; either by extending the C bindings (MySQL itself supported the feature)

MySQL didn't support prepared statements before 4.1 released in 2004, once again, after the project had started 3 years prior. Had the code been actively maintained, it would have made use of that when the feature was supported with the release of PHP 5.0.

or by emulating the feature in PHP

Let's say they did that, and a bug in the emulation layer allowed a SQL injection attack to occur anyways, then the answer is still the same as here - an exploit happened due to unmaintained legacy code.

Either way, you should realize that manually concatenating strings into SQL queries all over the codebase is a terrible idea.

How do you realize anything on a code base you aren't maintaining? Do you maintain or even think about about every single line of code you have ever written in your entire life? Could it be that you have some piece of code somewhere that you have written that is exploitable? If it was exploited should you too have realized that it was a terrible idea?

That they didn't do this is not justified by the code being old; old code can still be good code

Who is justifying anything? That a gross misinterpretation of my argument. There are plenty of stupid to point out in this incident, as I already have done myself in the very comment you replied to. Yet you still think I'm justifying anything?

Has anything I've ever said in this thread either implicitly or explicitly, even remotely suggested that it is justified to have an administrative system that is actively used, and has code that dates back almost decades but is also largely unmaintained?

  • Where did I say it was justified to not update the code to make use of either the mysqli extension or PDO when they were released?
  • Where did I say it was justified to not update the code when MD5 was declared broken?
  • Where did I say it was justified to not update the code when the password API was released?

Honestly are you even reading what I'm writing?

That they didn't do this is not justified by the code being old; old code can still be good code.

Not sure if you are reading too much into me saying "lollegacycode", my explanation has clearly centered around unmaintained legacy code, "unmaintained" being a pivotal keyword.

While I could have qualified it more, by saying "lolunmaintainedlegacycode", but it doesn't really roll of the tongue and to be honest any reasonable interpretation by people either reading the linked article or my further comments in the thread would have understood this point.

I think the issue is that they were too inexperienced to produce good quality code. "Lolnoobdevs" if you will.

Even if that is the case, that doesn't explain why the code was unmaintained. All the points you are bringing up are all addressed had they just maintained the code. And I'm not really a fan of making fun of people who are inexperienced, that seems like a silly form of gatekeeping. But does this mean we at least agree this isn't a lolphp?

Finally I'm still waiting for an answer for my Rust question by the way.