Skip to content

Improve secure mode and secure screens documentation#13501

Merged
seanbudd merged 7 commits into
masterfrom
clarifyLoggingInSecureMode
Mar 29, 2022
Merged

Improve secure mode and secure screens documentation#13501
seanbudd merged 7 commits into
masterfrom
clarifyLoggingInSecureMode

Conversation

@seanbudd

@seanbudd seanbudd commented Mar 17, 2022

Copy link
Copy Markdown
Member

Link to issue number:

None

Summary of the issue:

As raised in comments on #13488, the technical design overview could be clarified to be more clear and add more information.
Using the serviceDebug parameter to prevent secure mode on secure screens is a more universal solution than patching source code.

Users have been unclear on what secure mode and secure screens are when reading about recent security fixes.

Description of how this pull request fixes the issue:

Improves the documentation based on the discussion on #13488.

Adds definitions of secure mode and secure screens to the user guide.

Testing strategy:

None

Known issues with pull request:

This is developer centric documentation and as such didn't need to be added to the 2021.3.4 patch release or 2022.1 beta.

Change log entries:

None

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

@seanbudd seanbudd requested a review from a team as a code owner March 17, 2022 07:00
@seanbudd seanbudd requested a review from feerrenrut March 17, 2022 07:00
@seanbudd

Copy link
Copy Markdown
Member Author

cc @CyrilleB79 @lukaszgo1

@CyrilleB79 CyrilleB79 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks!

Comment thread devDocs/technicalDesignOverview.md Outdated
This is because it is a security concern to log during secure mode (e.g. passwords are logged).
To change this for testing, patch the source build.
`nvda.log` files are then generated in the System profile's `%TEMP%` directory.
This is because it is a security concern to log during secure mode (e.g. passwords are logged on secure screens).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a definition for 'secure screens' in this doc? There should be, and we should link to it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While at it,, could the difference between secure mode and secure screens be documented (if not already)? I have not checked in the docs nor in the code but I think that:

  • NVDA runs in secure mode when executed on secure screens (unless the debug service reg key is enabled)
  • NVDA can be run in secure mode in any situation providing the -s flag when started; except for easier testing of secure mode, I do not know what is the use case for this flag however.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@CyrilleB79 As you mention, easier testing is one.
Other use-cases are:

  • Locked down environments, perhaps sensitive corporate systems.
  • It gets used during standardized testing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Other use-cases are:

* Locked down environments, perhaps sensitive corporate systems.

Actually, that's my concern. Could a command line flag really guarantee such security? This would mean that on such locked environments, the following actions would be forbidden in order to avoid that anyone executes NVDA without the -s flag:

  • Open Windows Explorer and click on nvda.exe
  • open a console (e.g. cmd or PowerShell)
  • Runs "nvda.exe" from other locations such as Windows+R dialog, Explorer address bar, task manager, etc.
  • etc.

For reference, see the discussion in #10018. The final recommendation in this discussion goes to adding a new system-wide registry key for such usage.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is there a definition for 'secure screens' in this doc? There should be, and we should link to it.

As secure screens and secure mode are referenced regularly in the user guide, without clear definition, I'm going to create the definitions there and reference them from the technicalDesignOverview.
I'm going to add to the technicalDesignOverview a reference to MS Docs on UAC.
I haven't been able to find any MS definitions on secure screens.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@CyrilleB79 That seems like a valid concern, however it's a separate issue from documenting the 'intent' and how the command line args are currently used for the benefit of developers.

@seanbudd seanbudd requested a review from a team as a code owner March 18, 2022 02:03
@seanbudd seanbudd requested a review from Qchristensen March 18, 2022 02:03
@seanbudd seanbudd force-pushed the clarifyLoggingInSecureMode branch from c8f5fdf to 216ebf2 Compare March 18, 2022 02:04
@AppVeyorBot

This comment was marked as outdated.

@feerrenrut feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 18, 2022
@seanbudd seanbudd requested a review from feerrenrut March 21, 2022 05:46
@seanbudd seanbudd changed the title Further clarify logging in secure mode in technical design overview Improve secure mode and secure screens documentation Mar 27, 2022
Comment thread user_docs/en/userGuide.t2t Outdated
@seanbudd seanbudd marked this pull request as draft March 28, 2022 02:31
@seanbudd seanbudd marked this pull request as ready for review March 28, 2022 03:19
@seanbudd seanbudd force-pushed the clarifyLoggingInSecureMode branch from 0dbbe13 to 428154a Compare March 28, 2022 23:42

@Qchristensen Qchristensen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reads well and will hopefully reduce any possible confusion, great work!

@seanbudd seanbudd merged commit 9032a5a into master Mar 29, 2022
@seanbudd seanbudd deleted the clarifyLoggingInSecureMode branch March 29, 2022 00:12
@nvaccessAuto nvaccessAuto added this to the 2022.2 milestone Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants