Merge pull request #787 from pkgw/security-api

Add a new centralized security API

Cargo.toml

@@ -125,7 +125,7 @@ x86_64-unknown-linux-gnu = { install = ["fontconfig","freetype","harfbuzz[icu,gr
 x86_64-pc-windows-msvc = { triplet = "x64-windows-static", install = ["fontconfig","freetype","harfbuzz[icu,graphite2]"] }
 
 [package.metadata.internal_dep_versions]
-tectonic_bridge_core = "4e16bf963700aae59772a6fb223981ceaa9b5f57"
+tectonic_bridge_core = "thiscommit:2021-06-14:3sp2O1O"
 tectonic_bridge_flate = "thiscommit:2021-01-01:eer4ahL4"
 tectonic_bridge_graphite2 = "2c1ffcd702a662c003bd3d7d0ca4d169784cb6ad"
 tectonic_bridge_harfbuzz = "2c1ffcd702a662c003bd3d7d0ca4d169784cb6ad"

crates/bridge_core/README.md

@@ -28,6 +28,6 @@ use tectonic_bridge_core;
 
 ## Cargo features
 
-This crate does not currently provides any [Cargo features][features].
+This crate does not currently provide any [Cargo features][features].
 
 [features]: https://doc.rust-lang.org/cargo/reference/features.html

crates/bridge_core/src/lib.rs

@@ -216,14 +216,32 @@ impl std::error::Error for EngineAbortedError {}
 pub struct CoreBridgeLauncher<'a> {
     hooks: &'a mut dyn DriverHooks,
     status: &'a mut dyn StatusBackend,
+    security: SecuritySettings,
 }
 
 impl<'a> CoreBridgeLauncher<'a> {
     /// Set up a new context for launching bridged FFI code.
+    ///
+    /// This function uses the default security stance, which disallows all
+    /// known-insecure engine features. Use [`Self::new_with_security`] to
+    /// provide your own security settings that can attempt to allow the use of
+    /// such features.
     pub fn new(hooks: &'a mut dyn DriverHooks, status: &'a mut dyn StatusBackend) -> Self {
-        CoreBridgeLauncher { hooks, status }
+        Self::new_with_security(hooks, status, SecuritySettings::default())
     }
 
+    /// Set up a new context for launching bridged FFI code.
+    pub fn new_with_security(
+        hooks: &'a mut dyn DriverHooks,
+        status: &'a mut dyn StatusBackend,
+        security: SecuritySettings,
+    ) -> Self {
+        CoreBridgeLauncher {
+            hooks,
+            status,
+            security,
+        }
+    }
     /// Invoke a function to launch a bridged FFI engine with a global mutex
     /// held.
     ///
@@ -242,7 +260,7 @@ impl<'a> CoreBridgeLauncher<'a> {
         F: FnOnce(&mut CoreBridgeState<'_>) -> Result<T>,
     {
         let _guard = ENGINE_LOCK.lock().unwrap();
-        let mut state = CoreBridgeState::new(self.hooks, self.status);
+        let mut state = CoreBridgeState::new(self.security.clone(), self.hooks, self.status);
         let result = callback(&mut state);
 
         if let Err(ref e) = result {
@@ -262,6 +280,9 @@ impl<'a> CoreBridgeLauncher<'a> {
 /// these state structures into the C/C++ layer. It is essential that lifetimes
 /// be properly managed across the Rust/C boundary.
 pub struct CoreBridgeState<'a> {
+    /// The security settings for this invocation
+    security: SecuritySettings,
+
     /// The driver hooks associated with this engine invocation.
     hooks: &'a mut dyn DriverHooks,
 
@@ -286,10 +307,12 @@ pub struct CoreBridgeState<'a> {
 
 impl<'a> CoreBridgeState<'a> {
     fn new(
+        security: SecuritySettings,
         hooks: &'a mut dyn DriverHooks,
         status: &'a mut dyn StatusBackend,
     ) -> CoreBridgeState<'a> {
         CoreBridgeState {
+            security,
             hooks,
             status,
             output_handles: Vec::new(),
@@ -636,22 +659,119 @@ impl<'a> CoreBridgeState<'a> {
     }
 
     fn shell_escape(&mut self, command: &str) -> bool {
-        match self.hooks.sysrq_shell_escape(command, self.status) {
-            Ok(_) => false,
+        if self.security.allow_shell_escape() {
+            match self.hooks.sysrq_shell_escape(command, self.status) {
+                Ok(_) => false,
 
-            Err(e) => {
-                tt_error!(
-                    self.status,
-                    "failed to execute the shell-escape command \"{}\": {}",
-                    command,
-                    e
-                );
-                true
+                Err(e) => {
+                    tt_error!(
+                        self.status,
+                        "failed to execute the shell-escape command \"{}\": {}",
+                        command,
+                        e
+                    );
+                    true
+                }
             }
+        } else {
+            tt_error!(
+                self.status,
+                "forbidden to execute shell-escape command \"{}\"",
+                command
+            );
+            true
         }
     }
 }
 
+/// A type for storing settings about potentially insecure engine features.
+///
+/// This type encapsulates configuration about which potentially insecure engine
+/// features are enabled. Methods that configure or instantiate engines require
+/// values of this type, and values of this type can only be created through
+/// centralized methods that respect standard environment variables, ensuring
+/// that there is some level of uniform control over the activation of any
+/// known-insecure features.
+///
+/// The purpose of this framework is to manage the use of engine features that
+/// are known to create security risks with *untrusted* input, but that trusted
+/// users may wish to use due to the extra functionalities they bring. (This is
+/// why these are settings and not simply security flaws!) The primary example
+/// of this is the TeX engine’s shell-escape feature.
+///
+/// Of course, this framework is only as good as our understanding of Tectonic’s
+/// security profile. Future versions might disable or restrict different pieces
+/// of functionality as new risks are discovered.
+#[derive(Clone, Debug)]
+pub struct SecuritySettings {
+    /// While we might eventually gain finer-grained enable/disable settings,
+    /// there should always be a hard "disable everything known to be risky"
+    /// option that supersedes everything else.
+    disable_insecures: bool,
+}
+
+/// Different high-level security stances that can be adopted when creating
+/// [`SecuritySettings`].
+#[derive(Clone, Debug)]
+pub enum SecurityStance {
+    /// Ensure that all known-insecure features are disabled.
+    ///
+    /// Use this stance if you are processing untrusted input.
+    DisableInsecures,
+
+    /// Request to allow the use of known-insecure features.
+    ///
+    /// Use this stance if you are processing trusted input *and* there is some
+    /// user-level request to use such features. The request to allow insecure
+    /// features might be overridden if the environment variable
+    /// `TECTONIC_UNTRUSTED_MODE` is set.
+    MaybeAllowInsecures,
+}
+
+impl Default for SecurityStance {
+    fn default() -> Self {
+        // Obvi, the default is secure!!!
+        SecurityStance::DisableInsecures
+    }
+}
+
+impl SecuritySettings {
+    /// Create a new security configuration.
+    ///
+    /// The *stance* argument specifies the high-level security stance. If your
+    /// program will be run by a trusted user, they should be able to control
+    /// the setting through a command-line argument or something comparable.
+    /// Even if there is a request to enable known-insecure features, however,
+    /// such a request might be overridden by other mechanisms. In particular,
+    /// if the environment variable `TECTONIC_UNTRUSTED_MODE` is set to any
+    /// value, insecure features will always be disabled regardless of the
+    /// user-level setting. Other mechanisms for disable known-insecure features
+    /// may be added in the future.
+    pub fn new(stance: SecurityStance) -> Self {
+        let disable_insecures = if std::env::var_os("TECTONIC_UNTRUSTED_MODE").is_some() {
+            true
+        } else {
+            match stance {
+                SecurityStance::DisableInsecures => true,
+                SecurityStance::MaybeAllowInsecures => false,
+            }
+        };
+
+        SecuritySettings { disable_insecures }
+    }
+
+    /// Query whether the shell-escape TeX engine feature is allowed to be used.
+    pub fn allow_shell_escape(&self) -> bool {
+        !self.disable_insecures
+    }
+}
+
+impl Default for SecuritySettings {
+    fn default() -> Self {
+        SecuritySettings::new(SecurityStance::default())
+    }
+}
+
 // The entry points.
 
 /// Issue a warning.

docs/src/v2cli/build.md

@@ -17,6 +17,7 @@ tectonic -X build
   [--only-cached]
   [--print]
   [--open]
+  [--untrusted]
 ```
 
 #### Remarks
@@ -52,3 +53,15 @@ identical to, the contents of the log file. By default, this output is only
 printed if the engine encounteres a fatal error.
 
 The `--open` option will open the built document using the system handler.
+
+Use the `--untrusted` option if building untrusted content. This is not the
+default because in most cases you *will* trust the document that you’re
+building, probably because you have created it yourself, and it would be very
+annoying to have to pass `--trusted` every time you build a document that uses
+shell-escape. See the security discussion in the documentation of the
+[compile](./compile.md) command for details. In actual usage, it would obviously
+be easy to forget to use this option; in cases where untrusted inputs are a
+genuine concern, we recommend setting the environment variable
+`TECTONIC_UNTRUSTED_MODE` to a non-empty value. This has the same effect as the
+`--untrusted` option. Note, however, that a hostile shell user can trivially
+clear this variable.

docs/src/v2cli/compile.md

@@ -36,6 +36,7 @@ tectonic -X compile  # full form
   [--print] [-p]
   [--reruns COUNT] [-r COUNT]
   [--synctex]
+  [--untrusted]
   [--web-bundle URL] [-w]
   [-Z UNSTABLE-OPTION]
   TEXPATH
@@ -63,6 +64,27 @@ This will compile the file and create `myfile.pdf` if nothing went wrong. You
 can use an input filename of `-` to have Tectonic process standard input. (In
 this case, the output file will be named `texput.pdf`.)
 
+##### Security
+
+By default, the document is compiled in a “trusted” mode. This means that the
+calling user can request to enable certain engine features that could raise
+security concerns if used with untrusted input: the classic example of this
+being TeX's “shell-escape” functionality. These features are *not* enabled by
+default, but they can be enabled on the command line; in the case of
+shell-escape, this is done with `-Z shell-escape`.
+
+If the command-line argument `--untrusted` is provided, these features cannot be
+enabled, regardless of other settings such as `-Z shell-escape`. So if you are
+going to process untrusted input in a command-line script, as long as you make
+sure that `--untrusted` is provided, the known-dangerous features will be
+disabled.
+
+Furthermore, if the environment variable `TECTONIC_UNTRUSTED_MODE` is set to a
+non-empty value, Tectonic will behave as if `--untrusted` were specified,
+regardless of the actual command-line arguments. Setting this variable can
+provide a modest extra layer of protection if the Tectonic engine is being run
+outside of its CLI form. Keep in mind that untrusted shell scripts and the like
+can trivially defeat this by explicitly clearing the environment variable.
 
 #### Options
 
@@ -87,6 +109,7 @@ The following are the available flags.
 | `-p`  | `--print`                 | Print the engine's chatter during processing |
 | `-r`  | `--reruns <COUNT>`        | Rerun the TeX engine exactly this many times after the first |
 |       | `--synctex`               | Generate SyncTeX data |
+|       | `--untrusted`             | Input is untrusted: disable all known-insecure features |
 | `-V`  | `--version`               | Prints version information |
 | `-w`  | `--web-bundle <URL>`      | Use this URL find resource files instead of the default |
 | `-Z`  | `-Z <UNSTABLE-OPTION>`    | Activate experimental “unstable” options |
@@ -102,5 +125,5 @@ the set of unstable options is subject to change at any time.
 | `-Z continue-on-errors`  | Keep compiling even when severe errors occur |
 | `-Z min-crossrefs=<num>` | Equivalent to bibtex's `-min-crossrefs` flag. Default vaue: 2 |
 | `-Z paper-size=<spec>`   | Change the initial paper size. Default: `letter` |
-| `-Z shell-escape`        | Enable `\write18` |
+| `-Z shell-escape`        | Enable `\write18` (unless `--untrusted` has been specified) |
 

src/bin/tectonic/compile.rs

@@ -1,18 +1,18 @@
-// Copyright 2016-2020 the Tectonic Project
+// Copyright 2016-2021 the Tectonic Project
 // Licensed under the MIT License.
 
 //! Standalone compilation of TeX documents. This implements the "classic" /
 //! "V1" / "rustc-like" Tectonic command-line interface, as well as the
 //! `compile` subcommand of the "V2" / "cargo-like" interface.
 
-use structopt::StructOpt;
-
 use std::{
     env,
     path::{Path, PathBuf},
     str::FromStr,
     time,
 };
+use structopt::StructOpt;
+use tectonic_bridge_core::{SecuritySettings, SecurityStance};
 
 use tectonic::{
     config::PersistentConfig,
@@ -87,6 +87,10 @@ pub struct CompileOptions {
     #[structopt(name = "outdir", short, long, parse(from_os_str))]
     outdir: Option<PathBuf>,
 
+    /// Input is untrusted -- disable all known-insecure features
+    #[structopt(long)]
+    untrusted: bool,
+
     /// Unstable options. Pass -Zhelp to show a list
     // TODO we can't pass -Zhelp without also passing <input>
     #[structopt(name = "option", short = "Z", number_of_values = 1)]
@@ -97,7 +101,18 @@ impl CompileOptions {
     pub fn execute(self, config: PersistentConfig, status: &mut dyn StatusBackend) -> Result<i32> {
         let unstable = UnstableOptions::from_unstable_args(self.unstable.into_iter());
 
-        let mut sess_builder = ProcessingSessionBuilder::default();
+        // Default to allowing insecure since it would be super duper annoying
+        // to have to pass `--trusted` every time to build a personal document
+        // that uses shell-escape! This default can be overridden by setting the
+        // environment variable TECTONIC_UNTRUSTED_MODE to a nonempty value.
+        let stance = if self.untrusted {
+            SecurityStance::DisableInsecures
+        } else {
+            SecurityStance::MaybeAllowInsecures
+        };
+
+        let mut sess_builder =
+            ProcessingSessionBuilder::new_with_security(SecuritySettings::new(stance));
         let format_path = self.format;
         sess_builder
             .unstables(unstable)

src/bin/tectonic/v2cli.rs

@@ -15,6 +15,7 @@ use tectonic::{
     status::{termcolor::TermcolorStatusBackend, ChatterLevel, StatusBackend},
     tt_error, tt_note,
 };
+use tectonic_bridge_core::{SecuritySettings, SecurityStance};
 use tectonic_bundles::Bundle;
 use tectonic_docmodel::workspace::{Workspace, WorkspaceCreator};
 use tectonic_errors::Error as NewError;
@@ -178,6 +179,10 @@ impl Commands {
 /// `build`: Build a document
 #[derive(Debug, PartialEq, StructOpt)]
 pub struct BuildCommand {
+    /// Document is untrusted -- disable all known-insecure features
+    #[structopt(long)]
+    untrusted: bool,
+
     /// Use only resource files cached locally
     #[structopt(short = "C", long)]
     only_cached: bool,
@@ -206,8 +211,18 @@ impl BuildCommand {
         let ws = Workspace::open_from_environment()?;
         let doc = ws.first_document();
 
-        // XXX NO WAY TO DISABLE INSECURE FEATURES
-        let mut setup_options = DocumentSetupOptions::new(false);
+        // Default to allowing insecure since it would be super duper annoying
+        // to have to pass `--trusted` every time to build a personal document
+        // that uses shell-escape! This default can be overridden by setting the
+        // environment variable TECTONIC_UNTRUSTED_MODE to a nonempty value.
+        let stance = if self.untrusted {
+            SecurityStance::DisableInsecures
+        } else {
+            SecurityStance::MaybeAllowInsecures
+        };
+
+        let mut setup_options =
+            DocumentSetupOptions::new_with_security(SecuritySettings::new(stance));
         setup_options.only_cached(self.only_cached);
 
         for output_name in doc.output_names() {
@@ -283,7 +298,7 @@ fn get_a_bundle(
     match Workspace::open_from_environment() {
         Ok(ws) => {
             let doc = ws.first_document();
-            let mut options = DocumentSetupOptions::new(true);
+            let mut options: DocumentSetupOptions = Default::default();
             options.only_cached(only_cached);
             doc.bundle(&options, status)
         }