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

Disable logging in secure mode #13488

Merged
merged 1 commit into from
Mar 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions devDocs/technicalDesignOverview.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ Aside from accessibility and native APIs, Windows provides many functions which
Information that can be obtained includes the class name of a window, the current foreground window and system battery status.
Tasks that can be performed include moving/clicking the mouse and sending key presses.

### Logging

#### 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@seanbudd seanbudd Mar 16, 2022

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


## NVDA Components
NVDA is built with an extensible, modular, object oriented, abstract design.
It is divided into several distinct components.
Expand Down
71 changes: 46 additions & 25 deletions source/gui/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# -*- coding: UTF-8 -*-
# A part of NonVisual Desktop Access (NVDA)
# Copyright (C) 2006-2021 NV Access Limited, Peter Vágner, Aleksey Sadovoy, Mesar Hameed, Joseph Lee,
# Thomas Stivers, Babbage B.V., Accessolutions, Julien Cochuyt
# Copyright (C) 2006-2022 NV Access Limited, Peter Vágner, Aleksey Sadovoy, Mesar Hameed, Joseph Lee,
# Thomas Stivers, Babbage B.V., Accessolutions, Julien Cochuyt, Cyrille Bougot
# This file is covered by the GNU General Public License.
# See the file COPYING for more details.

Expand Down Expand Up @@ -44,6 +44,8 @@
import speechViewer
import winUser
import api
from utils.displayString import DisplayStringEnum
from enum import auto, unique

try:
import updateCheck
Expand Down Expand Up @@ -608,6 +610,31 @@ def run():
wx.CallAfter(run)


@unique
class _ExitAction(DisplayStringEnum):

EXIT = auto()
RESTART = auto()
RESTART_WITH_ADDONS_DISABLED = auto()
RESTART_WITH_DEBUG_LOGGING_ENABLED = auto()
INSTALL_PENDING_UPDATE = auto()

@property
def _displayStringLabels(self):
return {
# Translators: An option in the combo box to choose exit action.
self.EXIT: _("Exit"),
# Translators: An option in the combo box to choose exit action.
self.RESTART: _("Restart"),
# Translators: An option in the combo box to choose exit action.
self.RESTART_WITH_ADDONS_DISABLED: _("Restart with add-ons disabled"),
# Translators: An option in the combo box to choose exit action.
self.RESTART_WITH_DEBUG_LOGGING_ENABLED: _("Restart with debug logging enabled"),
# Translators: An option in the combo box to choose exit action.
self.INSTALL_PENDING_UPDATE: _("Install pending update"),
}


class ExitDialog(wx.Dialog):
_instance = None

Expand Down Expand Up @@ -638,21 +665,18 @@ def __init__(self, parent):

# Translators: The label for actions list in the Exit dialog.
labelText=_("What would you like to &do?")
self.actions = [
# Translators: An option in the combo box to choose exit action.
_("Exit"),
# Translators: An option in the combo box to choose exit action.
_("Restart")
]
allowedActions = list(_ExitAction)
# Windows Store version of NVDA does not support add-ons yet.
if not config.isAppX:
# Translators: An option in the combo box to choose exit action.
self.actions.append(_("Restart with add-ons disabled"))
# Translators: An option in the combo box to choose exit action.
self.actions.append(_("Restart with debug logging enabled"))
if updateCheck and updateCheck.isPendingUpdate():
# Translators: An option in the combo box to choose exit action.
self.actions.append(_("Install pending update"))
if config.isAppX:
allowedActions.remove(_ExitAction.RESTART_WITH_ADDONS_DISABLED)
# Changing debug level on secure screen is not allowed.
# Logging on secure screens could allow keylogging of passwords and retrieval from the SYSTEM user.
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()):
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. Only pending updates can be applied when in secure mode, and all executable files are excluded when copying system config in config._setSystemConfig
  2. updateCheck is set to None when in secure mode since it raised RuntimeError when globalVars.appArgs.secure is true.

Copy link
Member Author

@seanbudd seanbudd Mar 16, 2022

Choose a reason for hiding this comment

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

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

allowedActions.remove(_ExitAction.INSTALL_PENDING_UPDATE)
self.actions = [i.displayString for i in allowedActions]
self.actionsList = contentSizerHelper.addLabeledControl(labelText, wx.Choice, choices=self.actions)
self.actionsList.SetSelection(0)

Expand All @@ -668,25 +692,22 @@ def __init__(self, parent):
self.CentreOnScreen()

def onOk(self, evt):
action=self.actionsList.GetSelection()
# Because Windows Store version of NVDA does not support add-ons yet, add 1 if action is 2 or above if this is such a case.
if action >= 2 and config.isAppX:
action += 1
if action == 0:
action = [a for a in _ExitAction if a.displayString == self.actionsList.GetStringSelection()][0]
if action == _ExitAction.EXIT:
WelcomeDialog.closeInstances()
if core.triggerNVDAExit():
# there's no need to destroy ExitDialog in this instance as triggerNVDAExit will do this
return
else:
log.error("NVDA already in process of exiting, this indicates a logic error.")
return
elif action == 1:
elif action == _ExitAction.RESTART:
queueHandler.queueFunction(queueHandler.eventQueue,core.restart)
elif action == 2:
elif action == _ExitAction.RESTART_WITH_ADDONS_DISABLED:
queueHandler.queueFunction(queueHandler.eventQueue,core.restart,disableAddons=True)
elif action == 3:
elif action == _ExitAction.RESTART_WITH_DEBUG_LOGGING_ENABLED:
queueHandler.queueFunction(queueHandler.eventQueue,core.restart,debugLogging=True)
elif action == 4:
elif action == _ExitAction.INSTALL_PENDING_UPDATE:
if updateCheck:
destPath, version, apiVersion, backCompatTo = updateCheck.getPendingUpdate()
from addonHandler import getIncompatibleAddons
Expand Down
21 changes: 18 additions & 3 deletions source/logHandler.py
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,23 @@ def _excepthook(*exc_info):
def _showwarning(message, category, filename, lineno, file=None, line=None):
log.debugWarning(warnings.formatwarning(message, category, filename, lineno, line).rstrip(), codepath="Python warning")


def _shouldDisableLogging() -> bool:
"""Disables logging based on command line options and if secure mode is active.
See NoConsoleOptionParser in nvda.pyw, #TODO and #8516.

Secure mode disables logging.
Logging on secure screens could allow keylogging of passwords and retrieval from the SYSTEM user.

* `--secure` overrides any logging preferences by disabling logging.
* `--debug-logging` or `--log-level=X` overrides the user config log level setting.
* `--debug-logging` and `--log-level=X` override `--no-logging`.
"""
logLevelOverridden = globalVars.appArgs.debugLogging or not globalVars.appArgs.logLevel == 0
noLoggingRequested = globalVars.appArgs.noLogging and not logLevelOverridden
return globalVars.appArgs.secure or noLoggingRequested


def initialize(shouldDoRemoteLogging=False):
"""Initialize logging.
This must be called before any logging can occur.
Expand All @@ -391,9 +408,7 @@ def initialize(shouldDoRemoteLogging=False):
fmt="{levelname!s} - {codepath!s} ({asctime}) - {threadName} ({thread}):\n{message}",
style="{"
)
if (globalVars.appArgs.secure or globalVars.appArgs.noLogging) and (not globalVars.appArgs.debugLogging and globalVars.appArgs.logLevel == 0):
# Don't log in secure mode.
# #8516: also if logging is completely turned off.
if _shouldDisableLogging():
logHandler = logging.NullHandler()
# There's no point in logging anything at all, since it'll go nowhere.
log.root.setLevel(Logger.OFF)
Expand Down