GNOME Bugzilla – Bug 781728
Extensions enabled in screenshield if one extension fails to reload
Last modified: 2017-07-17 11:07:39 UTC
Hi, It is possible to get extensions enabled in the screenshield if one extension failed to reload. To trigger this bug, you need to: - have one extension that fails to reload - have other extensions enabled. order of the enabled-extensions key matters - change disable-extension-version-validation to trigger a call to _onVersionValidationChanged() _onVersionValidationChanged() doesn't handle exceptions to enableExtension(), which can happen with some extensions that fail to reload. One example is the EasyScreenCast extension before I fixed it, see: https://github.com/EasyScreenCast/EasyScreenCast/issues/46 https://github.com/EasyScreenCast/EasyScreenCast/pull/131 If you have that extension installed (a broken version of it, that is), and trigger a _onVersionValidationChanged() you'll get: Apr 25 17:12:12 tatooine gnome-shell[5006]: JS ERROR: Error: Type name EasyScreenCastSettingsWidget is already registered GObjectMeta<._construct@resource:///org/gnome/gjs/modules/overrides/GObject.js:145 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 Class.prototype._construct/newClass@resource:///org/gnome/gjs/modules/lang.js:213 @/home/emilio/.local/share/gnome-shell/extensions/EasyScreenCast@iacopodeenosee.gmail.com/prefs.js:633 @/home/emilio/.local/share/gnome-shell/extensions/EasyScreenCast@iacopodeenosee.gmail.com/convenience.js:35 @/home/emilio/.local/share/gnome-shell/extensions/EasyScreenCast@iacopodeenosee.gmail.com/extension.js:34 initExtension@resource:///org/gnome/shell/ui/extensionSystem.js:220 enableExtension@resource:///org/gnome/shell/ui/extensionSystem.js:109 _onVersionValidationChanged/<@resource:///org/gnome/shell/ui/extensionSystem.js:284 _onVersionValidationChanged@resource:///org/gnome/shell/ui/extensionSystem.js:283 Then if you lock and unlock your screen, you'll get: Apr 25 17:12:27 tatooine gnome-shell[5006]: JS ERROR: Exception in callback for signal: updated: Error: Type name EasyScreenCastSettingsWidget is already registered GObjectMeta<._construct@resource:///org/gnome/gjs/modules/overrides/GObject.js:145 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 Class.prototype._construct/newClass@resource:///org/gnome/gjs/modules/lang.js:213 @/home/emilio/.local/share/gnome-shell/extensions/EasyScreenCast@iacopodeenosee.gmail.com/prefs.js:633 @/home/emilio/.local/share/gnome-shell/extensions/EasyScreenCast@iacopodeenosee.gmail.com/convenience.js:35 @/home/emilio/.local/share/gnome-shell/extensions/EasyScreenCast@iacopodeenosee.gmail.com/extension.js:34 initExtension@resource:///org/gnome/shell/ui/extensionSystem.js:220 enableExtension@resource:///org/gnome/shell/ui/extensionSystem.js:109 enableAllExtensions/<@resource:///org/gnome/shell/ui/extensionSystem.js:311 enableAllExtensions@resource:///org/gnome/shell/ui/extensionSystem.js:310 _sessionUpdated@resource:///org/gnome/shell/ui/extensionSystem.js:338 _emit@resource:///org/gnome/gjs/modules/signals.js:124 SessionMode<._sync@resource:///org/gnome/shell/ui/sessionMode.js:205 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 SessionMode<.popMode@resource:///org/gnome/shell/ui/sessionMode.js:174 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 ScreenShield<._continueDeactivate@resource:///org/gnome/shell/ui/screenShield.js:1200 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 ScreenShield<.deactivate/<@resource:///org/gnome/shell/ui/screenShield.js:1185 AuthPrompt<.finish@resource:///org/gnome/shell/gdm/authPrompt.js:506 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 UnlockDialog<.finish@resource:///org/gnome/shell/ui/unlockDialog.js:147 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 ScreenShield<.deactivate@resource:///org/gnome/shell/ui/screenShield.js:1184 wrapper@resource:///org/gnome/gjs/modules/lang.js:178 ScreenShield<._init/</<@resource:///org/gnome/shell/ui/screenShield.js:536 _emit@resource:///org/gnome/gjs/modules/signals.js:124 _convertToNativeSignal@resource:///org/gnome/gjs/modules/overrides/Gio.js:129 That first time, the lock screen is fine, i.e. there are no extensions enabled. However if you lock it again, you'll finally get the other extensions enabled on the screenshield. If one of those extensions is e.g. the applications menu, you can start any program from there. I have come up with a patch that solves this, though I'm not entirely sure of how getting the exception in _onVersionValidationChanged() leads to all this. Perhaps someone more familiar with all this can double check this. I'm not sure if we should catch exceptions in other places, e.g. in disableAllExtensions(). Perhaps we should to be on the safe side, though this patch is sufficient to fix this bug here... Patch coming in a moment. BTW all this is with gnome-shell 3.22.3. I haven't tried with a newer version, though the extensionSystem.js hasn't changed much, so this likely affects newer versions.
Created attachment 350411 [details] [review] extensionSystem: handle reloading broken extensions
FWIW this was initially seen in Kali (https://bugs.kali.org/view.php?id=2513) but I can reproduce it on Debian sid as well.
Review of attachment 350411 [details] [review]: LGTM
Thanks, pushed. To ssh://git.gnome.org/git/gnome-shell 9a65f20d9..ff425d1db master -> master
For the record, this has been assigned CVE-2017-8288.
So I took another look at this, and concluded that the commit patch is more of a workaround than a fix (see the reasoning in the upcoming patch). Unfortunately I'm unable to reproduce the original problem, so I'm attaching some patches in the hope that someone else can confirm that they address the issue as well ...
Created attachment 352173 [details] [review] Revert "extensionSystem: handle reloading broken extensions" Both reloadExtensions() and enableExtensions() are already expected to catch extension errors. If they don't, this is the bug that should be fixed instead of catching unhandled exceptions in the caller. This reverts commit ff425d1db7082e2755d2a405af53861552acf2a1.
Created attachment 352174 [details] [review] extensions: Consistently handle createExtenionObject() errors The method may throw an error, for example when metadata.json is missing or cannot be parsed, however we are currently not always handling it.
Created attachment 352175 [details] [review] extensionSystem: Catch import errors While we catch errors that occur when calling init(), enable() or disable(), the import itself can throw an exception, for instance if the extension imports an unavailable typelib or tries to draw in a conflicting library.
Created attachment 352176 [details] [review] extensionSystem: Consistently handle initExtension() errors The method currently catches errors that occur when calling the extension's init() method, but throws itself an error if the expected extension.js file is missing. The former is pointless if we expect all callers to handle errors themselves anyway, and we should avoid the latter if we don't - opt for the second option and handle a missing extension.js file gracefully.
Comment on attachment 352173 [details] [review] Revert "extensionSystem: handle reloading broken extensions" I agree on the workaround nature, but would it perhaps be better to change the logged error to something around the "exception caught while reloading extension, this is a gnome-shell bug" lines? The try/catch block here caters for present (not anymore soon!) and future screwups. If this happens, it is indeed a bug and should be fixed asap, but IMHO sounds better not to let that turn into another CVE :).
(In reply to Carlos Garnacho from comment #11) > I agree on the workaround nature, but would it perhaps be better to change > the logged error to something around the "exception caught while reloading > extension, this is a gnome-shell bug" lines? Dunno. If we keep catching errors here, then we should also guard against the same error elsewhere (for instance when triggered from `gnome-shell-extension-prefs --reload`). And as errors in reloadExtension() essentially boil down to an error in disableExtension() or enableExtension(), we'd have to guard all those calls as well. I'm not overly keen on spreading try/catch blocks all over the place, but it's certainly a valid position. However keeping just this particular error handling because someone happened to stamp a CVE on it really seems silly ...
Comment on attachment 352174 [details] [review] extensions: Consistently handle createExtenionObject() errors Looks good! btw, typo in log summary, "Extenion" :)
Comment on attachment 352175 [details] [review] extensionSystem: Catch import errors Makes total sense.
Comment on attachment 352176 [details] [review] extensionSystem: Consistently handle initExtension() errors The code change and the rationale in the commit log make sense, but the "throw new error" right above in the patch context kinda defeats my sense of "consistent" :/
(In reply to Carlos Garnacho from comment #13) > Looks good! btw, typo in log summary, "Extenion" :) Gah, fixed locally. (In reply to Carlos Garnacho from comment #15) > the "throw new error" right above in the patch context kinda defeats my sense > of "consistent" :/ Mmh, I can see that. The reasoning for not touching that code is that it's not an error that can be triggered by an extension, but only by a bug in the extension system itself. Still, might be a good idea to turn that into a log warning as well and just return normally ...
(In reply to Florian Müllner from comment #12) > (In reply to Carlos Garnacho from comment #11) > > I agree on the workaround nature, but would it perhaps be better to change > > the logged error to something around the "exception caught while reloading > > extension, this is a gnome-shell bug" lines? > > Dunno. If we keep catching errors here, then we should also guard against > the same error elsewhere (for instance when triggered from > `gnome-shell-extension-prefs --reload`). And as errors in reloadExtension() Fair enough. > essentially boil down to an error in disableExtension() or > enableExtension(), we'd have to guard all those calls as well. I'm not > overly keen on spreading try/catch blocks all over the place, but it's > certainly a valid position. However keeping just this particular error > handling because someone happened to stamp a CVE on it really seems silly ... Your call :), I get your point, and I like neat code too. I just thought it's actually not that bad as a safety net (because decisions and code direction get hazy over the years), but it's indeed a too specific place for it to be. (In reply to Florian Müllner from comment #16) > (In reply to Carlos Garnacho from comment #15) > > the "throw new error" right above in the patch context kinda defeats my sense > > of "consistent" :/ > > Mmh, I can see that. The reasoning for not touching that code is that it's > not an error that can be triggered by an extension, but only by a bug in the > extension system itself. Still, might be a good idea to turn that into a log > warning as well and just return normally ... Right, makes some sense to keep that as an error then... maybe the log summary just needs a s/errors/extension errors/ to lower my brow :)
Comment on attachment 350411 [details] [review] extensionSystem: handle reloading broken extensions Attachment 350411 [details] pushed as ff425d1 - extensionSystem: handle reloading broken extensions
Created attachment 355745 [details] [review] extensionSystem: Handle all initExtension() extension errors Updated the commit message to not imply that non-extension errors were handled as well.
Attachment 352173 [details] pushed as 8d6efde - Revert "extensionSystem: handle reloading broken extensions" Attachment 352175 [details] pushed as d461d02 - extensionSystem: Catch import errors Attachment 355745 [details] pushed as e845f41 - extensionSystem: Handle all initExtension() extension errors