Disallow opening the browse for addons dialog on secure screens to avoid a security exploit. #13059

TheQuinbox · 2021-11-14T17:09:55Z

Link to issue number:

Extension of #13056, but I slightly broke it, so redoing here.

Summary of the issue:

It was possible for a user to bind a gesture to open the addons manager, copy their config to secure screens, press it, press the install button, and get a browse dialog, allowing them to run CMD as systemroot, and do all sorts of things.

Description of how this pull request fixes the issue:

Only show the addon manager if we're not on a secure screen.

Testing strategy:

Run NVDA from source.
Copy my config to secure screens.
Press Windows+L to lock my machine.
Press my hotkey.

Known issues with pull request:

None

Change log entries:

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English

…at the log on screen via addon manager.

lukaszgo1 · 2021-11-14T17:17:59Z

source/gui/addonGui.py

@@ -248,8 +248,9 @@ def __init__(self, parent):
 		self.getAddonsButton = generalActions.addButton(self, label=_("&Get add-ons..."))


This button should also be disabled when in secure mode since it opens a web browser from which you can easily do a lot of insecure stuff.

AppVeyorBot · 2021-11-14T17:29:24Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/qo2p4kwbow3uv32o/artifacts/output/nvda_snapshot_pr13059-24211,eaf52ee0.exe

See test results for failed build of commit eaf52ee0e5

TheQuinbox · 2021-11-14T17:55:14Z

OK, getAddons is also now disabled, and translation should work again. Not sure why system tests broke though.

mzanm · 2021-11-14T21:17:39Z

Why not just disable the entire add-ons manager from a secure screen, as far as I know you can't do anything useful in it since NVDA's configuration doesn't save in secure screens.

TheQuinbox · 2021-11-14T21:45:12Z

I did that in the initial PR, but people said they'd prefer this approach.

CyrilleB79 · 2021-11-14T21:59:34Z

Since the add-on manager is not reachable via NVDA menu in secure screen, I would also recommend not to be able to open it via a shortcut at all.

TheQuinbox · 2021-11-14T23:41:17Z

That was what my initial PR did, but no one seemed to like it. I'm cool with either way, just want to patch the security problem.

cary-rowen · 2021-11-14T23:58:46Z

Why not just disable the entire add-ons manager from a secure screen, as far as I know you can't do anything useful in it since NVDA's configuration doesn't save in secure screens.

Hi, We can at least remove the add-on from it, I actually did this often in the past,

cary-rowen · 2021-11-15T00:05:39Z

In addition to the install button, the button to get addon is also meaningless.
Why can't we allow the add-ons manager to be opened from the menu on the security screen,After all, NVDA could not manage the addon on the security screen now.
I apologize if there is a use case that I did not expect and must do this.

CyrilleB79 · 2021-11-15T07:57:47Z

Yes there are two options here:

Disable completely add-on management from secured screen
Enable it in a limited way, e.g. allow add-on suppression. In this case, the add-on manager should be reachable via the menu and via the shortcut but some buttons should be greyed out (Add, Help, Get add-ons). And clarify the allowed actions: remove, enable/disable?

michaelDCurran · 2021-11-15T08:19:22Z

I think the most appropriate direction here is to disable the add-ons manager from being opened at all on secure screens.

TheQuinbox · 2021-11-15T10:28:26Z

@michaelDCurran Done.

LeonarddeR · 2021-11-17T07:26:47Z

How is someone supposed to know what add-ons are active in the secure copy? Not everyone is able to browse to C:\Program Files (x86)\NVDA\systemConfig\addons

CyrilleB79 · 2021-11-17T08:28:34Z

This PR is aimed to fix a security issue. So I think it should be addressed as soon as possible and not be delayed by "almost new feature" requests.

The best way to address the security issue is to disable the possibility to open the add-on manager dialog in secure screen; it seems it was the original intention of NVDA developers when they implemented the menu item removal.

In a subsequent way we may open a new issue and then PR to discuss various possibilities regarding add-on in secure screen among which:

view installed add-ons and their state
view incompatible add-ons and the reason
uninstall add-ons
enable/disable add-ons
open the "About" dialog
In case of issue, this second PR could be reverted without impacting the security concern.

TheQuinbox · 2021-11-17T15:56:28Z

I agree. For now, the manager is just disabled. We can discuss possible implimentations in a different issue/pr.

lukaszgo1 · 2022-02-11T12:29:11Z

@michaelDCurran This PR seems ready to be merged. Are you planning to look into it? Frankly if I were a new contributor and my one line security bug fix would not be looked at in almost three months this would be my first and last contribution. I really believe external contributions from newcomers should be given more attention!

seanbudd

@michaelDCurran - can I merge this?

TheQuinbox · 2022-02-16T07:01:21Z

