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

Initialize object caches in a safer manner #14333

Merged
merged 10 commits into from Nov 7, 2022
Merged

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Nov 4, 2022

Link to issue number:

none

Summary of the issue:

During NVDA initialization, the api module is used to set initial object caches.
These api functions check if an object is behind the lock screen when Windows is locked before setting the cache.
Until #14301, the api module initialised the object caches to the desktop object.
When NVDA started on the lock screen, it failed to set the desktop object due to the added security checks.
Now the object caches are initialised to the foreground window, to avoid leaking insecure information.
Unfortunately, setting those object caches causes side-effects, which may rely on uninitialized dependencies.

Description of user facing changes

None

Description of development approach

Revert to setting initial object caches to the desktop object.
Ensure security failures are not raised when initialising caches by ignoring the windows lock state.

Testing strategy:

Manual testing
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
  • Test the STR for security issues fixed in 2022.3.1

Known issues with pull request:

None

Change log entries:

None, fixes unreleased regression

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 requested a review from a team as a code owner November 4, 2022 00:20
@seanbudd seanbudd changed the base branch from master to rc November 4, 2022 00:20
@seanbudd seanbudd marked this pull request as draft November 4, 2022 00:20
@AppVeyorBot
Copy link

See test results for failed build of commit 87a0aef2eb

source/core.py Outdated Show resolved Hide resolved
seanbudd and others added 2 commits November 4, 2022 15:35
Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>
source/core.py Show resolved Hide resolved
@seanbudd seanbudd marked this pull request as ready for review November 7, 2022 01:24
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that this couples isWindowsLocked() with NVDA initialization and each of:

api. setDesktopObject()
api.setForegroundObject()
api.setFocusObject()
api.setNavigatorObject()
api.setMouseObject()

The approach taken here allows for a smaller change, and lower risk in this PR.

Ideally, each code path that might be blocked by isWindowsLocked() is considered and explicitly allows an exception for initialization. I expect that when the api.setXXX functions are refactored to allow independent initialization without calling through to app modules etc. the new init versions of those methods will be the only place the exception (for not calling isWindowsLocked() is required.

@seanbudd seanbudd merged commit a8e910e into rc Nov 7, 2022
@seanbudd seanbudd deleted the safer-object-cache-init branch November 7, 2022 03:31
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Nov 7, 2022
@seanbudd seanbudd modified the milestones: 2023.1, 2022.3.2 Nov 7, 2022
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.

None yet

5 participants