Build schema fails for password with semicolon - command injection

[marjus45]

Describe the bug

The test run fails to build the schema when providing password with semicolon (;), e.g. abc;xyz, with the following error message:

Changed tpcc:pg_superuserpass from postgres to abc for PostgreSQL
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 17, in <module>
  File "/home/hammerdb/HammerDB-4.7/lib/tclpy0.4/hammerdb.py", line 51, in diset
    eval_hammerdb_command('diset',*args)
  File "/home/hammerdb/HammerDB-4.7/lib/tclpy0.4/hammerdb.py", line 12, in eval_hammerdb_command
    output = tclpy.eval(to_hdb)
RuntimeError: invalid command name "xyz"

To Reproduce

1. Start a PostgreSQL database and set a user with password abc;xyz
2. Run the docker image from tpcorg/hammerdb

   docker run -it tpcorg/hammerdb bash

3. Edit the scripts/python/postgres/tprocc/pg_tprocc_buildschema.py and set the pg_superuserpass as abc;xyz

   ...
   diset('tpcc','pg_superuserpass','abc;xyz')
   ...

4. Edit any other configuration settings to point to your PostgreSQL database and run the buildschema script

   ./hammerdbcli py auto scripts/python/postgres/tprocc/pg_tprocc_buildschema.py

Expected behavior

I expect for the schema to be build successfully.

HammerDB Version (please complete the following information):

• Version: 4.7
• Build: Release download (digest:sha256:d0df547d32a028a884adc3d7eb39b28e8a074dd42811eea61b4404c186307c63)

HammerDB Interface (please complete the following information):

• CLI

Operating System (please complete the following information):

• Client OS: Container which uses Ubuntu 20.04

Database Server (please complete the following information):

• Database name: PostgreSQL
• Database Release Version: 15.3

Additional context

I narrowed down a bit the problem:

1. Enter hammerdbcli python environment

   root@0db65de51c49:/home/hammerdb/HammerDB-4.7# ./hammerdbcli python

2. Set PostgreSQL variables:

   hammerdb>>>dbset('db','pg')
   Database set to PostgreSQL
   
   hammerdb>>>dbset('bm','TPC-C')
   Benchmark set to TPC-C for PostgreSQL

3. Run the following:

   hammerdb>>>diset('tpcc','pg_superuserpass','abc;xyz')
   Changed tpcc:pg_superuserpass from pass to abc for PostgreSQL
   Traceback (most recent call last):
     File "<stdin>", line 1, in <module>
     File "/home/hammerdb/HammerDB-4.7/lib/tclpy0.4/hammerdb.py", line 51, in diset
       eval_hammerdb_command('diset',*args)
     File "/home/hammerdb/HammerDB-4.7/lib/tclpy0.4/hammerdb.py", line 12, in eval_hammerdb_command
       output = tclpy.eval(to_hdb)
   RuntimeError: invalid command name "xyz"

I also looked a bit further and I noticed that indeed in the tclsh interpreter:

user$ tclsh

% set password abc;xyz
invalid command name "xyz"

Something interesting is that we can set a semicolon as unicode hex:

% set password \x61\x62\x63\x3B\x78\x79\x7A
abc;xyz

[sm-shaw]

Hi, This has been discussed in other discussion topics: https://github.com/TPC-Council/HammerDB/discussions/90 https://github.com/TPC-Council/HammerDB/issues/523 https://github.com/TPC-Council/HammerDB/discussions/349 and is known with a straightforward solution where you want to use special characters in a field.

1. simple cases use: {}
2. more complex cases use: {[ concat {} ]}

This example falls under (1) so wrap the filed with special characters with curly brackets, and it won't be interpreted as a special character.

print("SETTING CONFIGURATION")
dbset('db','pg')
dbset('bm','TPC-C')

diset('connection','pg_host','localhost')
diset('connection','pg_port','5432')
diset('connection','pg_sslmode','prefer')

diset('tpcc','pg_superuser','postgres')
diset('tpcc','pg_superuserpass','{abc;xyz}')
...

Then when you run it, this is what you see.

