Ubuntu
curtin package

Insecure key file permissions

Bug #1382632 reported by Andreas Hasenack
28
This bug affects 3 people
Affects Status Importance Assigned to Milestone
curtin
Fix Released
High
Unassigned
curtin (Ubuntu)
Fix Released
High
Unassigned
Trusty
Fix Released
High
Unassigned
Utopic
Fix Released
High
Unassigned
Vivid
Fix Released
High
Unassigned

Bug Description

=== SRU Information ===
[Impact]
Systems installed using curtin inadvertantly have a default set of acl applied
to the root directory. Those default acl can wreak havoc with seemingly
sane expectations of users or packages or administrators.

For example, the problem that was noticed essentially boiled down to a
program doing:
  ( umask 0066 ; rm -f secret-file; echo "passw0rd" > secret-file )
and then later that program checked permissions of the file
and found:
  $ ls -l secret-file
  -rw-r--r-- 1 smoser smoser 0 Oct 27 12:00 secret-file
instead of
  -rw------- 1 smoser smoser 0 Oct 27 12:00 secret-file
And raised exception.

This is not at all an unreasonable expectation.
Essentially, this boils down to all packages not being ready to handle
having filesystem ACL in place. Additionally curtin did not intend on
installing the target with default ACLs that was a unexpected behavior of
tar (raised in bug 1386237)

[Test Case]
 * Install system with MAAS and fast path installer (curtin).
 * mkdir /tmp/mydir
 * cd /tmp/mydir
 * ( umask 0066 ; rm -f secret-file; echo "passw0rd" > secret-file )
 * ls -l secret-file

Expected output is that file has 600 permissions. Failure case, is 644.

[Regression Potential]
Fairly small chance for regression as the tar files created for consumption
are not created with acl information inside. Generally ubuntu installations
do not have default ACL in place on /, and thus the change creates less
chance for unexpected behavior than is currently present.

[Other Info]
This bug is not actually present in the version of curtin in trusty.
However, the fix for this issue is in the code added to fix bug 1313550.
The bug is present in utopic's version of curtin.

=== End SRU Information ===

openstack-dashboard 1:2014.2-0ubuntu1~cloud0 from http://ppa.launchpad.net/ubuntu-cloud-archive/juno-staging/ubuntu/

Got this during installation with the charm:
(...)
2014-10-17 17:17:07 INFO install Setting up openstack-dashboard (1:2014.2-0ubuntu1~cloud0) ...
2014-10-17 17:17:07 INFO install Collecting and compressing static assets...
2014-10-17 17:17:07 INFO install Traceback (most recent call last):
2014-10-17 17:17:07 INFO install File "manage.py", line 25, in <module>
2014-10-17 17:17:07 INFO install execute_from_command_line(sys.argv)
2014-10-17 17:17:07 INFO install File "/usr/lib/python2.7/dist-packages/django/core/management/__init__.py", line 399, in execute_from_command_line
2014-10-17 17:17:07 INFO install utility.execute()
2014-10-17 17:17:07 INFO install File "/usr/lib/python2.7/dist-packages/django/core/management/__init__.py", line 392, in execute
2014-10-17 17:17:07 INFO install self.fetch_command(subcommand).run_from_argv(self.argv)
2014-10-17 17:17:07 INFO install File "/usr/lib/python2.7/dist-packages/django/core/management/__init__.py", line 261, in fetch_command
2014-10-17 17:17:07 INFO install commands = get_commands()
2014-10-17 17:17:07 INFO install File "/usr/lib/python2.7/dist-packages/django/core/management/__init__.py", line 107, in get_commands
2014-10-17 17:17:07 INFO install apps = settings.INSTALLED_APPS
2014-10-17 17:17:07 INFO install File "/usr/lib/python2.7/dist-packages/django/conf/__init__.py", line 54, in __getattr__
2014-10-17 17:17:07 INFO install self._setup(name)
2014-10-17 17:17:07 INFO install File "/usr/lib/python2.7/dist-packages/django/conf/__init__.py", line 49, in _setup
2014-10-17 17:17:07 INFO install self._wrapped = Settings(settings_module)
2014-10-17 17:17:07 INFO install File "/usr/lib/python2.7/dist-packages/django/conf/__init__.py", line 128, in __init__
2014-10-17 17:17:07 INFO install mod = importlib.import_module(self.SETTINGS_MODULE)
2014-10-17 17:17:07 INFO install File "/usr/lib/python2.7/dist-packages/django/utils/importlib.py", line 40, in import_module
2014-10-17 17:17:07 INFO install __import__(name)
2014-10-17 17:17:07 INFO install File "/usr/share/openstack-dashboard/openstack_dashboard/settings.py", line 316, in <module>
2014-10-17 17:17:07 INFO install from local.local_settings import * # noqa
2014-10-17 17:17:07 INFO install File "/usr/share/openstack-dashboard/openstack_dashboard/local/local_settings.py", line 98, in <module>
2014-10-17 17:17:07 INFO install SECRET_KEY = secret_key.generate_or_read_from_file('/var/lib/openstack-dashboard/secret_key')
2014-10-17 17:17:07 INFO install File "/usr/lib/python2.7/dist-packages/horizon/utils/secret_key.py", line 61, in generate_or_read_from_file
2014-10-17 17:17:07 INFO install raise FilePermissionError("Insecure key file permissions!")
2014-10-17 17:17:07 INFO install horizon.utils.secret_key.FilePermissionError: Insecure key file permissions!
2014-10-17 17:17:07 INFO install dpkg: error processing package openstack-dashboard (--configure):
2014-10-17 17:17:07 INFO install subprocess installed post-installation script returned error exit status 1
2014-10-17 17:17:07 INFO install dpkg: dependency problems prevent configuration of openstack-dashboard-ubuntu-theme:
2014-10-17 17:17:07 INFO install openstack-dashboard-ubuntu-theme depends on openstack-dashboard (= 1:2014.2-0ubuntu1~cloud0); however:
2014-10-17 17:17:07 INFO install Package openstack-dashboard is not configured yet.
2014-10-17 17:17:07 INFO install
2014-10-17 17:17:07 INFO install dpkg: error processing package openstack-dashboard-ubuntu-theme (--configure):
2014-10-17 17:17:07 INFO install dependency problems - leaving unconfigured
2014-10-17 17:17:07 INFO install No apport report written because the error message indicates its a followup error from a previous failure.
2014-10-17 17:17:07 INFO install Errors were encountered while processing:
2014-10-17 17:17:07 INFO install openstack-dashboard
2014-10-17 17:17:07 INFO install openstack-dashboard-ubuntu-theme
2014-10-17 17:17:08 INFO install E: Sub-process /usr/bin/dpkg returned an error code (1)

