SQL Injection in insert statements

[Karkunow]

Hello!

I've been using HoneySQL for a few years on a project. And yesterday I've found this error:

1. Assume you have some insertion like this, where name and enabled variables come directly from the user HTTP-request:

 (sql/format {:insert-into :table
              :values [{:name name
                        :enabled enabled}]})

2. Now, if someone will try and pass through HTTP-request non-boolean value into the enabled field like this:
   [true, "); SELECT case when (SELECT current_setting('is_superuser'))='off' then pg_sleep(0.2) end; -- "]
   They will be able to execute SQL injection, because that will be inlined directly into query, instead of being separated as a parameter.

So, it seems to me that there is a bug in seq->sql function in format.cljc, which probably treats that case as an alias or something.

Appreciate any help on this issue! Thank you!

[seancorfield]

I'm working on v2 at the moment and that is not an injection problem there:

honey.sql=> (let [name "Name" enabled? [true, "); SELECT case when (SELECT current_setting('is_superuser'))='off' then pg_sleep(0.2) end; -- "]]
       #_=>   (format {:insert-into :table
       #_=>                       :values [{:name name
       #_=>                                      :enabled enabled?}]})
       #_=>   )
["INSERT INTO table (name, enabled) VALUES (?, (TRUE, ?))" "Name" "); SELECT case when (SELECT current_setting('is_superuser'))='off' then pg_sleep(0.2) end; -- "]
honey.sql=>

Note the (TRUE, ?) so the attempted injection goes into a parameter so it is safe (and the insert will fail).

However, on 1.0, you are correct that it a bug:

dev=> (require '[honeysql.core :as sql])
nil
dev=> (let [name "Name" enabled? [true, "); SELECT case when (SELECT current_setting('is_superuser'))='off' then pg_sleep(0.2) end; -- "]]
(sql/format {:insert-into :table :values [{:name name :enabled enabled?}]}))
["INSERT INTO table (name, enabled) VALUES (?, TRUE ); SELECT case when (SELECT current_setting('is_superuser'))='off' then pg_sleep(0.2) end; __ )" "Name"]
dev=> 

Since this is a security issue, I will take a look and fix it on the 1.0 branch in the next few days. Thanks for the report!

[Karkunow]

@seancorfield thank you! appreciate your help!

[Karkunow]

@seancorfield could you please tell me when this bug is expected to be resolved? thank you!

[seancorfield]

@Karkunow I was sick last week (horrible reaction to a shingles vaccination!) but I'm working on it today, sanctioned by my employer.

[Karkunow]

@seancorfield sorry to hear that! hope you're feeling good now!
Thank you for the fix! It is really on time for my project :)

[viebel]

@seancorfield What king of safety does V2 provide in terms of SQL injection?

[seancorfield]

@viebel All literal values in the DSL expression should be lifted out to parameters (unless you deliberately use :inline or :raw). Then the JDBC library you use -- next.jdbc or clojure.java.jdbc -- will create a PreparedStatement from the SQL (which will have ? placeholders for all those parameters) and then it will "set" each parameter value into that PreparedStatement before execution. If anyone finds a situation where that's not the case, I would consider it an important bug and fix it a.s.a.p.