-
-
Notifications
You must be signed in to change notification settings - Fork 637
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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. | ||||||
`nvda.log` files are then generated in the System profile's `%TEMP%` directory. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again this is not true. when starting nvda with the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. It would be fine to amend this in a future release, such as:
Suggested change
This paragraph in the technicalDesignOverview.md aims to answer:
|
||||||
|
||||||
## NVDA Components | ||||||
NVDA is built with an extensible, modular, object oriented, abstract design. | ||||||
It is divided into several distinct components. | ||||||
|
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. | ||
|
||
|
@@ -44,6 +44,8 @@ | |
import speechViewer | ||
import winUser | ||
import api | ||
from utils.displayString import DisplayStringEnum | ||
from enum import auto, unique | ||
|
||
try: | ||
import updateCheck | ||
|
@@ -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 | ||
|
||
|
@@ -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()): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been answered here: #13488 (comment) |
||
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) | ||
|
||
|
@@ -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 | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 theuserGuide
.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.