Create registry key to force secure mode by seanbudd · Pull Request #15049 · nvaccess/nvda

seanbudd · 2023-06-23T03:06:48Z

Link to issue number:

Summary of the issue:

System administrators may wish to configure NVDA to restrict unauthorized system access.
Secure mode can be forced through the --secure CLI parameter, however, forcing this parameter is not easily done for system administrators.

In a corporate environment where application usage is restricted, NVDA should be forced into secure mode via a registry key parameter, rather than a CLI parameter

Description of user facing changes

Added a registry key parameter to force secure mode of NVDA.

Improved documentation for system administrators on secure mode.

Description of development approach

Added a registry key parameter to force secure mode of NVDA.

make isRunningOnSecureDesktop and isLockScreenModeActive part of public API

Testing strategy:

Tested setting the reg key parameter.

Known issues with pull request:

None

Change log entries:

After 2023.2 release, relevant info should be added to the NV Access corporate page

New features

A system wide parameter has been added to allow users and system administrators to force NVDA to start in secure mode.

Deprecations
config.CONFIG_IN_LOCAL_APPDATA_SUBKEY is deprecated. Instead use config.RegistryKey.CONFIG_IN_LOCAL_APPDATA_SUBKEY.

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
Security precautions taken.

Qchristensen

Good work, this will be very useful for some of our corporate users

LeonarddeR

I'm pretty sure this registry key gets lost after an update. To prevent this, you probably need to adapt installer.install in the same way as this has been done for the configInLocalAppData flag. So you'd also need adding an extra parameter to installer.registerInstallation.

Also, while I Really like this change, I'm pretty sure globalVars.appArgs.secure is treated by add-ons as a way of checking whether the add-on is running on a secure screen. Pretty sure NVDA Remote uses it like this. My NVDA Remote Desktop add-on uses systemutils._isSecureDesktop, but as the name of that function implies, it's meant as a private API. I'm not saying this should hold this change back, but it must be carefully considered that both add-ons and NVDA Core doesn't assume globalVars.appArgs.secure and secure desktop are the same.
I'd suggest going forward as follows:

Do a scan of NVDA Internals for the secure flag. Ensure that there's no logic using this that expects a secure desktop instead of just a secure copy.
Provide a public API to find out whether we're on a secure desktop, preferably a read-only flag that doesn't have to get the current desktop every time.
Clearly communicate to add-on authors that they should change from globalVars.appArgs.secure to the new API to check whether running on a secure desktop.

seanbudd · 2023-06-23T06:41:38Z

@LeonarddeR

Thanks for the pointers surrounding preventing the registry key from being lost during an update.

Also, while I Really like this change, I'm pretty sure globalVars.appArgs.secure is treated by add-ons as a way of checking whether the add-on is running on a secure screen.

This is unexpected usage, and has been incorrect for a long time. The --secure CLI parameter has existed for 10 years and contradicts this assumption (b7821e1). The SecureDesktopNVDAObject has been the historic method of checking if NVDA is on a secure screen. Moving away from this is proposed in #14488, and staged for 2024.1.

Do a scan of NVDA Internals for the secure flag. Ensure that there's no logic using this that expects a secure desktop instead of just a secure copy.

As this underlying assumption has been incorrect for a long time, and with recent security focused rewrites, I am fairly confident that this sort of logic does not exist. The way secure mode is defined has always been based on being a secure mode, the CLI parameter has existed for 10+ years , and should not be conflated with secure screens.

Provide a public API to find out whether we're on a secure desktop, preferably a read-only flag that doesn't have to get the current desktop every time.

This should be covered by the gain focus event for SecureDesktopNVDAObject currently, and in future the proposed change in #14488.

Clearly communicate to add-on authors that they should change from globalVars.appArgs.secure to the new API to check whether running on a secure desktop.

Again, I am surprised by this assumption, but we can always remind API consumers of the difference between secure mode, secure screens, etc.

CyrilleB79 · 2023-06-23T06:42:17Z

@seanbudd, you write:

After merging, relevant info should be added to the NV Access corporate page

IMO, the website should be updated after the stable release instead.

seanbudd · 2023-06-23T06:46:04Z

Also note that NVDA Remote uses SecureDesktopNVDAObject as intended: NVDARemote/NVDARemote#309

LeonarddeR · 2023-06-23T06:58:42Z

This is unexpected usage, and has been incorrect for a long time. The --secure CLI parameter has existed for 10 years and contradicts this assumption (b7821e1). The SecureDesktopNVDAObject has been the historic method of checking if NVDA is on a secure screen. Moving away from this is proposed in #14488, and staged for 2024.1.

I'm afraid you're mixing up two things here. The SecureDesktopNVDAObject is indeed meant to check whether we're on a secure desktop when you're talking about the instance of NVDA that runs on the user desktop. However, how about the copy that runs on the secure desktop itself?

