Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL escape passwords #356

Merged
merged 1 commit into from
Apr 27, 2021

Conversation

grooverdan
Copy link
Member

To correctly SQL escape passwords, escaping \ first is
required. Then we need to escape ' in the password to
prevent it being treated as a end of SQL statement quote.

All escaping needs to use , so we cannot be in
NO_BACKSLASH_ESCAPES sql_mode otherwise no escaping will
work.

As an added complication when using the config file within
the entrypoint, for waiting until MariaDB can initialize,
a password mechanism a different escaping applies
(https://mariadb.com/kb/en/configuring-mariadb-with-option-files/).

The printf %q is close, however it provides a much
richer escape of non-printable characters than what can be
read in the configuration file. As such the password
complexity of the root password is limited to escaping
\n, \r, \t, \b, \s, ", ', and \ while for a user
MARIADB_PASSWORD anything, including positively crazy strings
such as \0 will work.

Closes #183

@grooverdan
Copy link
Member Author

todo: look at https://github.com/grooverdan/mariadb/runs/2092312601?check_suite_focus=true fails CI there but not on the PR here.

Copy link
Contributor

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General idea seems sane (and honestly shouldn't be too surprising to the few users it might break, IMO -- they had to have known they were skating on thin ice by using a password that required escaping 😬).

# default root to listen for connections from anywhere
if [ -n "$MARIADB_ROOT_HOST" ] && [ "$MARIADB_ROOT_HOST" != 'localhost' ]; then
# no, we don't care if read finds a terminating character in this heredoc
# https://unix.stackexchange.com/questions/265149/why-is-set-o-errexit-breaking-this-read-heredoc-expression/265151#265151
read -r -d '' rootCreate <<-EOSQL || true
CREATE USER 'root'@'${MARIADB_ROOT_HOST}' IDENTIFIED BY '${MARIADB_ROOT_PASSWORD}' ;
CREATE USER 'root'@'${MARIADB_ROOT_HOST}' IDENTIFIED BY '${rootPasswordEscaped}' ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this one also need the NO_BACKSLASH_ESCAPES you added below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its in the same session where rootCreate is used.

docker_process_sql --database=mysql <<<"CREATE USER '$MARIADB_USER'@'%' IDENTIFIED BY '$MARIADB_PASSWORD' ;"
# SQL escape the user password, \ followed by '
local userPasswordEscaped=${MARIADB_PASSWORD//\\/\\\\}
userPasswordEscaped=${userPasswordEscaped//\'/\\\'}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it might be worthwhile to make a new function for this? 😇

Comment on lines 276 to 299
# we need the SQL_MODE NO_BACKSLASH_ESCAPES mode to be clear for the password to be set.
# --binary-mode to save us from the semi-mad users who put nulls into passwords
docker_process_sql --dont-use-mysql-root-password --database=mysql --binary-mode <<-EOSQL
-- What's done in this file shouldn't be replicated
-- or products like mysql-fabric won't work
SET @@SESSION.SQL_LOG_BIN=0;
SET @@SESSION.SQL_MODE=REPLACE(@@SESSION.SQL_MODE, 'NO_BACKSLASH_ESCAPES', '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's legal to have comments in SQL, do you think it's worth bringing the comment a bit closer to what it's about? ala:

Suggested change
# we need the SQL_MODE NO_BACKSLASH_ESCAPES mode to be clear for the password to be set.
# --binary-mode to save us from the semi-mad users who put nulls into passwords
docker_process_sql --dont-use-mysql-root-password --database=mysql --binary-mode <<-EOSQL
-- What's done in this file shouldn't be replicated
-- or products like mysql-fabric won't work
SET @@SESSION.SQL_LOG_BIN=0;
SET @@SESSION.SQL_MODE=REPLACE(@@SESSION.SQL_MODE, 'NO_BACKSLASH_ESCAPES', '');
# --binary-mode to save us from the semi-mad users who put nulls into passwords
docker_process_sql --dont-use-mysql-root-password --database=mysql --binary-mode <<-EOSQL
-- What's done in this file shouldn't be replicated
-- or products like mysql-fabric won't work
SET @@SESSION.SQL_LOG_BIN=0;
-- we need the SQL_MODE NO_BACKSLASH_ESCAPES mode to be clear for the password to be set
SET @@SESSION.SQL_MODE=REPLACE(@@SESSION.SQL_MODE, 'NO_BACKSLASH_ESCAPES', '');

[client]
password="${MARIADB_ROOT_PASSWORD}"
EOF
echo -e "[client]\npassword=$(printf \"%q\" "${MARIADB_ROOT_PASSWORD}")\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably worth just bringing completely into the printf, ala:

Suggested change
echo -e "[client]\npassword=$(printf \"%q\" "${MARIADB_ROOT_PASSWORD}")\n"
printf '[client]\npassword=%q\n' "$MARIADB_ROOT_PASSWORD"
$ printf '[client]\npassword=%q\n' "foo bar baz"
[client]
password=foo\ bar\ baz

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll need to check how MariaDB processes escapes on non-quoted config items.

I would of hoped for a [ -S "${SOCKET}" ] however it seems there's a little too much between listen and the poll/select loop.

@grooverdan
Copy link
Member Author

After far too long looking at this:

  • yes, echo -e isn't needed and the printf %q output is the same
  • quoting and without quotes, single or double, in .my.cnf has the same effect, except obviously that only the inside the quotes it taken as the string.
  • .my.cnf is parsing highly intolerant of \ that printf %q generates.
  • printf %q has two output forms - $'......' - remove the $ and it can be passes as a password='......', and "aa\ \"\ a" where \ needs to go to , what other whitespace is quoted this way? ❓ no idea. I think this was my intent with echo -e except it converts too much back, like \r '\n` etc.

The '\x00' character will confuse printf %q, escaping in ESC below will generate some unexpected output, AND any tempt to pass a \x00 containing bash variable to a process will terminate at that character - (strace -fe trace=network -s99 -p $(pidof mysqld) with mysql -u root <<<$'CREATE OR REPLACE USER bob@localhost IDENTIFIED BY \'rr\x00My#1PasswordIsSecure\''. In the entrypoint read -d '' is always \x00 terminated. read -N will stop an '\x00' before EOF (so the bash man pages lies).

So any handling of '\x00' in bash is for the totally mad.

P=$'\'\\aa-\x00-zz"_%'; ESC="${P//\\/\\\\}"; ESC="${ESC//\'/\\\'}"; od -h <<<"$ESC" ; echo LENP=${#P} LENESC=${#ESC}; printf ESC=%q "${ESC}" ; mysql -u root  -p"${MARIADB_ROOT_PASSWORD}" <<<"create or replace user bob@localhost identified by '$ESC';";  mysql -u bob -p"${P}" -e "select 1" ; printf "[client]\npassword=%q\n" "$P" | tee ~/.my.cnf ; mysql -u bob -e 'select 1'; echo "\"$(printf "%q" "$P")\"" 
0000000 275c 5c5c 6161 0a2d
0000010
LENP=5 LENESC=7
ESC=\\\'\\\\aa-+---+
| 1 |
+---+
| 1 |
+---+
[client]
password=\'\\aa-
+---+
| 1 |
+---+
| 1 |
+---+
"\'\\aa-"

ref: ansi string https://wiki.bash-hackers.org/syntax/quoting#ansi_c_like_strings

To correctly SQL escape passwords, escaping \ first is
required. Then we need to escape ' in the password to
prevent it being treated as a end of SQL statement quote.

All escaping needs to use \, so we cannot be in
NO_BACKSLASH_ESCAPES sql_mode otherwise no escaping will
work.

Getting bash to escape the root password for its
internal uses and in a form that was recognized
by MariaDB reading the configuration file was
becoming exceptionally complicated, and unsuccessful.

As such we use the MYSQL_PWD environment variable
to contain the password when needed.

Occasionally the socket wasn't ready on a clean install
by the time docker_process_sql was called so allow
up to 5 seconds just in case.

example logs of this:
2021-03-16 00:41:21+00:00 [Note] [Entrypoint]: Waiting for server startup
2021-03-16  0:41:21 140680894076608 [Note] mysqld (mysqld 10.2.37-MariaDB-1:10.2.37+maria~bionic) starting as process 111 ...
2021-03-16 00:41:21+00:00 [Note] [Entrypoint]: Temporary server started.
2021-03-16  0:41:21 140680894076608 [Note] InnoDB: Mutexes and rw_locks use GCC atomic builtins
2021-03-16  0:41:21 140680894076608 [Note] InnoDB: Uses event mutexes
2021-03-16  0:41:21 140680894076608 [Note] InnoDB: Compressed tables use zlib 1.2.11
2021-03-16  0:41:21 140680894076608 [Note] InnoDB: Using Linux native AIO
2021-03-16  0:41:21 140680894076608 [Note] InnoDB: Number of pools: 1
2021-03-16  0:41:21 140680894076608 [Note] InnoDB: Using SSE2 crc32 instructions
ERROR 2002 (HY000): Can't connect to local MySQL server through socket '/var/run/mysqld/mysqld.sock' (2)

Closes MariaDB#183
@grooverdan grooverdan force-pushed the issue-183-quoting-passwords branch from 7c0795b to 8a481e5 Compare March 16, 2021 02:05
@grooverdan grooverdan self-assigned this Apr 23, 2021
@grooverdan grooverdan merged commit 58f4020 into MariaDB:master Apr 27, 2021
@grooverdan grooverdan deleted the issue-183-quoting-passwords branch April 27, 2021 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[issue] entrypoint script does not escape password properly
2 participants