[Mac] Automatically determine the repo dir so that MOZ_DEVELOPER_REPO_DIR isn't needed

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: haik, Assigned: haik)

Tracking

56 Branch
mozilla56
Unspecified
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: sb+, crash signature)

Attachments

(2 attachments)

(Assignee)

Description

a year ago
With bug 1294641, on Mac, for local builds, the Firefox content sandbox must know the path to the source repo and object dir. We've been updating mach to set the environment MOZ_DEVELOPER_REPO_DIR before launching the browser. For developers who run the binary directly out of the .app/, MOZ_DEVELOPER_REPO_DIR must be manually set. If we could automatically determine the repo and obj dir path in the browser, that would eliminate the need for developers to manually set these.
(Assignee)

Updated

a year ago
Whiteboard: sb+
If we were able to use hardlinks (bug 1380416), that would address this right?
(Assignee)

Comment 2

a year ago
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #1)
> If we were able to use hardlinks (bug 1380416), that would address this
> right?

Yes, but I wanted to have a bug to track this. I think bug 1380416 should be what we try first.
(Assignee)

Updated

a year ago
Duplicate of this bug: 1379784
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
I'm moving forward with this fix because changing developer builds to not use symlinks and instead hardlinks or copying is somewhat of a big hammer. (I think it'd be preferable to not use symlinks for Mac/Linux builds because we wouldn't have to whitelist the repo and object directory in our sandboxes, but it's a bigger change.) For example, on Mac, the objdir/dist/NightlyDebug.app is approximately 700M which means that a debug build that copied files would result in an additional ~700M of file I/O.

Comment 10

a year ago
mozreview-review
Comment on attachment 8886974 [details]
Bug 1380690 - Part 2 - Whitelist repo and object dirs using paths from the Info.plist files.

https://reviewboard.mozilla.org/r/157728/#review163014

