Hi,
please, find attached a rework of the AppArmor patch to harden/confine possible
side effects of converters via an AppArmor profile on Linux.
The major challenge here is to ship with a meaningful AA profile -- I'd be happy to
hear feedback about this very point, namely what do we need to deny-vs-grant access
to, while executing external converters. Due to the fact that converters tend to
handle their own temporary and preference files et al., I'd be for a permissive
settings, where we deny more and more things as we get awareness of potentially
sensitive files. I'm reporting right here the relevant part of the patch (e.g.,
trying to deny access to any SSH keys, X auth info, shell history, private e-mail
and browser history/whatever, plus networking & access to sound/video devices
should not be possible with this set-up -- might be incomplete though):
+@prefix@/bin/lyxwrap@version_suffix@ {
+ owner /tmp/** ixrwl,
+ @{HOME}/** ixr,
+ @{HOME}/.lyx@version_suffix@** ixrwl,
+ @{HOME}/.config/LyX** ixrwl,
+ / ixr,
+ /** ixr,
+
+ deny @{HOME}/.ssh/** xrwl,
+ deny @{HOME}/.Xauthority* xrwl,
+ deny @{HOME}/.xsession* xrwl,
+ deny @{HOME}/.bash_history xrwl,
+ deny @{HOME}/.mozilla/** xrwl,
+ deny @{HOME}/.thunderbird/** xrwl,
+ deny @{HOME}/.git/** xrwl,
+ deny /etc/security/ xrwl,
+ deny /etc/security/** xrwl,
+ deny /etc/passwd* xrwl,
+ deny /etc/shadow* xrwl,
+}
Thanks,
T.
>From 7fd71406a7a35e57b9e70f9d716eb8493073c58d Mon Sep 17 00:00:00 2001
From: Tommaso Cucinotta <tomm...@lyx.org>
Date: Fri, 11 Nov 2016 23:42:11 +0100
Subject: [PATCH] Hardening LyX: external converters sandboxed via AppArmor on
Linux.
External converters are now run using a wrapper (lyxwrap), whose
pathname is matches within a corresponding AppArmor profile,
specifying several access restrictions, such as no ability to
write into the user's home directory (only writing into LyX's own
temporary directory is allowed), no ability to access SSH keys,
no ability to read any system-wide security critical files like
/etc/{passwd,shadow} etc..
The new wrapping through lyxwrap, and the corresponding
AppArmor profile installation, are only meaningful on Linux,
and are only used if --enable-apparmor is used at configure time.
In the future, similar features might be provided on other OSes.
For more information, see #10481.
---
config/apparmor.m4 | 15 +++++++++++++++
configure.ac | 2 ++
lib/Makefile.am | 4 ++++
lib/usr.bin.lyxwrap.in | 25 +++++++++++++++++++++++++
src/Converter.cpp | 12 ++++++++----
src/LyXRC.cpp | 19 +++++++++++++++++++
src/LyXRC.h | 3 +++
src/frontends/qt4/GuiPrefs.cpp | 4 ++++
src/frontends/qt4/ui/PrefConvertersUi.ui | 10 ++++++++++
src/graphics/GraphicsConverter.cpp | 2 +-
src/graphics/PreviewLoader.cpp | 4 ++--
src/support/Systemcall.cpp | 8 ++++----
src/support/Systemcall.h | 2 +-
src/support/filetools.cpp | 14 ++++++++++++++
src/support/filetools.h | 3 +++
15 files changed, 115 insertions(+), 12 deletions(-)
create mode 100644 config/apparmor.m4
create mode 100644 lib/usr.bin.lyxwrap.in
diff --git a/config/apparmor.m4 b/config/apparmor.m4
new file mode 100644
index 00000000..714fc514
--- /dev/null
+++ b/config/apparmor.m4
@@ -0,0 +1,15 @@
+dnl Check whether to configure with AppArmor-based hardening
+dnl
+AC_DEFUN([LYX_CHECK_APPARMOR],[
+ AC_MSG_CHECKING([whether to enable AppArmor support])
+ AC_ARG_ENABLE([apparmor],
+ AC_HELP_STRING([--enable-apparmor],[compile with AppArmor support]))
+ if test "$enable_apparmor" = "yes"; then
+ AC_MSG_RESULT(yes)
+ AC_DEFINE(USE_APPARMOR, 1, [Define as 1 to enable AppArmor support])
+ lyx_flags="$lyx_flags use-apparmor"
+ else
+ AC_MSG_RESULT(no)
+ fi
+ AM_CONDITIONAL([USE_APPARMOR], test "$enable_apparmor" = "yes")
+])
diff --git a/configure.ac b/configure.ac
index 75df6c75..066dbbd2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -221,6 +221,8 @@ AC_FUNC_SELECT_ARGTYPES
LYX_CHECK_SPELL_ENGINES
+LYX_CHECK_APPARMOR
+
lyx_client_subdir=true
dnl AC_LANG_PUSH(C)
dnl LIBS already contains some X extra libs that may interfere.
diff --git a/lib/Makefile.am b/lib/Makefile.am
index ee7e2b14..1ce12f76 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -2485,6 +2485,10 @@ install-data-hook:
chmod 644 "$(DESTDIR)$(pkgdatadir)/$$i"; \
fi; \
done
+if USE_APPARMOR
+ mkdir -p $(DESTDIR)/etc/apparmor.d && chmod 755 $(DESTDIR)/etc/apparmor.d
+ cp usr.bin.lyxwrap $(DESTDIR)/etc/apparmor.d/
+endif
alltests: check alltests-recursive
diff --git a/lib/usr.bin.lyxwrap.in b/lib/usr.bin.lyxwrap.in
new file mode 100644
index 00000000..d9504986
--- /dev/null
+++ b/lib/usr.bin.lyxwrap.in
@@ -0,0 +1,25 @@
+# vim:syntax=apparmor
+# Author: Tommaso Cucinotta <tomm...@lyx.org>
+
+#include <tunables/global>
+
+@prefix@/bin/lyxwrap@version_suffix@ {
+ owner /tmp/** ixrwl,
+ @{HOME}/** ixr,
+ @{HOME}/.lyx@version_suffix@** ixrwl,
+ @{HOME}/.config/LyX** ixrwl,
+ / ixr,
+ /** ixr,
+
+ deny @{HOME}/.ssh/** xrwl,
+ deny @{HOME}/.Xauthority* xrwl,
+ deny @{HOME}/.xsession* xrwl,
+ deny @{HOME}/.bash_history xrwl,
+ deny @{HOME}/.mozilla/** xrwl,
+ deny @{HOME}/.thunderbird/** xrwl,
+ deny @{HOME}/.git/** xrwl,
+ deny /etc/security/ xrwl,
+ deny /etc/security/** xrwl,
+ deny /etc/passwd* xrwl,
+ deny /etc/shadow* xrwl,
+}
diff --git a/src/Converter.cpp b/src/Converter.cpp
index 1027af65..e70448ed 100644
--- a/src/Converter.cpp
+++ b/src/Converter.cpp
@@ -364,7 +364,8 @@ bool Converters::convert(Buffer const * buffer,
Systemcall one;
one.startscript(Systemcall::Wait, command,
buffer ? buffer->filePath() : string(),
- buffer ? buffer->layoutPos() : string());
+ buffer ? buffer->layoutPos() : string(),
+ false, lyxrc.use_converter_hardening);
if (to_file.isReadableFile()) {
if (conversionflags & try_cache)
ConverterCache::get().add(orig_from,
@@ -537,7 +538,8 @@ bool Converters::convert(Buffer const * buffer,
res = one.startscript(Systemcall::DontWait,
to_filesystem8bit(from_utf8(command)),
buffer ? buffer->filePath() : string(),
- buffer ? buffer->layoutPos() : string());
+ buffer ? buffer->layoutPos() : string(),
+ false, lyxrc.use_converter_hardening);
// We're not waiting for the result, so we can't do anything
// else here.
} else {
@@ -546,7 +548,8 @@ bool Converters::convert(Buffer const * buffer,
buffer ? buffer->filePath()
: string(),
buffer ? buffer->layoutPos()
- : string());
+ : string(),
+ false, lyxrc.use_converter_hardening);
if (!real_outfile.empty()) {
Mover const & mover = getMover(conv.to());
if (!mover.rename(outfile, real_outfile))
@@ -567,7 +570,8 @@ bool Converters::convert(Buffer const * buffer,
one.startscript(Systemcall::Wait,
to_filesystem8bit(from_utf8(command2)),
buffer->filePath(),
- buffer->layoutPos());
+ buffer->layoutPos(),
+ false, lyxrc.use_converter_hardening);
if (!scanLog(*buffer, command, makeAbsPath(logfile, path), errorList))
return false;
}
diff --git a/src/LyXRC.cpp b/src/LyXRC.cpp
index 587e1e7c..1c8a2f69 100644
--- a/src/LyXRC.cpp
+++ b/src/LyXRC.cpp
@@ -191,6 +191,7 @@ LexerKeyword lyxrcTags[] = {
{ "\\use_converter_cache", LyXRC::RC_USE_CONVERTER_CACHE },
{ "\\use_converter_needauth", LyXRC::RC_USE_CONVERTER_NEEDAUTH },
{ "\\use_converter_needauth_forbidden", LyXRC::RC_USE_CONVERTER_NEEDAUTH_FORBIDDEN },
+ { "\\use_converter_hardening", LyXRC::RC_USE_CONVERTER_HARDENING },
{ "\\use_lastfilepos", LyXRC::RC_USELASTFILEPOS },
{ "\\use_pixmap_cache", LyXRC::RC_USE_PIXMAP_CACHE },
{ "\\use_qimage", LyXRC::RC_USE_QIMAGE },
@@ -320,6 +321,7 @@ void LyXRC::setDefaults()
use_converter_cache = true;
use_converter_needauth_forbidden = true;
use_converter_needauth = true;
+ use_converter_hardening = true;
use_system_colors = false;
use_tooltip = true;
use_pixmap_cache = false;
@@ -1122,6 +1124,9 @@ LyXRC::ReturnValues LyXRC::read(Lexer & lexrc, bool check_format)
case RC_USE_CONVERTER_NEEDAUTH:
lexrc >> use_converter_needauth;
break;
+ case RC_USE_CONVERTER_HARDENING:
+ lexrc >> use_converter_hardening;
+ break;
case RC_CONVERTER_CACHE_MAXAGE:
lexrc >> converter_cache_maxage;
break;
@@ -1621,6 +1626,15 @@ void LyXRC::write(ostream & os, bool ignore_system_lyxrc, string const & name) c
if (tag != RC_LAST)
break;
+ case RC_USE_CONVERTER_HARDENING:
+ if (ignore_system_lyxrc ||
+ use_converter_hardening != system_lyxrc.use_converter_hardening) {
+ os << "\\use_converter_hardening "
+ << convert<string>(use_converter_hardening) << '\n';
+ }
+ if (tag != RC_LAST)
+ break;
+
case RC_CONVERTER_CACHE_MAXAGE:
if (ignore_system_lyxrc ||
converter_cache_maxage != system_lyxrc.converter_cache_maxage) {
@@ -2863,6 +2877,7 @@ void actOnUpdatedPrefs(LyXRC const & lyxrc_orig, LyXRC const & lyxrc_new)
case LyXRC::RC_USE_CONVERTER_CACHE:
case LyXRC::RC_USE_CONVERTER_NEEDAUTH_FORBIDDEN:
case LyXRC::RC_USE_CONVERTER_NEEDAUTH:
+ case LyXRC::RC_USE_CONVERTER_HARDENING:
case LyXRC::RC_USE_SYSTEM_COLORS:
case LyXRC::RC_USE_TOOLTIP:
case LyXRC::RC_USE_PIXMAP_CACHE:
@@ -2963,6 +2978,10 @@ string const LyXRC::getDescription(LyXRCTags tag)
str = _("Ask user before calling external converters with 'needauth' option to prevent undesired effects.");
break;
+ case RC_CONVERTER_HARDENING:
+ str = _("Apply hardening techniques when calling external converters to prevent undesired effects.");
+ break;
+
case RC_COPIER:
break;
diff --git a/src/LyXRC.h b/src/LyXRC.h
index 8a9cc172..d7540671 100644
--- a/src/LyXRC.h
+++ b/src/LyXRC.h
@@ -169,6 +169,7 @@ public:
RC_USE_CONVERTER_CACHE,
RC_USE_CONVERTER_NEEDAUTH_FORBIDDEN,
RC_USE_CONVERTER_NEEDAUTH,
+ RC_USE_CONVERTER_HARDENING,
RC_USE_SYSTEM_COLORS,
RC_USE_TOOLTIP,
RC_USE_PIXMAP_CACHE,
@@ -449,6 +450,8 @@ public:
bool use_converter_needauth_forbidden;
/// Ask user before calling external converters with 'needauth' option
bool use_converter_needauth;
+ /// Apply hardening when calling external converters
+ bool use_converter_hardening;
/// The maximum age of cache files in seconds
unsigned int converter_cache_maxage;
/// Sort layouts alphabetically
diff --git a/src/frontends/qt4/GuiPrefs.cpp b/src/frontends/qt4/GuiPrefs.cpp
index ee43b4fd..f80c67e4 100644
--- a/src/frontends/qt4/GuiPrefs.cpp
+++ b/src/frontends/qt4/GuiPrefs.cpp
@@ -1614,6 +1614,8 @@ PrefConverters::PrefConverters(GuiPreferences * form)
this, SIGNAL(changed()));
connect(needauthCB, SIGNAL(toggled(bool)),
this, SIGNAL(changed()));
+ connect(hardeningCB, SIGNAL(toggled(bool)),
+ this, SIGNAL(changed()));
converterED->setValidator(new NoNewLineValidator(converterED));
converterFlagED->setValidator(new NoNewLineValidator(converterFlagED));
@@ -1627,6 +1629,7 @@ void PrefConverters::applyRC(LyXRC & rc) const
rc.use_converter_cache = cacheCB->isChecked();
rc.use_converter_needauth_forbidden = needauthForbiddenCB->isChecked();
rc.use_converter_needauth = needauthCB->isChecked();
+ rc.use_converter_hardening = hardeningCB->isChecked();
rc.converter_cache_maxage = int(widgetToDouble(maxAgeLE) * 86400.0);
}
@@ -1636,6 +1639,7 @@ void PrefConverters::updateRC(LyXRC const & rc)
cacheCB->setChecked(rc.use_converter_cache);
needauthForbiddenCB->setChecked(rc.use_converter_needauth_forbidden);
needauthCB->setChecked(rc.use_converter_needauth);
+ hardeningCB->setChecked(rc.use_converter_hardening);
QString max_age;
doubleToWidget(maxAgeLE, (double(rc.converter_cache_maxage) / 86400.0), 'g', 6);
updateGui();
diff --git a/src/frontends/qt4/ui/PrefConvertersUi.ui b/src/frontends/qt4/ui/PrefConvertersUi.ui
index f9c02a86..eb022e2f 100644
--- a/src/frontends/qt4/ui/PrefConvertersUi.ui
+++ b/src/frontends/qt4/ui/PrefConvertersUi.ui
@@ -326,6 +326,16 @@
</property>
</widget>
</item>
+ <item>
+ <widget class="QCheckBox" name="hardeningCB" >
+ <property name="text" >
+ <string>Use &hardening with converters</string>
+ </property>
+ <property name="toolTip">
+ <string>When enabled, run converters in a sandbox, to harden LyX against possible undesired side effects.</string>
+ </property>
+ </widget>
+ </item>
</layout>
</item>
</layout>
diff --git a/src/graphics/GraphicsConverter.cpp b/src/graphics/GraphicsConverter.cpp
index 55ae2462..9ebebf8b 100644
--- a/src/graphics/GraphicsConverter.cpp
+++ b/src/graphics/GraphicsConverter.cpp
@@ -396,7 +396,7 @@ static void build_script(string const & doc_fname,
// See comment about extra " quotes above (although that
// applies only for the first loop run here).
- string command = conv.command();
+ string command = commandWrap(conv.command(), lyxrc.use_converter_hardening);
command = subst(command, token_from, "' + '\"' + infile + '\"' + '");
command = subst(command, token_base, "' + '\"' + infile_base + '\"' + '");
command = subst(command, token_to, "' + '\"' + outfile + '\"' + '");
diff --git a/src/graphics/PreviewLoader.cpp b/src/graphics/PreviewLoader.cpp
index df025e41..0fb52a45 100644
--- a/src/graphics/PreviewLoader.cpp
+++ b/src/graphics/PreviewLoader.cpp
@@ -731,7 +731,7 @@ void PreviewLoader::Impl::startLoading(bool wait)
if (wait) {
ForkedCall call(buffer_.filePath(), buffer_.layoutPos());
- int ret = call.startScript(ForkedProcess::Wait, command);
+ int ret = call.startScript(ForkedProcess::Wait, commandWrap(command, lyxrc.use_converter_hardening));
static atomic_int fake((2^20) + 1);
int pid = fake++;
inprogress.pid = pid;
@@ -747,7 +747,7 @@ void PreviewLoader::Impl::startLoading(bool wait)
convert_ptr->connect(bind(&Impl::finishedGenerating, this, _1, _2));
ForkedCall call(buffer_.filePath());
- int ret = call.startScript(command, convert_ptr);
+ int ret = call.startScript(commandWrap(command, lyxrc.use_converter_hardening), convert_ptr);
if (ret != 0) {
LYXERR(Debug::GRAPHICS, "PreviewLoader::startLoading()\n"
diff --git a/src/support/Systemcall.cpp b/src/support/Systemcall.cpp
index cc2c3814..c8956576 100644
--- a/src/support/Systemcall.cpp
+++ b/src/support/Systemcall.cpp
@@ -103,11 +103,11 @@ ProgressInterface * ProgressInterface::instance()
#ifndef USE_QPROCESS
int Systemcall::startscript(Starttype how, string const & what,
string const & path, string const & lpath,
- bool /*process_events*/)
+ bool /*process_events*/, bool wrap)
{
string command =
to_filesystem8bit(from_utf8(latexEnvCmdPrefix(path, lpath)))
- + commandPrep(what);
+ + commandWrap(commandPrep(what), wrap);
if (how == DontWait) {
switch (os::shell()) {
@@ -237,9 +237,9 @@ string const parsecmd(string const & incmd, string & infile, string & outfile,
int Systemcall::startscript(Starttype how, string const & what,
string const & path, string const & lpath,
- bool process_events)
+ bool process_events, bool wrap)
{
- string const what_ss = commandPrep(what);
+ string const what_ss = commandWrap(commandPrep(what), wrap);
if (verbose)
lyxerr << "\nRunning: " << what_ss << endl;
else
diff --git a/src/support/Systemcall.h b/src/support/Systemcall.h
index 876aa357..62cae499 100644
--- a/src/support/Systemcall.h
+++ b/src/support/Systemcall.h
@@ -51,7 +51,7 @@ public:
int startscript(Starttype how, std::string const & what,
std::string const & path = empty_string(),
std::string const & lpath = empty_string(),
- bool process_events = false);
+ bool process_events = false, bool wrap = false);
};
} // namespace support
diff --git a/src/support/filetools.cpp b/src/support/filetools.cpp
index 12457cb3..88213569 100644
--- a/src/support/filetools.cpp
+++ b/src/support/filetools.cpp
@@ -458,6 +458,20 @@ string const commandPrep(string const & command_in)
}
+string const commandWrap(string const & cmd, bool wrap)
+{
+ string wrap_cmd = cmd;
+ if (wrap) {
+#ifdef USE_APPARMOR
+ if (os::shell() == os::UNIX)
+ wrap_cmd = string("lyxwrap") + PROGRAM_SUFFIX + " " + cmd;
+#endif
+ }
+ LYXERR(Debug::FILES, "commandWrap(): " + wrap_cmd);
+ return wrap_cmd;
+}
+
+
static string createTempFile(QString const & mask)
{
// FIXME: This is not safe. QTemporaryFile creates a file in open(),
diff --git a/src/support/filetools.h b/src/support/filetools.h
index 5177cddd..eb1bfb00 100644
--- a/src/support/filetools.h
+++ b/src/support/filetools.h
@@ -140,6 +140,9 @@ enum quote_style {
*/
std::string const commandPrep(std::string const & command);
+/// If wrap is true, then the command is launched through lyxwrap.
+std::string const commandWrap(std::string const & command, bool wrap);
+
enum latex_path_extension {
PROTECT_EXTENSION,
EXCLUDE_EXTENSION
--
2.9.3