Wow, this is still open? I honestly forgot this pR even existed in the first place, it's a single line change and I don't get what's so hard about merging that.
It's sad to me that new features are seemingly prioritized over simple, 1-line bugfixes (especially ones like this). Granted the chances are slim, but if I had my NVDA set up in the right configuration without this patch, someone could get access to my machine when I don't want them to.
If there are any steps I missed that I need to do in order to merge this, please let me know, but after this experience, I'm honestly not sure if I'll be contributing to NVDA again. If a simple PR can't be merged, I'm just trying to imagine a larger one...

feerrenrut · 2022-02-17T01:39:17Z

@seanbudd if you're happy with the change / testing, please go ahead with the merge. @TheQuinbox Sorry this fell through the cracks, we'll review our processes to try to ensure this doesn't happen in the future.

…oid a security exploit. (#13059) Summary of the issue: It was possible for a user to bind a gesture to open the addons manager, copy their config to secure screens, press it, press the install button, and get a browse dialog, allowing them to run CMD as systemroot, and do all sorts of things. Description of how this pull request fixes the issue: Only show the addon manager if we're not on a secure screen.

TheQuinbox added 2 commits November 13, 2021 17:19

Fixed a security exploit that could allow you to get a browse dialog …

9c7b763

…at the log on screen via addon manager.

Disabled the install button in the addons manager on secure screens.

6760238

TheQuinbox requested a review from a team as a code owner November 14, 2021 17:09

TheQuinbox requested a review from michaelDCurran November 14, 2021 17:09

lukaszgo1 reviewed Nov 14, 2021

View reviewed changes

Translation fix, and also nuke get Addons button if secure.

b07a4fc

Disallow opening the addons manager from secure screens completely.

4e51f51

seanbudd approved these changes Feb 16, 2022

View reviewed changes

CyrilleB79 mentioned this pull request Feb 16, 2022

Made the description of NVDA+Control+Space clearer #13349

Merged

5 tasks

seanbudd added 2 commits February 17, 2022 12:45

Merge branch 'master' into addonManagerSecureScreens

64ed158

update changes

e91d1d5

seanbudd merged commit 7afc3ed into nvaccess:master Feb 17, 2022

nvaccessAuto added this to the 2022.1 milestone Feb 17, 2022

CyrilleB79 mentioned this pull request Feb 17, 2022

Disable context help in secure screen to avoid a security exploit. #13353

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow opening the browse for addons dialog on secure screens to avoid a security exploit. #13059

Disallow opening the browse for addons dialog on secure screens to avoid a security exploit. #13059

TheQuinbox commented Nov 14, 2021 •

edited by seanbudd

Loading

lukaszgo1 Nov 14, 2021

AppVeyorBot commented Nov 14, 2021

TheQuinbox commented Nov 14, 2021

mzanm commented Nov 14, 2021

TheQuinbox commented Nov 14, 2021

CyrilleB79 commented Nov 14, 2021

TheQuinbox commented Nov 14, 2021 •

edited

Loading

cary-rowen commented Nov 14, 2021

cary-rowen commented Nov 15, 2021

CyrilleB79 commented Nov 15, 2021

michaelDCurran commented Nov 15, 2021 via email

TheQuinbox commented Nov 15, 2021

LeonarddeR commented Nov 17, 2021

CyrilleB79 commented Nov 17, 2021

TheQuinbox commented Nov 17, 2021

lukaszgo1 commented Feb 11, 2022

seanbudd left a comment

TheQuinbox commented Feb 16, 2022

feerrenrut commented Feb 17, 2022

		@@ -248,8 +248,9 @@ def __init__(self, parent):
		self.getAddonsButton = generalActions.addButton(self, label=_("&Get add-ons..."))

Disallow opening the browse for addons dialog on secure screens to avoid a security exploit. #13059

Disallow opening the browse for addons dialog on secure screens to avoid a security exploit. #13059

Conversation

TheQuinbox commented Nov 14, 2021 • edited by seanbudd Loading

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

lukaszgo1 Nov 14, 2021

Choose a reason for hiding this comment

AppVeyorBot commented Nov 14, 2021

TheQuinbox commented Nov 14, 2021

mzanm commented Nov 14, 2021

TheQuinbox commented Nov 14, 2021

CyrilleB79 commented Nov 14, 2021

TheQuinbox commented Nov 14, 2021 • edited Loading

cary-rowen commented Nov 14, 2021

cary-rowen commented Nov 15, 2021

CyrilleB79 commented Nov 15, 2021

michaelDCurran commented Nov 15, 2021 via email

TheQuinbox commented Nov 15, 2021

LeonarddeR commented Nov 17, 2021

CyrilleB79 commented Nov 17, 2021

TheQuinbox commented Nov 17, 2021

lukaszgo1 commented Feb 11, 2022

seanbudd left a comment

Choose a reason for hiding this comment

TheQuinbox commented Feb 16, 2022

feerrenrut commented Feb 17, 2022

TheQuinbox commented Nov 14, 2021 •

edited by seanbudd

Loading

TheQuinbox commented Nov 14, 2021 •

edited

Loading