::: security/sandbox/common/SandboxSettings.cpp:74
(Diff revision 2)
> +nsresult
> +GetRepoDir(nsIFile **aRepoDir)
> +{
> +  MOZ_ASSERT(IsDevelopmentBuild());
> +  nsresult rv;
> +  nsCOMPtr<nsIFile> repoDir;
> +#if defined(XP_MACOSX)
> +  rv = GetResourceSymlinkTarget(NS_LITERAL_STRING("topsrcdirlink"),
> +                                getter_AddRefs(repoDir));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  repoDir.swap(*aRepoDir);
> +  return NS_OK;
> +#else
> +  char *developer_repo_dir = PR_GetEnv("MOZ_DEVELOPER_REPO_DIR");
> +  if (!developer_repo_dir) {
> +    return NS_ERROR_FILE_NOT_FOUND;
> +  }
> +
> +  rv = NS_NewLocalFile(NS_ConvertUTF8toUTF16(developer_repo_dir),
> +                       false,
> +                       getter_AddRefs(repoDir));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = repoDir->Normalize();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  repoDir.swap(*aRepoDir);
> +  return NS_OK;
> +#endif /* XP_MACOSX */
> +}
> +
> +nsresult
> +GetObjDir(nsIFile **aObjDir)
> +{
> +  MOZ_ASSERT(IsDevelopmentBuild());
> +  nsresult rv;
> +  nsCOMPtr<nsIFile> objDir;
> +#if defined(XP_MACOSX)
> +  rv = GetResourceSymlinkTarget(NS_LITERAL_STRING("topobjdirlink"),
> +                                getter_AddRefs(objDir));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  objDir.swap(*aObjDir);
> +  return NS_OK;
> +#else
> +  char *developer_obj_dir = PR_GetEnv("MOZ_DEVELOPER_OBJ_DIR");
> +  if (!developer_obj_dir) {
> +    return NS_ERROR_FILE_NOT_FOUND;
> +  }
> +
> +  rv = NS_NewLocalFile(NS_ConvertUTF8toUTF16(developer_obj_dir),
> +                       false,
> +                       getter_AddRefs(objDir));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = objDir->Normalize();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  objDir.swap(*aObjDir);
> +  return NS_OK;
> +#endif /* XP_MACOSX */

These two functions are basically identical besides the symlink name and the env var. Can we factor the implementations to share more code?

Comment 11

a year ago
mozreview-review
Comment on attachment 8886974 [details]
Bug 1380690 - Part 2 - Whitelist repo and object dirs using paths from the Info.plist files.

https://reviewboard.mozilla.org/r/157728/#review163050

I think glandium should look at this in the context of what I've implemented in bug 1377216. Specifically, we should consider setting a #define for these paths so you don't need to perform path manipulation in C++. Although for artifact builds you still need something that is specific to the environment to overwrite the paths from the builders in CI.
Attachment #8886974 - Flags: review?(agaynor) → review?(mh+mozilla)

Comment 12

a year ago
mozreview-review
Comment on attachment 8886973 [details]
Bug 1380690 - Part 1 - Save repo and object dir paths in Mac bundle Info.plist files.

https://reviewboard.mozilla.org/r/157726/#review163054

This feels like a leaky abstraction to me. I feel that passing a list of paths to exclude through a single mechanism (env variable, file, etc) is better than teaching the sandbox code about how the build system works. I don't want to get into a situation where we can't change the build system without having to muck about in sandbox C++ code.
(Assignee)

Comment 13

a year ago
(In reply to Gregory Szorc [:gps] from comment #12)
> Comment on attachment 8886973 [details]
> Bug 1380690 - Part 1 - Save repo and object dir symlinks in dist Mac bundle
> for dev builds.
> 
> https://reviewboard.mozilla.org/r/157726/#review163054
> 
> This feels like a leaky abstraction to me. I feel that passing a list of
> paths to exclude through a single mechanism (env variable, file, etc) is
> better than teaching the sandbox code about how the build system works. I
> don't want to get into a situation where we can't change the build system
> without having to muck about in sandbox C++ code.

I understand the concern and agree this isn't ideal. But, then again, this is a consequence of how we are already including hundreds of symlinks from the .app/ directory to the repo and object dir on dev builds. We just don't have explicit links to the base of those the repo and objdir. 

I would be content with using something other than a symlink as long as it lets the browser determine the directories it needs without requiring a change to the mach commands (like what we've tried to do so far in the tree) and allows the browser to be run directly without mach. Using an environment var doesn't work well.

I will look into adding keys to the Info.plist instead of the new symlinks if you agree that's an improvement.

Could we try to use bug 1380416 to unify our dev builds and package builds so that we don't use symlinks at all and we could then do away with the sandbox whitelisting specially for the dev build environment.
Flags: needinfo?(gps)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

a year ago
I've updated the code to store these directories in the Info.plist for Nightly.app as well as the inner plugin-container.app. (They're used by both types of processes.) The repo dir and object dir values are substituted in like we do for other values in the plists. The C++ side is just responsible for resolving symlinks (needed for sandboxing) and making sure these refer to a directory.

Flagged :spohl for review of the GetStringValueFromBundlePlist() function in SandboxSettings.cpp.

One issue is that the Info.plist values are also substituted in on packaged builds. Packaged builds don't need the paths so this is a bit of an information leak. i.e., the paths used to build our shipping packages would be included. Still looking into how to avoid it on packaged builds. The browser won't use the values in a packaged build so it won't affect sandboxing. I consider this a minor issue to tackle in a follow-up if reviewers don't object.

@gps, any tips on which variables to check or how to skip the Info.plist changes on packaged builds?

Refactored SandboxInfo.cpp changes a bit to factor out some common code per Alex's comment.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e52a38f7147a83974d936213992d628384b1a516
(Assignee)

Updated

a year ago
Flags: needinfo?(gps)

Comment 17

a year ago
mozreview-review
Comment on attachment 8886974 [details]
Bug 1380690 - Part 2 - Whitelist repo and object dirs using paths from the Info.plist files.

https://reviewboard.mozilla.org/r/157728/#review163552

::: security/sandbox/common/SandboxSettings.cpp:59
(Diff revision 3)
> +  key = CFStringCreateWithCString(kCFAllocatorDefault,
> +                                  keyAutoCString.get(),
> +                                  encoding);
> +
> +  CFStringRef value = (CFStringRef)CFDictionaryGetValue(bundleInfoDict, key);
> +  const char* valueCString = CFStringGetCStringPtr(value, encoding);

Is the handling of encodings here correct? (I'm not sure!)

The docs for `CFStringGetSystemEncoding` say that it returns an encoding baesd on the user's preferred language. The docs also encourage you to use `GetApplicationTextEncoding` (I don't know if that's right for our use case).

The docs for `CFGetCStringPtr` say that it must be an 8-bit encoding (which basically means ASCII or UTF-8 are the only valid choices I think). Does `CFStringGetSystemEncoding` always return one of those?
(Assignee)

Comment 18

a year ago
mozreview-review-reply
Comment on attachment 8886974 [details]
Bug 1380690 - Part 2 - Whitelist repo and object dirs using paths from the Info.plist files.

https://reviewboard.mozilla.org/r/157728/#review163552

> Is the handling of encodings here correct? (I'm not sure!)
> 
> The docs for `CFStringGetSystemEncoding` say that it returns an encoding baesd on the user's preferred language. The docs also encourage you to use `GetApplicationTextEncoding` (I don't know if that's right for our use case).
> 
> The docs for `CFGetCStringPtr` say that it must be an 8-bit encoding (which basically means ASCII or UTF-8 are the only valid choices I think). Does `CFStringGetSystemEncoding` always return one of those?

Thanks for catching that. It think the right thing to do is to hardcode this as kCFStringEncodingUTF8 given that the Info.plist files are "<?xml version="1.0" encoding="UTF-8"?>". Will test that out and update the code.

Comment 19

a year ago
mozreview-review-reply
Comment on attachment 8886974 [details]
Bug 1380690 - Part 2 - Whitelist repo and object dirs using paths from the Info.plist files.

https://reviewboard.mozilla.org/r/157728/#review163552

> Thanks for catching that. It think the right thing to do is to hardcode this as kCFStringEncodingUTF8 given that the Info.plist files are "<?xml version="1.0" encoding="UTF-8"?>". Will test that out and update the code.

Always UTF-8 is definitely the easiest thing :-)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

a year ago
mozreview-review
Comment on attachment 8886974 [details]
Bug 1380690 - Part 2 - Whitelist repo and object dirs using paths from the Info.plist files.

https://reviewboard.mozilla.org/r/157728/#review163720
Attachment #8886974 - Flags: review?(agaynor) → review+
(Assignee)

Comment 23

a year ago
(In reply to Haik Aftandilian [:haik] from comment #16)
> @gps, any tips on which variables to check or how to skip the Info.plist
> changes on packaged builds?

To respond to my own question, I think we'd have to strip out these values when the plist is copied to create the dmg which is done here in python/mozbuild/mozpack/dmg.py:create_dmg_from_staged(). If we could get a plutil-equivalent command on the build machines we could use plutil -remove 'key'.
(Assignee)

Updated

a year ago
Assignee: nobody → haftandilian

Comment 24

a year ago
mozreview-review
Comment on attachment 8886974 [details]
Bug 1380690 - Part 2 - Whitelist repo and object dirs using paths from the Info.plist files.

https://reviewboard.mozilla.org/r/157728/#review163660

r+ on GetStringValueFromBundlePlist with comment addressed.

::: security/sandbox/common/SandboxSettings.cpp:48
(Diff revision 3)
> +
> +  // Read this app's bundle Info.plist as a dictionary
> +  CFDictionaryRef bundleInfoDict;
> +  bundleInfoDict = CFBundleGetInfoDictionary(mainBundle);
> +  if (bundleInfoDict == NULL) {
> +    return NS_ERROR_INVALID_ARG;

This isn't a very good error to return here. NS_ERROR_INVALID_ARG refers to the arguments passed to the function, but they play no role in whether or not bundleInfoDict is null. I suggest NS_ERROR_FAILURE or NS_ERROR_UNEXPECTED.
Attachment #8886974 - Flags: review?(spohl.mozilla.bugs) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 26

a year ago
mozreview-review-reply
Comment on attachment 8886974 [details]
Bug 1380690 - Part 2 - Whitelist repo and object dirs using paths from the Info.plist files.

https://reviewboard.mozilla.org/r/157728/#review163660

> This isn't a very good error to return here. NS_ERROR_INVALID_ARG refers to the arguments passed to the function, but they play no role in whether or not bundleInfoDict is null. I suggest NS_ERROR_FAILURE or NS_ERROR_UNEXPECTED.

Replaced NS_ERROR_INVALID_ARG with NS_ERROR_FAILURE here. Thanks.

Comment 27

a year ago
mozreview-review
Comment on attachment 8886973 [details]
Bug 1380690 - Part 1 - Save repo and object dir paths in Mac bundle Info.plist files.

https://reviewboard.mozilla.org/r/157726/#review164380

So we need a hack so MacOS builds run directly (not via `mach run`) will "just work." This hack is implemented as data in plist files. That part makes sense and I can justify it.

What I have a harder time justifying is that the plist entry is present on all builds - including builds we ship to users. We also have an environment variable workaround for !MacOS platforms despite the fact that such a workaround isn't actually needed??

The plist changes leaking into shipped builds is not something I'm comfortable with. We either limit this to builds that developers produce or consume (in the case of artifact builds - where we take builds from automation and unpack the binaries locally to prevent developers from having to spend minutes compiling) or we find some other way.

Having the plist entry in shipped builds is essentially shipping a backdoor (although so is the environment variable approach). Having this backdoor activated when `mozilla::Omnijar::GetPath(mozilla::Omnijar::GRE) == nullptr` (what `mozilla::IsDevelopmentBuild()` does) seem dangerous and is giving me an unsettling feeling. Nobody would ever think that removing the omni.ja file would enable a sandbox bypass. I feel the **presence** of the backdoor should be controlled by a build-time flag not a run-time flag. In this world, we'd only enable the backdoor for local builds and specific builds in automation - not for builds we ever ship to users. If you make it run-time (as this series does), someone will find a way to exploit it and this feature will become a CVE. That being said, I'm not a security expert and don't fully understand what all is going on here, so maybe I'm missing something obvious. Please weigh my opinion accordingly.

I think a concrete path forward is to introduce a configure flag to control the presence of the sandbox bypass. That sets a #define which results in code conditionally being compiled. The bypass is enabled for developer builds and builds used to produce binaries consumed by "artifact builds." This may require a new build variation just for artifact builds. This solution is a bit more involved, sadly. But it doesn't involve shipping a backdoor to users.
Attachment #8886973 - Flags: review?(gps) → review-
glandium and I were talking in #build and we think that hardlinks or copying is better than introducing a backdoor - even one we don't ship to users.

However, removing symlinks means that many file changes now require a build step so changes are reflected. This would be a severe detriment to front-end developers (people touching JS, HTML, XUL, etc). The tooling has come a long way since we last had this discussion. So we may be able to mitigate things reasonably well.

What's the priority of this sandbox workaround? Could we potentially add support for a configure-based opt-in backdoor/workaround as a stop-gap?
(Assignee)

Comment 29

a year ago
(In reply to Gregory Szorc [:gps] from comment #27)
> Comment on attachment 8886973 [details]
> Bug 1380690 - Part 1 - Save repo and object dir paths in Mac bundle
> Info.plist files.
> 
> https://reviewboard.mozilla.org/r/157726/#review164380
> 
> So we need a hack so MacOS builds run directly (not via `mach run`) will
> "just work." This hack is implemented as data in plist files. That part
> makes sense and I can justify it.
> 
> What I have a harder time justifying is that the plist entry is present on
> all builds - including builds we ship to users. We also have an environment
> variable workaround for !MacOS platforms despite the fact that such a
> workaround isn't actually needed??

We are looking into how to make Linux not depend on them, but the environment variables are needed on Nightly Linux builds today. Our Linux read-access sandbox (not yet landed) also uses the environment variables in its current implementation, but we might be able to fix that and remove the Linux dependency that's in the tree right now.
 
> The plist changes leaking into shipped builds is not something I'm
> comfortable with. We either limit this to builds that developers produce or
> consume (in the case of artifact builds - where we take builds from
> automation and unpack the binaries locally to prevent developers from having
> to spend minutes compiling) or we find some other way.
> 
> Having the plist entry in shipped builds is essentially shipping a backdoor
> (although so is the environment variable approach). Having this backdoor
> activated when `mozilla::Omnijar::GetPath(mozilla::Omnijar::GRE) == nullptr`
> (what `mozilla::IsDevelopmentBuild()` does) seem dangerous and is giving me
> an unsettling feeling. Nobody would ever think that removing the omni.ja
> file would enable a sandbox bypass. I feel the **presence** of the backdoor
> should be controlled by a build-time flag not a run-time flag. In this
> world, we'd only enable the backdoor for local builds and specific builds in
> automation - not for builds we ever ship to users. If you make it run-time
> (as this series does), someone will find a way to exploit it and this
> feature will become a CVE. That being said, I'm not a security expert and
> don't fully understand what all is going on here, so maybe I'm missing
> something obvious. Please weigh my opinion accordingly.

A build-time flag is preferable, I agree, but to characterize this as a backdoor is definitely misleading. The main flaw in the above argument is that if a user's system is compromised in such a way that allows an attacker to modify Firefox to disable the sandbox, the attacker already has control of the system and could replace Firefox completely. Making the fact that one can disable the sandbox by changing the .app contents be somewhat of a moot point. And Firefox is not unique in this regard. At present, on all platforms, it is possible to disable the content sandbox in Firefox with a runtime change. It is controlled by a pref. We definitely have talked about disabling that in the future and that is something we want to do after we are on release. For now, being able to disable the sandbox for debugging and testing without a recompile is a useful feature.

To restate, if an attacker can disable the content sandbox by modifying the installed Firefox binaries or other contents of the .app file, this implies the system is already compromised. Effectively the same is true if an attacker just has write access to the files in the user's home directory. On the other hand, a bug that allows an attacker to disable the sandbox from content would be a severe security issue we have to fix.

> I think a concrete path forward is to introduce a configure flag to control
> the presence of the sandbox bypass. That sets a #define which results in
> code conditionally being compiled. The bypass is enabled for developer
> builds and builds used to produce binaries consumed by "artifact builds."
> This may require a new build variation just for artifact builds. This
> solution is a bit more involved, sadly. But it doesn't involve shipping a
> backdoor to users.

I'd be happy to drive that, but will need some help from you.

For now, would you consider moving forward with this approach to alleviate test crashes developers are hitting, given my explanation of the security impact? I would rather do that than make several changes to mach to set add missing MOZ_DEVELOPER_REPO_DIR while we work on the more involved build-system fix.
Flags: needinfo?(gps)
There are merits to your concern that I am over-stating the security concern. Your logic around threat model assessment seems reasonable to me. I think you may be brushing aside some concerns around local system compromise a bit, as some operating systems have safeguards to mitigate tampering (e.g. code signing). But I don't know the domain well enough to say anything with certainty. And what you say about prefs allowing the disabling of the sandbox is obviously true and tolerated today.

A tenet of good security is to reduce attack surface area. Even if it is acceptable today, the feature implemented in this bug does increase our attack surface area. Even if it is within our tolerances today, it may not be that way tomorrow. I worry that sometime down the road we will tighten the ratchet on the sandbox and this bypass feature will be targeted for elimination. This could even be in reaction to a security incident (at which point it becomes high priority). The build system would become a blocker to improving Firefox's security. This is something that rarely occurs (at least high urgency scenarios like "change the build system to fix a CVE") and something I would very, very, very much prefer to avoid. Hence my reluctance to introduce the feature and potential for a big fire drill in the first place.

Anyway, we're both biased in our desires. You want to ship a content sandbox. I want to make the build system as fast and simple as possible. And we both want to achieve those things with as little effort as possible. The build system is basically a long series of ugly compromises. And some solutions in this bug dovetail with other build system efforts. I think we can find a satisfactory solution in the build system that doesn't require increasing Firefox's attack surface area and too much work on your part. Can you give me a day or two to poll the build peers?
(Assignee)

Comment 31

a year ago
(In reply to Gregory Szorc [:gps] from comment #30)
> I think we
> can find a satisfactory solution in the build system that doesn't require
> increasing Firefox's attack surface area and too much work on your part. Can
> you give me a day or two to poll the build peers?

Sure.

A couple more points:

Earlier discussion of adding the env vars is in bug 1294641.

We currently use IsDevelopmentBuild() in the browser to determine whether or not we need to whitelist the repo and object dir. We added that for this purpose, but it's really telling us if this is an unpackaged build. We only need to whitelist the repo and object dir on unpackaged builds (because of the symlinks).

If we don't want the whitelisting to be a run-time decision, the build should provide a compile time flag that is only set on developer unpackaged builds. Ideally, when the flag is set, the build includes #defines that contain the source repo path and the object dir path. 

Or, if dev builds don't use symlinks in the .app/ dir, that's another solution.

Lastly, this fix is replacing use of env vars on Mac and doesn't have to be the final solution. Linux sandboxing works differently and we're looking into removing the env var requirement there too.
(In reply to Gregory Szorc [:gps] from comment #27)
> We also have an environment
> variable workaround for !MacOS platforms despite the fact that such a
> workaround isn't actually needed??

Linux is… complicated.  We're brokering file accesses in code that we've written / are writing, so in principle we can do anything, but it's all exposed to hostile input (and doing string processing), so simpler is better.

Right now, there's some nontrivial path manipulation ready to land in bug 1308400 to try to “do the right thing” around symlinks, both in this case and for system-level things that might be symlinked.  But if we didn't have to handle these test files, we might be able to replace it all with realpath() and string prefix checks, which is essentially what OS X is doing internally.

<hr />

On the topic of accidental backdoors: On the one hand, we already have environment variables and (at least on Linux) prefs that can effectively disable sandboxing.  On the other hand, the history of security.turn_off_all_security_so_that_viruses_can_take_over_this_computer suggests that we need to be careful whenever “reduce security for testing” flags are involved, and especially about tying them to things that aren't very obviously “this will reduce security if you do it”.
(Assignee)

Updated

a year ago
Duplicate of this bug: 1381517
(Assignee)

Comment 34

a year ago
(In reply to Gregory Szorc [:gps] from comment #28)
> What's the priority of this sandbox workaround? Could we potentially add
> support for a configure-based opt-in backdoor/workaround as a stop-gap?

I missed responding to this question. This is a high priority because devs are starting to hit test crashes due to the problem being solved here which has a time/productivity cost. We are planning to merge the read-access sandbox to Beta 55 and I would rather the feature not hinge on a major build change/restructuring/refactoring which we'll also need to merge to Beta.
> This is a high priority because devs are starting to hit test crashes due to the problem being solved here

To be more clear, unless you know about the env vars, it's impossible to run various test suites locally.  For example, "mach wpt" just dies instead of running anything useful.

This is being a serious bar for new contributors especially.
(In reply to Boris Zbarsky [:bz] from comment #35)
> 
> This is being a serious bar for new contributors especially.

And for engineers coming back from PTO who then spend a morning searching bugzilla to find out why...

Can we revert the offending patches or update mach to insert the environment variable into mach to make it work everywhere which can be removed when a proper fix is in place.
Flags: needinfo?(haftandilian)
(Assignee)

Comment 37

a year ago
(In reply to David Burns :automatedtester from comment #36)
> (In reply to Boris Zbarsky [:bz] from comment #35)
> > 
> > This is being a serious bar for new contributors especially.
> 
> And for engineers coming back from PTO who then spend a morning searching
> bugzilla to find out why...
> 
> Can we revert the offending patches or update mach to insert the environment
> variable into mach to make it work everywhere which can be removed when a
> proper fix is in place.

:gps, were you able to discuss this with the build peers and come to a decision?
Flags: needinfo?(haftandilian)
First, I have a stack of patches in bug 1382697 that introduces a configure flag to control how the build system installs files. It will allow us to copy or hard link files into the objdir instead of symlinking them. When symlinks aren't present, the sandbox won't need to be aware of them.

While that approach works, there are downsides. By removing symlinks, many workflows go from edit-test to edit-compile-test. That's a very tough pill to swallow. In addition, if we don't symlink within the objdir, we could be copying large files around, bloating the size of the objdir and slowing down builds. We can mitigate this by installing files to their final location in the objdir. But those patches need to be written. That build system optimization is long overdue, so hopefully this spurs us into action.

The end state of the build system will likely be to have an edit-compile-test cycle. And if we're at that point, we can use hard links (even on Windows) to mitigate sandbox concerns. But we're not going to get there over night. I think we need a stop gap to placate developers so their workflows don't go completely south.

Here's what I'm proposing.

Build peers continue with work to make file installation in builds configurable and more efficient. We offer a build configuration option that lets you use symlinks or force copies.

Sandbox code is added that recognizes paths to source and object directories. However, this code is conditionally compiled (behind #ifdef) and only enabled for local/developer builds. --enable-release disables this code. So, this code/backdoor is never shipped to users nor does it run in automation (at least for normal build configurations). This allows the sandbox to work with local builds using workflows that people practice today. Once the build system is optimized and we're ready to impose an edit-compile-test cycle, we stop using symlinks and the special sandbox code/workaround is removed.

To implement the sandbox workaround today, we'd need some minor moz.configure changes to set a #define and the sandbox code in this build would be reworked to be behind preprocessor directives.

Is this an acceptable plan?
Flags: needinfo?(gps)

Comment 39

a year ago
mozreview-review
Comment on attachment 8886973 [details]
Bug 1380690 - Part 1 - Save repo and object dir paths in Mac bundle Info.plist files.

https://reviewboard.mozilla.org/r/157726/#review165896

Per IRC conversation, let's land this now as a very short term workaround.

Please file a follow-up to make this code/feature not enabled in release builds. Please have that bug set as a release blocker so we don't forget to do the work.
Attachment #8886973 - Flags: review- → review+

Comment 40

a year ago
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db6d70745294
Part 1 - Save repo and object dir paths in Mac bundle Info.plist files. r=gps
https://hg.mozilla.org/integration/autoland/rev/225827d9f375
Part 2 - Whitelist repo and object dirs using paths from the Info.plist files. r=Alex_Gaynor,spohl
Duplicate of this bug: 1383915

Comment 42

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/db6d70745294
https://hg.mozilla.org/mozilla-central/rev/225827d9f375
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

a year ago
Duplicate of this bug: 1379906
(Assignee)

Updated

a year ago
Duplicate of this bug: 1383089
Crash Signature: [mozalloc_abort | NS_DebugBreak | ErrorLoadingSheet]
(Assignee)

Updated

a year ago
Duplicate of this bug: 1380560
Having seen this in my nightly's Info.plist, I'm wondering... wasn't this supposed to be in there only on local builds?
Flags: needinfo?(haftandilian)
(Assignee)

Comment 47

11 months ago
(In reply to Mike Hommey [:glandium] from comment #46)
> Having seen this in my nightly's Info.plist, I'm wondering... wasn't this
> supposed to be in there only on local builds?

It was expected to be in all packaged builds. (See comment 16). I filed follow-up bug 1384271 "Let --enable-release disable sandbox whitelisting for repo and object dir" to address that (and neglected to link to it here.) That bug was meant to address more than just the Info.plist entries and its blocked on some larger work, but maybe we can strip out the plist entries without the dependency. Moving discussion to 1384271.
Flags: needinfo?(haftandilian)
See Also: → bug 1384271
You need to log in before you can comment on or make changes to this bug.