Skip to content

Fixed a security issue where you could get a browse dialog on a secure screen via addon manager#13056

Closed
trypsynth wants to merge 1 commit into
nvaccess:masterfrom
trypsynth:master
Closed

Fixed a security issue where you could get a browse dialog on a secure screen via addon manager#13056
trypsynth wants to merge 1 commit into
nvaccess:masterfrom
trypsynth:master

Conversation

@trypsynth

Copy link
Copy Markdown
Contributor

Link to issue number:

None

Summary of the issue:

If a user bound a gesture to open the addon manager, and copied it to their config at secure screens, someone could press it, get a browse dialog, and run CMD as systemroot, as well as do all sorts of other things

Description of how this pull request fixes the issue:

Added a check in globalCommands.py to see if we're on a secure screen before opening the addon manager via a gesture.

Testing strategy:

  1. Ran NVDA from source.
  2. Bound a gesture to open the addon manager.
  3. Copied my config to secure screens.
  4. Pressed windows+l to log out, and attempted to press it

Known issues with pull request:

None

Change log entries:

Not really sure if this deserves a CL entry, but if so...

Bug fixes

  • It is no longer possible to open the addons manager from a secure screen, in order to avoid a security exploit.

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

@trypsynth trypsynth requested a review from a team as a code owner November 14, 2021 00:28
@trypsynth trypsynth requested a review from seanbudd November 14, 2021 00:28
@trypsynth

Copy link
Copy Markdown
Contributor Author

Oops... I totally just realised I forgot to set this as a draft. Sorry, I can't figure out how to do it.

@trypsynth trypsynth marked this pull request as draft November 14, 2021 00:30
@trypsynth

Copy link
Copy Markdown
Contributor Author

Never mind, I'm such a newb. This should work

@cary-rowen

Copy link
Copy Markdown
Contributor

To be honest, I didn’t think this PR was very useful before NVDA did not allow the management of addon on the security screen. I can now use this vulnerability to manage addon on the security screen. I admit that there are security risks, but I think NVDA should Provide users with a feasible solution as soon as possible to make it more convenient for users to manage the addon of the security screen.

@trypsynth

Copy link
Copy Markdown
Contributor Author

Yeah, maybe allow disabling/enabling of addons or something, but definitly not the browse dialog. This works for now though, I think.

@cary-rowen

Copy link
Copy Markdown
Contributor

Yes, I hope NVDA can take this into consideration, even if the addon manager is opened on the security screen, the install plugin button is not displayed.

@josephsl

josephsl commented Nov 14, 2021 via email

Copy link
Copy Markdown
Contributor

@trypsynth

Copy link
Copy Markdown
Contributor Author

@josephsl, yes. Something like this in gestures.ini
[globalCommands.GlobalCommands]
activateAddonsManager = kb(laptop):a+control+nvda+shift
And I'm slightly confused what you mean about cmd output? Fair enough about the issue/PR thing though. I just figured because it was a 2-line change, I'd just open a PR. But will keep that in mind for the future.

@Brian1Gaff

Brian1Gaff commented Nov 14, 2021 via email

Copy link
Copy Markdown

@lukaszgo1

Copy link
Copy Markdown
Contributor

My preference would be either to disable the 'install' button in add-ons manager when in secure mode, or if we really want to disallow opening it on secure screens move this check inside onAddonsManagerCommand method. As an aside contributing guide asks not to use master when submitting PR's and create a separate topic branch for each patch. It should be possible to cherry-pick this single commit to a new branch and just change the base for this PR.

@trypsynth

Copy link
Copy Markdown
Contributor Author

OK. I found the lines in the addonGui to spawn the install button. Will close this PR, and open a new one (on a fresh branch) disabling that on secure screens :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants