Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Feb 18, 2022

Conversation

CyrilleB79
Copy link
Collaborator

@CyrilleB79 CyrilleB79 commented Feb 17, 2022

Link to issue number:

None
Can be considered a follow-up of #13059.

Summary of the issue:

If you open NVDA parameter from secure screen and press F1, context help opens in Internet explorer.
I am not an expert in security but I think that it's not secure to have access to the browser from secure screen.
More specifically, via the browser open dialog, I am able to open any file in notepad and modify it, e.g. allowing me to activate NVDA console.

Description of how this pull request fixes the issue:

Since context help is displayed in a browser, disable it on secure screen.

Testing strategy:

Manual testing

Test 1

Simulate secure screen and checked that context help is not available anymore (nothing happens):

  • Launch NVDA
  • Open NVDA Python console
  • Enter the following command:
    import globalVars;globalVars.appArgs.secure = True
  • Press NVDA+control+G to open NVDA settings window
  • Press F1 and checked that nothing ahppens

Test 2

Checked that context help is still working in normal mode:

  • Launch NVDA
  • Press NVDA+control+G to open NVDA settings window
  • Press F1 and checked that nothing ahppens

Known issues with pull request:

Context help is not available anymore on secure screen.
If this is a need, we may open an issue for it and try to address it later.
But I think that the security concern should be addressed first and should not wait for a solution to have context help on secure screen.

Change log entries:

Changes
In the existing change log, replace:
- Security fix: The addons manager dialog is now disabled on secure screens. (#13059)
by:
- Security fixes: The addons manager dialog and context help are now disabled on secure screens. (#13059, #13353)

Additional note

I think ui.browseableMessage is also based on a browser. Is there any security concern allowing it on secure screen?
I have not been able to do anything from it since there is no menu or shortcut to do anything it this windows.
Just mentionning it in case someone knows more than I on this topic.

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

@AppVeyorBot
Copy link

See test results for failed build of commit face28b908

@CyrilleB79 CyrilleB79 marked this pull request as ready for review February 17, 2022 13:46
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner February 17, 2022 13:46
@CyrilleB79
Copy link
Collaborator Author

Also cc @seanbudd since you have finally reviewed #13059 instead of @michaelDCurran.

@TheQuinbox
Copy link
Contributor

Nice to see other people also helping with security problems.

From my testing, and a look at the code, I don't think ui.browsableMessage has any serious security concerns. You can't control O or alt D, and the only menu is a system menu with 4 items:

  1. Move
  2. size
  3. A sepperator
  4. close.
    So I think we're safe as far as that goes.

@michaelDCurran michaelDCurran merged commit 9525b63 into nvaccess:master Feb 18, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Feb 18, 2022
seanbudd pushed a commit that referenced this pull request Feb 18, 2022
…13353)

If you open NVDA parameter from secure screen and press F1, context help opens in Internet explorer.
I am not an expert in security but I think that it's not secure to have access to the browser from secure screen.
More specifically, via the browser open dialog, I am able to open any file in notepad and modify it, e.g. allowing me to activate NVDA console.

Description of how this pull request fixes the issue:
Since context help is displayed in a browser, disable it on secure screen.
seanbudd pushed a commit that referenced this pull request Feb 21, 2022
…13353)

If you open NVDA parameter from secure screen and press F1, context help opens in Internet explorer.
I am not an expert in security but I think that it's not secure to have access to the browser from secure screen.
More specifically, via the browser open dialog, I am able to open any file in notepad and modify it, e.g. allowing me to activate NVDA console.

Description of how this pull request fixes the issue:
Since context help is displayed in a browser, disable it on secure screen.
@CyrilleB79 CyrilleB79 deleted the disableContextHelp branch March 3, 2022 20:56
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