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

Check Z-Order for making content accessible on the lock screen. #14416

Merged
merged 28 commits into from
Dec 16, 2022

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Dec 2, 2022

Link to issue number:

Supercedes #14358
Fixes: #14379
Fixes: #14368

Summary of the issue:

Issue 1: Session tracking notification failures (#14358)

If NVDA freezes, session tracking notifications may be dropped.
If this happens when locking, NVDA will be insecure while on the Windows lock screen.
If this happens when unlocking, NVDA will not behave correctly and be inaccessible while Windows is unlocked.

This is fixed by querying the session directly, and caching this every core cycle.
If a query fails, NVDA should fall back accessible behaviour, rather than secure.

Issue 2: Forgot my PIN workflow is inaccessible (#14368)

NVDA cannot read content on the forgot my PIN workflow screen.
This is a similar situation to the lock screen, except an Edge Spartan window is used for the workflow.
This runs on a temporary user profile.

This is fixed by detecting the z-order of windows, and making an window above the lock screen window accessible.

Issue 2a: Object navigation does not work on the PIN workflow screen (#14416)

This is because TextInfo.obj can be a TreeInterceptor, where it was previously documented as just NVDAObject.
This assumption caused the _isSecureObjectWhileLockScreenActivated function to fail, making object navigation fail.
In those cases, the TreeInterceptor.rootNVDAObject should be checked instead.

Issue 3: NVDA fails to install in some environments (#14379)

Sometimes an NVDA session query returns an unexpected value.
In this case, default to the "session unknown" behaviour.
If a session query fails, NVDA should roll back to accessible behaviour rather than failing to run.

Description of user facing changes

PIN workflow screen should become accessible.
NVDA has better session tracking management (i.e. is aware of the lock state more accuractely).
NVDA should handle session query failures without preventing installation, blocking usage, etc.

Description of development approach

There are 4 security modes of NVDA:

  • normal, authenticated user
  • secure mode: secure desktop mode (when serviceDebug param not set), or --secure param provided.
    Refer to existing docs on this.
  • secure desktop mode: enabled secure mode (when serviceDebug param not set).
    Also prevents access to some controls, that should not be accessible from the sign-in/UAC dialog.
  • lock screen mode: prevents access to user data. Used on the lock screen, which runs on a user desktop.
    Also include the reset PIN workflow and out of box experience. (Only Win 10+)

Lock state session tracking is handled by NVDA now, by querying the session state every core pump cycle.

When on lock screen mode, we need to check the z-order of windows to confirm if an NVDA object should be accessible.
The window associated with the NVDAObject should be above the lowest lock screen window.
Lock screen windows can be identified by known class names.
This is risky as class names may change, but the lockapp appModule isn't detectable on the forgot PIN workflow.

We can confirm the windows order by starting at the lowest lock screen, then navigating our way up.
If we can confirm that the NVDA Object is not below the lock screen window, we can make the object accessible.
This method is risky, as z-ordering is dynamic.
There are unit tests to cover this, code aims to make NVDAObjects accessible where the order is unknown.

Testing strategy:

Issue 1

As it is difficult to time and simulate a freeze, general smoke testing on the new session tracking method should be performed instead.

With a try-build, use speech, enable error sounds, and monitor the logs for errors/warnings.

  • Smoke test the sign-in flow from lock, switch users, sleep, restart, log in and log out, checking the lock screen, sign-in screen, and general NVDA usage after sign in.
  • Smoke test the UAC dialog
  • Smoke test the forgot PIN workflow
  • Test the STR for known security advisories on the lockscreen, secure screens and PIN workflow:
    • Object navigation getting behind the lock screen
    • Various methods of opening system dialogs to gain system access
    • Various methods of opening improper NVDA dialogs to gain system / user profile access

Issue 2

  • Smoke test the forgot PIN workflow and object navigation on the screen

Unit testing
When on lock screen mode, we need to check the z-order of windows to confirm if an NVDA object should be accessible.
The window associated with the NVDAObject should be above the lowest lock screen window.
We can confirm this by starting at the lowest lock screen, then navigating our way up.
If we can confirm that the NVDA Object is not below the lock screen window, we can make the object accessible.
This method is risky, as z-ordering is dynamic.
There are unit tests to cover this.

Issue 3

Known issues with pull request:

none

Change log entries:

Refer to diff

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.

@seanbudd seanbudd added this to the 2022.3.3 milestone Dec 2, 2022
@seanbudd seanbudd force-pushed the try-new-lock-tracking branch from 38bdc48 to 6039b53 Compare December 2, 2022 05:01
source/utils/security.py Outdated Show resolved Hide resolved
@seanbudd
Copy link
Member Author

seanbudd commented Dec 2, 2022

The general approach for this PR is ready.
It still needs further documentation, and a thorough testing plan

@AppVeyorBot
Copy link

See test results for failed build of commit 8d319a47c1

tests/unit/test_util/test_security.py Outdated Show resolved Hide resolved
tests/unit/test_util/test_security.py Outdated Show resolved Hide resolved
tests/unit/test_util/test_security.py Outdated Show resolved Hide resolved
source/utils/security.py Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

See test results for failed build of commit 7ea24b5e6f

source/nvda.pyw Show resolved Hide resolved
source/textInfos/__init__.py Outdated Show resolved Hide resolved
source/api.py Outdated Show resolved Hide resolved
source/utils/security.py Outdated Show resolved Hide resolved
source/utils/security.py Outdated Show resolved Hide resolved
source/winAPI/sessionTracking.py Show resolved Hide resolved
source/winAPI/sessionTracking.py Outdated Show resolved Hide resolved
source/winAPI/sessionTracking.py Outdated Show resolved Hide resolved
source/winAPI/sessionTracking.py Outdated Show resolved Hide resolved
source/winAPI/sessionTracking.py Show resolved Hide resolved
@AppVeyorBot
Copy link

See test results for failed build of commit 6a94a34e3f

@seanbudd seanbudd requested review from michaelDCurran and removed request for a team December 14, 2022 03:17
source/utils/security.py Outdated Show resolved Hide resolved
source/utils/security.py Show resolved Hide resolved
source/utils/security.py Outdated Show resolved Hide resolved
source/NVDAObjects/__init__.py Outdated Show resolved Hide resolved
source/winAPI/sessionTracking.py Outdated Show resolved Hide resolved
source/utils/security.py Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

See test results for failed build of commit 2fd223eab9

michaelDCurran
michaelDCurran previously approved these changes Dec 15, 2022
@AppVeyorBot
Copy link

See test results for failed build of commit 267bb3fa17

@seanbudd seanbudd merged commit 56070da into rc Dec 16, 2022
@seanbudd seanbudd deleted the try-new-lock-tracking branch December 16, 2022 05:02
@seanbudd
Copy link
Member Author

The API changes for this PR have been posted here: https://groups.google.com/a/nvaccess.org/g/nvda-api/c/f8Xml7SGILE

seanbudd added a commit that referenced this pull request Jan 4, 2023
Closes #14475
Follow up for #14416

Summary of the issue:
#14416 added a log message at exception level.
This log message is for when the relative z-order for an NVDAObject and the lock screen cannot be determined.
While a noisy exception here is useful for alpha/beta testing of this code, this log message is almost always triggered once when locking windows. This is because the lock screen may not exist while windows is in the process of locking.

Description of user facing changes
Lower log level
seanbudd added a commit that referenced this pull request Jan 4, 2023
#14416 deprecated several functions for removal from winAPI.sessionTracking.
This was released in 2022.3.3, and scheduled for removal in 2023.1
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.

4 participants