Prevent root password from being accessible by normal users on first run #53

ltangvald · 2015-03-06T10:07:15Z

The first time the server was started, the file containing the root password in plaintext would be readable by any user on the database (and the filename listed in the variable list).

We changed the first run to do the initialization separately, then restart the server.

thaJeztah · 2015-03-06T10:23:49Z

5.5/docker-entrypoint.sh

 	fi
-
-	chown -R mysql:mysql "$DATADIR"


Permissions are now only set during initialization. Would this be a problem when using an already initialized data-dir (e.g. bind-mounted)?

Yes, it almost certainly will. The line should have been copied instead of moved. Thanks :)

md5 · 2015-03-08T05:06:59Z

If I understand this correctly, the issue is that the file /tmp/mysql-first-time.sql can be read by any user who can connect to the database by using LOAD DATA INFILE. In order to do that, the user would have to have the FILE permission granted on some table in the database. Is that right?

I tried changing the permissions on the file to 0400 in the hope that MySQL would read it as root before dropping privileges to the mysql user, but that was not the case.

One thing I don't like about the entrypoint changes is the fact that mysqld is started and backgrounded in the middle of the initialization. It would be good if there were a more graceful way to achieve the same fix.

On a side note, it looks like the branch you're using has other changes in it that are unrelated to the password fix (e.g. the removal of Perl for the 5.7 Dockerfile). Those changes should probably go in a separate PR.

md5 · 2015-03-08T05:08:15Z

P.S. It's really cool to see this PR coming from the mysql/docker repo. It would be great to see more official images taken over by the upstream teams.

ltangvald · 2015-03-09T07:56:35Z

Is the desired procedure to only have a single commit, or just no unrelated features? The perl dependency change could be removed, but the others are bug fixes.

With regards to the server run in init, I agree it's fairly ugly, but don't really know a more graceful way to do it. We do have suggestions on features that would help, such as single user mode, but the server doesn't currently support it.

md5 · 2015-03-09T16:48:30Z

I mainly suggested breaking out the Perl change because it looks like something that can be merged without much discussion, whereas I think sorting out this MYSQL_ROOT_PASSWORD thing may take a bit more time (given stuff like the backgrounded mysqld).

If you think any of the other fixes are unambiguous bugs that can be easily merged after a short discussion, I'd probably break those into their own PRs too (or possibly combine them with the Perl change).

ltangvald · 2015-03-10T06:34:53Z

The bugfixes are related to the security issue, so they need to be included. We'll separate the Perl change, though.

ltangvald · 2015-03-11T07:44:27Z

Removed the perl change.

While the system of starting and restarting the server is clunky, it does have the benefit of making it easier to support features such as those requested in issue #24 and pull request #18 without using --bootstrap or --init-file, which have their own problems.

tianon · 2015-03-11T18:40:04Z

5.5/docker-entrypoint.sh

+    local section=$1
+    local option=$2
+    local default=$3
+    ret=$(/usr/local/mysql/bin/my_print_defaults $section | grep '^--'${option}'=' | cut -d= -f2-)


Why not just rely on the PATH for finding the right my_print_defaults binary?

Yes, that should work better.

tianon · 2015-03-11T18:41:36Z

+1 to being able to solve things like #24 in a clean way -- this was the solution we were considering for that, but were having trouble bringing ourselves to actually start up and background mysqld just to talk to it for a little bit, shut it down, and then start it back up again.

tianon · 2015-03-11T18:42:23Z

The whitespace in this PR seems to be all over the place -- some places it's a tab (like it was originally in these files), but some places it's a 4-space indent or stranger incantations. 😄

ltangvald · 2015-03-12T14:39:47Z

Whitespace issue is my bad. Managed to set up my editor and git so I missed the issue :)

tianon · 2015-04-02T16:36:32Z

5.5/docker-entrypoint.sh

-
+	SOCKET=$(get_option  mysqld socket "/tmp/mysql.sock")
+	HOSTNAME=$(hostname)
+	PIDFILE=$(get_option mysqld pid-file "$DATADIR/$HOSTNAME.pid")


What I meant with my comment on this line before is why are we bothering with HOSTNAME here at all? The "hostname" in our container is likely to be something random like 9d3cb2141e44, so why not just go with something like mysql.pid and make sure it's deleted appropriately before we start up mysqld?

Yeah, it's probably a better idea to specify the pid-file instead of doing it like this.

ltangvald · 2015-04-14T12:43:37Z

The 5.7.6+ issue wasn't given very high priority before we started the work with Docker, but it's a bigger problem here because of the way we use the --verbose --help command to fetch options. We're looking into ways to handle it, but at least it's not an issue on Debian right now :)

tianon · 2015-06-02T22:30:35Z

Good enough for me -- let's get this in; sorry for the delay!

LGTM

yosifkit · 2015-06-02T23:44:48Z

