Ticket #56: code_audit

Remarks:

    * Each bug is only reported the first time it has been seen, though it may
      occur multiple times.

    * Many of the bugs and minor issues reported are merely defensive coding
      practices and may therefor seem overly pedantic. But remember, even
      though code may not be exploitable now does not mean it won't become so
      in the future. Defensive code minimizes uncaught and future security
      problems.


Global

    Bug:

        Check for usage of magic_quotes_gpc(). If this setting is on, all
        incoming data should be stripped of slash-escaped quotes. MvBlog already
        escapes dangerous characters itself and if magic_quotes_gpc() is turned
        on, quotes will be escaped twice.

    Bug:

        Users can impersonate other users when commenting on a story. There 
        does not seem to be a way to prevent people from impersonating 
        a user. A possible remedy for this would be to allow users to 
        create an account.

    Bug:

        Use of sprintf() for query building might cause unintended behaviour.
        All %d replacements whose parameter is not an integer will be converted
        to '0'. This will in many cases not be a problem, but in some cases
        it could be. Perhaps parameters should be checked for is_int() and
        > 0 to prevent bogus entries from being entered into the database
        > (even though these probably do not cause any 
        security risks)

    Security Enhancement:

        Defined states. MvBlog has many places where states are undefined.
        Undefined states in a program are a cause of possible (but unseen)
        security problems and bugs. An undefined state is an atomic piece
        of code (object, method, function, include) which recieves input about
        which no assumptions can be made. For instance:

        function show_posts($options = array()) {
            global $db;
            if (!$options["top"]) { $options["top"] = 0; } else { $options["top"] = (int)$options["top"]; }
            if (!$options["limit"]) { $options["limit"] = 15; } else { $options["limit"] = (int)$options["limit"]; }
            //put all categories in array
            $res =& $db->query("SELECT * FROM categories");
            if (PEAR::isError($res)) {
                die($res->getMessage());
            }
            while ($res->fetchInto($row, DB_FETCHMODE_ASSOC)) {
                $cats[$row["id"]] = $row["name"];
            }
            $cats[-1] = "asides";
            if ($options["month"] && $options["year"]) {

        In the above code (taken from admin/index.php), the following variables
        are in an undefined state:

            $db
                global variables tend to be problematic. What kind of variable
                is $db? From inside the function, no assumption can (and
                should) be made about $db being a database connection, making
                security auditing problematic.

            $options[]

                No assumptions can be made about the contents of $options.

    Security enhancement:

        MvBlog uses exclusion instead of inclusion. For instance, variables
        that will go into the database are stripped of quotes (') because 
        they may give problems but all other characters are considered valid.

        Good security practice requires code to use inclusion by default. For
        instance, a value, lets say an authors name, which will go into the
        database should be checked against characters which may only appear in
        the value. In the case of an author's name, only a through z and
        possibly a space should be allowed.

        Many security problems can thus be contained. Lets look at two possible
        examples:

        preg_quote()ing for "'" does not prevent users from entering percentage
        signs into queries. Thus the programmer has to make sure that for every
        query that uses the LIKE syntax not only the quotes are stripped, but
        also all special characters which can be used in the LIKE syntax.

        Another example is that of character sets. Does preg_quote() work with
        different character sets? Suppose our database is of the charset UTF8, 
        but PHP is of the charset ISO8859. Will PHP strip out UTF8 version
        of quotes? This behaviour is very hard to determine and can't be
        counted on. It all depends on how PHP _and_ MySQL convert characters
        from one character set to another. If only certain characters where
        allowed though, the chances of the above happening would be practically
        zero. If any other character could be used to perform SQL injections,
        they would never get through because the code only allows characters in
        the, say, A-Z range through and nothing else.

xinha/

    Security enhancement:

        Remove examples/ directory. 
        
        Rationale: 
            Common security vulnerabilities are related to unsecure example
            scripts left in default installs.

/INSTALL

    Security enhancement:

        Default admin username and password is possibly bad practice. Disable
        mvblog admin until username and password has been manually configured.
        If this will not be fixed, mention it also in chapter 8 of the README.
    
        Rationale: 
            MvBlog is vulnerable to an account take-over for a short while
            between installation and configuration. Many users are too lazy to
            change the default password and must therefor be protected from
            themselves.

/index.php
    
    Bug:

        Unclosed div (if_page)

/sql/mvblog.sql

    Minor issue:

        'ip' field in the 'comments' table is unnecessarily large (255 chars)

    Minor issue:

        'website' field in the 'authors' table is not large enough. Though no
        official URL size is specified in any RFC, the minimum supported by all
        browsers is 2048.

    Minor issue:

        'tb_uri' field in the 'articles' table is not large enough. See above.


/site_images/empty.txt

    Minor issue:

        'empty.txt' should not appear in public releases.

/debian/

    Minor issue:

        'debian' directory should not appear in public releases.


common/functions_blog.php

    Security enhancement:

        line 50: if (!$_SESSION["author_id"]) {

        Reliance on boolean value of non-boolean variable. This piece of code
        tries to check for the availability of the 'author_id' key in the
        $_SESSION array, but in reality it checks if the value of the
        'author_id' key in the $_SESSION array is false. Use
        array_key_exists() instead. This problem occurs a multitude of times
        throughout the code. 

        Rationale:

            Undefined or hard to define behaviour in software is a lurking
            place for bugs and security problems. If an attacker were to be
            able to get the value of 'false' into the "author_id" index, the
            check would result in true and the code would be run, while that is
            not the intended behaviour. In this case that would hardly be a
            problem, but code changes and things that are currently not a
            problem may change into one later.

        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?

    Minor issue:

        line 165: <?="http://".$_SERVER["SERVER_NAME"].substr($_SERVER["PHP_SELF"],0,strrpos($_SERVER["PHP_SELF"], "/"))."/common/tb.php?id=".$row["id"]?>

        Code does not work when using HTTPS

    Minor issue:

        line 192: echo nl2br(stripslashes(htmlspecialchars(strip_invalid_xml($row["comment"]))));

        Possible reversed order of stripslashes, htmlspecialchars,
        strip_invalid_xml??  When inserting information into the database,
        stripslashes() is called right before inserting the information. It
        should then also be the very first thing that is called right after
        retrieving information from the database, to prevent undefined
        behaviour.

    Security enhancement:

        line 320: exec("ls style", $dirindex);

        Unnecessary use of exec() function. Use opendir() instead.

        
    Security enhancement:

        line 569: $q = "FROM articles WHERE active=1 AND public=1 AND aside = 0 AND date BETWEEN $time_start AND $time_end";

        Minimize query injectability. Move 'FROM articles WHERE active=1' to
        the non-injectable code on line 587, 593 and 595. 


        Rationale:

            $q might accidentally become an injectable variable in the future.
            By moving the 'FROM articles' portion to the actual query (making
            that portion unmuttable), an attack only has access to the
            'articles' table instead of all tables.

    Minor issue:

        line 580: case 0;

        Pointless switch case. Remove.

        Rationale:

            The 'default' switch case already handles the 0 case. In the
            current code '0' is a seperate case which trickels down into the
            default case. If, in the future, someone accidentally puts a
            'break' in the 0 case or the 0 case is moved, the $q variable might
            become unset or injectable causing query errors and possibly 
            SQL injections.

    Bug:

        line 593: $res =& $db->limitQuery("SELECT * $q ORDER BY date DESC", $start, $limit);

        DB::limitQuery does not specify that it converts parameters to ints.
        Cast to int when calling to avoid possible SQL injection.

/rss.php

    Bug:

        line 87: echo "\t\t\t<author>".$authors[$row["authors_id"]]["email"]." (".$authors[$row["authors_id"]]["fullname"].")</author>\n";

        $authors[$row["authors_id"]]["email"] is not striped of HTML/XML chars
        which may cause invalid XML code in the RSS feed.
        

admin/

    Feature enhancement:

        Detection for disabled cookies.

    Feature enhancement:

        Error message on wrong login.