Skip to main content
Code Review

Return to Answer

added 1581 characters in body
Source Link
tim
tim
  • 25.1k
  • 3
  • 30
  • 76

Regarding your security code

I'm glad to see that you have some defenses in place so that you are not completely vulnerable.

That being said, having one (or more) input filter functions in place is not really a good approach to security. It will catch some attacks (maybe even most), but there are always cases where it will not work.

It will also lead to usability problems and bugs, which isn't good, but not necessarily your main concern. Still, these bugs can have security implications. For example, my super secure password i<3super_secure_passwords becomes the way less secure i for your website (because of strip_tags).

You are also still vulnerable to XSS. strip_tags doesn't work when you are already in a tag, eg here: <input name=\"game_name\" size=\"20\" value=\"$game_name\" /> ($game_name is user supplied, and an attacker can add additional attributes to get into a JavaScript context).

Allowing your mods to post any tags is also not a good idea, as they can relatively easily escalate their privileges to admin. You don't need to write some kind of filter yourself (and you shouldn't, as it's quite difficult), just use an existing library, eg HTML Purifier.

Regarding SQL injection, the code you posted is probably safe (although strongly discuraged; there is a reason that magic quotes isn't enabled by default anymore), but if you have queries where user input is not surrounded by quotes, it will not work (eg when passing ids, when using limit, etc).

Also, don't store passwords in cookies, and make cookies httpOnly.

Regarding your security code

I'm glad to see that you have some defenses in place so that you are not completely vulnerable.

That being said, having one (or more) input filter functions in place is not really a good approach to security. It will catch some attacks (maybe even most), but there are always cases where it will not work.

It will also lead to usability problems and bugs, which isn't good, but not necessarily your main concern. Still, these bugs can have security implications. For example, my super secure password i<3super_secure_passwords becomes the way less secure i for your website (because of strip_tags).

You are also still vulnerable to XSS. strip_tags doesn't work when you are already in a tag, eg here: <input name=\"game_name\" size=\"20\" value=\"$game_name\" /> ($game_name is user supplied, and an attacker can add additional attributes to get into a JavaScript context).

Allowing your mods to post any tags is also not a good idea, as they can relatively easily escalate their privileges to admin. You don't need to write some kind of filter yourself (and you shouldn't, as it's quite difficult), just use an existing library, eg HTML Purifier.

Regarding SQL injection, the code you posted is probably safe (although strongly discuraged; there is a reason that magic quotes isn't enabled by default anymore), but if you have queries where user input is not surrounded by quotes, it will not work (eg when passing ids, when using limit, etc).

Also, don't store passwords in cookies, and make cookies httpOnly.

Source Link
tim
tim
  • 25.1k
  • 3
  • 30
  • 76

If this code is representative of all your code, then yes, I would suggest a complete rewrite, using more modern concepts (OOP, MVC, template engine, some framework, etc).

Until then, I would probably take the website offline, or at the very least add a web application firewall to the server.

Security

Your code is currently vulnerable to SQL injection, XSS, and CSRF.

You need to look into prepared statements to be secure against SQL injection. Some frameworks also provide ORMs or query builders, which may make it easier for you to securely access your database.

You then need to HTML encode user input when echoing it. Most templating engines also provide this functionality, and some encode per default (which I would recommend).

Most frameworks also offer some mechanism to protect against CSRF.

Structure

Your code is too long and not well structured, as you don't have any functions.

Because of this, it also contains quite a bit of duplication (eg the edit/add forms which are quite similar, duplicated select queries, etc).

Most frameworks would make it easier to structure your code well, as they for example use MVC, or expect you to program things a certain way.

Misc

  • extract is a function that you mostly shouldn't use. It can have security implications, and it leads to less readable code. Just assign things to variables explicitly yourself.
  • You can use ' and " for strings. You can use this to avoid having to escape " in all your forms.
  • you can use guard clauses to decrease the nesting of your code and make it more readable. Eg instead of if (authenticate ("9", "0", "1") == "verified") {long block of code} you would write if (authenticate ("9", "0", "1") !== "verified") {return or die} long block of code.