Can you rebase so that the pretty new tests can run (#73)? Just a git pull --rebase https://github.com/docker-library/mysql.git master and then git push -f.

ltangvald · 2015-06-03T06:33:04Z

Will do

- File with root password in plaintext was readable from database the first time server was run

* Removed hardcoded path from my_print_defaults command * Moved command for setting datadir ownership to before server is started

* Removed unnecessary folder check * Specify pid-file for 5.5 instead of using hostname * Change server shutdown check to be more reliable (socket is closed before all resources are released, which could lead to issues)

ltangvald · 2015-06-03T08:24:56Z

A couple of things to note:

Adding --innodb-read-only to the --verbose --help command prevents it from writing to an empty datadir, should this become an issue on Debian as well.
We're working on adding functionality that will remove the need for this whole -start-stop-check-if-stopped weirdness, so should be able to improve this in the future :)

tianon · 2015-06-03T20:06:01Z

LGTM

(YOU TEASE -- looking forward to something better than our start-stop hackery 👍)

tianon · 2015-06-03T20:06:14Z

Also, green tests are 💚.

yosifkit · 2015-06-03T20:06:38Z

LGTM: https://travis-ci.org/docker-library/mysql/jobs/65208290#L611 😃

Prevent root password from being accessible by normal users on first run

thaJeztah reviewed Mar 6, 2015
View reviewed changes

ltangvald mentioned this pull request Mar 6, 2015

Provide mechanism to extend mysql image with additional SQL init scripts #24

Closed

ltangvald force-pushed the master branch from d0fd702 to 42d6d5a Compare March 10, 2015 10:08

tianon reviewed Mar 11, 2015
View reviewed changes

tianon reviewed Apr 2, 2015
View reviewed changes

ltangvald added 3 commits June 3, 2015 08:42

Fixed security issue

b5ca4c2

- File with root password in plaintext was readable from database the first time server was run

Tell mysql_install_db where mysqld is located

6e9444e

Changed some paths to reflect how 5.5 is set up differently

8e2518a

ltangvald added 4 commits June 3, 2015 08:44

Ensure correct ownership of an existing datadir

355b0d2

* Fix whitespace inconsistencies

3e88d68

* Removed hardcoded path from my_print_defaults command * Moved command for setting datadir ownership to before server is started

Fix variable quoting inconsistencies

b141456

Various fixes.

ae647b4

* Removed unnecessary folder check * Specify pid-file for 5.5 instead of using hostname * Change server shutdown check to be more reliable (socket is closed before all resources are released, which could lead to issues)

ltangvald force-pushed the master branch from 4a68bb5 to ae647b4 Compare June 3, 2015 08:09

Removed workaround that isn't needed on current Debian image

0fd8c68

tianon added a commit that referenced this pull request Jun 3, 2015

Merge pull request #53 from mysql/master

f1b05ac

Prevent root password from being accessible by normal users on first run

tianon merged commit f1b05ac into docker-library:master Jun 3, 2015

yosifkit mentioned this pull request Jun 3, 2015

update mysql with better startup script docker-library/official-images#784

Merged

edmorley mentioned this pull request Jun 11, 2015

Make get_option() handle duplicate options in my_print_defaults output #75

Merged

tianon mentioned this pull request Jul 23, 2015

Improve initdb logic to properly source *.sql files as well, without the use of --single docker-library/postgres#75

Merged

Prevent root password from being accessible by normal users on first run #53

Prevent root password from being accessible by normal users on first run #53

Uh oh!

Conversation

ltangvald commented Mar 6, 2015

Uh oh!

thaJeztah Mar 6, 2015

Choose a reason for hiding this comment

Uh oh!

ltangvald Mar 6, 2015

Choose a reason for hiding this comment

Uh oh!

md5 commented Mar 8, 2015

Uh oh!

md5 commented Mar 8, 2015

Uh oh!

ltangvald commented Mar 9, 2015

Uh oh!

md5 commented Mar 9, 2015

Uh oh!

ltangvald commented Mar 10, 2015

Uh oh!

ltangvald commented Mar 11, 2015

Uh oh!

tianon Mar 11, 2015

Choose a reason for hiding this comment

Uh oh!

ltangvald Mar 12, 2015

Choose a reason for hiding this comment

Uh oh!

tianon commented Mar 11, 2015

Uh oh!

tianon commented Mar 11, 2015

Uh oh!

ltangvald commented Mar 12, 2015

Uh oh!

tianon Apr 2, 2015

Choose a reason for hiding this comment

Uh oh!

ltangvald Apr 7, 2015

Choose a reason for hiding this comment

Uh oh!

ltangvald commented Apr 14, 2015

Uh oh!

tianon commented Jun 2, 2015

Uh oh!

yosifkit commented Jun 2, 2015

Uh oh!

ltangvald commented Jun 3, 2015

Uh oh!

ltangvald commented Jun 3, 2015

Uh oh!

tianon commented Jun 3, 2015

Uh oh!

tianon commented Jun 3, 2015

Uh oh!

yosifkit commented Jun 3, 2015

Uh oh!

Uh oh!