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

Question

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 (!$this->result)
 return -1;
 }
 parent::Success();
 return @odbc_num_rows($this->result);
}

• Bob Dalgleish: $itemstatus isn't included in the query. It is bound to a placeholder in a prepared statement. It comes down to whether you are using a proper statement preparation package.
• Monk25: So, overall it should be totally fine, is that correct? Thanks for taking your time to help me out. Its appreciated.
• Elias Van Ootegem: 1) preg_replace are not multibyte safe by default, so the preg_replace call is not safe as such. 2) Please, please, please don't use the @ operator of death. If PHP issues a notice or warning, don't ignore it or hush it up: fix it!
• Monk25: I understand but users cannot use non-multibyte characters, so it should do the work in that case, correct?

Answer 1

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).

• Monk25: Thanks a lot for your detailed post. It's greatly appreciated. I've got my last 2 questions: 1. I use the same function for usernames and all should only contain alphanumeric characters and underscore _, so is SanitizeIt function fine? I got scared a bit by that you wouldn't use my SanitizeIt but let me know if that's all good with it since mostly its alphanumeric and only _ allowed. And also, 2. I've updated my main post with part of the ODBC.class file ---> doQuery() function.
• tim: @Monk25 I didn't want to imply that there is a security issue with SanitizeIt. Your doQuery class does seem to use prepared statements, so that should be sufficient to prevent SQL injection. SanitizeIt on top is not really necessary, but my main concern with it was usability: I couldn't use my email address as username, or a common german name like Jörg (same problem with french names for example). If the registration already complains, then this is annoying, but not a major problem. But if it allows it, but then the user cannot use all functionality, it would be a real problem
• Monk25: Usernames should only contain alphanumeric characters and possible to include _ underscore on it. That's all. It shouldn't allow to use e-mail address as username or any special characters. Does that means that SanitizeIt() and doQuery() are totally fine and SQL injection shouldn't be a concern? Overall is it fine? Please let me know so I can accept the answer. I've already upvote it since its really helpful to me. Thanks!