Full logs attached.

See original description
Revision history for this message
Andreas Hasenack (ahasenack) wrote :
tags: added: cloud-installer landscape
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

The secret_key file is created when python manage.py collectstatic is run, it seems.

We have two packages that run this command in postinst: openstack-dashboard-ubuntu-theme and openstack-dashboard. In this scenario, -ubuntu-theme is installed first. It runs that command in postinst, the file is created, and apparently with the incorrect permissions: 0644

Then openstack-dashboard runs it again, and this time the secret_key file already exists, and it complains about the incorrect permissions.

Revision history for this message
Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in horizon (Ubuntu):
status: New → Confirmed
Ryan Beisner (1chb1n)
tags: added: openstack
Revision history for this message
Björn Tillenius (bjornt) wrote :

I don't think the ordering of the package installs are important. They run the same command. The problem seem to be that the code that creates the secret key doesn't create it with the right permissions. The current code in horizon/utils/secret_key.py does this:

            old_umask = os.umask(0o177) # Use '0600' file permissions
            with open(key_file, 'w') as f:
                f.write(key)
            os.umask(old_umask)

That doesn't work on the systems we're installing to. It works if I change the code to:

            with open(key_file, 'w') as f:
                os.chmod(key_file, 0o600)
                f.write(key)

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Of course the problem isn't that it is run twice :) I said above that in the first run the file is created with the incorrect permissions, and then the second run barfs at that :)

James Page (james-page)
summary: - Insecure key file permissions
+ Insecure key file permissions when running under LXC
Revision history for this message
Björn Tillenius (bjornt) wrote : Re: Insecure key file permissions when running under LXC

The system where we saw this bug uses file system ACLs, with defauls, and thus the umask is ignored:

ubuntu@juju-machine-0-lxc-5:/var/lib/openstack-dashboard$ getfacl .
# file: .
# owner: root
# group: root
user::rwx
group::r-x
other::r-x
default:user::rwx
default:group::r-x
default:other::r-x

James Page (james-page)
summary: - Insecure key file permissions when running under LXC
+ Insecure key file permissions
Revision history for this message
Björn Tillenius (bjornt) wrote :

To clarify, I see the same behavior on bare metal, deploying machines with MAAS using the fast-path installer, so it's not LXC-specific.

Revision history for this message
Scott Moser (smoser) wrote :

This ends up being a regression of the curtin changes in bug 1313550.
curtin is extracting a tarball with '--xattrs --xattrs-include=* --acl'.

Its the '--acl' that is problematic.

Even though the tarball being extracted did not have acl stored in it tar creates default acl on extraction.

The simplist fix is to for us to remove '--acl' from curtin's extraction parameters.
http://bazaar.launchpad.net/~curtin-dev/curtin/trunk/view/head:/curtin/commands/extract.py

This change can be made locally on the maas region controller in the installed 'python-curtin' package.

Revision history for this message
Scott Moser (smoser) wrote :

fixed in revno 194 of trunk.

Changed in curtin:
importance: Undecided → High
status: New → Fix Committed
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

How do we get this on trusty? Or the MAAS PPA perhaps?

