Disable logging in secure mode #13488

seanbudd · 2022-03-16T03:42:55Z

Thanks to @CyrilleB79 for reporting

Link to issue number:

GitHub Advisory GHSA-354r-wr4v-cx28: GHSA-354r-wr4v-cx28

Summary of the issue:

With the --debug-logging NVDA command line option, it is possible to enable debug logging in secure mode.
From a secure screen, it is possible to activate debug logging by restarting NVDA and selecting "Restart with debug logging" in the Exit Dialog.
This creates an instance of NVDA performing debug logging from the system profile, from a secure context.

Description of how this pull request fixes the issue:

Prevents debug logging in secure mode.
Removes "Restart with debug logging" from the exit dialog options.
Removes "Install pending update" from the exit dialog options.

Testing strategy:

Run nvda with -s and --debug-logging.

Confirm that no new nvda.log is created
- (eg in source/nvda.log when running from source, in %TEMP%/nvda.log when running as installed).

Run nvda with -s.

Open the exit dialog with nvda+q.
Check that "Restart with debug logging enabled" is not listed.
Check that "Install pending update" is not listed.

Start NVDA normally and with the -s option. For each run:

Test each restart option

Known issues with pull request:

None

Change log entries:

Security fixes

- Addressed security advisory ``GHSA-354r-wr4v-cx28``. (#13488)
  - Remove the ability to start NVDA with debug logging enabled when NVDA runs in secure mode.
  - Remove the ability to update NVDA when NVDA runs in secure mode.
  -

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

feerrenrut · 2022-03-16T03:44:58Z

The advisory will be published after the patch release is published.

codeofdusk · 2022-03-16T03:52:05Z

Why should updating be blocked in secure mode?

feerrenrut · 2022-03-16T04:42:10Z

@codeofdusk when it comes to security questions really we need to ask the opposite question "why should updating NVDA from secure screens be allowed"

The use case for that isn't clear to me. It would allow an unauthenticated user to make a significant change to a users machine.
Consider:

Alice doesn't want to upgrade yet, she is waiting for an update to an add-on she relies so it is compatible. 
Mallory notices that Alice's computer is not signed in, and that the update can be applied. 
Mallory applies the update.

Further, applying updates from secure screens is definitely not the common/expected use-case, and is an unnecessary variation to support.

GitHub Advisory GHSA-354r-wr4v-cx28: Summary: With the --debug-logging NVDA command line option, it was possible to enable debug logging in secure mode. From a secure screen, it was possible to activate debug logging by restarting NVDA and selecting "Restart with debug logging" in the Exit Dialog. This created an instance of NVDA performing debug logging from the system profile, from a secure context. Description of change Prevent debug logging in secure mode. Remove "Restart with debug logging" from the exit dialog options. Remove "Install pending update" from the exit dialog options.

lukaszgo1

I understand that this is merged already, but...

lukaszgo1 · 2022-03-16T11:50:25Z

devDocs/technicalDesignOverview.md

+#### Logging in secure mode
+`logHandler.initialize` prevents logging in secure mode.
+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.


This is misleading for any current developer who sees this text, and does not look into the code. When I saw it my first reaction was "why ability to set serviceDebug in registry (which was the recommended way to have logging in secure screens) is removed". After looking into the code it turns out it was not - just this documentation does not take it into account.

I asked myself a similar question when reading this text: why do not use serviceDebug in registry?

A PR to beta to amend this might be worthwhile.
I don't think this is misleading enough to patch 2021.3.4.
Using serviceDebug is still a documented option in the userGuide.
The important part of the patch is that you cannot enable debug logging in secure mode without this.
The security advisory, when published, should make the security implications clear.

Suggested change

To change this for testing, patch the source build.

To change this for testing, use the [serviceDebug](https://www.nvaccess.org/files/nvda/documentation/userGuide.html#SystemWideParameters) system wide parameter.

lukaszgo1 · 2022-03-16T11:57:39Z

source/gui/__init__.py

+		if globalVars.appArgs.secure:
+			allowedActions.remove(_ExitAction.RESTART_WITH_DEBUG_LOGGING_ENABLED)
+		# Installing updates should not happen in secure mode.
+		if globalVars.appArgs.secure or not (updateCheck and updateCheck.isPendingUpdate()):


Why this change? Has anyone managed to update NVDA in secure mode? As far as I can tell this was not possible even before this PR, and should not be advertised as a new security improvement since:

Only pending updates can be applied when in secure mode, and all executable files are excluded when copying system config in config._setSystemConfig

updateCheck is set to None when in secure mode since it raised RuntimeError when globalVars.appArgs.secure is true.

This has been answered here: #13488 (comment)
It is misleading to show this action in the exit options.

lukaszgo1 · 2022-03-16T12:00:31Z

devDocs/technicalDesignOverview.md

+`logHandler.initialize` prevents logging in secure mode.
+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.


Again this is not true. when starting nvda with the --secure switch and running as a normal user the log would be created in your temporary directory as usual. In general we should differentiate between secure mode on a secure screens and secure mode requested by the user from the CLI.

I don't think we need to be this specific in the technicalDesignOverview.
Logging should be disabled in secure mode, not just secure screens.

It would be fine to amend this in a future release, such as:

Suggested change

`nvda.log` files are then generated in the System profile's `%TEMP%` directory.

When logging from a secure screen, `nvda.log` files are generated in the System profile's `%TEMP%` directory.

This paragraph in the technicalDesignOverview.md aims to answer:

is logging disabled in secure mode? why?

how can I change this for testing? This can be updated, but the current description works too.

Where can I find these logs? It is difficult to guess this when logging from a secure screen.

Summary of the issue: When working on #13488, the exitDialog was lengthened, and probably should be moved to its own submodule file. Description of how this pull request fixes the issue: Moves and lints ExitDialog and _ExitAction from gui.__init__ into gui.exit.

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.

seanbudd requested a review from a team as a code owner March 16, 2022 03:42

seanbudd requested a review from feerrenrut March 16, 2022 03:42

seanbudd changed the base branch from master to rc March 16, 2022 03:44

feerrenrut previously approved these changes Mar 16, 2022

View reviewed changes

seanbudd added this to the 2021.3.4 milestone Mar 16, 2022

seanbudd dismissed feerrenrut’s stale review via 476b02b March 16, 2022 03:50

seanbudd force-pushed the disableLoggingSecureMode branch from f2c2811 to 476b02b Compare March 16, 2022 03:50

This comment was marked as resolved.

Sign in to view

feerrenrut force-pushed the disableLoggingSecureMode branch from 476b02b to 8faadd6 Compare March 16, 2022 06:47

feerrenrut merged commit b4f21e7 into rc Mar 16, 2022

feerrenrut deleted the disableLoggingSecureMode branch March 16, 2022 07:07

feerrenrut mentioned this pull request Mar 16, 2022

Merge security fixes from 2021.3.4 #13492

Merged

5 tasks

lukaszgo1 reviewed Mar 16, 2022

View reviewed changes

This was referenced Mar 17, 2022

Move exit dialog to submodule #13498

Merged

Improve secure mode and secure screens documentation #13501

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable logging in secure mode #13488

Disable logging in secure mode #13488

seanbudd commented Mar 16, 2022 •

edited

feerrenrut commented Mar 16, 2022

codeofdusk commented Mar 16, 2022

This comment was marked as resolved.

feerrenrut commented Mar 16, 2022

This comment was marked as resolved.

lukaszgo1 left a comment

lukaszgo1 Mar 16, 2022

CyrilleB79 Mar 16, 2022

seanbudd Mar 16, 2022 •

edited

lukaszgo1 Mar 16, 2022

seanbudd Mar 16, 2022 •

edited

lukaszgo1 Mar 16, 2022

seanbudd Mar 16, 2022

	To change this for testing, patch the source build.
	To change this for testing, use the [serviceDebug](https://www.nvaccess.org/files/nvda/documentation/userGuide.html#SystemWideParameters) system wide parameter.

	`nvda.log` files are then generated in the System profile's `%TEMP%` directory.
	When logging from a secure screen, `nvda.log` files are generated in the System profile's `%TEMP%` directory.

Disable logging in secure mode #13488

Disable logging in secure mode #13488

Conversation

seanbudd commented Mar 16, 2022 • edited

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

feerrenrut commented Mar 16, 2022

codeofdusk commented Mar 16, 2022

This comment was marked as resolved.

feerrenrut commented Mar 16, 2022

This comment was marked as resolved.

lukaszgo1 left a comment

Choose a reason for hiding this comment

lukaszgo1 Mar 16, 2022

Choose a reason for hiding this comment

CyrilleB79 Mar 16, 2022

Choose a reason for hiding this comment

seanbudd Mar 16, 2022 • edited

Choose a reason for hiding this comment

lukaszgo1 Mar 16, 2022

Choose a reason for hiding this comment

seanbudd Mar 16, 2022 • edited

Choose a reason for hiding this comment

lukaszgo1 Mar 16, 2022

Choose a reason for hiding this comment

seanbudd Mar 16, 2022

Choose a reason for hiding this comment

seanbudd commented Mar 16, 2022 •

edited

seanbudd Mar 16, 2022 •

edited

seanbudd Mar 16, 2022 •

edited