Decide on a design for a minimum viable v0.1

[ssokolow]

As a "Just Works™ baseline sandboxing wrapper for any build automation", what features does this project need for a minimum viable product, and what features should be agreed on as definitely suitable for a later release?

Current ideas for the design:

So far I'm thinking:

• To ensure that prepending this command doesn't changing the argument-parsing behaviour of the command being sandboxed, the command-line handling won't use an existing argument parser but, instead, will just check...
  • If there are no arguments, or there's only one argument and it's -h or --help, display help.
  • If the first argument is a valid profile name, launch the argument list (minus argv[0]), unmodified, as a command inside Firejail.
  • Otherwise, display a helpful "couldn't find a profile for that. See --help for more details." error and exit.
• Recommend the use of something like alias nargo=nodo cargo so that, if the alias is missing, they'll get an error message rather than an un-sandboxed execution.
• Try not to tie the sandboxing configuration to Firejail's CLI options specifically.

...and a configuration file design which provides this sort of functionality, but not necessarily with a schema anything like this:

# Default list of root-relative paths to be denied access to
# (The idea being to provide an analogue to `chattr +a foo.log`
# so `git diff` can be used to reveal shenanigans)
blacklist=[".git", ".hg", ".bzr", ".svn"]

[profile.cargo]
root_marked_by=["Cargo.toml"]
root_find_outermost=true  # For workspaces
projectless_subcommands=["init", "new"] # Assume $PWD is project root
allow_dbus_subcommands=["run"]
allow_gui_subcommands=["run"]
# allow_network=false
allow_network_subcommands=["add", "audit", "b", "build", "c", "check", "fetch", "run"] # etc. etc. etc.
deny_subcommands=["install", "uninstall"]  # must be run unconstrained

[profile.make]
root_marked_by=["Makefile"]
cwd_to_root=true  # run `make` in project root no matter where `nodo make` is run from

Requirements for v0.1 MVP

• [ ] Settle on a name. nodo (Like "sudo" but "no") is a placeholder I'm hoping to replace because there are too many Google results for it already.
• [x] Identify which Firejail restrictions will be hard-coded for the v0.1 MVP and which ones should map to which config keys.
• [x] Decide whether to implement root_find_outermost for workspaces support for v0.1 or defer it to v0.2
• [x] cwd_to_root to allow tools like make to handle being invoked from subdirectories
• [ ] Automatically blacklist all files that affect sandboxing behaviour
• [ ] A global blacklist config key to allow blacklisting things like .git so malicious packages can't hide their changes by amending un-pushed commits
• [ ] root_marked_by for automatically determining where to place the sandboxing boundary if something like nodo cargo or nodo just gets run in a subdirectory.
• [ ] projectless_subcommands to avoid forcing users to bypass the wrapper for cargo new and cargo init. (May need to redesign the actual config key for v0.2 to support things akin to npm install -g where it's not the subcommand that decides local vs. global)
• [ ] Decide on a redesign of deny_subcommands to support transparently allowing npm install but refusing with a "please bypass the wrapper" message for npm install -g.
• [ ] Decide what rules need to be in place to avoid pathological "ascend out of the project for lack of a root_marked_by file" situations. (Maybe stop and refuse if we hit $HOME, the home directory as identified by getpwnam/getpwuid, or a directory we lack write or execute/search permissions on the way up the tree before we find such a file.)
• [ ] Decide whether there are better names for any of these configuration keys

v0.1.1

• [ ] allow_network and allow_network_subcommands (Just allowing network for all commands is an acceptable v0.1 as long as it still constrains filesystem access and blocks off D-Bus and the like)
• [ ] root_resolve_symlinks=false to allow using a differentially fuzzed port of Python's os.path.abspath instead of std::fs::canonicalize. (Needed to ensure we can have root_marked_by match the behaviour of all types of build automation if there's a symlink between $PWD and the root directory it finds.)
• [ ] Consider allow_dbus_subcommands and allow_gui_subcommands as a design for allowing things like cargo run access to D-Bus and X11 and the like.

v0.1.x

