Bugzilla

(In reply to infinity0 from comment #16)
> Hey Haik, thanks for the background! That roughly matches what I was guessing, and I made my patches along the lines of those security considerations.
> 
> > With Debian, do we need to support the symlink target being arbitrary locations on the filesystem or are the symlink targets always within a particular directory?
> 
> We use symlinks quite a lot in Debian, which makes the current situation a bit awkward. However I have some working patches, so please read on:
> 
> In the general case, things on Debian can look like this:
> 
> ~~~~
> /usr/share/mozilla/extensions/{uuid-of-firefox}/extension1
> /usr/share/mozilla/extensions/{uuid-of-firefox}/extension1/file1
> /usr/share/mozilla/extensions/{uuid-of-firefox}/extension1/file2 -> ../../../../javascript/package1/file1
> /usr/share/mozilla/extensions/{uuid-of-firefox}/extension2 -> ../../../webext/extension2
> /usr/share/webext/extension2
> /usr/share/webext/extension2/file3
> /usr/share/webext/extension2/file4 -> ../../javascript/package2/file2
> /usr/share/javascript/package1/file1
> /usr/share/javascript/package2/file2
> ~~~~
> 
> The symlink targets can in theory be anywhere, though typically they are under `/usr`. The extension directories (i.e. the symlink sources) are always underneath either `/usr/share/mozilla/extensions/{uuid-of-firefox}/` or `/usr/share/webext`, for extensions shared between firefox and chromium.

OK, got it. The example makes it clear.

> `ExtensionProtocolHandler::AllowExternalResource` actually already contains logic to handle this type of situation, but only when `IsDevelopmentBuild()` is switched on, and it only works for a single whitelisted path - anything underneath `NS_GRE_DIR` is assumed to be trusted, and allowed to symlink anywhere else. This is the behaviour that exists in firefox today.

`IsDevelopmentBuild()` is used to identify cases where the developer is running Firefox out of a source tree. e.g., they have a mozilla-central repo built and are running `./mach run`. On Mac and Linux, our builds use symlinks where the target of the symlink is the build object dir. This is to make builds more efficient by reducing copying and storage space (the object dir could be on another filesystem.) 

> My patches extend this behaviour a bit, to provide a solution to this current issue for Debian. I will follow up this post with 2 patches, here is the explanation for them:
> 
> In my patch #1, I ignore `IsDevelopmentBuild()` and then change `NS_GRE_DIR` to `/usr/share/mozilla/extensions` on non-Mac systems. This matches similar logic from `nsXREDirProvider::GetSystemExtensionsDirectory` elsewhere in the source tree - although I'm not sure if the latter is actually used, XRE according to online docs has been totally replaced. In general a hardcoded path is unclean, and an enhancement to this patch might define a more "official" systems-extension-directory instead of hardcoding. Anyway, this fairly straightforward patch is sufficient to solve this issue for extension1 in my example above.

Better not to change NS_GRE_DIR/GreD for this because we use it as the directory containing the executable and it is used in many places. I think that would break the development build case

From a security perspective, it would make sense to allow following symlinks out of unpacked extensions if the extension is in the system extension dir (from the directory service provider using key XRESysSExtPD) where we expect it to require administrator privileges to write to. Regardless of `IsDevelopmentBuild()`, we could additionally allow following symlinks out of XRESysSExtPD. I think that would solve the extension1 issue, but not extension2 in your example.

> Then we run into another problem - firefox resolves symlinks when recording the extension base directory. So extension2 is considered to reside in `/usr/share/webext`, not To make our solution work for extension2, we have to also apply patch #2, to make firefox keep the symlink source. However, we run into another issue: Firefox apparently purposefully breaks symlink resolution in `file://` URLs for security reasons I didn't fully dig into. To work around this, we therefore add a `Normalize()` step in `SubstitutingProtocolHandler::ResolveURI` which is called when loading the extension's files. There is one weirdness which you can read in the patch, but it worked fine for me when I compiled and tested them locally.
> 
> Feedback, and especially about this patch #2, would be greatly appreciated.

To support the example2 case, adding a new directory service key to map to a directory that contains system extensions shared with other browsers sounds reasonable to me--now that our WebExtension API is compatible with other browsers. That would be /usr/share/webext for Debian. 

Can you say if that would address the problem for Debian? By that I mean allowing symlinks out of XRESysSExtPD and additionally a new directory service entry that would be for shared extensions.

Edit: let's address the issue raised by Shane and Andrew first. Namely, Debian doesn't need to use unpacked WebExtensions. If debian switched to using XPI's for the extensions, symlinks would not be used because each extension is a single XPI file. A WebExtension can be in .XPI form or unpacked (unzipped) as files on the filesystems. The latter is intended for use by developers working on an extension.

(In reply to infinity0 from comment #16)
> Hey Haik, thanks for the background! That roughly matches what I was guessing, and I made my patches along the lines of those security considerations.
> 
> > With Debian, do we need to support the symlink target being arbitrary locations on the filesystem or are the symlink targets always within a particular directory?
> 
> We use symlinks quite a lot in Debian, which makes the current situation a bit awkward. However I have some working patches, so please read on:
> 
> In the general case, things on Debian can look like this:
> 
> ~~~~
> /usr/share/mozilla/extensions/{uuid-of-firefox}/extension1
> /usr/share/mozilla/extensions/{uuid-of-firefox}/extension1/file1
> /usr/share/mozilla/extensions/{uuid-of-firefox}/extension1/file2 -> ../../../../javascript/package1/file1
> /usr/share/mozilla/extensions/{uuid-of-firefox}/extension2 -> ../../../webext/extension2
> /usr/share/webext/extension2
> /usr/share/webext/extension2/file3
> /usr/share/webext/extension2/file4 -> ../../javascript/package2/file2
> /usr/share/javascript/package1/file1
> /usr/share/javascript/package2/file2
> ~~~~
> 
> The symlink targets can in theory be anywhere, though typically they are under `/usr`. The extension directories (i.e. the symlink sources) are always underneath either `/usr/share/mozilla/extensions/{uuid-of-firefox}/` or `/usr/share/webext`, for extensions shared between firefox and chromium.

OK, got it. The example makes it clear.

> `ExtensionProtocolHandler::AllowExternalResource` actually already contains logic to handle this type of situation, but only when `IsDevelopmentBuild()` is switched on, and it only works for a single whitelisted path - anything underneath `NS_GRE_DIR` is assumed to be trusted, and allowed to symlink anywhere else. This is the behaviour that exists in firefox today.

`IsDevelopmentBuild()` is used to identify cases where the developer is running Firefox out of a source tree. e.g., they have a mozilla-central repo built and are running `./mach run`. On Mac and Linux, our builds use symlinks where the target of the symlink is the build object dir. This is to make builds more efficient by reducing copying and storage space (the object dir could be on another filesystem.) 

> My patches extend this behaviour a bit, to provide a solution to this current issue for Debian. I will follow up this post with 2 patches, here is the explanation for them:
> 
> In my patch #1, I ignore `IsDevelopmentBuild()` and then change `NS_GRE_DIR` to `/usr/share/mozilla/extensions` on non-Mac systems. This matches similar logic from `nsXREDirProvider::GetSystemExtensionsDirectory` elsewhere in the source tree - although I'm not sure if the latter is actually used, XRE according to online docs has been totally replaced. In general a hardcoded path is unclean, and an enhancement to this patch might define a more "official" systems-extension-directory instead of hardcoding. Anyway, this fairly straightforward patch is sufficient to solve this issue for extension1 in my example above.

Better not to change NS_GRE_DIR/GreD for this because we use it as the directory containing the executable and it is used in many places. I think that would break the development build case

From a security perspective, it would make sense to allow following symlinks out of unpacked extensions if the extension is in the system extension dir (from the directory service provider using key XRESysSExtPD) where we expect it to require administrator privileges to write to. Regardless of `IsDevelopmentBuild()`, we could additionally allow following symlinks out of XRESysSExtPD. I think that would solve the extension1 issue, but not extension2 in your example.

> Then we run into another problem - firefox resolves symlinks when recording the extension base directory. So extension2 is considered to reside in `/usr/share/webext`, not To make our solution work for extension2, we have to also apply patch #2, to make firefox keep the symlink source. However, we run into another issue: Firefox apparently purposefully breaks symlink resolution in `file://` URLs for security reasons I didn't fully dig into. To work around this, we therefore add a `Normalize()` step in `SubstitutingProtocolHandler::ResolveURI` which is called when loading the extension's files. There is one weirdness which you can read in the patch, but it worked fine for me when I compiled and tested them locally.
> 
> Feedback, and especially about this patch #2, would be greatly appreciated.

To support the example2 case, adding a new directory service key to map to a directory that contains system extensions shared with other browsers sounds reasonable to me--now that our WebExtension API is compatible with other browsers. That would be /usr/share/webext for Debian. 

Can you say if that would address the problem for Debian? By that I mean allowing symlinks out of XRESysSExtPD and additionally a new directory service entry that would be for shared extensions.