I am fairly confident that this sort of logic does not exist.

As per above, I'm slightly in doubt of that. Furthermore, just a git grep for globalVars.appArgs.secure comes up with a check in jabHandler that looks to me like a way to avoid access bridge registration on secure desktops. I couldn't think of a reason why the jab handler shouldn't work with the secure parameter otherwise.

Provide a public API to find out whether we're on a secure desktop, preferably a read-only flag that doesn't have to get the current desktop every time.

This should be covered by the gain focus event for SecureDesktopNVDAObject currently

To clarify, I'm talking about the copy that runs on the secure desktop itself, not the user copy.

seanbudd · 2023-06-26T07:32:09Z

@LeonarddeR - ah thanks I see that NVDARemote does indeed rely on globalVars.appArgs.secure. We definitely should commit systemutils._isSecureDesktop (name as runningOnSecureDesktop?) to the public API to handle this.

CyrilleB79 · 2023-06-26T07:43:34Z

@seanbudd , while at it, may you put winAPI.sessionTracking._isLockScreenModeActive in the public API?
We had discussed it together during 2022.3.x dev cycle regarding a request for Character Information add-on. And you wrote:

A public API can be requested by add-on developers 2-3 month after 2022.3.3 release if no issue is found.

AppVeyorBot · 2023-06-27T07:11:31Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/450v8xc503qa9ug4/artifacts/output/nvda_snapshot_pr15049-28508,9a7c58cf.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 25.1,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.9,
FINISH_END 0.2

See test results for failed build of commit 9a7c58cfa0

seanbudd · 2023-06-28T00:38:06Z

@LeonarddeR - I've done a review of .secure instances. All seem generally correct.

The disabled JAB handler code encompassed setting a flag to let Java know we are using a11y.
It was disabled as we generally should not write to disk from secure mode.
However, this is an one-off system operation for NVDA and it is not user configurable.

In 2a24b14 I also changed blackAction.Context.WINDOWS_LOCKED, which is currently only checked by the add-on store.

stale

Qchristensen

This will be of benefit to many corporate users wanting to roll out NVDA securely, great work!

michaelDCurran

All good, with just one suggestion fix for a typo in user guide.

Co-authored-by: Michael Curran <mick@nvaccess.org>

Create registry key to force secure mode

8ccf075

seanbudd requested review from a team as code owners June 23, 2023 03:06

seanbudd requested review from Qchristensen and michaelDCurran June 23, 2023 03:06

Qchristensen previously approved these changes Jun 23, 2023

View reviewed changes

LeonarddeR suggested changes Jun 23, 2023

View reviewed changes

seanbudd marked this pull request as draft June 23, 2023 06:41

seanbudd added 3 commits June 27, 2023 16:00

preserve forceSecureMode value on upgrade

372c6a2

make isRunningOnSecureDesktop part of public API

69d65f4

make isLockScreenModeActive part of public API

ea770dd

LeonarddeR approved these changes Jun 27, 2023

View reviewed changes

seanbudd added 2 commits June 28, 2023 09:23

fix lint

eb88984

Fix up incorrect secure mode usages

2a24b14

seanbudd marked this pull request as ready for review June 28, 2023 00:33

seanbudd added 2 commits June 28, 2023 10:47

fix up deprecation

235cbb2

fix up mistake

34062a1

seanbudd requested a review from Qchristensen June 28, 2023 00:51

Qchristensen approved these changes Jun 28, 2023

View reviewed changes

michaelDCurran approved these changes Jul 7, 2023

View reviewed changes

Comment thread user_docs/en/userGuide.t2t Outdated

fix typo

fa11a02

Co-authored-by: Michael Curran <mick@nvaccess.org>

seanbudd added 2 commits July 7, 2023 12:00

Merge remote-tracking branch 'origin/master' into forceSecureMode

6238269

update changes

aa0988b

seanbudd merged commit 67ecd25 into master Jul 7, 2023

seanbudd deleted the forceSecureMode branch July 7, 2023 02:02

nvaccessAuto added this to the 2023.2 milestone Jul 7, 2023

Uh oh!

Conversation

seanbudd commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrilleB79 commented Jun 23, 2023

Uh oh!

seanbudd commented Jun 23, 2023

Uh oh!

LeonarddeR commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanbudd commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrilleB79 commented Jun 26, 2023

Uh oh!

AppVeyorBot commented Jun 27, 2023

Uh oh!

seanbudd commented Jun 28, 2023

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

michaelDCurran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

seanbudd commented Jun 23, 2023 •

edited

Loading

seanbudd commented Jun 23, 2023 •

edited

Loading

LeonarddeR commented Jun 23, 2023 •

edited

Loading

seanbudd commented Jun 26, 2023 •

edited

Loading