lowdown: add flag to disable the Darwin sandbox #346945
Conversation
emilazy
requested review from
mmahut,
RaghavSood,
RaitoBezarius and
a team
as code owners
|
|
that's correct, new entries to instead you can do something like |
|
That’s not true; it doesn’t complain about It might be reasonable to use |
|
Result of 1 package marked as broken and skipped:
13 packages failed to build:
152 packages built:
I believe the failures are pre‐existing. |
emilazy
force-pushed
the
push-wpmkxnzkkzpr
branch
from
12e8e49
to
cd6cf03
Compare
|
I switched to an override so that users overlaying |
This is a program written in a memory‐unsafe language that processes potentially‐untrusted user input. We shouldn’t disable upstream’s sandboxing mechanisms for all downstream consumers without good reason. Although the sandbox API is officially marked as deprecated, it is used as the basis for the supported App Sandbox and it is extremely unlikely to ever be removed as it is used extensively throughout the OS for service hardening and by third parties like the Chrome sandbox. Nix itself uses it to sandbox builds, and its lack of support for nesting is why this caused problems in the first place. Instead, introduce a `lowdown-unsandboxed` package that can be used in the `nativeBuildInputs` of Nix builds, while keeping the sandboxed version of the program for general use. The name might not be ideal, as it remains identical to `lowdown` on non‐Darwin platforms, but I couldn’t think of a better one. See: NixOS#125004 Closes: NixOS#346933
emilazy
force-pushed
the
push-wpmkxnzkkzpr
branch
from
cd6cf03
to
b370a68
Compare
|
@emilazy yes, you are correct, it should be allowed but i interpreted requirement as implementation, so it made sense to me that you can't add |
|
I think the actually reasonable thing that should have been done here is to make sandbox setup failure not fatal in lowdown. Disabling it completely isn't great, and nor is the current situation, where things like Lix which iirc use lowdown at runtime and at build time had their builds broken and also cannot use the sandbox (see: https://git.lix.systems/lix-project/lix/issues/547) |
|
@lf- Sorry, I should have sent a change to the Lix |
|
I think the Nix flake will also probably need fixing. I don’t think I’ll have time for that today, but maybe tomorrow if nobody beats me to it. |
|
We can fix Lix but the problem is that it will break NixOS 24.05 assuming we don't put in funny feature detection code because -unsandboxed exists only on unstable. That's the real issue with this sort of change for stuff like Lix which needs to be compatible with both branches. |
|
Does e.g. {
lowdown,
lowdown-unsandboxed ? lowdown,
}:
…work for you? |
|
I am about to try that, but I think this is still fundamentally busted without any override, since we quite plausibly might test lowdown-using functionality in installCheckPhase. That being said, it does seem like the offending sandbox should only be applied to the CLI so maybe it's okay? It still gives me a bad feeling that tools have the potential to arbitrarily not work in the nix sandbox if someone puts the path to the CLI in a wrapper script or whatever. And as for the sandbox setup failing being fatal, it should be changed to print a warning IMO. The sandbox isn't fundamental to it working properly, it's not that weird for it to be unavailable (just like Linux's user namespaces!), and we have very bad CI testing of the Nix sandbox on macOS in general, so it seems like it's likely to be a liability in the future. Or if it is going to be fatal at least there needs to be an env-var to override it or so and we will have another |
|
It only applies to the CLI, yeah. I’m not really sure what override we could provide for install checks other than the option this already introduces to disable the sandboxing. The macOS sandbox just doesn’t support nesting AFAIK, so either you have code that never invokes the sandbox, test code other than what you install, or it can’t run in the sandbox. Lots of stuff doesn’t really work well in the macOS sandbox, ultimately, and we don’t have clear ways to fix that for many of them. But at least downstream derivations can make any of those possible choices after this PR. I’m not sure what you mean about the sandbox setup being fatal and environment variables. It’s |
|
We could make it so that Edit: Actually, this is a waste of time because I forgot that even “unsandboxed” builds use |
Yes, so lowdown should not completely fail in a way that requires all package maintainers paying attention to macOS by having to have two separate versions of it in a way that cannot be correct for any dependents-of-dependents. This PR adds extra configuration in a way that only affects macOS and breaks builds in ways that may only happen in sandboxed Nix builds. Furthermore, it might only affect a second order consumer of lowdown: perhaps there is a lowdown wrapper script in nixpkgs; there is no way to correctly write that package, except by always using the unsandboxed variant, if that package can be used by any other packages at build time. This whole configuration mess is solved by not having sandbox setup failure be fatal, either by requiring an override environment variable to allow that condition (which then just can be set by any second-order dependents), or by issuing a warning and proceeding anyhow. The solution of having two packages adds more work to everyone on a platform that already has packages randomly break too much due to needing macOS-only changes. This is done in order to "improve security", which would also be achieved by just trying to enable the sandbox every time and just not dying if it doesn't work. I don't see there being any realistic case where even failing sandbox setup silently (let alone with a warning) would be a security issue; executing lowdown from a sandbox is basically only ever going to happen in a nix build, in which case you should not be processing malicious markdown docs. I don't see any conceivable case where the macOS sandbox state and markdown input would be controlled by an attacker without them simply having code execution already. |
They happen in all builds on macOS because the sandbox is unconditionally initialized; unsandboxed builds just use a minimal profile. It’s not possible to hold wrong here unless you don’t test on macOS at all. If an executable that wraps Previously, such packages would always be using the unsandboxed version on I really think that upstream should be making the decisions about how |
FWIW, all this work is directly aimed towards making far more packages build out‐of‐the‐box on macOS and significantly reducing the maintenance burden of macOS support both in Nixpkgs and for downstream derivations, which it is already starting to pay dividends for even before making it out of |
|
Apologies for my frustration. I realize that I am currently a bit emotionally destabilized and burned out and I was very unreasonable in this thread. I will do my best to fix the conditions that led to this (chiefly trying to push back more on being the backstop maintainer) and do better in the future. Thank you for your work in making it more maintainable, and I hope after we get through the growing pains, it will be easier to deal with. I will file an issue upstream to get their input on possibly changing the sandbox setup design; it looks like a possible oversight. |
|
Thank you; I sincerely do appreciate that. I know upstream backwards‐incompatibilities are frustrating, whether motivated by security or not, and I had made a mental note to check if I needed to mirror the changes in the Lix and Nix flakes that I neglected to follow up on. I also know that macOS support has been a persistent pain point for the ecosystem, and that both Apple decisions and Nixpkgs neglect have made it difficult to improve for a long time. I’m hoping that 24.11 and 25.05 will be vast improvements in the state of support, but I understand that a lot of people already feel fed up with the platform and that it’ll be on us to prove its newfound health. FWIW, I agree that it seems relatively unlikely that a fail‐open approach to sandboxing could be a critical security failure, but when an upstream has explicitly written code to sandbox itself and fail if it can’t be initialized, I’m very hesitant to go against that more than is absolutely necessary. It’s hard to predict what states people will be able to direct weird machines into, and I don’t want a Nixpkgs decision to one day end up as one of the nodes in a chain like this. How exactly to handle sandboxing in programs that process untrusted user input isn’t our area of expertise, so I feel it is best to make any divergence upstream as localized as possible when we absolutely must do it because of technical limitations. Sadly, in this case, that interacts with the limitations of Apple’s APIs in such a way as to force consumers to make explicit decisions about the trade‐off. I think in the absence of the upstream decision it’s better for us to expose the trade‐off so downstreams can decide what’s best for them, but it’s certainly an awkward situation. Sandboxing APIs should be able to nest and it sucks that this one can’t. |
This is a program written in a memory‐unsafe language that processes potentially‐untrusted user input. We shouldn’t disable upstream’s sandboxing mechanisms for all downstream consumers without good reason.
Although the sandbox API is officially marked as deprecated, it is used as the basis for the supported App Sandbox and it is extremely unlikely to ever be removed as it is used extensively throughout the OS for service hardening and by third parties like the Chrome sandbox. Nix itself uses it to sandbox builds, and its lack of support for nesting is why this caused problems in the first place. Instead, introduce a
lowdown-unsandboxedpackage that can be used in thenativeBuildInputsof Nix builds, while keeping the sandboxed version of the program for general use. The name might not be ideal, as it remains identical tolowdownon non‐Darwin platforms, but I couldn’t think of a better one.See: #125004
Closes: #346933
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.