Re: Boost MySQL Review Starts Today

From: Ruben Perez (rubenperez038_at_[hidden])
Date: 2022-05-11 21:06:08

On Wed, 11 May 2022 at 07:52, Rainer Deyke via Boost
<boost_at_[hidden]> wrote:
>
> On 10.05.22 16:59, Rainer Deyke via Boost wrote:
> > On 10.05.22 09:14, Richard Hodges via Boost wrote:
> >> The Boost formal review of the MySQL library starts Today, taking place
> >> from May 10th, 2022 to May 19th, 2022 (inclusive) - We are starting
> >> one day
> >> after the announced date and extending the period by one day to
> >> compensate.
> > I took a quick look, and my first impression is that the library doesn't
> > do enough to prevent SQL injection attacks.  Yes, text queries are
> > convenient when the full query is known at compile-time.  Yes, security
> > is ultimately the responsibility of those who use the API.  Yes, this is
> > C++, where far worse security flaws are a constant threat.  Even so,
> > connection::query gives me shivers.
>
> So, I've been thinking about what the library can do to prevent SQL
> injection attacks.  Ideas:
>    - connection::query can be renamed to connection::unsafe_query.

That reminds me of React naming. It inspires me "don't use this function
unless you're really knowledgeable of the library's internals and you know
what you are doing". I don't think that's the case. For example,
if you're using transactions, you'll be calling `conn.query("COMMIT")`.
I'm not very keen on making this `conn.unsafe_query("COMMIT")`.

>    - A big red warning about SQL injection attacks can be added to the
> documentation.

I've raised https://github.com/anarthal/mysql/issues/66 to address it.

>    - Syntax sugar for a one-off parametrized query wouldn't hurt either.

This requires a decent amount of work, as it requires implementing
SQL sanitizing client-side. I'm not very keen on it, as it's very possible
to get it wrong and end up introducing a vulnerability that wouldn't have
existed with prepared statements. I can have a look at how complex would
this be if the community thinks it really adds a lot of value.

>    - As a nuclear option, the query string can be changed into a
> template argument to prevent its use with strings that aren't known at
> compile-time.  Unfortunately this would also prevent some valid uses of
> connection::query.

As you already say, I think it's too nuclear - this is C++ and I think we
should treat our users as adults.

>
> If I were to design an API, I might use a combination of the above:
>
>    // Allowed:
>    con.query("SELECT * FROM employee WHERE company_id = ?", "HGS");
>    con.query<"SELECT * FROM employee WHERE company_id = 'HGS'">();
>    con.unsafe_query("SELECT * FROM employee WHERE company_id = 'HGS'");
>
>    // Error: literals in runtime-supplied query string not allowed.
>    con.query("SELECT * FROM employee WHERE company_id = 'HGS'");
>
> But I'm just throwing out ideas random here.
>
>
> --
> Rainer Deyke (rainerd_at_[hidden])
>
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost