Ryzom API

kirlack wrote:

Currently, the ryzom_character_valid_key & ryzom_guild_valid_key are not safe if you want to check if an user entry seems to be a valid API key before storing it in a database : checking only the number of 'R' separators and the first letter, this can't be used to prevent SQL injections.

you right, but you 'fixing' it in the wrong place and actually making it worse.
right way for it is http://us2.php.net/manual/en/function.mysql-real-escape-string.php

also, they only way to know if key is valid, is to ask api server and *_valid_key() functions could be used to see if user entered char or guild key and to decide which api function to use to download the xml.
i even would go so far that to only check first two chars for 'fr', 'pr', 'gr' ;-)

nimetu wrote:

you right, but you 'fixing' it in the wrong place and actually making it worse.
right way for it is http://us2.php.net/manual/en/function.mysql-real-escape-string.php

No, when expect an entry to respect a specific format you shall not use mysql_real_escape_string. This function should be used only for human-written strings and nothing else, abusing of this function could remain security holes opened. Here is an example, in french and not really applicable for API keys, I know :
http://www.phpcs.com/forum/sujet-PARSE-ERROR-SYNTAX-ERROR-UNEXPECTED-T_IF-IN_1321400.aspx#m_CPH1_UCForumHome1_Message_UCForumMessage1_DGMsg_ctl06_divContent

kirlack wrote:

nimetu wrote:

you right, but you 'fixing' it in the wrong place and actually making it worse.
right way for it is http://us2.php.net/manual/en/function.mysql-real-escape-string.php

No, when expect an entry to respect a specific format you shall not use mysql_real_escape_string.

i think i forgot to add something important in my first reply, so i try again.

To prevent SQL injection from API key input, you need to fix it in your code by using mysql_real_escape_string right before you adding key to database and not to rely on api function to say if string is secure for DB or not.

remember, you said "can't be used to prevent SQL injections.....a simple use of ctype_digit and ctype_xdigit could prevent such issues.". this description makes it 'SQL injection fix patch' and not 'improved api key detection patch'. for the later, the patch is wonderful (even tho unneccessary). for the first.... plain evil.

This function should be used only for human-written strings and nothing else, abusing of this function could remain security holes opened. Here is an example, in french and not really applicable for API keys, I know :
http://www.phpcs.com/forum/sujet-PARSE-ERROR-SYNTAX-ERROR-UNEXPECTED-T_IF-IN_1321400.aspx#m_CPH1_UCForumHome1_Message_UCForumMessage1_DGMsg_ctl06_divContent

first thing that caught my eye was that return value from mysql_real_escape_string was not inside quotes as it should be. ofcourse it insecure that way.

nimetu wrote:

To prevent SQL injection from API key input, you need to fix it in your code by using mysql_real_escape_string right before you adding key to database and not to rely on api function to say if string is secure for DB or not.

No. To prevent SQL injection (in a general case) you must check if the entry conforms with what you expect. For an int you should check if that's an int, idem for everything else. mysql_real_escape_string should be used only if what you expect can contain special chars.

Here, API keys never contains any of those special chars and specials functions (*_valid_key()) are here to return true or false if the key seems to be valid. And because a normal API key must not contains chars that could be used for an SQL injection, *_valid_key() must return false if the key contains such chars.

nimetu wrote:

remember, you said "can't be used to prevent SQL injections.....a simple use of ctype_digit and ctype_xdigit could prevent such issues.". this description makes it 'SQL injection fix patch' and not 'improved api key detection patch'. for the later, the patch is wonderful (even tho unneccessary). for the first.... plain evil.

Nope, the prevention of SQL injection is only a consequence of an improvement of theses functions. Let's stop talking about SQl injections. *_valid_key() must return true if the key seems to be a valid key, that's written in the documentation, we are all ok on this point. No, tell me if "PRis_that_a_valid_keyRI_don't_think_so" is a valid character key. Of course not, but ryzom_character_valid_key returns true ... If *_valid_key() returns true for NON valid API keys, there's a huge problem isn't it ? The path I proposed fix this problem, and because this problem is solved the test functions could be safely used to check an user entry (in a semantic way that's really better than using mysql_real_escape_string to safely store invalid keys). In fact, I should better have posted this patch in "Bugs", if a function that doesn't returns what we expect it to return, that's a bug.

nimetu wrote:

first thing that caught my eye was that return value from mysql_real_escape_string was not inside quotes as it should be. ofcourse it insecure that way.

In SQL quotes are only for strings, adding some you expect MySQL to convert the value. It works, but that's not a good practice. This example shows that when mysql_real_escape_string isn't a good choice for non-strings values.

kirlack wrote:

For an int you should check if that's an int, idem for everything else. mysql_real_escape_string should be used only if what you expect can contain special chars.

well... every user input is SQL injection... they are evil you know ;-)

