[Pkg-mozext-maintainers] Bug#919557: Bug#922944: #922944: handling symbolic links in webextensions

[Ximin Luo]

CC bug 919557 & interested parties
CC firefox Debian maintainer

As I mentioned on firefox bugzilla [1], I have figured out the exact place in the firefox code responsible for this issue.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1420286

Near the bottom of ExtensionProtocolHandler::NewStream in netwerk/protocol/res/ExtensionProtocolHandler.cpp we have: 

|   bool isResourceFromExtensionDir = false;
|   MOZ_TRY(extensionDir->Contains(requestedFile, &isResourceFromExtensionDir));
|   if (!isResourceFromExtensionDir) {
|     bool isAllowed;
|     MOZ_TRY_VAR(isAllowed, AllowExternalResource(extensionDir, requestedFile));
|     if (!isAllowed) {
|       LogExternalResourceError(extensionDir, requestedFile);
|       return Err(NS_ERROR_FILE_ACCESS_DENIED);
|     }
|   }

The sledgehammer fix (A) to this problem is to simply remove this piece of code. I did not look closely at the surrounding code, but it is possible this also applies for user-loaded extensions, in which case if we take this option, users could shoot themselves in the foot by loading a malicious extension that can then read from /home or something.

A more nuanced fix (B) would be:

In ExtensionProtocolHandler::AllowExternalResource, in the same file:

|   if (!mozilla::IsDevelopmentBuild()) {
|     return false;
|   }
| 
|   // On Mac and Linux unpackaged dev builds, system extensions use
|   // symlinks to point to resources in the repo dir which we have to
|   // allow loading. Before we allow an unpacked extension to load a
|   // resource outside of the extension dir, we make sure the extension
|   // dir is within the app directory.
|   bool result;
|   MOZ_TRY_VAR(result, AppDirContains(aExtensionDir));
|   if (!result) {
|     return false;
|   }

Drop the first "if (!mozilla::IsDevelopmentBuild())" (since it bypasses the rest of the function), and then in ExtensionProtocolHandler::AppDirContains:

|     MOZ_TRY(NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(mAppDir)));

We would simply replace this line with a hard-coded /usr/share/webext. On Debian NS_GRE_DIR is /usr/lib/firefox and AFAICT no extensions are installed here anyway, so there is no need to keep the existing check.

It would be good to know the firefox maintainer's opinion on this.

X

Dmitry Smirnov:
> On Wed, 21 Aug 2019 19:32:26 +0200 Bruno Kleinert <fuddl at debian.org> wrote:
>> I've got an ITP ticket open for keepassxc-browser [1] waiting to be
>> processed in the NEW packages queue. It's also affected by the reported
>> behavior, because I replaced code copies by symbolic links to files in
>> existing debian packages. My intention is to reduce the burden of the
>> security team by avoiding code copies.
>>
>> [...]
>>
>> I searched the web for a solution without success. Guidance for package
>> maintainers of web extensions with symbolic links is highly
>> appreciated.
> 
> I've stumbled upon the issue with symlinks with "form-history-control" 
> package. Under new Firefox rule, extensions should bundle everything they 
> need without using links to files outside of the extension directory. This 
> effectively makes webextentions an equivalent of statically linked binaries.
> 
> The way to handle such situation is to Build-Depend on required packages and 
> copy their resources into webextension directory. Every dependency that we 
> borrow resources from should be noted in "Built-Using" field.
> 
> This is how I have implemented this (not entirely satisfying) solution for 
> the "form-history-control" package:
> 
>   https://salsa.debian.org/webext-team/form-history-control/commit/98b0a3ab
> 


-- 
GPG: ed25519/56034877E1F87C35
GPG: rsa4096/1318EFAC5FBBDBCE
https://github.com/infinity0/pubkeys.git