• [ ] Brainstorm how much network whitelisting is viable with a "Just Works™ by default" config. (eg. What ports? Does crates.io's data schema allow it to arbitrarily point requests for bulk data at random CDN endpoints that are only known after Cargo has started? etc.)
• [ ] Brainstorm what is appropriate for per-project overrides (configured by files outside the project root the sandbox enforces, of course)

v2.0

• [ ] Brainstorm options for allowing a single command to map to a multi-stage build with each stage having different restrictions. (Might be out-of-scope for this tool and instead handled by some helper tool that invokes this tool separately for cargo fetch, cargo build, etc.)

[NobodyXu]

That's why I'm not going for full whitelisting until at least 0.2.x. You wind up deep in the weeds reinventing parsing every command's arguments if you try for that.

Yeah indeed. It's not worth it at this stage of the project.

Edit:

Though I am fairly certain we can make Cargo.toml, LICENSE, REAMD.md, examples, src, tests and benches read-only.

No sane build.rs would modify these directories. build.rs that modifies these directories are either extremely poorly written and is very rare, or they are straight-up hacked by others.

[ssokolow]

...umm, actually, I think I might have written a build.rs at one point that wrote to src because I couldn't figure out how to get certain things to play nicely together with the officially recommended output directory.

That sort of thing is why I err very much on the side of "Make it just work in the face of sloppy legitimate uses" things for early plans. The more I err in that direction, the less configurable the early releases need to be in order to meet design goals for everyone.

[NobodyXu]

...umm, actually, I think I might have written a build.rs at one point that wrote to src because I couldn't figure out how to get certain things to play nicely together with the officially recommended output directory.

For bindgen, it writes to somewhere in target/ and set the OUT_DIR, which you can include that file using:

include!(concat!(env!("OUT_DIR"), "/binding.rs"));

That sort of thing is why I err very much on the side of "Make it just work in the face of sloppy legitimate uses" things for early plans. The more I err in that direction, the less configurable the early releases need to be in order to meet design goals for everyone.

I agree with it, but I really think for build.rs we should enforce stricter rules.

According to the official doc:

Build scripts may save any output files in the directory specified in the OUT_DIR environment variable. Scripts should not modify any files outside of that directory.

If the build.rs doesn’t play by the rules, then it should be fixed.

[ssokolow]

we should enforce

This phrase is the problem. We're not in a position to make demands of the end-users we service or they'll consider it too onerous for the tool to achieve the popularity I want of it.

The purpose of the tool, first and foremost, is to improve the baseline. Beyond that, we'll see how things go later.

That means we facilitate users improving the security of what they're already doing for now.

[NobodyXu]

we should enforce

This phrase is the problem. We're not in a position to make demands of the end-users we service or they'll consider it too onerous for the tool to achieve the popularity I want of it.

The purpose of the tool, first and foremost, is to improve the baseline. Beyond that, we'll see how things go later.

OK, that’s totally reasonable.

[NobodyXu]

On a second though, we should makes $HOME/.rustup read-only for all cargo operations.

There is absolutely no way for cargo to modify that directory.

[ssokolow]

That makes sense.

• Everything outside the project directory should be inaccessible unless a need is demonstrated otherwise. (With the exception of things like /usr where we follow Firejail's lead on trusting the OS permissions for our "retrofit an improved baseline" approach, given how many projects may want to use some APT/RPM/etc.-installed tools or libraries.)
• Everything that's demonstrated to be necessary should be read-only unless a need is demonstrated otherwise.

[NobodyXu]

That makes sense.

• Everything outside the project directory should be inaccessible unless a need is demonstrated otherwise. (With the exception of things like /usr where we follow Firejail's lead on trusting the OS permissions for our "retrofit an improved baseline" approach, given how many projects may want to use some APT/RPM/etc.-installed tools or libraries.)
• Everything that's demonstrated to be necessary should be read-only unless a need is demonstrated otherwise.

I agree with you.

IMO the only directory outside project which we should consider exposing is $HOME/.cargo and $HOME/.rustup.

The exception is “cargo run”/“npm run”, where user might want additional directory exposed.

[ssokolow]

The exception is “cargo run”/“npm run”, where user might want additional directory exposed.

Users can always alter the configuration as needed, so we don't need to think about that.

[NobodyXu]

Users can always alter the configuration as needed, so we don't need to think about that.

That’s good.

[NobodyXu]

@ssokolow I happen to be free recently, so why not start development now?

[ssokolow]

I'm trying to focus on getting things cleaned up for Christmas, but getting the features for v0.1 implemented without commiting to even the minimal API stability an MVP leading in to v0.1.1 might imply should be pretty quick.

I'll see if I can fit that in within the next few days.

[ssokolow]

Something that just occurred to me.

If it feels like it can be represented comfortably enough in the TOML, maybe, instead of *_subcommands fields, the CommandProfile struct should contain a subcommands: Vec<String, CommandProfile> field and things like "deny network by default but allow for these subcommands" would just be a "deny network" setting on the parent command and an "allow network" setting on each subcommand, with the subcommands literally just being command definitions that are in the subcommands field of another command definition, instead of in the top-level profile list.

The main two potential issues I see are:

1. Duplication for things where a subcommand has aliases or multiple subcommands need the same permissions
2. Whether something like [profiles.cargo.subcommands.fetch] and [profiles.npm.subcommands.install.subcommands."-g"] is the best I can do for user comfort in that concept.

*shrug* Something to think about after I go to bed.

[NobodyXu]

Maybe we can have something like this:

cmd_profile = [
    “b”: “build”,
    “builld”: […],
 ]

I am not very familiar with toml, so I could be wrong on the syntax.

[ssokolow]

Maybe we can have something like this:

The problem is that now you've got a schema which is hostile to commands that don't have subcommands or where all subcommands should use the same set of sandboxing rules.

If it were XML, I'd suggest using the availability of different tag names so rules and subcommands can exist at the same level without getting confused but, with JSON, YAML, or TOML, that's not an option.

I am not very familiar with toml, so I could be wrong on the syntax.

Here's a TOML<->JSON converter you can experiment with: https://pseitz.github.io/toml-to-json-online-converter/

...and here's the TOML spec: https://toml.io/en/

TOML is best when you design your schema so it can feel like a more powerful variant of .ini files.

[NobodyXu]

Thanks, I will take a look at it.

[NobodyXu]

The problem is that now you've got a schema which is hostile to commands that don't have subcommands or where all subcommands should use the same set of sandboxing rules.

Perhaps we can hardcode the aliases.

AFAIK, these aliases are for builtin commands only and is fairly limited.

Hard coding them might not be a problem at all and would make the configuration file simpler.

[ssokolow]

As far as I'm concerned, it's unacceptable to hard-code anything specific to any subprocess other than firejail beyond baking a default config file into the binary using include_str! which can be dumped and edited using something like --dump-config.

Cargo Workspaces need to be supported? Invent the root_find_outermost=true config key. make only checks the current directory for Makefile? Invent the cwd_to_root=true config key. etc.

Heck, I'm trying to be careful about how specific I am for the permissions so that, if a backend other than Firejail gets supported in the future, there will be minimal disruption involved.

[NobodyXu]

Sorry for that, I didn't take into account of that.

Maybe we should have another variable handling the aliases, e.g.:

let aliases = [
    "b": "build",
]

[ssokolow]

That is a good idea and probably along the lines I'd have come up with.

In TOML, it could use either the inline form for cases with few entries:

[profile.cargo]
subcommand_aliases = {r="run", t="test", b="build"}

...or the table form for cases with many entries:

[profile.cargo.subcommand_aliases]
r="run"
t="test"
b="build"

[ssokolow]

It looks like I might have to push this to the new year. Christmas-related preparation is taking longer than expected and I promised myself I'd stick to leisure activities I've been neglecting for two weeks starting on Christmas.

[NobodyXu]

@ssokolow Totally understandable!

I also want to have some fun and spend more time with my friends rather than sitting at home all day, since I have been under lockdown for at least 4 months in Sydney.

Have a good Christmas!

We can talk about this in 2022.

[ssokolow]

Sorry for taking so long. I've been working on the configuration side of the project over the last couple of days to get that hammered down and I realized that cwd_to_root should be removed as a feature.

Specifically, because changing the working directory will break things like make -f NameOtherThanMakefile and it just presents too many opportunities for surprising behaviour that could open up a security vulnerability.

[NobodyXu]

I am good with removing cwd_to_root.

This would actually make it easier to implement the initial version.

[ssokolow]

How does this look as a draft for a default "should work for most people" set of Firejail flags shared between all profiles?

# The set of Firejail flags that get applied to *all* profiles
#
# As flags in this list don't yet have more specific preferences which control
# them, feel free to change this if you're working on projects which have
# specialized needs. For example:
#
# - If you store your projects on removable media, remove `--blacklist=/media`
# - If you need to play sounds, remove both `--nosound` and `--noautopulse`
# - If you need access to serial ports, remove `--nogroups` since that is
#   typically achieved via membership in the `dialout` group.
# - If you need to display a GUI, remove `--no3d`, since
#   modern GUI toolkits tend to use at least some degree of GPU acceleration.
#
# Conversely, if you feel these "Just Work™ in most cases" defaults aren't
# strict enough, feel free to add flags to lock things down further.
#
# NOTE: `--x11=none` cannot be included here by default because Firejail will
#       refuse to start if you specify it without either setting your X server
#       configuration to `-nolisten local` or passing Firejail a suitable
#       `--net` flag, which is a per-profile choice.
firejail_base_flags=[
    "--blacklist=/media",
    "--blacklist=/mnt",
    "--blacklist=/srv",
    "--caps.drop=all",
    "--deterministic-exit-code",
    "--no3d",
    "--nogroups",
    "--nonewprivs",
    "--noroot",
    "--nosound",
    "--noautopulse",
    "--novideo",
    "--nou2f",
    "--private-dev",
    "--private-tmp",
    "--protocol=unix,inet,inet6,netlink",
    "--seccomp",
    "--shell=none",
    "--quiet",
]

(Bear in mind that --net=none and --whitelist and --blacklist flags will be provided by a mixture of the profile and dynamically generated rules like "don't let sandboxed apps mess with the sandboxing configuration file".)

[NobodyXu]

It looks good to me.

I think maybe it is better to also disable dbus access via --dbus-system=none and also blacklist /run/mount and /run/media?

Since we created a mount namespace, maybe we should also enable IPC namespace via --ipc-namespace?

[ssokolow]

I think maybe it is better to also disable dbus access via --dbus-system=none

--dbus-system isn't present in the version of Firejail on my Ubuntu 20.04 LTS system and I'd prefer to avoid having to do version detection.

As for --nodbus as an alternative, I don't want to give users a false sense of security and I haven't yet figured out how to get the "Parent pid 3156434, child pid 3156435"-silencing effect of --quiet without also silencing the "Warning: An abstract unix socket for session D-BUS might still be available. Use --net or remove unix from --protocol set" warning.

and also blacklist /run/mount and /run/media?

Where are those specified? I've got /run/mount but not /run/media, and it seems questionable to blacklist /run/mount/utab when I'm already blacklisting the mountpoints it lists and not blacklisting /etc/fstab.

(Remember, this is a set of restrictions that may get looser after beta testing, because it's intended as a baseline that the average user won't need to modify.)

Heck, the main reason I blacklist /mnt, /media, and /srv is that I've got things in those folders that are writable by the user I run as and firejail's --whitelist doesn't block them out by default.

Since we created a mount namespace, maybe we should also enable IPC namespace via --ipc-namespace?

Good point. I forgot about that and then missed it during my re-read of --help when I skipped past the options related to manipulating IPv4 and IPv6 more finely than using or not using --net=none.

[NobodyXu]

As for --nodbus as an alternative, I don't want to give users a false sense of security and I haven't yet figured out how to get the "Parent pid 3156434, child pid 3156435"-silencing effect of --quiet without also silencing the "Warning: An abstract unix socket for session D-BUS might still be available. Use --net or remove unix from --protocol set" warning.

Ok then, let's leave dbus options behind for the initial release.

Where are those specified?

According to here, udisks automatically mount drivers/devices to /run/media, so I think we should blacklist it to prevent access.

As for /run/mount, it stores information of the mount performed, including the user that mounted it according to here.

I think we should also blacklist it to prevent program from accessing it.

[ssokolow]

According to here, udisks automatically mount drivers/devices to /run/media, so I think we should blacklist it to prevent access.

Sounds good. Will do.

I think we should also blacklist it to prevent program from accessing it.

Again, what is the rationale for blacklisting /run/mount?

1. We're not blacklisting /etc/fstab
2. We're already blacklisting the partitions /run/mount points to
3. This is a general-purpose set of defaults that should Just Work™ as a baseline for most people.
4. The main reason I'm blacklisting those paths is because they're likely to contain writable stuff. (i.e. The baseline isn't generally intended to whitelist read access to stuff outside $HOME.)

[NobodyXu]

OK, I think it is reasonable to not blacklist /run/mount.

[ssokolow]

Anyway, it's late here, so I'll decide whether to iterate on how that is represented in the config file and the schema tomorrow.

(I spent today converting the existing config schema over to a bunch of newtypes, replacing the bools with custom enums, and writing unit tests to make it as difficult as possible for later stages to confuse things and possibly open up a security hole.)

For example, while it's generated by a helper macro, this is rustdoc's view of what the allow_network=true|false profile field parses into:

pub enum Network {
    ChildProcsOnly,
    AllNetworks,
}

EDIT: Here it is in context:

[NobodyXu]

It looks quite good.

Can you push it to the repository though?

It will be much easier to understand the current design with the source code.

[ssokolow]

My main concern is that I haven't yet gone through and made sure all my relevant accounts are as secure as I can make them and, once this is more than just a discussion thread, the clock is potentially ticking for me to become a target for supply-chain attacks.

However, since it's just the config parsing so far, I suppose there's no harm in that.

[NobodyXu]

It is understandable to have concern over supply-chain attacks, though I don't think there will be any people targeting this software at this stage.

[ssokolow]

There we go. Done.

...though I intentionally followed the reverse of GitHub's advice for the short term:

Help people interested in this repository [not find] your project by [not] adding a README.

[ssokolow]

Oh, don't hesitate to use cargo doc --open to get a feel for the codebase. I'm pretty proactive about polishing up my API documentation for easy reading.

Ideally, I'd also like to have this complete and polished enough to be using it to harden my own system before I start drawing attention to its existence.

[ssokolow]

Another "Oh, ...", feel free to let me know what you think of what I've got so far.

I do my best, and I already had over a decade of experience with other languages when Rust 1.0 hit, but I don't usually write Rust for other people to see and I have a bad habit of not finding time to read other people's codebases to pick up tips.

[NobodyXu]

I've currently reading the source code.

Once I finished reading, I will comment on it and maybe I will even submit a PR.

Edit:

I will open another issue for discussion on the implementation.

[ssokolow]

I pushed a first draft of the argument parsing. Do you have any feedback on the --help output?

[NobodyXu]

I think the help message for -- can be improved:

    --              All arguments after '--' is treated as part of the command to be executed inside firejail

[ssokolow]

The problem with that phrasing is that it could lead a user to doubt their understanding of how arguments are parsed because, in most cases, options can be mixed in among positional arguments.

(i.e. They could mistakenly assume that there are cases other than "<command> starts with - or --" where -- is necessary.)

I'll see if I can come up with a better alternative to my draft that doesn't have that problem.

[ssokolow]

OK. How does the revised version look?

[NobodyXu]

It is much better and is easy to understand.

[ssokolow]

Let me know what you think of the "look up the path to the configuration file" code I just pushed.

[NobodyXu]

Looks all good to me, except the use of deprecated env::home_dir.

[NobodyXu]

Maybe we can use dirs::home_dir or dirs_next::home_dir.

[ssokolow]

As it says in the code comment:

1. env::home_dir is deprecated because of its behaviour on Windows
2. This is a tool that's fundamentally cgroups-specific and, thus, Linux-specific
3. I don't want to add another external dependency and the associated rise in opportunity for supply-chain attacks.
4. In this niche specifically, the recommended crate already suffered from some drama surrounding how the maintainer chose to act on feeling unable to continue maintaining it and that prompted rust-lang/rust#71684.
5. The behaviour listed under "Unix" in the rustdoc page is perfectly acceptable.

TL;DR: I chose it with full awareness of why it's deprecated and having been around to see that the third-party alternatives have already been more iffy than usual.

In this case where I'm writing a security tool, I trust a function that's been deprecated for behaving too Unixy on Windows but is still subject to the Rust 1.0 stability promise as part of std over a third-party crate.

EDIT: If I didn't consider the deprecated function an acceptable option, I'd go straight for env::var_os("HOME") and sacrifice the getpwuid_r fallback to keep the lack of an external dependency.

[NobodyXu]

Thanks for the explanation, now I think the use of env::home_dir is reasonable.

[NobodyXu]

Pinging this issue

[ssokolow]

Thanks. I switched to some other stuff in my life (including being most of the way to finally doing the decade-delayed configuration rework for my QuickTile project), but then wound up having sleep issues.

I'll probably return to things in reverse order. (Fix sleep, then complete and release the QuickTile rework, then get back to nodo)