$ ./hammerdbcli py auto pg_tprocc_run.py
HammerDB CLI v4.8
Copyright (C) 2003-2023 Steve Shaw
Type "help()" for a list of commands
exec(open('pg_tprocc_run.py').read())
hammerdb>>>exec(open('pg_tprocc_run.py').read())
SETTING CONFIGURATION
Database set to PostgreSQL
Benchmark set to TPC-C for PostgreSQL
Value localhost for connection:pg_host is the same as existing value localhost, no change made
Value 5432 for connection:pg_port is the same as existing value 5432, no change made
Value prefer for connection:pg_sslmode is the same as existing value prefer, no change made
Value postgres for tpcc:pg_superuser is the same as existing value postgres, no change made
Changed tpcc:pg_superuserpass from postgres to abc;xyz for PostgreSQL
...

It is the same for the GUI

As discussed in #349 we want to maintain the flexibility in HammerDB for the fields to accept variables, special characters etc, so you can do whatever you want and build hammerdb into complex environments. So where you don't want to do this using the curlcy brackets or more complex {[ concat {} ]} allows us to use pretty much and scenario we want.

[sm-shaw]

As previously discussed by TPC-OSS subcommittee, flexibility in allowing programmability in fields is preferred over preventing special characters being interpreted as workaround of wrapping with {} or {[ concat {} ]} is a straightforward solution once learnt.

[marjus45]

Hi @sm-shaw and sorry for the late reply,

I want to clarify that by this:

flexibility in allowing programmability in fields is preferred over preventing special characters being interpreted as workaround of wrapping with {} or {[ concat {} ]} is a straightforward solution once learnt.

you mean that passing to input fields commands, is considered a feature, right?

I think that considering the history of all major databases with SQL injection (see also https://xkcd.com/327/), and this being a testing tool against those databases, we should make the effort to fix the interpretation of the commands.

If you consider re-opening the issue, I am willing to make a PR to solve this issue. I have already explored a bit, and I can identify that the issue is in the tclpy library, inside the function eval_hammerdb_command. I noticed that this library is not versioned controlled under this repository, thus I would also need to know how to contribute for that library, if possible.

Regarding the fix, I was thinking that we should convert all input for the Tcl interpreter (i.e. the argument on tclpy.eval) to hex strings, since the Tcl interpreter understands them, and this will also solve other command injections that are exposed by other commands, not just diset.

If you still think this is desired, I think that you should acknowledge that on the documentation (i.e. that input fields can also be used for command execution), since this issue will keep being reported, as, in my opinion, it is quite unexpected for a tool running against databases.

[sm-shaw]

It is more a case of this hasn't been considered an important enough issue to work on over and above other areas, as wrapping with {} works in most cases and is straightforward.

As HammerDB is a test tool we have all the source in the src directory that can be modified as desired to do whatever you want, and we also need the system/superuser/root passwords to create our schemas. In these schemas, all the data generated and loaded is random. So SQL injection tends to be a moot point as if you have that level of access to your database to create schemas and run a benchmark driving the CPU to 100% (DoS?) then SQL injection at a password (or other variable) is not going to be the most important consideration. You have the customscript script in the CLI to load whatever you want to run against your database in any case.

Nevertheless, we are certainly not going to discourage any PRs to add features that someone considers important, and I can reopen the issue to see how far you want to take this.

A key question is to what degree additional security features are added and how much these are wanted and/or needed for the majority of users. Rather than converting to Hex wrapping fields we don't want interpreted would be best using {[ concat {} ]} - however this would need adding for both CLI and GUI and testing for all databases. At the most basic level, we don't hide passwords in the GUI - although this really is simple as adding the -entry show "*" option to replace the characters, e.g. https://wiki.tcl-lang.org/page/A+little+login+dialog so is a 2-minute update. However then we store the passwords in text in variables, SQLite and send them over the network so to what extent do we 'harden' the password storage e.g. there is already this that could be adapted https://github.com/zdia/gorilla/blob/master/EuroTcl%202012/PWgorila.pdf but as above, is it needed? Do we spend time here that we don't elsewhere (when in most cases we can use {}).

The Python interface is here https://github.com/sm-shaw/libtclpy

I will also raise this at the TPC-OSS subcommittee to get opinion on how much focus on security is required in the area of database benchmarking. Additional opinion is most definitely welcome.