OpenSIPS / opensips

OpenSIPS is a GPL implementation of a multi-functionality SIP Server that targets to deliver a high-level technical solution (performance, security and quality) to be used in professional SIP server platforms.
https://opensips.org
Other
1.28k stars 581 forks source link

[core] Escaping values for SQL in script raw querys #1530

Open ar45 opened 5 years ago

ar45 commented 5 years ago

The SQL standard way to escape single quotes is by doubling them, and most databases do not recognize \ as an escape character. And thus aren't properly escaped by postgres leaving queries vulnerable to SQL injections.

From postgresql docs https://www.postgresql.org/docs/current/runtime-config-compatible.html

backslash_quote (enum) This controls whether a quote mark can be represented by \' in a string literal. The preferred, SQL-standard way to represent a quote mark is by doubling it ('') but PostgreSQL has historically also accepted \'. However, use of \' creates security risks because in some client character set encodings, there are multibyte characters in which the last byte is numerically equivalent to ASCII . If client-side code does escaping incorrectly then a SQL-injection attack is possible. This risk can be prevented by making the server reject queries in which a quote mark appears to be escaped by a backslash. The allowed values of backslash_quote are on (allow \' always), off (reject always), and safe_encoding (allow only if client encoding does not allow ASCII \ within a multibyte character). safe_encoding is the default setting.

Note that in a standard-conforming string literal, \ just means \ anyway. This parameter only affects the handling of non-standard-conforming literals, including escape string syntax (E'...').

So the only way to have strings containing ' escaped is by prefixing them with E'...'

bogdan-iancu commented 5 years ago

Hi @ar45 - the actual purpose of {s.escape.common} is fuzzy if you ask me, as it is not affiliated to any clear escaping policy. It does \ based escaping , but only for ', ", \ and 0 . The mysql escaping works with \ - see https://dev.mysql.com/doc/refman/8.0/en/string-literals.html, but also by doubling the single quotes (not sure about double quotes?!). Is there any SQL (accepted by all(most) implementations) in terms of quoting ?

bogdan-iancu commented 5 years ago

OK, some update here. After some research, it seems that the single quote is the standard SQL way of quoting strings. Escaping them is by doubling, indeed (and not with '\'). While the standard/formated queries are safe in opensips (as each DB backend is doing it's own escaping when packing the strings into the query), the raw queries from script are not safe, so you need to do standard SQL escaping (doubling the single quotes) - of course, this means you need to format your query by packing the string with single quote. Or many an {s.sql_string} transformation that will escape all the inner single quotes and also add the wrapping quotes too.

ar45 commented 5 years ago

Hi Bogdan Makes sense. I thought maybe it would be even better to provide transformations through the db module and each db_api module should implement the proper escaping according to which database connection being used.

db_postgres would use PQescapeLiteral() db_postgres would use mysql_real_escape_string()

The syntax could be something like {sql.escape_string,avpdb-id} where avpdb-id is the db url reference number. this will also surround the value with quotes, so that the user does not need to quote it manually. NULL values a avp with no value would be transformed into sql NULL without quotes. That could even be taken to the next level by choosing to quote the value based on the type of avp int or str, so only str will be surrounded by quotes.

-- Aron Podrigal 718-942-9990 x 221

Software Engineer MongoTel

On Fri, Jan 4, 2019 at 7:50 AM Bogdan Andrei IANCU notifications@github.com wrote:

OK, some update here. After some research, it seems that the single quote is the standard SQL way of quoting strings. Escaping them is by doubling, indeed (and not with ''). While the standard/formated queries are safe in opensips (as each DB backend is doing it's own escaping when packing the strings into the query), the raw queries from script are not safe, so you need to do standard SQL escaping (doubling the single quotes) - of course, this means you need to format your query by packing the string with single quote. Or many an {s.sql_string} transformation that will escape all the inner single quotes and also add the wrapping quotes too.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OpenSIPS/opensips/issues/1530#issuecomment-451448551, or mute the thread https://github.com/notifications/unsubscribe-auth/AEsfGLLZ-O1OvB9T8vaRSU49VoECAkBCks5u_1xAgaJpZM4YatgY .

bogdan-iancu commented 5 years ago

Hi @ar45, That's an interesting idea, but what what about drivers with out such escaping function? As the effort of doing this will be not small (as the DB API must be extended to export the escaping function), out of my curiosity, what are these escaping function (like the mysql and postgres) more than doubling the single quotes ? I'm asking as maybe there is no pay off for the effort of using the driver native escaping, versus simply escaping the single quotes.

ar45 commented 5 years ago

These functions do the escaping while being aware of the database character encoding.

https://security.stackexchange.com/a/9910

stale[bot] commented 5 years ago

Any updates here? No progress has been made in the last 15 days, marking as stale. Will close this issue if no further updates are made in the next 30 days.