Possible bug in SQL string escaping logic?

[saurabhnanda]

I got a dump of live data in an HSTORE column from our production DB using:

\copy (select properties from clients where array_length(akeys(properties), 1)>0) to '/tmp/client_properties.txt';

One of the rows in the resulting text file was:

"terms"=>"'No Show\\", 100% of the value of the booking will be charged."

Next, I have a utility function to help me pump all this data into a throwaway table (for testing), only to read it back and test my custom serialisation function, which takes an HStoreList -> CustomDataType:

testHStoreSerialistion :: Connection -> FilePath -> ((Only HStoreList) -> IO ()) -> IO ()
testHStoreSerialistion conn fname assertFn = do
  void $ withTransaction conn $ do
    _ <- execute_ conn "create table if not exists hstore_test(hstore_col hstore)"
    ls <- T.lines <$> T.readFile fname
    mapM_ (\l -> execute conn "INSERT INTO hstore_test(hstore_col) VALUES(?)" (Only l)) ls
    rows <- query conn "select hstore_col from hstore_test" ()
    mapM_ assertFn rows
    execute_ conn "drop table if exists hstore_test;"

The function above generates the following INSERT query:

INSERT INTO hstore_test(hstore_col) VALUES('"terms"=>"''No Show\\", 100% of the value of the booking will be charged."')

which fails with the following error:

Exception: SqlError {sqlState = "XX000", sqlExecStatus = FatalError, sqlErrorMsg = "Syntax error near 'o' at position 28", sqlErrorDetail = "", sqlErrorHint = ""}

No other row fails this round-tripping test. Just this particular row because of the \\" Am I doing something wrong or is there a bug in the SQL quoting in PG-simple?

[lpsmith]

Well, after playing with it a bit, the problem seems to be that you didn't unescape the string that you are supplying to postgresql-simple.

