Control: tags -1 + patch
Ximin Luo:
> Dmitry Smirnov:
>> On Sunday, 26 April 2020 9:25:06 AM AEST Ximin Luo wrote:
>>> The source code doesn't mention any particular reason, and one person on
>>> the upstream bug report mentions it in such an off-the-cuff and
>>> non-explanatory way I can't take it into account as a serious data point.
>>> We shouldn't just let a mere mention of "security" scare us into not
>>> touching stuff and using our own reasoning to fix bugs.
>>>
>>> And I *did* think about the possible security considerations, as I
>>> explained in my previous email, and derived my suggested patch based on
>>> these considerations. (FWIW, I have done and am doing various types of
>>> security work professionally, and I'm confident about this type of
>>> reasoning in general.)
>>
>> Did you consider the possibility of users having a mix of packaged and non-
>> packaged extensions? I think it is reasonable to contain/sandbox extensions
>> to prevent peeking to various file system locations through symlinks.
>>
>> Once Firefox is patched to allow symlinks, the threat might be from
>> malicious
>> symlinks in non-packaged extensions.
>>
>
> Yes, I covered this already. My suggested patch (B) would only traverse
> symlinks when the extension being loaded (the symlink being resolved) is
> itself underneath /usr/share/webext, other extensions would still not be
> allowed to traverse symlinks.
>
> Please do read through my first email in full.
>
Please see my attached patches, they work to fix the bug and I can now undo my
local umatrix workarounds, and firefox will traverse the symlinks fine.
IMO both of these patches should be applied. Although the second one is
slightly more intrusive, it results in much more flexible behaviour.
Specifically, firefox resolves extension directories if they are symlinks, so a
single "extensions directory" does not suffice for the typical Debian method of
symlinking stuff from /usr/share/webext.
In patch 1 to avoid complexity, only one single extensions directory is
whitelisted, and that is /usr/share/webext. This means extensions installed
directly into /usr/share/mozilla/extensions (e.g. uBlock) still won't work with
symlinks.
In patch 2, we avoid resolving extension directories if they are symlinks.
Then, anything installed into /usr/share/mozilla/extensions will be allowed to
traverse symlinks, including extensions that are symlinked into that directory.
By looking at the surrounding code, it is fairly clear that the patch only
affects unpacked system extensions and nothing else, so it should be
uncontroversial. However it does go a bit deeper into firefox internals, and I
am unclear exactly why one part of it works - but empirically it works, I have
tested it quite thoroughly. More testing is of course welcome, as well as any
feedback.
(It is possible to shortcut part of the Debian build process for firefox, and
sudo cp libxul.so and omni.ja into /usr/lib/firefox as soon as they have been
built. However you will need to rm -rf ~/.cache/mozilla/firefox/*/startupCache/
for firefox to see the updates to omni.ja. If you install the debs, this step
should be unnecessary, however it takes about 20-30 more minutes to produce
them.)
Best,
X
--
GPG: ed25519/56034877E1F87C35
GPG: rsa4096/1318EFAC5FBBDBCE
https://github.com/infinity0/pubkeys.git
--- a/netwerk/protocol/res/ExtensionProtocolHandler.cpp
+++ b/netwerk/protocol/res/ExtensionProtocolHandler.cpp
@@ -578,11 +578,8 @@
// allow a resource from outside of the extension dir.
return false;
#else
- if (!mozilla::IsDevelopmentBuild()) {
- return false;
- }
- // On Mac and Linux unpackaged dev builds, system extensions use
+ // On Mac and Linux builds (including Debian), 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
@@ -633,13 +630,22 @@
#if !defined(XP_WIN)
Result<bool, nsresult> ExtensionProtocolHandler::AppDirContains(
nsIFile* aExtensionDir) {
- MOZ_ASSERT(mozilla::IsDevelopmentBuild());
MOZ_ASSERT(!IsNeckoChild());
// On the first invocation, set mAppDir
if (!mAlreadyCheckedAppDir) {
mAlreadyCheckedAppDir = true;
+ // below defs copied from nsXREDirProvider.cpp
+#if defined(XP_MACOSX)
MOZ_TRY(NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(mAppDir)));
+#else
+# if defined(__OpenBSD__) || defined(__FreeBSD__)
+ static const char* const sysLExtDir2 = "/usr/local/share/webext";
+# else
+ static const char* const sysLExtDir2 = "/usr/share/webext";
+# endif
+ MOZ_TRY(NS_NewNativeLocalFile(nsDependentCString(sysLExtDir2), false, getter_AddRefs(mAppDir)));
+#endif
if (MOZ_LOG_TEST(gExtProtocolLog, LogLevel::Debug)) {
nsAutoCString appDirPath;
Unused << mAppDir->GetNativePath(appDirPath);
--- a/netwerk/protocol/res/ExtensionProtocolHandler.cpp
+++ b/netwerk/protocol/res/ExtensionProtocolHandler.cpp
@@ -640,9 +640,9 @@
MOZ_TRY(NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(mAppDir)));
#else
# if defined(__OpenBSD__) || defined(__FreeBSD__)
- static const char* const sysLExtDir2 = "/usr/local/share/webext";
+ static const char* const sysLExtDir2 = "/usr/local/share/mozilla/extensions";
# else
- static const char* const sysLExtDir2 = "/usr/share/webext";
+ static const char* const sysLExtDir2 = "/usr/share/mozilla/extensions";
# endif
MOZ_TRY(NS_NewNativeLocalFile(nsDependentCString(sysLExtDir2), false, getter_AddRefs(mAppDir)));
#endif
@@ -762,7 +762,8 @@
// Normalize paths for sane comparisons. nsIFile::Contains depends on
// it for reliable subpath checks.
- MOZ_TRY(extensionDir->Normalize());
+ //MOZ_TRY(extensionDir->Normalize());
+ // ^ don't normalize on Debian; extensions in /usr/share/mozilla/extensions are actually symlinks elsewhere
MOZ_TRY(requestedFile->Normalize());
#if defined(XP_WIN)
if (!widget::WinUtils::ResolveJunctionPointsAndSymLinks(extensionDir) ||
--- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm
+++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ -1009,6 +1009,7 @@
linkedDirectory = aFile.clone();
try {
linkedDirectory.normalize();
+ linkedDirectory = aFile.clone(); // keep these as symlinks, Debian uses them heavily
} catch (e) {
logger.warn(
`Symbolic link ${aFile.path} points to a path ` +
--- a/netwerk/protocol/res/SubstitutingProtocolHandler.cpp
+++ b/netwerk/protocol/res/SubstitutingProtocolHandler.cpp
@@ -632,6 +632,25 @@
path.Insert('.', 0);
}
rv = baseURI->Resolve(path, result);
+
+ // Normalise the path here because file:// doesn't do it properly, see
+ // https://bugzilla.mozilla.org/show_bug.cgi?id=803999
+ if (NS_WARN_IF(NS_FAILED(rv))) return rv;
+ nsCOMPtr<nsIFile> localFile;
+ rv = net_GetFileFromURLSpec(result, getter_AddRefs(localFile));
+ if (NS_FAILED(rv)) {
+ rv = NS_OK; // it wasn't a file URL, ignore failure
+ } else {
+ rv = localFile->Normalize();
+ if (NS_WARN_IF(NS_FAILED(rv))) {
+ // This sometimes fails, and it's better to continue with the old path
+ // instead of failing, because that blocks the rest of the browser. It
+ // never fails in the cases that matter, i.e. when the path is actually
+ // to be loaded into the extension. So, we don't worry about it.
+ MOZ_LOG(gResLog, LogLevel::Debug, ("Normalize failed; ignoring: %s\n", PromiseFlatCString(result).get()));
+ }
+ rv = net_GetURLSpecFromActualFile(localFile, result);
+ }
}
if (NS_WARN_IF(NS_FAILED(rv))) {
