Ticket #56 (closed enhancement: fixed)

Opened 3 years ago

Last modified 2 years ago

Complete code audit

Reported by: anonymous Owned by: michiel
Priority: low Milestone: MvBlog 2.0 - Web2.0 version
Component: global Version: 2.0
Severity: Keywords:
Cc:

Description

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.

Attachments

code_audit (9.9 KB) - added by spam1@… 3 years ago.
Code audit findings

Change History

Changed 3 years ago by spam1@…

Code audit findings

Changed 3 years ago by michiel

  • status changed from new to assigned
  • version set to 2.0
  • milestone set to MvBlog 2.0

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?

Changed 3 years ago by michiel

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

Re #56

Changed 3 years ago by 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.

Changed 3 years ago by 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

Changed 3 years ago by 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

Changed 3 years ago by michiel

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

Re #56

Changed 3 years ago by 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

Changed 3 years ago by michiel

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

Re #56

Changed 3 years ago by michiel

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

Re #56

Changed 3 years ago by 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

Changed 3 years ago by michiel

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

Re #56

Changed 3 years ago by michiel

/index.php Bug: Unclosed div (if_page)

This one is closed in html_footer.

Changed 3 years ago by spam1@…

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

Sure.

Changed 3 years ago by michiel

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

Re #56

Changed 3 years ago by michiel

  • milestone changed from MvBlog 1.7 to MvBlog 2.0

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

Changed 3 years ago by anonymous

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

Changed 2 years ago by 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

Changed 2 years ago by michiel

  • status changed from assigned to closed
  • resolution set to fixed
Note: See TracTickets for help on using tickets.