Updating an item's status in MS SQL Server using ODBC

Is the following query is vulnerable in terms of $itemstatus? I want to be sure about it. $yesorno is a user input which is sanitized and it's fine, however, $itemstatus is a not user input but is included in the query. Can $itemstatus be manipulated or overall if it's vulnerable?

$username = $this->site->SanitizeIt($_POST['username'], 20);
    $yesorno = $this->site->SanitizeIt($_POST['yesorno'], 1);

    if (intval($yesorno) == 1)
    {
    $itemstatus = '1';
    }
    else if (intval($yesorno) == 2)
    {
    $itemstatus = '2';
    }
    else
    {
        $this->content = Template::Load('error', array('error_message' => Template::GetLangVar('INVALID_STATUS')));
        return;
    }

    $this->database[DBB]->doQuery('UPDATE INFO SET ItemStatus = ? WHERE UserName = ?', $itemstatus, $username);
    $this->content = Template::Load('error', array('error_message' => Template::GetLangVar('ITEM_STATUS_SUCCESS')));  
}

And if you wanna look at the SanitizeIt .. (It's NOT required since I ask for $itemstatus but however let me include it as well) here it is: (BTW: I am using MSSQL Server 2005)

function SanitizeIt($intext, $lenght = 20)
{
    return substr(preg_replace("/[^a-zA-Z0-9_]/", "", htmlspecialchars(htmlentities(strip_tags($intext)))), 0, $lenght);
}

Here is my doQuery() which is part of my ODBC.class file:

```
function doQuery($query)
{
if (!parent::isConnected())
{
if (!$this->doConnect())
return -1;
}

if ($this->result)
@odbc_free_result($this->result);

$parameters = func_get_args();
array_shift($parameters);

parent::Query();
if (sizeof($parameters) > 0)
{
$this->result = @odbc_prepare($this->conn, $query);
if (!$this->result)
return -1;

$result = @odbc_execute($this->result, $parameters);
if (!$result)
return -1;
}
else
{
$this->result = @odbc_exec($this->conn, $query);
if

Security

$itemstatus

$itemstatus will either be 1 or 2. It cannot possibly be anything else, so it is not an attack vector.

doQuery

I don't know doQuery so I cannot say if it is secure. If it is this: MongoCursor::doQuery, I wouldn't use it. The documentation says this: Warning Please do not use me. It doesn't say why, but security concerns could be one reason.

SanitizeIt

I wouldn't use your SanitizeIt function. For yesorno you don't need it, and for username it is quite limiting.

htmlentities and htmlspecialchars are generally used to prevent XSS attacks when outputting data to the user, not to prevent SQL injection.

Limiting the input to alphanumeric characters is not only very limiting, it might also not be sufficient for multi-byte encodings. If you can ensure that usernames only ever contain alphanumeric characters[*] - and that this will never change - then you can leave it in (as part of a defense in depth strategy it's not bad)

The recommended approach to SQL injection is to use parametrized queries (see here for corner cases). If you post your doQuery function we could tell you if it does this correctly.

[*] This basically means that you have to use the exact same function when registering users and when writing the username to the database.

Other

Style

• Your indentation is off, which makes you code hard to read

• you can either use under_score or camelCase for variables, but if you use neither, variable names are hard to read (see yesorno, itemstatus).

StackExchange Code Review Q#66550, answer score: 4