Sanitizing function

Question

This is a sanitizing function. Is this function secure enough?

public function clean( $input, $fordb = true ) {

            global $db;

            //is it an array?
            if( is_array( $input ) ) {

                //yes it is! let's clean each item individually.
                foreach( $input as $key => $value ) {

                    $input[$key] = $this->clean( $value, $fordb );

                }

                return $input;

            }
            else {

                //okay, it isn't, so let's go and tidy it up.
                $input = trim( $input );
                $input = htmlentities( $input, ENT_COMPAT );

                if( $fordb == true and isset( $db ) ) {

                    $input = $db->escape( $input );

                }
                elseif( $fordb == true ) {

                    $input = addslashes( $input );

                }

                return $input;

            }

        }

Answer 1

This is the wrong approach to security. First, a bit of background…

In general, sanitize is a bad word to use, as it leads to confusion. I recommend striking that word from your programming vocabulary (even if the PHP documentation uses it), to be replaced by three specific terms:

1. Canonicalization (or "Normalization"): transforming input from multiple representations of the same data into one preferred form.

   For example, if your form accepts a credit card number, then you should strip out all whitespace, because they are not a meaningful part of the data. Another example would be to lower-case an e-mail address.

2. Validation: rejecting input that violates your rules. Validation failure should cause the user to have to resubmit after fixing the errors.

   For example, if your form accepts a credit card number, then you should reject any submission that does not contain the right number of digits, the correct leading digits for the accepted card types, and a correct Luhn checksum.

   For an e-mail form, your validation rules might require a name to be non-empty, a subject line that is one line of a reasonable length, and a plausible-looking e-mail address.

3. Escaping: transforming strings so that one kind of string can be safely embedded inside another kind of string without being misinterpreted.

   Any time you compose a string that will be interpreted by another computer system, be it an e-mail message, HTML page, or SQL query, assume that you are vulnerable to some kind of injection attack. Every single one of these "human-friendly" languages (as opposed to binary formats such as JPEG images) will have delimiters of special significance. Header-splitting, HTML/JavaScript injection, and SQL injection attacks all have the same root cause: careless string concatenation or interpolation.

Canonicalization provides user-friendliness. Validation enforces your business logic. Escaping, not canonicalization or validation, upholds security. The term "sanitize" conflates the three mechanisms, leading you do write improperly engineered code.

---

Given that background, I would say that your clean() function is somewhere between useless and harmful. Its main purpose is to escape text "just in case" — there is no clear purpose.

If the input is meant to be stored in a database, then $db->escape($input) is the right escaping call. addslashes($input) comes somewhat close to being right if you're using MySQL, but it's not, and it would be far from correct for most other SQL databases. If the input is destined to be stored in a database, then you certainly don't want to HTML-encode it first. That's the kind of careless escaping logic that leads to crappy websites that mis-render "&" characters as &. Furthermore, if you store text in the database in mangled form, then it's not properly searchable.

Conversely, if you need to HTML-escape your output, then that htmlentities() call has no business being mingled in the same function as your SQL-escaping function. SQL escaping is for writing strings to a database; HTML escaping is for writing text to the browser. You would therefore never have a situation where you would want to apply both escaping mechanisms simultaneously to the same string.