Closed Bug 1175856 Opened 5 years ago Closed 5 years ago
.update .url .override to an empty value disables certificate checking
Not sure this is security sensitive, but better safe than sorry. While one can set app.update.url.override to a different url than app.update.url, which would also disable certificate checking, the fact is that when app.update.url.override is set to an empty value by the user, downloads are still done using app.update.url, but they are not verified. Even if it's not a security issue, it's a footgun that's better fixed. For some background on how I came to find this and why this is a problem for me: I've abused the update code to provide kind-of-but-not-quite nightlies on the elm branch without involving extra build jobs and without involving releng. Those builds are using a default value for app.update.url.override for their own update. The reason I went with app.update.url.override is so that users can just reset that value if they want to switch back to normal nightlies. In retrospect, this is kind of a mistake because it disables the certificate verification (and ironically, when I first tried, I did it with app.update.url, so I did add app.update.certs.3.commonName and app.update.certs.3.issuerName for certificate verification). Anyways, that breaks the update tests on elm because they were trying to access the override url and that crashes the tests because of MOZ_DISABLE_NONLOCAL_CONNECTIONS. So I tried setting app.update.url.override to an empty value in the tests profile (since we can't set it as a default pref for tests only afaik), and that triggered different errors, where the update service would emit noupdatesfound instead of errorextra (for cert validation error).
Attachment #8624133 - Flags: review?(robert.strong.bugs)
Additionally, it would be good to add optional support for cert validation for app.update.url.override.
This breaks tests, obviously. That would need to be fixed if we go this path. Essentially, this forces to either set app.update.cert.checkAttributes to false or add the right certs if app.update.url.override is set. This can be footgun-ish if you go with app.update.cert.checkAttributes=false and then reset app.update.url.override. It /might/ be more desirable to add a separate certs branch for the override case (app.update.override.certs.*?). Please tell me what you prefer.
Assignee: nobody → mh+mozilla
Some background We've moved away from using cert checks on Win and Mac (hopefully soon on Linux as well) since mar signing provides much better security. Linux needs bug 1158870 and after that is landed the SSL cert checking can be disabled on Linux in bug 1151485. Bug 1158870 comment #21 has the current status of what needs to be done for that bug if you could help out with that. The elm branch is also available for testing this. After that I need to find out to what extent B2G is using app update to decide whether it is necessary to implement mar signing for B2G, if they can just use cert pinning, or if the SSL cert checking needs to stay in app update. My hope is the one-off SSL cert checking code can be removed from app update entirely.
OS: Unspecified → Linux
Hardware: Unspecified → All
I should have also mentioned that these checks were added as a stop gap and that they prevent updates in certain environments (so does cert pinning btw) such as some proxy configurations where they use a cert that is untrusted.
Attachment #8624150 - Attachment is obsolete: true
Comment on attachment 8624133 [details] [diff] [review] Still do cert validation when app.update.url.override is set to an empty value Review of attachment 8624133 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/update/nsUpdateService.js @@ +3633,5 @@ > LOG("Checker:onLoad - request completed downloading document"); > > var prefs = Services.prefs; > var certs = null; > + if (!getPref("getCharPref", PREF_APP_UPDATE_URL_OVERRIDE, null) && I switched elm itself to verify mar (and now have a new issue because it's not signed with the right key, but that's being solved), but that still doesn't fix the test issue on elm. So until the cert validation code is removed, I think this would be good to land. Worst case, I can land this on elm only. Or disable the cert validation tests. I'm confused as to what is blocking bug 1158870. There is a patch, and it should just work.
Comment on attachment 8624133 [details] [diff] [review] Still do cert validation when app.update.url.override is set to an empty value This should be fine. I haven't had time to look into why bug 1158870 had problems per bug 1158870 comment #21. If you apply the patch from bug 1158870 to elm and all is well it may very well have been issues in oak that Brian had ran into. If you do and all is well with updating on Linux then the patch from bug 1158870 should be able to just land as is.
Attachment #8624133 - Flags: review?(robert.strong.bugs) → review+
I didn't apply the patch from bug 1158870 to elm. I added --enable-verify-mar to some central mozconfig, which is equivalent (and the confvars.sh change is irrelevant on elm, since that only applies to windows and mac builds)
The presumed check error in comment 21 is what he got before he updated the patch to also change the release mozconfigs. A try push should tell if there are any discrepancies left.
So, if I'm going to land this on inbound, I'd rather know if this, in the end, is deemed security sensitive. (as in, should I obfuscate the commit message?)
It has been several years since there has been discussion regarding prefs causing issues with things like this so needinfo'ing dveditz. I think the last time was in regards to how extensions in the user profile can set default prefs and thereby get around only using the default pref.
Flags: needinfo?(robert.strong.bugs) → needinfo?(dveditz)
This is not really that sensitive, please go ahead and check in.
So, the patch changed behavior in some way: if app.update.url.override is now set as a default pref (as opposed to user pref), cert validation is enabled. This bites me on elm, but in the general case, I think this is fine. I'll just work around that on elm, but please feel free to tell me if you want that to be changed.
In case you didn't see comment 13. Otherwise, this made it to m-c a while ago: https://hg.mozilla.org/mozilla-central/rev/3c97d52675d7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Thanks! I made some headway on bug 1158870 and spohl is going to get it landed. After that it will be disabled and I'm hoping this code can be removed soon afterward since it is rather hacky.
You need to log in before you can comment on or make changes to this bug.