Complete code audit

[anonymous]

After the recent discovery of two security vulnerabilities (#54, #55), I decided to do a complete code audit of MvBlog. My findings are attached to this bugreport. In general, nothing important was discovered save for a few minor bugs and issues.

[spam1@…]

Code audit findings

[michiel]

Some really good points there. Thanks.

Most of the stuff will be in MvBlog 2.0
You ok that I put your real name as credits in the CHANGELOG when I commit the items?

[michiel]

(In [130]) remove unused code and examples.

Re #56

[michiel]

BTW: How does this function actually verify that the administrator is logged in and not just any other user? I assume there is no other user than the admin?

Yeah, there are no users right now. When we start to implement "commenting allowed by logged in users only" we should reconsider this and write some permission-checking function.

[michiel]

As a reminder to self: check out this page in concern of the magic_quotes issue: http://nl2.php.net/manual/en/function.get-magic-quotes-gpc.php

[michiel]

(In [131]) Detect wether magic_quotes_gpc is enabled. If yes, strip slashes from input, because our app already escapes everything that goes into a database.

Re #56

[michiel]

(In [132]) fix minor issue that prevented mvblog to work on https

Re #56

[michiel]

(In [133]) Fix order of stripslashes/htmlspecialchars etc. When inserting in the db the addslashes/preg_quote is done as first action. When reading the data in php, we should do stripslashes as first too. This to prevent undefined behaviour.

Re #56

[michiel]

(In [134]) Get rid of useles exec call.

Re #56

[michiel]

(In [135]) * Minimize query injectability. * Pointless switch case. Remove.

Re #56

[michiel]

(In [136]) DB::limitQuery does not specify that it converts parameters to ints.
Cast to int when calling to avoid possible SQL injection.

Re #56

[michiel]

(In [137]) Make urls work for https as well. strip html/xhtml/xml stuff from author info.

Re #56

[michiel]

/index.php Bug: Unclosed div (if_page)

This one is closed in html_footer.

[spam1@…]

You ok that I put your real name as credits in the CHANGELOG when I commit the items?

Sure.

[michiel]

(In [144]) do some extra checking on types

Re #56

[michiel]

all is done except the "default allow and filter". This will be done in the rewrite process to objects.

[anonymous]

Nice work Michiel. Why not close this bug and add the "default allow and filter" from this report to a new bugreport?

[spam1@…]

I've been receiving spam from MvBlog's Trac bugtracker, so I'm closing this email address (spam1@…). If you need to reach me you can find my email address at http://www.electricmonk.nl/index.php?page=Contact