Skip to content

MDEV-28823 Secure mariadb-secure-installation output file with chmod #4016

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

Open
wants to merge 1 commit into
base: 10.6
Choose a base branch
from

Conversation

akshatnehra
Copy link
Contributor

@akshatnehra akshatnehra commented Apr 25, 2025

Description

This commit addresses a security issue in the mariadb-secure-installation script where the temporary output file containing SQL commands and potentially password hashes was being created with default permissions (typically world-readable).

The fix involves modifying the prepare() function to:

  1. Create the $output file explicitly using touch before it's used
  2. Apply umask 0077 to restrict access to owner only before file creation

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

How can this PR be tested?

  1. Run the original mariadb-secure-installation script and observe file permissions:

    ./mariadb-server/scripts/mysql_secure_installation.sh --socket=/tmp/mysql.sock --basedir=/quick-rebuilds/build
    # In another terminal
    ls -la .my* | grep -v .mysql_history

    The .my.output.* file will have -rw-r--r-- permissions

  2. With the patched version:

    ./mariadb-server/scripts/mysql_secure_installation.sh --socket=/tmp/mysql.sock --basedir=/quick-rebuilds/build
    # In another terminal 
    ls -la .my* | grep -v .mysql_history

    The .my.output.* file will have -rw------- permissions

Results from my testing

  1. Before changes

    root@03b5517f4303:/quick-rebuilds# ls -la .my* | grep -v .mysql_history
    -rw------- 1 root root  70 Apr 22 16:55 .my.cnf.15643
    -rw-r--r-- 1 root root 130 Apr 22 16:55 .my.output.15643
    -rw------- 1 root root  32 Apr 22 16:55 .mysql.15643
    
  2. After Changes

    root@03b5517f4303:/quick-rebuilds# ls -la .my* | grep -v .mysql_history
    -rw------- 1 root root  70 Apr 22 17:04 .my.cnf.16290
    -rw------- 1 root root 130 Apr 22 17:04 .my.output.16290
    -rw------- 1 root root  32 Apr 22 17:04 .mysql.16290
    

Basing the PR against the correct MariaDB version

  • This is a security fix and the PR is based against `10.6` branch.

PR quality check

  • I have checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@CLAassistant
Copy link

CLAassistant commented Apr 25, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
9 out of 21 committers have signed the CLA.

✅ grooverdan
✅ FooBarrior
✅ svoj
✅ ayurchen
✅ janlindstrom
✅ ParadoxV5
✅ tonychen2001
✅ cvicentiu
✅ hemantdangi-gc
❌ dr-m
❌ Thirunarayanan
❌ sanja-byelkin
❌ mariadb-YuchenPei
❌ sysprg
❌ vuvova
❌ montywi
❌ holyfoot
❌ luiscontrerasdb
❌ akshatnehra
❌ vlad-lesin
❌ midenok
You have signed the CLA already but the status is still pending? Let us recheck it.

@svoj svoj added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Apr 25, 2025
@grooverdan
Copy link
Member

Thanks for the fix. I'm thinking the original premise of touch/chmod is incorrect. There there's a very small race condition where the file can be opened, before the chmod is applied, and then read when its its later populated.

Probably best fixed with umask and omit chmod. Does that sound better? As security sensitive fix, a rebase back to the 10.6 branch is ok and edit this PR (near title) to target 10.6 branch.

@akshatnehra
Copy link
Contributor Author

akshatnehra commented Apr 28, 2025

Thanks for the fix. I'm thinking the original premise of touch/chmod is incorrect. There there's a very small race condition where the file can be opened, before the chmod is applied, and then read when its its later populated.

Probably best fixed with umask and omit chmod. Does that sound better? As security sensitive fix, a rebase back to the 10.6 branch is ok and edit this PR (near title) to target 10.6 branch.

Thanks for reviewing my PR and providing suggestion, I will make the required changes soon.

@ottok
Copy link
Contributor

ottok commented May 1, 2025

Side note: I believe this script is a historical artifact from 20+ years ago and no longer serves any purpose. Anyone installing MariaDB will have a secure installation out-of-the-box. We should not perpetuate any assumptions that the default installation is somehow insecure and running this script somehow helps, as it does not. This whole script could probably be replaced with an echo statement saying that "MariaDB is secure by default and allows only a local root user to access it. To create a database and a user for an application, and to allow MariaDB to accept connections over the network and to have all connections TLS encrypted, please see tutorial at https://...".

@grooverdan
Copy link
Member

Side note: I believe this script is a historical artifact from 20+ years ago and no longer serves any purpose. Anyone installing MariaDB will have a secure installation out-of-the-box. We should not perpetuate any assumptions that the default installation is somehow insecure and running this script somehow helps, as it does not. This whole script could probably be replaced with an echo statement saying that "MariaDB is secure by default and allows only a local root user to access it. To create a database and a user for an application, and to allow MariaDB to accept connections over the network and to have all connections TLS encrypted, please see tutorial at https://...".

I don't disagree. Attempts and making it non-breaking where apparently too invasive (bb-10.4-anel-mysql-secureinstall)

Fix security issue where temporary output files containing SQL commands
and password hashes were created with default permissions (world-readable).
Modified prepare() function to create and umask the $output $config and
$command files. It is more secure than chmod as files can be opened, before
the chmod is applied, and then read when its its later populated.

All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
@akshatnehra akshatnehra changed the base branch from main to 10.6 May 21, 2025 19:01
@akshatnehra
Copy link
Contributor Author

Thank you Otto for the detailed explanation about the script's historical context. I have made the requested changes - using umask instead of touch/chmod to avoid the race condition, and rebased the PR to target the 10.6 branch as this is a security fix. I apologize for the delay in making these updates. Please let me know if any further changes are needed. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.
Development

Successfully merging this pull request may close these issues.

5 participants