From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52901) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ekBBe-0005ZX-5g for guix-patches@gnu.org; Fri, 09 Feb 2018 11:06:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ekBBb-0000Lc-0z for guix-patches@gnu.org; Fri, 09 Feb 2018 11:06:06 -0500 Received: from debbugs.gnu.org ([208.118.235.43]:55950) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1ekBBa-0000LV-TZ for guix-patches@gnu.org; Fri, 09 Feb 2018 11:06:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1ekBBa-0007ol-Le for guix-patches@gnu.org; Fri, 09 Feb 2018 11:06:02 -0500 Subject: [bug#30386] [PATCH v2 cuirass] database: Prevent SQL injection. Resent-Message-ID: From: ludo@gnu.org (Ludovic =?UTF-8?Q?Court=C3=A8s?=) References: <20180207231258.31169-1-dannym@scratchpost.org> <20180208163432.9468-1-dannym@scratchpost.org> <87r2pu4hk4.fsf@gnu.org> <20180209121612.09a0cf53@scratchpost.org> Date: Fri, 09 Feb 2018 17:05:34 +0100 In-Reply-To: <20180209121612.09a0cf53@scratchpost.org> (Danny Milosavljevic's message of "Fri, 9 Feb 2018 12:16:11 +0100") Message-ID: <87k1vmywq9.fsf@gnu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Danny Milosavljevic Cc: 30386@debbugs.gnu.org Danny Milosavljevic skribis: >> optimization looks good, provided the extra conditions don=E2=80=99t mak= e sqlite >> slower.=20=20 > > Compared to parsing the SQL text again and again (which is dead slow), I = think > an extra NULL check *on the same field* is not going to matter at all. > > Even compared to using lots of main memory and thus not being able to use > the processor's cache (if we had lots of prepared statements), I think an > extra NULL check is still better :) > > Of course once we have a lot of data in the tables, the actual lookup cos= ts > will dwarf any setup costs. Then still, it's checking the same field tha= t's > used anyway, so the extra cost should be neglible. Sounds good, let=E2=80=99s do that then. >> It might allow us to use =E2=80=98sqlite-exec=E2=80=99 directly, and thus >> benefit from the binding support in there, as in: >>=20 >> (sqlite-exec db "=E2=80=A6 WHERE " id " is NULL or =E2=80=A6") > > I added sqlite-bind-arguments with keyword arguments specifically so sqli= te-exec > doesn't suck. > > So it would be like (sqlite-exec db "SELECT =E2=80=A6 :a =E2=80=A6 :b =E2= =80=A6 :a" > #:a 42 > #:b 2) > > Before, it was: > > (sqlite-exec db "SELECT =E2=80=A6 ? =E2=80=A6 ? =E2=80=A6 ?" > 42 > 2 > 42) Right, but now it=E2=80=99s as I wrote above: you can include arguments in = the middle of the SQL strings, and =E2=80=98sqlite-exec=E2=80=99 takes care of = turning that into question marks and so on: https://git.savannah.gnu.org/cgit/guix/guix-cuirass.git/commit/?id=3Db0c3= 9b31f61cfc494e0dfbe823b3fe4275efbc7a Anyway, we can support both the keyword style you show above, and the other thing I mention, and use whichever is most convenient for the code at hand. I find the =E2=80=98sqlite-exec=E2=80=99 convenient for simple cases where = the query is a literal, but the keyword style might be more convenient for complex queries like =E2=80=98db-get-builds=E2=80=99. Thanks, Ludo=E2=80=99.