It seems that the COPY command is emitting the extended escape syntax, whereas postgresql-simple uses the standard conforming syntax. (Though we might move to the extended escape syntax, but that wouldn't really help your problem.) With the extended escape syntax, \ must be escaped, whereas with the SQL standard, \ must not be escaped.

[lpsmith]

Incidentally, the extended escape syntax appears to accept '' as a single ' character. You could probably solve this particular case by writing execute conn "INSERT INTO hstore_test(hstore_col) VALUES(E?)" but this is an abuse of postgresql-simple, and probably not a robust solution either.

[lpsmith]

Probably a better way of handling this would be to load the file into your temporary table using a copy command, then retrieve the hstore from postgres via postgresql-simple, and let it handle the unescaping for you.

[saurabhnanda]

Have I written the INSERT query incorrectly? What I gathered from the docs was that query substitution will handle all escaping for me. Also, the value being substituted is a regular Text and not a custom type (for which I may have incorrectly written the escaping logic).

What if this string was generated by user input? Is the way I've written my query not the recommended way to insert user input into the DB?

[saurabhnanda]

What is the underlying function in libpq being user here?

The primary advantage of PQexecParams over PQexec is that parameter values may be separated from the command string, thus avoiding the need for tedious and error-prone quoting and escaping. Unlike PQexec, PQexecParams allows at most one SQL command in the given string. (There can be semicolons in it, but not more than one nonempty command.) This is a limitation of the underlying protocol, but has some usefulness as an extra defense against SQL-injection attacks.

How does one use PQexecParams via pg-simple?

[saurabhnanda]

I re-read the docs and am still wondering why this shouldn't have worked out of the box? At most, it may have resulted in an malformed HSTORE string syntax, but why did this result in a malformed SQL query?

When constructing the real query to execute, these functions replace the first "?" in the template with the first element of the tuple, the second "?" with the second element, and so on. If necessary, each tuple element will be quoted and escaped prior to substitution; this defeats the single most common injection vector for malicious data.

[lpsmith]

It didn't result in a malformed query. That error message is coming from the hstore parser, not the sql parser. There is not an exploitable injection here, though there probably would be if you use the (E?) kludge I tepidly suggested.

Also, you are taking a Text type and treating it as an hstore. All that postgresql warranties in that case is that there is no injection (so long as you don't do anything bad in the template, like that kludge), not that it is valid hstore syntax. Which is what is happening here.

If you generated malformed hstore syntax from the Hstore module, then you would have a case.

You'll have the same problem with execParams, because the copy command is introducing a level of escaping of a variant variety that you aren't undoing. You can use Internal.withConnection to drop down into libpq but this can introduce some subtle issues with other postgresql-simple code (e.g. getNotification).

[lpsmith]

Concretely, the second line is what you should be supplying in this case:

"terms"=>"'No Show\\", 100% of the value of the booking will be charged."
"terms"=>"'No Show\", 100% of the value of the booking will be charged."

[lpsmith]

Also, it might help to consider the multiple levels of escaping/unescaping that postgres performs. First the sql parser parses one of three string syntaxes, with three different unescaping rules, then passes the unescaped result off to the hstore textual format parser. And that has a second layer of unescaping rules, i.e. by escaping " and \ by \.

So in this case, the escape syntax generated by the copy command (which resembles the extended escape syntax) is passing through the postgresql-simple's libpq based escaper and postgres's string parser unchanged, and then misinterpreted by the hstore unescaping rules.

[saurabhnanda]

I've hit a similar issue when doing QuickCheck/Arbitrary testing for HStoreMap in my application.

The following test fails a lot of times:

instance Arbitrary HStoreMap where
  arbitrary = (HStoreMap . Map.fromList) <$> (genericArbitrary uniform)

testHStoreDB :: Connection -> Assertion
testHStoreDB conn = void $ withTransaction conn $ do
  _ <- execute_ conn "create table if not exists hstore_test(hstore_col hstore)"
  h <- generate (arbitrary :: Gen HStoreMap)
  Prelude.putStrLn $ (show h)
  _ <- execute conn "insert into hstore_test(hstore_col) values(?)" (Only h)
  execute_ conn "drop table if exists hstore_test"

Passing case:

  HStore DB:                            HStoreMap {fromHStoreMap = fromList [("\SOH\191\&0]j9\EMJ\141.\218\150\SIj}","4C\201[e\DC42\178\&3\ra\180\217\fg)a>Eh"),("1\137/J\166\EOTB3aa\240(\129(\"x`\216[\SO7\147dEj!\148(\DC1\167","}\184\&9\246Q]s\ESCu\218\130\ESC\ESC\ESCl\141\237\&8\nUWf\f@B"),("AW+\215\196H\139Y*\EOTi\FST\168","L(\132g1#H\246\190\&8\EMGH\SYN\SYNf\197hS&9C"),("K!HO3:K\GSz\NAK#o-K$7g","9=V\\\RS\226M\193\EMp\129\157)o2\198+Hn"),("pMUh\169\246\227\RS[z\ns?Q\DLE\141\EOT\EM\229","G\SI;c\SOH\ETB\EOTN\"\162P\NAK\145n\STXI\b</V")]}
OK

Failing case:

  HStore DB:                            HStoreMap {fromHStoreMap = fromList [("\ACK\SIL\209o\214\191\203&\n\a\147\SYN)VL\239o\178+4\135q\232\&7","\141\249Ms\166v\afYF\ENQo\FS\SI\CAN<A\DC1\b\RS\SYNO\162\f_x"),("\a~V\n\237\219%","\DC2@C\NUL=\170\196\242j6\137.\ACK\255+"),("\tX\178,p\161j","I\bQ:"),("\v\145fQ\176T","%\150\138\232|qy\DLE,fiD[Z\204\168\217\193Z:\241\ETBW\244\149\DC2b"),("\NAK\219wUK\DC4^\152$\246","xm \132)S\DELl=w"),("\ETB={-Z-\145\222\229,\178\223`#\237\FS","k\SO`[X\GS\177\&0a\157\141]\SOH\179-r"),("\SUBud8\143rX\NAKK\CANP\149O=>\185\201*a&","?\213\a\180!\bp\EMx\DC4\DELd\GSm\131\194\170sZ`"),("\US\130M\162S9zG\156\238\n(\ACK&$\180B,O5","#V\170a["),(" \ESC\214\146=\ENQq\204\NULv[6\SYN\NUL\245\ESC\ACKS5\DC3%","!7et{o\SIk\"\EOT"),("#\202ZC\SUBk","x\f%Gi|\NAKcjE\140\174|i\166|g%*\"\235\182y\ETBs"),("/\154lyMjr\NUL\222\n\ETX\205\&7\DC49\244i\232#T\148\195\r\DC4\161&\176n",".\222\196G\ETXM\GS0`\CAN_"),("41\128\132%KT\ACK\tTx\"\rN","OG\DC4\NUL.\220\214\206\250uW"),("@JF\DC2\136\189\211R(A1\149\137<oO\216h\161\167z\217\134\rs\165","^2"),("@\215u=$_*","cn\DC3s"),("A\US\151\v:\254\140/\204w\204w\221`\RS","'\154\195Gv\EMf[C\155\&7ykU\SI:5"),("I","\ENQ\ny\159\SI\RS\NAK`IOf+\169\239\EM\234m"),("L\243~x&\239SP#h\212\t\SI","&\195Vo!,U0\ETB\225Z\rlsO\188G/B1Y\251\204\&9~\210"),("O\142\195am\140>\RS\203\163\178O$\167\a\SYNP\167\n]\CAN~hnr\DEL5","=\193\n1\SUBdw"),("jcGJM^\143q\"2>it","Pt\DC4\156U\211:AP\209\&3Px\130\&7#\EOT\139JO\180\DC4\a"),("k<\FS\\#F","k"),("mY\STXF\SI","V\130"),("ue\169\237oz\224F\SOM\SOH2\149\v\"tKn\132{\203\&6\250r6","R\240+\169x\237;\ESC24cB9Ew\250=\231z_+\US_\223,n\222"),("\186\DC4\159\186x|\DC3 -\tH\173s\CAN\ESCeHv*Z!(\129v\DC3\n","u\241y/{\ACK[XK\130\"\171\DC3LQ\CAN\DEL"),("\208Vq\"t\178k4g@\185_\192\FS","U\224\185!\DEL\"@\217\195a=\NAK\133\166\DLE\aFv\181\t~g\SUBQIUU\f"),("\212\207H\212I3\DC3\212rv;VV\254\219","\189\207\ETXV"),("\251V\ETX","\DC44hTu\175N\138\154\224w")]}
FAIL
    Exception: SqlError {sqlState = "XX000", sqlExecStatus = FatalError, sqlErrorMsg = "Unexpected end of string", sqlErrorDetail = "", sqlErrorHint = ""}


--- From the Postgres logs:

ERROR:  Unexpected end of string at character 44
STATEMENT:  insert into hstore_test(hstore_col) values('"LÑoÖ¿Ë&
	�)VLïo²+4�qè7"=>"�ùMs¦vfYFo<O¢
                                      _x","~V
	íÛ%"=>"@C')

[saurabhnanda]

@lpsmith would you be open to including Quickcheck in any one of the PRs that I've submitted? I'd like to put this test in the upstream test-suite to rule out any issue withthe pg-simple library.

[saurabhnanda]

@lpsmith I'm also investigating whether the Text values being generated in [(Text, Text)] passed to HStoreMap are legal or not - haskellari/qc-instances#9

[lpsmith]

Adding quickcheck to the test suite is quite acceptable. That failing log message I do find to be rather curious. Let me know when you figure something out.

[lpsmith]

Also, if you just want to round-trip hstore syntax through postgres and postgresql-simple, you can write values (?::hstore) as the query template instead of inserting it into a table.

[saurabhnanda]

Adding quickcheck to the test suite is quite acceptable. That failing log message I do find to be rather curious. Let me know when you figure something out.

Which of the two PRs is the way forward? I'm voting for filtering out NULL values when populating the HStoreList or HStoreMap. Your thoughts?

Also, if you just want to round-trip hstore syntax through postgres and postgresql-simple, you can write values (?::hstore) as the query template instead of insering it into a table.

Thanks for the tip! Did you mean to say, simply do SELECT (?::hstore) and read the value back?

[saurabhnanda]

@lpsmith I've modified the tests and implemented a shrink instance to come up with a minimal failing test case:

instance Arbitrary HStoreMap where
  arbitrary = (HStoreMap . fromList) <$> (genericArbitrary uniform)
  shrink h = case (toList $ fromHStoreMap h) of
    [(_)] -> [] -- if it's a single element hstore then we cannot shrink any further
    l -> Prelude.map (\x -> HStoreMap $ fromList [x]) l

propHStoreDB :: Connection -> HStoreMap -> Property
propHStoreDB conn h= monadicIO $ do
  (Only h_):_ <- run $ query conn "select (?::hstore)" (Only h)
  QCM.assert (h==h_)

Here are some minimal failing test cases:

*** Failed! Exception: 'SqlError {sqlState = "XX000", sqlExecStatus = FatalError, sqlErrorMsg = "Unexpected end of string", sqlErrorDetail = "", sqlErrorHint = ""}' (after 9 tests and 1 shrink): 
HStoreMap {fromHStoreMap = fromList [("\NUL%mS\146","{\174A<")]}

--- PG log

ERROR:  Unexpected end of string at character 9
STATEMENT:  select ('"'::hstore)
ERROR:  Unexpected end of string at character 9
STATEMENT:  select ('"'::hstore)

Another test case:

*** Failed! Exception: 'SqlError {sqlState = "XX000", sqlExecStatus = FatalError, sqlErrorMsg = "Unexpected end of string", sqlErrorDetail = "", sqlErrorHint = ""}' (after 10 tests and 1 shrink): 
HStoreMap {fromHStoreMap = fromList [("Hb$'\155\DC3\NUL","Y\SOH\a\245\231Wf ")]}

--- PG Log

ERROR:  Unexpected end of string at character 9
STATEMENT:  select ('"._2"=>"G","HB¬\"#?cÙ"=>"cóG2æ","Hb$''�'::hstore)
ERROR:  Unexpected end of string at character 9
STATEMENT:  select ('"Hb$''�'::hstore)

@lpsmith will you have time to replicate these test cases and confirm that something indeed is going wrong in the HStore => SQL conversion?

[saurabhnanda]

Is it the \NUL that is throwing the HStoreBuilder off?

[saurabhnanda]

@lpsmith Based on further investigation (given below), now I'm fairly convinced that there is some bug in the HSoreMap -> SQL conversion infrastructure.

Similar data for JSON works perfectly fine, whereas it fails for HSTORE.

instance Arbitrary HStoreMap where
  arbitrary = do
    k <- (arbitrary :: Gen Text)
    v <- (arbitrary :: Gen Text)
    return $ HStoreMap $ fromList [(k, v)]
    
instance Arbitrary Value where
  arbitrary = do
    k <- (arbitrary :: Gen Text)
    v <- (arbitrary :: Gen Text)
    return $ object [ k .= v ]

propHStoreDB :: Connection -> HStoreMap -> Property
propHStoreDB conn h= monadicIO $ do
  (Only h_):_ <- run $ query conn "select (?::hstore)" (Only h)
  QCM.assert (h==h_)

propJsonDB :: Connection -> Value -> Property
propJsonDB conn j = monadicIO $ do
  (Only j_):_ <- run $ query conn "select (?::json)" (Only j)
  QCM.assert (j==j_)

Output:

� quickCheck (propJsonDB conn)
+++ OK, passed 100 tests.
� quickCheck (propJsonDB conn)
+++ OK, passed 100 tests.
� quickCheck (propJsonDB conn)
+++ OK, passed 100 tests.
� quickCheck (propHStoreDB conn)
*** Failed! Exception: 'SqlError {sqlState = "XX000", sqlExecStatus = FatalError, sqlErrorMsg = "Unexpected end of string", sqlErrorDetail = "", sqlErrorHint = ""}' (after 19 tests): 
HStoreMap {fromHStoreMap = fromList [(")\240\215\157\219?\ETBy\150:c\DEL\r\b \253","\215UROW\NUL\131>,\b\FS\137]\239\CAN\150'\226")]}
� quickCheck (propHStoreDB conn)
*** Failed! Exception: 'SqlError {sqlState = "XX000", sqlExecStatus = FatalError, sqlErrorMsg = "Unexpected end of string", sqlErrorDetail = "", sqlErrorHint = ""}' (after 26 tests): 
HStoreMap {fromHStoreMap = fromList [("\157\202a#\199\ENQ","\233r5\212R\160\166E7\223H\t\GS^\RS\NUL+\ESCj\a\DC4")]}
� ```

[saurabhnanda]

@lpsmith should we be reusing this logic from Aeson -- https://github.com/bos/aeson/blob/2bc6f22f164d172d0d77d878422b67433a57a9d8/Data/Aeson/Encoding/Builder.hs#L121-L142

[saurabhnanda]

@lpsmith your suggested way forward?

[ldicarlo]

Hi @lpsmith, I also have this problem.
QuickCheckgenerates strings like ( '\NUL' : "HELLO" ) so \NULHELLO and when inserting, everything after the \NUL char, including the \NUL char is removed.
(What's before is not removed)
Could it come from here ?
Like if \NUL was considered as an EndOfInput ?