Would this function work for generally sanitizing db query variables?

Question

I know most people say to just use prepared statements, but I have a site with many existent queries and I need to sanitize the variables by the mysqli_real_escape_string() function method.

Also the php manual of mysqli_query() says mysqli_real_escape_string() is an acceptable alternative, so here I am ...

I want to do this type of queries:

$query = sprintf("SELECT * FROM users WHERE user_name = %s", 
                    query_var($user_name, "text"));
$Records = mysqli_query($db, $query) or die(mysqli_error($db));

I want to know below function would work, I am unsure if:

I should still do the stripslashes() at the start ? An old function I used from Adobe Dreamweaver did this.
Is it OK to add the quotes like $the_value = "'".$the_value."'"; after the mysqli_real_escape_string() ?
Does it have any obvious / big flaws ?

I noticed the stripslashes() removes multiple \\\\\\ and replaces it with one, so that migt not work well for general use, e.g when a user submits a text comment or an item description that might contain \\\\, is it generally OK not to use stripslashes() here ?

I am mostly worried about SQL injections, it is OK if submitted data included html tags and so, I deal with that when outputing / printing data.


if(!function_exists('query_var')){
    function query_var($the_value, $the_type="text"){
        
        global $db;
        
        // do I still need this ?
        // $the_value = stripslashes($the_value);
        
        $the_value = mysqli_real_escape_string($db, $the_value);
        
        // do not allow dummy type of variables
        if(!in_array($the_type, array('text', 'int', 'float', 'double'))){
            $the_type='text';
        }
        
        if($the_type=='text'){
            $the_value = "'".$the_value."'";
        } 
        
        if($the_type=='int'){
            $the_value = intval($the_value);
        }
         
        if($the_type == 'float' or $the_type=='double'){
            $the_value = floatval($the_value);
        } 
        
        return $the_value;
        
    }
}

If you're worried about SQL injection, why aren't you using query parameter binding? It's easier and more secure, and you don't need to ask if your function is sufficient. You don't need a function. — Bill Karwin
– Bill Karwin, Commented Nov 26, 2022 at 3:16
Because the site has over 1000 queries with 4-5 lines each. I cannot rewrite them all, they already contain sprintf() and a function that sanitizes the variables, I just need to fix the function to work with existent queries. I also don't find the PDO way very readable, the way it ads the variables and their type. Maybe for a new site I would use PDO. — adrianTNT
– adrianTNT, Commented Nov 26, 2022 at 3:27
You don't have to use the type. In the MySQL PDO driver, they all bind as strings anyway. You can just pass an array to the execute() function. — Bill Karwin
– Bill Karwin, Commented Nov 26, 2022 at 5:07
The variable type must have a purpose, I imagine x > 10 is different than x > '10' I didn't test PDO very much thou. — adrianTNT
– adrianTNT, Commented Nov 26, 2022 at 14:18
All I'm saying is that I'd convert the code to use PDO, and I'd rewrite the queries to use parameters. Even though there are 1000+ queries, that's honestly not that much work, compared to worrying about the security of your sanitize function. I'd be surprised if it takes two weeks of focused work. — Bill Karwin
– Bill Karwin, Commented Nov 26, 2022 at 14:42

O. Jones · Accepted Answer · 2022-11-26 01:15:04Z

0

A text string constant in MySQL / MariaDB starts and ends with a single quote ' character. If the text itself contains a quote character, we escape it by doubling it. So the name "O'Leary" looks like this in a SQL statement's text.

SET surname = 'O''Leary'

That's the only rule. If your users feed you data with backslashes or other escaping schemes, you can feed it verbatim to MySql with the kind of text string representation mentioned here.

Don't overthink this. But use carefully debugged escaping functions. Avoid writing your own, because any tiny bug will allow SQL injection.

answered Nov 26, 2022 at 1:15

O. Jones

109k17 gold badges134 silver badges187 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

adrianTNT Over a year ago

You meant SET surname = 'O\'Leary' there ? I a still unsure if I should do stripslashes before mysqli_real_escape_string.

O. Jones Over a year ago

I meant doubling the single quotes. Backslashes inside MySql string constants are just backslashes.

adrianTNT Over a year ago

Now I am even more confused :/ Are both '' and \' accepted and do the same ? When I insert a value O'Leary from phpMyAdmin it sets the query to this and it works: INSERT INTO 'test' ('name') VALUES ('O\'Leary');

adrianTNT · Accepted Answer · 2022-11-26 14:38:01Z

0

Looking at the PHP functions documentation, I found some references that made me decide the stripslashes() is not needed in that function.

https://www.php.net/manual/en/security.database.sql-injection.php

Generic functions like addslashes() are useful only in a very specific environment (e.g. MySQL in a single-byte character set with disabled NO_BACKSLASH_ESCAPES) so it is better to avoid them.

https://www.php.net/manual/en/function.addslashes.php

The addslashes() is sometimes incorrectly used to try to prevent SQL Injection. Instead, database-specific escaping functions and/or prepared statements should be used.

answered Nov 26, 2022 at 14:38

adrianTNT

4,1635 gold badges36 silver badges41 bronze badges

Collectives™ on Stack Overflow

Would this function work for generally sanitizing db query variables?

2 Answers 2

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related