Scott Moser (smoser)
Changed in curtin (Ubuntu):
status: New → Confirmed
no longer affects: horizon (Ubuntu Trusty)
no longer affects: horizon (Ubuntu Utopic)
Scott Moser (smoser)
description: updated
Scott Moser (smoser)
Changed in curtin (Ubuntu Utopic):
status: New → Confirmed
Changed in curtin (Ubuntu Trusty):
status: New → Confirmed
importance: Undecided → High
Changed in curtin (Ubuntu Utopic):
importance: Undecided → High
Changed in curtin (Ubuntu Vivid):
importance: Undecided → High
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package curtin - 0.1.0~bzr195-0ubuntu1

---------------
curtin (0.1.0~bzr195-0ubuntu1) vivid; urgency=medium

  * New upstream snapshot.
    * move install log from /var/log/curtin_install.log to
      /var/log/curtin/install.log (LP: #1378910)
    * to not use '--acl' when extracting tar files (LP: #1382632)
      as it inadvertently writes default directory acls.
    * invoke lsblk with '--output' rather than '--out' to avoid
      ambiguity in newer versions of lsblk (LP: #1386275)
 -- Scott Moser <email address hidden> Mon, 27 Oct 2014 12:25:27 -0400

Changed in curtin (Ubuntu Vivid):
status: Confirmed → Fix Released
Scott Moser (smoser)
description: updated
Greg Lutostanski (lutostag)
tags: added: oil
Revision history for this message
Brian Murray (brian-murray) wrote : Please test proposed package

Hello Andreas, or anyone else affected,

Accepted curtin into trusty-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/curtin/0.1.0~bzr195-0ubuntu1~14.04.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in curtin (Ubuntu Trusty):
status: Confirmed → Fix Committed
tags: added: verification-needed
Revision history for this message
Brian Murray (brian-murray) wrote :

Hello Andreas, or anyone else affected,

Accepted curtin into utopic-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/curtin/0.1.0~bzr195-0ubuntu1~14.10.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in curtin (Ubuntu Utopic):
status: Confirmed → Fix Committed
Revision history for this message
Andres Rodriguez (andreserl) wrote :

This has been tested and works as expected!

tags: added: verification-done
removed: verification-needed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package curtin - 0.1.0~bzr195-0ubuntu1~14.10.1

---------------
curtin (0.1.0~bzr195-0ubuntu1~14.10.1) utopic-proposed; urgency=medium

  * New upstream snapshot / sync to vivid version.
    - move install log from /var/log/curtin_install.log to
      /var/log/curtin/install.log (LP: #1378910)
    - to not use '--acl' when extracting tar files (LP: #1382632)
      as it inadvertently writes default directory acls.
    - invoke lsblk with '--output' rather than '--out' to avoid
      ambiguity in newer versions of lsblk (LP: #1386275)
 -- Scott Moser <email address hidden> Mon, 27 Oct 2014 13:08:50 -0400

Changed in curtin (Ubuntu Utopic):
status: Fix Committed → Fix Released
Revision history for this message
Scott Kitterman (kitterman) wrote : Update Released

The verification of the Stable Release Update for curtin has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

James Page (james-page)
Changed in horizon (Ubuntu Vivid):
status: Confirmed → Invalid
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package curtin - 0.1.0~bzr195-0ubuntu1~14.04.1

---------------
curtin (0.1.0~bzr195-0ubuntu1~14.04.1) trusty-proposed; urgency=medium

  * New upstream snapshot.
    - hardware enablement: ppc64 support (LP: #1386394)
    - hardware enablement: know kernel mapping for utopic (hwe-u = 3.16)
      (LP: #1386394)
    - feature: support installing disk images including windows. (LP: #1386394)
    - feature: support creating swap file (LP: #1386394)
    - feature: support reporting logs back to MAAS (LP: #1386394)
    - feature: enable logging of installation to /var/log/curtin/install.log
      (LP: #1378910)
    - bug fix: extract tar files with xattr support when available (LP: #1313550)
    - bug fix: fix broken use of os.path.join for curtin hooks (LP: #1328521)
    - bug fix: util.subp to decode command output as utf-8 (LP: #1370249).
    - bug fix: call update-grub to ensure that /boot/grub/grub.cfg is created
      (LP: #1373137)
    - bug fix: do not use '--acl' when extracting tar files (LP: #1382632)
      as it inadvertently writes default directory acls.
    - bug fix: invoke lsblk with '--output' rather than '--out' to avoid
      ambiguity in newer versions of lsblk (LP: #1386275)
    - internal: part2bd helper added in helpers/common
    - internal: helpers: inherit curtin_verbosity (make the helper tools
      verbose if curtin invoked with verbose flags)
 -- Scott Moser <email address hidden> Mon, 27 Oct 2014 20:58:43 -0400

Changed in curtin (Ubuntu Trusty):
status: Fix Committed → Fix Released
Mathew Hodson (mhodson)
no longer affects: horizon (Ubuntu)
no longer affects: horizon (Ubuntu Vivid)
Revision history for this message
Scott Moser (smoser) wrote : Fixed in Curtin 17.1

This bug is believed to be fixed in curtin in 17.1. If this is still a problem for you, please make a comment and set the state back to New

Thank you.

Changed in curtin:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.