In fact, I should better have posted this patch in "Bugs", if a function that doesn't returns what we expect it to return, that's a bug.

bug report accepted... well for my fork anyway ;-).
it's found at http://github.com/nimetu/ryzom_api

PS!! even tho key check is more strict, it does not mean you may just stuff it to database.
key format might change in future and it might allow quotes in it, so mysql_real_escape_string (or equivalent) should still be used.

In SQL quotes are only for strings, adding some you expect MySQL to convert the value. It works, but that's not a good practice. This example shows that when mysql_real_escape_string isn't a good choice for non-strings values.

mysql_real_escape_string only works when you put the return value inside quotes.

I applied the nimetu patched in the main tree. Thanks for the report kirlack.

nimetu wrote:

kirlack wrote:

For an int you should check if that's an int, idem for everything else. mysql_real_escape_string should be used only if what you expect can contain special chars.

well... every user input is SQL injection... they are evil you know ;-)

Yes of course, but I don't understand why you told it, there's no link with what I said. Btw, I was a bit wrong to talk about SQL injection, before storing the API key it should be encrypted :)

nimetu wrote:

bug report accepted... well for my fork anyway ;-).
it's found at http://github.com/nimetu/ryzom_api

You're right to use your regex instead of my ctype_* functions, I totally forgot to check the slot number's range and the length of the last part of the key. Btw, I have a question about the uid/gui : doesn't it also have a min or max length ? If so, you should also check it.

PS!! even tho key check is more strict, it does not mean you may just stuff it to database.
key format might change in future and it might allow quotes in it, so mysql_real_escape_string (or equivalent) should still be used.

I'm sure that even if you can receive a shoe in you head you don't wear a helmet everyday. As you said the API key may change, and you don't know what it will be or even if a change will come. Because you cannot prevent every change there's no reasons to prevent a specific one.

In SQL quotes are only for strings, adding some you expect MySQL to convert the value. It works, but that's not a good practice. This example shows that when mysql_real_escape_string isn't a good choice for non-strings values.

mysql_real_escape_string only works when you put the return value inside quotes.

I suppose you wanted to say "mysql_real_escape_string is prevent SQL injection only when the returned value is inside quotes". Quotes or not, this function will work, all specials chars will be escaped, the only problem is that sometimes it's useless to escape it ;)

kirlack wrote:

Btw, I have a question about the uid/gui : doesn't it also have a min or max length ? If so, you should also check it.

dont know ;-), but probably. my keys seems to give 9+ digits (and start with 108) for gid and 6+ for uid, but there might be exceptions hehe.

kirlack wrote:

No, when expect an entry to respect a specific format you shall not use mysql_real_escape_string. This function should be used only for human-written strings and nothing else, abusing of this function could remain security holes opened. Here is an example, in french and not really applicable for API keys, I know :
http://www.phpcs.com/forum/sujet-PARSE-ERROR-SYNTAX-ERROR-UNEXPECTED-T_IF-IN_1321400.aspx#m_CPH1_UCForumHome1_Message_UCForumMessage1_DGMsg_ctl06_divContent

Your arguing is wrong. Escape datas are mandatory every time you pass datas from PHP to some sort of view. For my sake, I have a class that escapes automagicaly all datas, and build the query with a sprintf.

And now, for checking HTTP input, yes you must do. But this must be done by your model (or control, it is a question of outlook), not data access work.

LupusMichaelis wrote:

Your arguing is wrong. Escape datas are mandatory every time you pass datas from PHP to some sort of view.

No, escaping datas is useful only when you have something to escape. If you can be sure there's nothing to escape, why are you still tying to escape something ? If an user entry doesn't respect the format you expect, then you have to stop the execution before trying to build your query or anything else. An API key cannot contain MySQL special chars, so if there's some you have to raise an error instead of building & executing your query.

LupusMichaelis wrote:

For my sake, I have a class that escapes automagicaly all datas, and build the query with a sprintf.

To my mind that's a bad idea : the chars to escape won't be the same every time. Unless your class can differ a MySQL query from a PostgreSQL query or a shell exec, this method is useless. When you have reasons to escape something, it's strongly recommended to use the specific functions, a generic escape class won't work every time.