From 8a7884d47601d468fdd6b3f9dab7150ad21554fe Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Thu, 4 Jul 2019 19:30:11 +0200 Subject: [PATCH 01/15] [luksbootkeyfile] More debugging of the crypt file --- src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp b/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp index 9b49b91cb..e521e83a6 100644 --- a/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp +++ b/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp @@ -120,6 +120,9 @@ generateTargetKeyfile() cWarning() << "Could not create LUKS keyfile:" << r.getOutput() << "(exit code" << r.getExitCode() << ')'; return false; } + // Give ample time to check that the file was created correctly + r = CalamaresUtils::System::instance()->targetEnvCommand( { "ls", "-la", "/" } ); + cDebug() << "In target system" << r.getOutput(); return true; } From 7d7d4c69ef7bd57809e11d4c0ec486dcdfdd7ffe Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Thu, 4 Jul 2019 19:37:37 +0200 Subject: [PATCH 02/15] [luksbootkeyfile] Don't log passphrase --- src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp b/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp index e521e83a6..06cb5f9cd 100644 --- a/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp +++ b/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp @@ -174,7 +174,8 @@ LuksBootKeyFileJob::exec() auto it = std::partition( s.devices.begin(), s.devices.end(), []( const LuksDevice& d ) { return d.isRoot; } ); for ( const auto& d : s.devices ) { - cDebug() << Logger::SubEntry << d.isRoot << d.device << d.passphrase; + cDebug() << Logger::SubEntry << ( d.isRoot ? "root" : "dev." ) << d.device << "passphrase?" + << !d.passphrase.isEmpty(); } if ( it == s.devices.begin() ) From efd409cf78759c48280a167d2463295ae37fdb70 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Thu, 4 Jul 2019 19:38:10 +0200 Subject: [PATCH 03/15] [luksbootkeyfile] Refactor static function to outside class --- .../luksbootkeyfile/LuksBootKeyFileJob.cpp | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp b/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp index 06cb5f9cd..c84e79083 100644 --- a/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp +++ b/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp @@ -66,6 +66,31 @@ struct LuksDevice QString passphrase; }; +/** @brief Extract the luks passphrases setup. + * + * Given a list of partitions (as set up by the partitioning module, + * so there's maps with keys inside), returns just the list of + * luks passphrases for each device. + */ +static QList< LuksDevice > +getLuksDevices( const QVariantList& list ) +{ + QList< LuksDevice > luksItems; + + for ( const auto& p : list ) + { + if ( p.canConvert< QVariantMap >() ) + { + LuksDevice d( p.toMap() ); + if ( d.isValid ) + { + luksItems.append( d ); + } + } + } + return luksItems; +} + struct LuksDeviceList { LuksDeviceList( const QVariant& partitions ) @@ -78,31 +103,6 @@ struct LuksDeviceList } } - /** @brief Extract the luks passphrases setup. - * - * Given a list of partitions (as set up by the partitioning module, - * so there's maps with keys inside), returns just the list of - * luks passphrases for each device. - */ - static QList< LuksDevice > - getLuksDevices( const QVariantList& list ) - { - QList< LuksDevice > luksItems; - - for ( const auto& p : list ) - { - if ( p.canConvert< QVariantMap >() ) - { - LuksDevice d( p.toMap() ); - if ( d.isValid ) - { - luksItems.append( d ); - } - } - } - return luksItems; - } - QList< LuksDevice > devices; bool valid; }; From d5340f97431d0e94ba4b284511a4cdc4354757a0 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Thu, 4 Jul 2019 20:05:48 +0200 Subject: [PATCH 04/15] [initramfs] Drop timeout entirely, even two minutes too short --- src/modules/initramfs/InitramfsJob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/initramfs/InitramfsJob.cpp b/src/modules/initramfs/InitramfsJob.cpp index d8a9ed6c3..e96855d23 100644 --- a/src/modules/initramfs/InitramfsJob.cpp +++ b/src/modules/initramfs/InitramfsJob.cpp @@ -45,7 +45,7 @@ InitramfsJob::exec() cDebug() << "Updating initramfs with kernel" << m_kernel; auto r = CalamaresUtils::System::instance()->targetEnvCommand( - { "update-initramfs", "-k", m_kernel, "-c", "-t" }, QString(), QString(), 120 ); + { "update-initramfs", "-k", m_kernel, "-c", "-t" }, QString(), QString(), 0 ); return r.explainProcess( "update-initramfs", 10 ); } From bb6530577db46b0c976f8e0fed29949b507da304 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Thu, 4 Jul 2019 20:21:13 +0200 Subject: [PATCH 05/15] [initcpio] Replace Python implementation with C++ - This is a simple variation on the theme of things-that-call-a- initramfs-updater, so the code is mostly a copy of initramfs/ module. I didn't even bother to strip out the configuration- handling (I figure it might be good for *something*) so now "" and "$uname" are valid kernel names as well. - Fixes security issue where the initramfs ends up readable by all, and that includes the cryptfile for LUKS. SEE #1190 --- src/modules/initcpio/CMakeLists.txt | 9 ++++ src/modules/initcpio/InitcpioJob.cpp | 77 ++++++++++++++++++++++++++++ src/modules/initcpio/InitcpioJob.h | 49 ++++++++++++++++++ src/modules/initcpio/initcpio.conf | 15 ++++++ src/modules/initcpio/main.py | 50 ------------------ src/modules/initcpio/module.desc | 5 -- 6 files changed, 150 insertions(+), 55 deletions(-) create mode 100644 src/modules/initcpio/CMakeLists.txt create mode 100644 src/modules/initcpio/InitcpioJob.cpp create mode 100644 src/modules/initcpio/InitcpioJob.h delete mode 100644 src/modules/initcpio/main.py delete mode 100644 src/modules/initcpio/module.desc diff --git a/src/modules/initcpio/CMakeLists.txt b/src/modules/initcpio/CMakeLists.txt new file mode 100644 index 000000000..d35a12f06 --- /dev/null +++ b/src/modules/initcpio/CMakeLists.txt @@ -0,0 +1,9 @@ +calamares_add_plugin( initcpio + TYPE job + EXPORT_MACRO PLUGINDLLEXPORT_PRO + SOURCES + InitcpioJob.cpp + LINK_PRIVATE_LIBRARIES + calamares + SHARED_LIB +) diff --git a/src/modules/initcpio/InitcpioJob.cpp b/src/modules/initcpio/InitcpioJob.cpp new file mode 100644 index 000000000..89c8a906d --- /dev/null +++ b/src/modules/initcpio/InitcpioJob.cpp @@ -0,0 +1,77 @@ +/* === This file is part of Calamares - === + * + * Copyright 2019, Adriaan de Groot + * + * Calamares is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Calamares is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Calamares. If not, see . + */ + +#include "InitcpioJob.h" + +#include "utils/CalamaresUtilsSystem.h" +#include "utils/Logger.h" +#include "utils/UMask.h" +#include "utils/Variant.h" + +InitcpioJob::InitcpioJob( QObject* parent ) + : Calamares::CppJob( parent ) +{ +} + +InitcpioJob::~InitcpioJob() {} + + +QString +InitcpioJob::prettyName() const +{ + return tr( "Creating initramfs with mkinitcpio." ); +} + + +Calamares::JobResult +InitcpioJob::exec() +{ + CalamaresUtils::UMask m( CalamaresUtils::UMask::Safe ); + + cDebug() << "Updating initramfs with kernel" << m_kernel; + auto r = CalamaresUtils::System::instance()->targetEnvCommand( + { "mkinitcpio", "-p", m_kernel }, QString(), QString(), 0 ); + return r.explainProcess( "mkinitcpio", 10 ); +} + +void +InitcpioJob::setConfigurationMap( const QVariantMap& configurationMap ) +{ + m_kernel = CalamaresUtils::getString( configurationMap, "kernel" ); + if ( m_kernel.isEmpty() ) + { + m_kernel = QStringLiteral( "all" ); + } + else if ( m_kernel == "$uname" ) + { + auto r = CalamaresUtils::System::runCommand( + CalamaresUtils::System::RunLocation::RunInHost, { "/bin/uname", "-r" }, QString(), QString(), 3 ); + if ( r.getExitCode() == 0 ) + { + m_kernel = r.getOutput(); + cDebug() << "*initcpio* using running kernel" << m_kernel; + } + else + { + cWarning() << "*initcpio* could not determine running kernel, using 'all'." << Logger::Continuation + << r.getExitCode() << r.getOutput(); + } + } +} + +CALAMARES_PLUGIN_FACTORY_DEFINITION( InitcpioJobFactory, registerPlugin< InitcpioJob >(); ) diff --git a/src/modules/initcpio/InitcpioJob.h b/src/modules/initcpio/InitcpioJob.h new file mode 100644 index 000000000..7c0bcf2df --- /dev/null +++ b/src/modules/initcpio/InitcpioJob.h @@ -0,0 +1,49 @@ +/* === This file is part of Calamares - === + * + * Copyright 2019, Adriaan de Groot + * + * Calamares is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Calamares is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Calamares. If not, see . + */ + +#ifndef INITCPIOJOB_H +#define INITCPIOJOB_H + +#include "CppJob.h" +#include "PluginDllMacro.h" +#include "utils/PluginFactory.h" + +#include +#include + +class PLUGINDLLEXPORT InitcpioJob : public Calamares::CppJob +{ + Q_OBJECT + +public: + explicit InitcpioJob( QObject* parent = nullptr ); + virtual ~InitcpioJob() override; + + QString prettyName() const override; + + Calamares::JobResult exec() override; + + void setConfigurationMap( const QVariantMap& configurationMap ) override; + +private: + QString m_kernel; +}; + +CALAMARES_PLUGIN_FACTORY_DECLARATION( InitcpioJobFactory ) + +#endif // INITCPIOJOB_H diff --git a/src/modules/initcpio/initcpio.conf b/src/modules/initcpio/initcpio.conf index 466a8785d..487a0289d 100644 --- a/src/modules/initcpio/initcpio.conf +++ b/src/modules/initcpio/initcpio.conf @@ -1,3 +1,18 @@ # Run mkinitcpio(8) with the given preset value --- +# There is only one configuration item for this module, +# the kernel to be loaded. This can have the following +# values: +# - empty or unset, interpreted as "all" +# - the literal string "$uname" (without quotes, with dollar), +# which will use the output of `uname -r` to determine the +# running kernel, and use that. +# - any other string. +# +# Whatever is set, that string is passed as *preset* argument to the +# `-p` option of *mkinitcpio*. Take care that both "$uname" operates +# in the host system, and might not be correct if the target system is +# updated (to a newer kernel) as part of the installation. +# +# Note that "all" is probably not a good preset to use either. kernel: linux312 diff --git a/src/modules/initcpio/main.py b/src/modules/initcpio/main.py deleted file mode 100644 index 796f68721..000000000 --- a/src/modules/initcpio/main.py +++ /dev/null @@ -1,50 +0,0 @@ -#!/usr/bin/env python3 -# -*- coding: utf-8 -*- -# -# === This file is part of Calamares - === -# -# Copyright 2014, Philip Müller -# Copyright 2019, Adriaan de Groot -# -# Calamares is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# Calamares is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Calamares. If not, see . - -import libcalamares -from libcalamares.utils import check_target_env_call - -import gettext -_ = gettext.translation("calamares-python", - localedir=libcalamares.utils.gettext_path(), - languages=libcalamares.utils.gettext_languages(), - fallback=True).gettext - - -def pretty_name(): - return _("Creating initramfs with mkinitcpio.") - -def run(): - """ Calls routine to create kernel initramfs image. - - :return: - """ - from subprocess import CalledProcessError - - kernel = libcalamares.job.configuration['kernel'] - try: - check_target_env_call(['mkinitcpio', '-p', kernel]) - except CalledProcessError as e: - libcalamares.utils.warning(str(e)) - return ( _( "Process Failed" ), - _( "Process 
mkinitcpio
failed with error code {!s}. The command was 
{!s}
." ).format( e.returncode, e.cmd ) ) - - return None diff --git a/src/modules/initcpio/module.desc b/src/modules/initcpio/module.desc deleted file mode 100644 index f84fc43a8..000000000 --- a/src/modules/initcpio/module.desc +++ /dev/null @@ -1,5 +0,0 @@ ---- -type: "job" -name: "initcpio" -interface: "python" -script: "main.py" From 315e1ac54e74852100543ced3dffbd8c1eb688a1 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Thu, 4 Jul 2019 22:08:36 +0200 Subject: [PATCH 06/15] [luksbootkeyfile] Improve logging to distinguish from other modules --- src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp b/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp index c84e79083..292c768a9 100644 --- a/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp +++ b/src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp @@ -122,7 +122,7 @@ generateTargetKeyfile() } // Give ample time to check that the file was created correctly r = CalamaresUtils::System::instance()->targetEnvCommand( { "ls", "-la", "/" } ); - cDebug() << "In target system" << r.getOutput(); + cDebug() << "In target system after creating LUKS file" << r.getOutput(); return true; } From b6974614974842020be591adccf8ebbe5c50ee13 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 5 Jul 2019 11:29:14 +0200 Subject: [PATCH 07/15] [libcalamares] Add System::createTargetFile() - Calamares may need to create files in the target system; provide a convenient API for doing so. - This is mostly intended for small files with constant contents. --- .../utils/CalamaresUtilsSystem.cpp | 51 +++++++++++++++++++ src/libcalamares/utils/CalamaresUtilsSystem.h | 14 +++++ 2 files changed, 65 insertions(+) diff --git a/src/libcalamares/utils/CalamaresUtilsSystem.cpp b/src/libcalamares/utils/CalamaresUtilsSystem.cpp index 5990fbc42..698a96f30 100644 --- a/src/libcalamares/utils/CalamaresUtilsSystem.cpp +++ b/src/libcalamares/utils/CalamaresUtilsSystem.cpp @@ -243,6 +243,57 @@ System::runCommand( return ProcessResult(r, output); } +QString +System::createTargetFile( const QString& path, const QByteArray& contents ) +{ + QString completePath; + + if ( doChroot() ) + { + Calamares::GlobalStorage* gs = Calamares::JobQueue::instance() ? Calamares::JobQueue::instance()->globalStorage() : nullptr; + + if ( !gs || !gs->contains( "rootMountPoint" ) ) + { + cWarning() << "No rootMountPoint in global storage, cannot create target file" << path; + return QString(); + } + + completePath = gs->value( "rootMountPoint" ).toString() + '/' + path; + } + else + { + completePath = QStringLiteral( "/" ) + path; + } + + QFile f( completePath ); + if ( f.exists() ) + { + return QString(); + } + + QIODevice::OpenMode m = +#if QT_VERSION >= QT_VERSION_CHECK( 5, 11, 0 ) + // New flag from Qt 5.11, implies WriteOnly + QIODevice::NewOnly | +#endif + QIODevice::WriteOnly | QIODevice::Truncate; + + if ( !f.open( m ) ) + { + return QString(); + } + + if ( f.write( contents ) != contents.size() ) + { + f.close(); + f.remove(); + return QString(); + } + + f.close(); + return QFileInfo( f ).canonicalFilePath(); +} + QPair System::getTotalMemoryB() const diff --git a/src/libcalamares/utils/CalamaresUtilsSystem.h b/src/libcalamares/utils/CalamaresUtilsSystem.h index c17d52e93..8c8e61a5e 100644 --- a/src/libcalamares/utils/CalamaresUtilsSystem.h +++ b/src/libcalamares/utils/CalamaresUtilsSystem.h @@ -205,6 +205,20 @@ public: return targetEnvOutput( QStringList{ command }, output, workingPath, stdInput, timeoutSec ); } + /** @brief Create a (small-ish) file in the target system. + * + * @param path Path to the file; this is **always** interpreted + * from the root of the target system (whatever that may be, + * but / in the chroot, or / in OEM modes). + * @param contents Actual content of the file. + * + * Will not overwrite files. Returns an empty string if the + * target file already exists. + * + * @return The complete canonical path to the target file, or empty on failure. + */ + QString createTargetFile( const QString& path, const QByteArray& contents ); + /** * @brief getTotalMemoryB returns the total main memory, in bytes. * From 43eb664e7d44d963bb7b82d03215d84b47100ba0 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 5 Jul 2019 11:43:40 +0200 Subject: [PATCH 08/15] [initramfs] Configure mkinitramfs to be safe SEE #1191 --- src/modules/initramfs/InitramfsJob.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/modules/initramfs/InitramfsJob.cpp b/src/modules/initramfs/InitramfsJob.cpp index e96855d23..c96bbb059 100644 --- a/src/modules/initramfs/InitramfsJob.cpp +++ b/src/modules/initramfs/InitramfsJob.cpp @@ -44,6 +44,17 @@ InitramfsJob::exec() CalamaresUtils::UMask m( CalamaresUtils::UMask::Safe ); cDebug() << "Updating initramfs with kernel" << m_kernel; + + // First make sure we generate a safe initramfs with suitable permissions. + static const char confFile[] = "/etc/initramfs-tools/conf.d/calamares-safe-initramfs.conf"; + static const char contents[] = "UMASK=0077\n"; + if ( CalamaresUtils::System::instance()->createTargetFile( confFile, QByteArray( contents ) ).isEmpty() ) + { + cWarning() << Logger::SubEntry << "Could not configure safe UMASK for initramfs."; + // But continue anyway. + } + + // And then do the ACTUAL work. auto r = CalamaresUtils::System::instance()->targetEnvCommand( { "update-initramfs", "-k", m_kernel, "-c", "-t" }, QString(), QString(), 0 ); return r.explainProcess( "update-initramfs", 10 ); From 1a8543537265cd63bcb38d736c5cb0e15adb3fc0 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 5 Jul 2019 13:05:49 +0200 Subject: [PATCH 09/15] [libcalamares] Get target path relative to host / --- .../utils/CalamaresUtilsSystem.cpp | 14 +++++++++- src/libcalamares/utils/CalamaresUtilsSystem.h | 26 ++++++++++++++++--- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/libcalamares/utils/CalamaresUtilsSystem.cpp b/src/libcalamares/utils/CalamaresUtilsSystem.cpp index 698a96f30..10c76e482 100644 --- a/src/libcalamares/utils/CalamaresUtilsSystem.cpp +++ b/src/libcalamares/utils/CalamaresUtilsSystem.cpp @@ -244,7 +244,7 @@ System::runCommand( } QString -System::createTargetFile( const QString& path, const QByteArray& contents ) +System::targetPath( const QString& path ) const { QString completePath; @@ -265,6 +265,18 @@ System::createTargetFile( const QString& path, const QByteArray& contents ) completePath = QStringLiteral( "/" ) + path; } + return completePath; +} + +QString +System::createTargetFile( const QString& path, const QByteArray& contents ) const +{ + QString completePath = targetPath( path ); + if ( completePath.isEmpty() ) + { + return QString(); + } + QFile f( completePath ); if ( f.exists() ) { diff --git a/src/libcalamares/utils/CalamaresUtilsSystem.h b/src/libcalamares/utils/CalamaresUtilsSystem.h index 8c8e61a5e..f778b67c9 100644 --- a/src/libcalamares/utils/CalamaresUtilsSystem.h +++ b/src/libcalamares/utils/CalamaresUtilsSystem.h @@ -205,9 +205,27 @@ public: return targetEnvOutput( QStringList{ command }, output, workingPath, stdInput, timeoutSec ); } + + /** @brief Gets a path to a file in the target system, from the host. + * + * @param path Path to the file; this is interpreted + * from the root of the target system (whatever that may be, + * but / in the chroot, or / in OEM modes). + * + * @return The complete path to the target file, from + * the root of the host system, or empty on failure. + * + * For instance, during installation where the target root is + * mounted on /tmp/calamares-something, asking for targetPath("/etc/passwd") + * will give you /tmp/calamares-something/etc/passwd. + * + * No attempt is made to canonicalize anything, since paths might not exist. + */ + DLLEXPORT QString targetPath( const QString& path ) const; + /** @brief Create a (small-ish) file in the target system. * - * @param path Path to the file; this is **always** interpreted + * @param path Path to the file; this is interpreted * from the root of the target system (whatever that may be, * but / in the chroot, or / in OEM modes). * @param contents Actual content of the file. @@ -215,9 +233,11 @@ public: * Will not overwrite files. Returns an empty string if the * target file already exists. * - * @return The complete canonical path to the target file, or empty on failure. + * @return The complete canonical path to the target file from the + * root of the host system, or empty on failure. (Here, it is + * possible to be canonical because the file exists). */ - QString createTargetFile( const QString& path, const QByteArray& contents ); + DLLEXPORT QString createTargetFile( const QString& path, const QByteArray& contents ) const; /** * @brief getTotalMemoryB returns the total main memory, in bytes. From 5f6efd2822ada666c29effd8d3b0ce9eb79e1b63 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 5 Jul 2019 13:17:55 +0200 Subject: [PATCH 10/15] [initcpio] Improve security by making initramfs files not world-readable --- src/modules/initcpio/InitcpioJob.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/modules/initcpio/InitcpioJob.cpp b/src/modules/initcpio/InitcpioJob.cpp index 89c8a906d..0b96ddcd2 100644 --- a/src/modules/initcpio/InitcpioJob.cpp +++ b/src/modules/initcpio/InitcpioJob.cpp @@ -23,6 +23,9 @@ #include "utils/UMask.h" #include "utils/Variant.h" +#include +#include + InitcpioJob::InitcpioJob( QObject* parent ) : Calamares::CppJob( parent ) { @@ -37,12 +40,31 @@ InitcpioJob::prettyName() const return tr( "Creating initramfs with mkinitcpio." ); } +static void +fixPermissions( const QDir& d ) +{ + for ( const auto& fi : d.entryInfoList( { "initramfs*" }, QDir::Files ) ) + { + QFile f( fi.absoluteFilePath() ); + if ( f.exists() ) + { + cDebug() << "initcpio fixing permissions for" << f.fileName(); + f.setPermissions( QFileDevice::ReadOwner | QFileDevice::WriteOwner ); + } + } +} Calamares::JobResult InitcpioJob::exec() { CalamaresUtils::UMask m( CalamaresUtils::UMask::Safe ); + QDir d( CalamaresUtils::System::instance()->targetPath( "/boot" ) ); + if ( d.exists() ) + { + fixPermissions( d ); + } + cDebug() << "Updating initramfs with kernel" << m_kernel; auto r = CalamaresUtils::System::instance()->targetEnvCommand( { "mkinitcpio", "-p", m_kernel }, QString(), QString(), 0 ); From 39d618c61e7ecdc9c068c799c26bcf811cd867ae Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 5 Jul 2019 13:28:32 +0200 Subject: [PATCH 11/15] [initcpio] Simple test for fixPermissions() --- src/modules/initcpio/CMakeLists.txt | 17 ++++++++ src/modules/initcpio/InitcpioJob.cpp | 2 +- src/modules/initcpio/Tests.cpp | 59 ++++++++++++++++++++++++++++ src/modules/initcpio/Tests.h | 36 +++++++++++++++++ 4 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 src/modules/initcpio/Tests.cpp create mode 100644 src/modules/initcpio/Tests.h diff --git a/src/modules/initcpio/CMakeLists.txt b/src/modules/initcpio/CMakeLists.txt index d35a12f06..990220b10 100644 --- a/src/modules/initcpio/CMakeLists.txt +++ b/src/modules/initcpio/CMakeLists.txt @@ -7,3 +7,20 @@ calamares_add_plugin( initcpio calamares SHARED_LIB ) + +if( ECM_FOUND AND BUILD_TESTING ) + ecm_add_test( + Tests.cpp + TEST_NAME + initcpiotest + LINK_LIBRARIES + ${CALAMARES_LIBRARIES} + calamares + calamares_job_initcpio # From above + ${YAMLCPP_LIBRARY} + Qt5::Core + Qt5::Test + ) + set_target_properties( initcpiotest PROPERTIES AUTOMOC TRUE ) + target_include_directories( initcpiotest PRIVATE /usr/local/include ) +endif() diff --git a/src/modules/initcpio/InitcpioJob.cpp b/src/modules/initcpio/InitcpioJob.cpp index 0b96ddcd2..d0d825cbf 100644 --- a/src/modules/initcpio/InitcpioJob.cpp +++ b/src/modules/initcpio/InitcpioJob.cpp @@ -40,7 +40,7 @@ InitcpioJob::prettyName() const return tr( "Creating initramfs with mkinitcpio." ); } -static void +void fixPermissions( const QDir& d ) { for ( const auto& fi : d.entryInfoList( { "initramfs*" }, QDir::Files ) ) diff --git a/src/modules/initcpio/Tests.cpp b/src/modules/initcpio/Tests.cpp new file mode 100644 index 000000000..e0590ba25 --- /dev/null +++ b/src/modules/initcpio/Tests.cpp @@ -0,0 +1,59 @@ +/* === This file is part of Calamares - === + * + * Copyright 2019, Adriaan de Groot + * + * Calamares is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Calamares is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Calamares. If not, see . + */ + +#include "Tests.h" + +#include "GlobalStorage.h" +#include "JobQueue.h" +#include "Settings.h" + +#include "utils/Logger.h" +#include "utils/Yaml.h" + +#include + +#include +#include + +extern void fixPermissions( const QDir& d ); + +QTEST_GUILESS_MAIN( InitcpioTests ) + +InitcpioTests::InitcpioTests() +{ +} + +InitcpioTests::~InitcpioTests() +{ +} + +void +InitcpioTests::initTestCase() +{ +} + +void InitcpioTests::testFixPermissions() +{ + Logger::setupLogLevel( Logger::LOGDEBUG ); + cDebug() << "Fixing up /boot"; + fixPermissions( QDir( "/boot" ) ); + cDebug() << "Fixing up /nonexistent"; + fixPermissions( QDir( "/nonexistent/nonexistent" ) ); + QVERIFY( true ); +} + diff --git a/src/modules/initcpio/Tests.h b/src/modules/initcpio/Tests.h new file mode 100644 index 000000000..5bab26d3f --- /dev/null +++ b/src/modules/initcpio/Tests.h @@ -0,0 +1,36 @@ +/* === This file is part of Calamares - === + * + * Copyright 2019, Adriaan de Groot + * + * Calamares is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Calamares is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Calamares. If not, see . + */ + +#ifndef TESTS_H +#define TESTS_H + +#include + +class InitcpioTests : public QObject +{ + Q_OBJECT +public: + InitcpioTests(); + ~InitcpioTests() override; + +private Q_SLOTS: + void initTestCase(); + void testFixPermissions(); +}; + +#endif From 76ce0e4f2b85522509d7c1abfd81c5eb7d3cfacc Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 5 Jul 2019 23:20:19 +0200 Subject: [PATCH 12/15] [libcalamares] Don't crash when creating System object - In tests, a System object might be created without first setting up a JobQueue. In that case, there's no instance, so no GS to insert into. Avoid crash here. --- src/libcalamares/utils/CalamaresUtilsSystem.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libcalamares/utils/CalamaresUtilsSystem.cpp b/src/libcalamares/utils/CalamaresUtilsSystem.cpp index 10c76e482..7e55e6119 100644 --- a/src/libcalamares/utils/CalamaresUtilsSystem.cpp +++ b/src/libcalamares/utils/CalamaresUtilsSystem.cpp @@ -85,8 +85,10 @@ System::System( bool doChroot, QObject* parent ) { Q_ASSERT( !s_instance ); s_instance = this; - if ( !doChroot ) + if ( !doChroot && Calamares::JobQueue::instance() && Calamares::JobQueue::instance()->globalStorage() ) + { Calamares::JobQueue::instance()->globalStorage()->insert( "rootMountPoint", "/" ); + } } From a761bf02807c74fdf36092ecd2172d51a28b145d Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 5 Jul 2019 23:21:24 +0200 Subject: [PATCH 13/15] [initramfs] Add tests - These tests exercise the createTargetFile() logic, which is essential for creating a safe initramfs configuration snippet. - Could be moved into libcalamares instead, since the tests are not really initramfs specific. --- src/modules/initramfs/CMakeLists.txt | 17 +++++ src/modules/initramfs/Tests.cpp | 96 ++++++++++++++++++++++++++++ src/modules/initramfs/Tests.h | 39 +++++++++++ 3 files changed, 152 insertions(+) create mode 100644 src/modules/initramfs/Tests.cpp create mode 100644 src/modules/initramfs/Tests.h diff --git a/src/modules/initramfs/CMakeLists.txt b/src/modules/initramfs/CMakeLists.txt index 79bb650c5..c2496b2a1 100644 --- a/src/modules/initramfs/CMakeLists.txt +++ b/src/modules/initramfs/CMakeLists.txt @@ -7,3 +7,20 @@ calamares_add_plugin( initramfs calamares SHARED_LIB ) + +if( ECM_FOUND AND BUILD_TESTING ) + ecm_add_test( + Tests.cpp + TEST_NAME + initramfstest + LINK_LIBRARIES + ${CALAMARES_LIBRARIES} + calamares + calamares_job_initramfs # From above + ${YAMLCPP_LIBRARY} + Qt5::Core + Qt5::Test + ) + set_target_properties( initramfstest PROPERTIES AUTOMOC TRUE ) + target_include_directories( initramfstest PRIVATE /usr/local/include ) +endif() diff --git a/src/modules/initramfs/Tests.cpp b/src/modules/initramfs/Tests.cpp new file mode 100644 index 000000000..936c94097 --- /dev/null +++ b/src/modules/initramfs/Tests.cpp @@ -0,0 +1,96 @@ +/* === This file is part of Calamares - === + * + * Copyright 2019, Adriaan de Groot + * + * Calamares is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Calamares is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Calamares. If not, see . + */ + +#include "Tests.h" + +#include "GlobalStorage.h" +#include "JobQueue.h" +#include "Settings.h" + +#include "utils/CalamaresUtilsSystem.h" +#include "utils/Logger.h" +#include "utils/Yaml.h" + +#include + +#include +#include + +QTEST_GUILESS_MAIN( InitramfsTests ) + +InitramfsTests::InitramfsTests() +{ +} + +InitramfsTests::~InitramfsTests() +{ +} + +void +InitramfsTests::initTestCase() +{ + Logger::setupLogLevel( Logger::LOGDEBUG ); +} + +static const char contents[] = "UMASK=0077\n"; +static const char confFile[] = "/tmp/calamares-safe-umask"; + +void InitramfsTests::cleanup() +{ + QFile::remove( confFile ); +} + +void InitramfsTests::testCreateHostFile() +{ + + CalamaresUtils::System s( false ); // don't chroot + QString path = s.createTargetFile( confFile, QByteArray( contents ) ); + QVERIFY( !path.isEmpty() ); + QCOMPARE( path, confFile ); // don't chroot, so path create relative to / + QVERIFY( QFile::exists( confFile ) ); + + QFileInfo fi( confFile ); + QVERIFY( fi.exists() ); + QCOMPARE( fi.size(), sizeof( contents )-1 ); // don't count trailing NUL + + QFile::remove( confFile ); +} + +void InitramfsTests::testCreateTargetFile() +{ + static const char short_confFile[] = "/calamares-safe-umask"; + + CalamaresUtils::System s( true ); + QString path = s.createTargetFile( short_confFile, QByteArray( contents ) ); + QVERIFY( path.isEmpty() ); // because no rootmountpoint is set + + Calamares::JobQueue j; + j.globalStorage()->insert( "rootMountPoint", "/tmp" ); + + path = s.createTargetFile( short_confFile, QByteArray( contents ) ); + QVERIFY( path.endsWith( short_confFile ) ); // chroot, so path create relative to + QVERIFY( path.startsWith( "/tmp/" ) ); + QVERIFY( QFile::exists( path ) ); + + QFileInfo fi( path ); + QVERIFY( fi.exists() ); + QCOMPARE( fi.size(), sizeof( contents )-1 ); // don't count trailing NUL + + QFile::remove( path ); + +} diff --git a/src/modules/initramfs/Tests.h b/src/modules/initramfs/Tests.h new file mode 100644 index 000000000..01394d4fa --- /dev/null +++ b/src/modules/initramfs/Tests.h @@ -0,0 +1,39 @@ +/* === This file is part of Calamares - === + * + * Copyright 2019, Adriaan de Groot + * + * Calamares is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Calamares is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Calamares. If not, see . + */ + +#ifndef TESTS_H +#define TESTS_H + +#include + +class InitramfsTests : public QObject +{ + Q_OBJECT +public: + InitramfsTests(); + ~InitramfsTests() override; + +private Q_SLOTS: + void initTestCase(); + void cleanup(); + + void testCreateHostFile(); + void testCreateTargetFile(); +}; + +#endif From 940c99026869079a676bdc2c918d4f6b6455e33c Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Sat, 6 Jul 2019 00:04:16 +0200 Subject: [PATCH 14/15] [initcpio] [initramfs] Allow turning off CVE mitigations - The mitigations are slightly intrusive, and may clash with other, similar mitigations (especially for initramfs, the recommended solution is to configure the system with the snippet outside of Calamares). --- src/modules/initcpio/InitcpioJob.cpp | 17 +++++++++++++---- src/modules/initcpio/InitcpioJob.h | 1 + src/modules/initcpio/initcpio.conf | 5 +++++ src/modules/initramfs/InitramfsJob.cpp | 25 +++++++++++++++++-------- src/modules/initramfs/InitramfsJob.h | 1 + src/modules/initramfs/initramfs.conf | 5 +++++ 6 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/modules/initcpio/InitcpioJob.cpp b/src/modules/initcpio/InitcpioJob.cpp index d0d825cbf..38017d83e 100644 --- a/src/modules/initcpio/InitcpioJob.cpp +++ b/src/modules/initcpio/InitcpioJob.cpp @@ -59,12 +59,19 @@ InitcpioJob::exec() { CalamaresUtils::UMask m( CalamaresUtils::UMask::Safe ); - QDir d( CalamaresUtils::System::instance()->targetPath( "/boot" ) ); - if ( d.exists() ) + if ( m_unsafe ) { - fixPermissions( d ); + cDebug() << "Skipping mitigations for unsafe initramfs permissions."; } - + else + { + QDir d( CalamaresUtils::System::instance()->targetPath( "/boot" ) ); + if ( d.exists() ) + { + fixPermissions( d ); + } + } + cDebug() << "Updating initramfs with kernel" << m_kernel; auto r = CalamaresUtils::System::instance()->targetEnvCommand( { "mkinitcpio", "-p", m_kernel }, QString(), QString(), 0 ); @@ -94,6 +101,8 @@ InitcpioJob::setConfigurationMap( const QVariantMap& configurationMap ) << r.getExitCode() << r.getOutput(); } } + + m_unsafe = CalamaresUtils::getBool( configurationMap, "be_unsafe", false ); } CALAMARES_PLUGIN_FACTORY_DEFINITION( InitcpioJobFactory, registerPlugin< InitcpioJob >(); ) diff --git a/src/modules/initcpio/InitcpioJob.h b/src/modules/initcpio/InitcpioJob.h index 7c0bcf2df..11358d749 100644 --- a/src/modules/initcpio/InitcpioJob.h +++ b/src/modules/initcpio/InitcpioJob.h @@ -42,6 +42,7 @@ public: private: QString m_kernel; + bool m_unsafe = false; }; CALAMARES_PLUGIN_FACTORY_DECLARATION( InitcpioJobFactory ) diff --git a/src/modules/initcpio/initcpio.conf b/src/modules/initcpio/initcpio.conf index 487a0289d..8ad71e9f5 100644 --- a/src/modules/initcpio/initcpio.conf +++ b/src/modules/initcpio/initcpio.conf @@ -16,3 +16,8 @@ # # Note that "all" is probably not a good preset to use either. kernel: linux312 + +# Set this to true to turn off mitigations for lax file +# permissions on initramfs (which, in turn, can compromise +# your LUKS encryption keys, CVS-2019-13179). +be_unsafe: false diff --git a/src/modules/initramfs/InitramfsJob.cpp b/src/modules/initramfs/InitramfsJob.cpp index c96bbb059..01d400443 100644 --- a/src/modules/initramfs/InitramfsJob.cpp +++ b/src/modules/initramfs/InitramfsJob.cpp @@ -44,16 +44,23 @@ InitramfsJob::exec() CalamaresUtils::UMask m( CalamaresUtils::UMask::Safe ); cDebug() << "Updating initramfs with kernel" << m_kernel; - - // First make sure we generate a safe initramfs with suitable permissions. - static const char confFile[] = "/etc/initramfs-tools/conf.d/calamares-safe-initramfs.conf"; - static const char contents[] = "UMASK=0077\n"; - if ( CalamaresUtils::System::instance()->createTargetFile( confFile, QByteArray( contents ) ).isEmpty() ) + + if ( m_unsafe ) { - cWarning() << Logger::SubEntry << "Could not configure safe UMASK for initramfs."; - // But continue anyway. + cDebug() << "Skipping mitigations for unsafe initramfs permissions."; } - + else + { + // First make sure we generate a safe initramfs with suitable permissions. + static const char confFile[] = "/etc/initramfs-tools/conf.d/calamares-safe-initramfs.conf"; + static const char contents[] = "UMASK=0077\n"; + if ( CalamaresUtils::System::instance()->createTargetFile( confFile, QByteArray( contents ) ).isEmpty() ) + { + cWarning() << Logger::SubEntry << "Could not configure safe UMASK for initramfs."; + // But continue anyway. + } + } + // And then do the ACTUAL work. auto r = CalamaresUtils::System::instance()->targetEnvCommand( { "update-initramfs", "-k", m_kernel, "-c", "-t" }, QString(), QString(), 0 ); @@ -84,6 +91,8 @@ InitramfsJob::setConfigurationMap( const QVariantMap& configurationMap ) << r.getExitCode() << r.getOutput(); } } + + m_unsafe = CalamaresUtils::getBool( configurationMap, "be_unsafe", false ); } CALAMARES_PLUGIN_FACTORY_DEFINITION( InitramfsJobFactory, registerPlugin< InitramfsJob >(); ) diff --git a/src/modules/initramfs/InitramfsJob.h b/src/modules/initramfs/InitramfsJob.h index 63aed4136..9eeb81fea 100644 --- a/src/modules/initramfs/InitramfsJob.h +++ b/src/modules/initramfs/InitramfsJob.h @@ -42,6 +42,7 @@ public: private: QString m_kernel; + bool m_unsafe = false; }; CALAMARES_PLUGIN_FACTORY_DECLARATION( InitramfsJobFactory ) diff --git a/src/modules/initramfs/initramfs.conf b/src/modules/initramfs/initramfs.conf index 4e5eda202..a989d83c3 100644 --- a/src/modules/initramfs/initramfs.conf +++ b/src/modules/initramfs/initramfs.conf @@ -29,3 +29,8 @@ # 3.2.9 and earlier which passed "all" as version. kernel: "all" + +# Set this to true to turn off mitigations for lax file +# permissions on initramfs (which, in turn, can compromise +# your LUKS encryption keys, CVS-2019-13179). +be_unsafe: false From 937dac47d8c7a9ad2944e306dac7a9e6b374cd39 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 5 Jul 2019 23:33:46 +0200 Subject: [PATCH 15/15] Changes: refer to CVE numbers for both issues --- CHANGES | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGES b/CHANGES index 723747e44..37dc7a71c 100644 --- a/CHANGES +++ b/CHANGES @@ -16,7 +16,7 @@ understanding the issues (alphabetically by first name): - Seth Arnold - Simon Quigley - Thomas Ward - +Both CVE's have been resolved. ## Core ## @@ -26,7 +26,7 @@ No core changes. - *initramfs* could create an initramfs with insecure permissions. Since the keyfile is included in the initramfs, an attacker could - read the file from the initramfs. #1190 + read the file from the initramfs. #1190 CVE-2019-13178 - *luksbootkeyfile* created a key file where a window of opportunity existed where the key file could have too-lax file permissions. #1191 CVE-2019-13179