From 40d1a1aaa4f2da8e9874f902c0682d90cab6ff4f Mon Sep 17 00:00:00 2001 From: Greg Neagle Date: Tue, 8 May 2018 16:56:29 -0700 Subject: [PATCH 1/9] Fix for inaccurate detection of already-mounted disk images --- code/client/munkilib/dmgutils.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/code/client/munkilib/dmgutils.py b/code/client/munkilib/dmgutils.py index cd3f3659..82660b9e 100644 --- a/code/client/munkilib/dmgutils.py +++ b/code/client/munkilib/dmgutils.py @@ -112,8 +112,10 @@ def diskImageIsMounted(dmgpath): if 'image-path' in imageProperties: imagepath = imageProperties['image-path'] if imagepath == dmgpath: - isMounted = True - break + for entity in imageProperties.get('system-entities', []): + if entity.get('mount-point'): + isMounted = True + break return isMounted From 6a2586e6554d293fc9e7fb81c921c159e49e075e Mon Sep 17 00:00:00 2001 From: Greg Neagle Date: Wed, 16 May 2018 06:22:49 -0700 Subject: [PATCH 2/9] Bumping version for future release --- code/client/munkilib/version.plist | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/client/munkilib/version.plist b/code/client/munkilib/version.plist index d667c788..d4e8e5da 100644 --- a/code/client/munkilib/version.plist +++ b/code/client/munkilib/version.plist @@ -3,6 +3,6 @@ CFBundleShortVersionString - 3.3.0 + 3.3.1 From ff0832fdf376154ea6ec50d6b40a2c7635402333 Mon Sep 17 00:00:00 2001 From: Greg Neagle Date: Wed, 30 May 2018 08:10:01 -0700 Subject: [PATCH 3/9] Don't crash if DomainName is not set in the current network configuration when attempting to auto-detect the repo URL --- code/client/munkilib/updatecheck/autoconfig.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/code/client/munkilib/updatecheck/autoconfig.py b/code/client/munkilib/updatecheck/autoconfig.py index f73a7640..d8d79051 100644 --- a/code/client/munkilib/updatecheck/autoconfig.py +++ b/code/client/munkilib/updatecheck/autoconfig.py @@ -36,7 +36,7 @@ from .. import prefs def get_domain_name(): '''Return current domain name''' dns_config = SCDynamicStoreCopyValue(None, 'State:/Network/Global/DNS') - return dns_config['DomainName'] + return dns_config.get('DomainName') def guess_repo_url(): @@ -47,9 +47,12 @@ def guess_repo_url(): autodetected_url = 'http://munki/repo' domain_name = get_domain_name() + if domain_name is None: + # No DomainName set + return autodetected_url + if domain_name == 'local': # no guesses if we are on a .local domain - print 'Warning: .local' return autodetected_url possible_urls = [ From 0bf8d5d23c9cbf8b737478c307f680269135de11 Mon Sep 17 00:00:00 2001 From: Greg Neagle Date: Wed, 30 May 2018 13:54:50 -0700 Subject: [PATCH 4/9] When checking for evidence of installation when evaluating a managed_uninstall, do not be fooled by an empty installs array. --- .../munkilib/updatecheck/installationstate.py | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/code/client/munkilib/updatecheck/installationstate.py b/code/client/munkilib/updatecheck/installationstate.py index 2c5acba7..de0eadef 100644 --- a/code/client/munkilib/updatecheck/installationstate.py +++ b/code/client/munkilib/updatecheck/installationstate.py @@ -264,17 +264,18 @@ def evidence_this_is_installed(item_pl): item_pl.get('uninstall_method') != 'removepackages'): display.display_debug2("Checking 'installs' items...") installitems = item_pl['installs'] - foundallinstallitems = True - for item in installitems: - if 'path' in item: - # we can only check by path; if the item has been moved - # we're not clever enough to find it, and our removal - # methods are currently even less clever - if not os.path.exists(item['path']): - # this item isn't on disk - display.display_debug2( - '%s not found on disk.', item['path']) - foundallinstallitems = False + if installitems: + foundallinstallitems = True + for item in installitems: + if 'path' in item: + # we can only check by path; if the item has been moved + # we're not clever enough to find it, and our removal + # methods are currently even less clever + if not os.path.exists(item['path']): + # this item isn't on disk + display.display_debug2( + '%s not found on disk.', item['path']) + foundallinstallitems = False if (foundallinstallitems and item_pl.get('uninstall_method') != 'removepackages'): return True From d248ec8121ce8883863e9688758b28cd28112b5d Mon Sep 17 00:00:00 2001 From: Greg Neagle Date: Mon, 4 Jun 2018 15:35:18 -0700 Subject: [PATCH 5/9] When storing password for authrestart, also store (and retreive) username for use with APFS on High Sierra+ --- .../MSCPasswordAlertController.py | 2 +- code/client/authrestartd | 10 +++++++--- code/client/munkilib/authrestart/__init__.py | 18 ++++++++++-------- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/code/apps/Managed Software Center/Managed Software Center/MSCPasswordAlertController.py b/code/apps/Managed Software Center/Managed Software Center/MSCPasswordAlertController.py index 4d7e8bfe..016f2ff7 100644 --- a/code/apps/Managed Software Center/Managed Software Center/MSCPasswordAlertController.py +++ b/code/apps/Managed Software Center/Managed Software Center/MSCPasswordAlertController.py @@ -88,7 +88,7 @@ class MSCPasswordAlertController(NSObject): password = self.passwordField.stringValue() if passwdutil.verifyPassword(username, password): # store password and end modal alert - authrestart.store_password(password) + authrestart.store_password(password, username=username) code = NSAlertFirstButtonReturn NSApplication.sharedApplication().stopModalWithCode_(code) NSApplication.sharedApplication().endSheet_returnCode_( diff --git a/code/client/authrestartd b/code/client/authrestartd index 78f2787e..025031fe 100755 --- a/code/client/authrestartd +++ b/code/client/authrestartd @@ -72,9 +72,11 @@ class FDEUtil(object): if not key in self.request: raise FDEUtilError('No %s in request' % key) - def store_password(self, password): + def store_password(self, password, username=None): '''Stores a password for later use for authrestart''' self.server.stored_password = password + if username: + self.server.stored_username = username def handle(self): '''Handle our request''' @@ -86,7 +88,8 @@ class FDEUtil(object): self.log.info('Restart request from uid %s', self.uid) if self.uid == 0: authrestart.do_authorized_or_normal_restart( - password=self.server.stored_password) + password=self.server.stored_password, + username=self.server.stored_username) return 'RESTARTING' else: self.log.info('Restart request denied') @@ -106,7 +109,7 @@ class FDEUtil(object): if not password: self.log.info('No password in request') raise FDEUtilError('No password provided') - self.store_password(password) + self.store_password(password, username=username) self.log.info('Password stored.') return 'DONE' @@ -291,6 +294,7 @@ class RestartHelperDaemon(SocketServer.UnixStreamServer): self, self.socket.getsockname(), RequestHandlerClass) self.timed_out = False self.stored_password = None + self.stored_username = None self.log = logging.getLogger(APPNAME) def setup_logging(self): diff --git a/code/client/munkilib/authrestart/__init__.py b/code/client/munkilib/authrestart/__init__.py index 267c2ef8..27859c8d 100644 --- a/code/client/munkilib/authrestart/__init__.py +++ b/code/client/munkilib/authrestart/__init__.py @@ -138,12 +138,12 @@ def can_attempt_auth_restart(have_password=False): for us to attempt an authrestart''' os_version_tuple = osutils.getOsVersion(as_tuple=True) return (os_version_tuple >= (10, 8) and - prefs.pref('PerformAuthRestarts') and + prefs.pref('PerformAuthRestarts') and filevault_is_active() and supports_auth_restart() and (get_auth_restart_key(quiet=True) != '' or have_password)) -def perform_auth_restart(password=None): +def perform_auth_restart(username=None, password=None): """When called this will perform an authorized restart. Before trying to perform an authorized restart it checks to see if the machine supports the feature. If supported it will look for the defined plist containing @@ -157,11 +157,13 @@ def perform_auth_restart(password=None): "Machine doesn't support Authorized Restarts...") return False display.display_debug1('Machine supports Authorized Restarts...') - recovery_key = get_auth_restart_key() or password - if not recovery_key: + password = get_auth_restart_key() or password + if not password: return False - key = {'Password': recovery_key} - inputplist = FoundationPlist.writePlistToString(key) + keys = {'Password': password} + if username: + keys['Username'] = username + inputplist = FoundationPlist.writePlistToString(keys) display.display_info('Attempting an Authorized Restart now...') proc = subprocess.Popen( ['/usr/bin/fdesetup', 'authrestart', '-inputplist'], @@ -179,7 +181,7 @@ def perform_auth_restart(password=None): return True -def do_authorized_or_normal_restart(password=None): +def do_authorized_or_normal_restart(username=None, password=None): '''Do an authrestart if allowed/possible, else do a normal restart.''' display.display_info('Restarting now.') os_version_tuple = osutils.getOsVersion(as_tuple=True) @@ -189,7 +191,7 @@ def do_authorized_or_normal_restart(password=None): if filevault_is_active(): display.display_debug1('Configured to perform AuthRestarts...') # try to perform an auth restart - if not perform_auth_restart(password=password): + if not perform_auth_restart(username=username, password=password): # if we got to here then the auth restart failed # notify that it did then perform a normal restart display.display_warning( From 68a5628a34b40cfb3a616951c380aec40deb48d9 Mon Sep 17 00:00:00 2001 From: Greg Neagle Date: Tue, 5 Jun 2018 16:06:51 -0700 Subject: [PATCH 6/9] Add more logging to authrestartd; update comments in MSCPasswordAlertController.py --- .../MSCPasswordAlertController.py | 2 +- code/client/authrestartd | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/code/apps/Managed Software Center/Managed Software Center/MSCPasswordAlertController.py b/code/apps/Managed Software Center/Managed Software Center/MSCPasswordAlertController.py index 016f2ff7..0f0976c3 100644 --- a/code/apps/Managed Software Center/Managed Software Center/MSCPasswordAlertController.py +++ b/code/apps/Managed Software Center/Managed Software Center/MSCPasswordAlertController.py @@ -87,7 +87,7 @@ class MSCPasswordAlertController(NSObject): username = NSUserName() password = self.passwordField.stringValue() if passwdutil.verifyPassword(username, password): - # store password and end modal alert + # store username and password and end modal alert authrestart.store_password(password, username=username) code = NSAlertFirstButtonReturn NSApplication.sharedApplication().stopModalWithCode_(code) diff --git a/code/client/authrestartd b/code/client/authrestartd index 025031fe..b002ec45 100755 --- a/code/client/authrestartd +++ b/code/client/authrestartd @@ -87,6 +87,8 @@ class FDEUtil(object): # restart self.log.info('Restart request from uid %s', self.uid) if self.uid == 0: + self.log.info('Stored username for authrestart: %s', + self.server.stored_username) authrestart.do_authorized_or_normal_restart( password=self.server.stored_password, username=self.server.stored_username) @@ -98,17 +100,18 @@ class FDEUtil(object): if self.request['task'] == 'store_password': # store a password for later fdesetup authrestart self.log.info('Store password request') + password = self.request.get('password') + if not password: + self.log.info('No password in request') + raise FDEUtilError('No password provided') username = self.request.get('username') + self.log.info('Request username: %s', username) # don't store the password if the user isn't enabled for FileVault if (username and not authrestart.can_attempt_auth_restart_for(username)): self.log.info('User %s can\'t do auth restart', username) raise FDEUtilError( 'User %s can\'t do FileVault authrestart' % username) - password = self.request.get('password') - if not password: - self.log.info('No password in request') - raise FDEUtilError('No password provided') self.store_password(password, username=username) self.log.info('Password stored.') return 'DONE' From 7468a441cf956066299de0b8d8ee165ed8b1597d Mon Sep 17 00:00:00 2001 From: Elliot Jordan Date: Tue, 12 Jun 2018 08:21:33 -0700 Subject: [PATCH 7/9] Add missing spaces in help strings (#857) --- code/client/makecatalogs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code/client/makecatalogs b/code/client/makecatalogs index 55126e38..1be73483 100755 --- a/code/client/makecatalogs +++ b/code/client/makecatalogs @@ -45,8 +45,8 @@ def main(): parser.add_option('--force', '-f', action='store_true', dest='force', help='Disable sanity checks.') parser.add_option('--skip-pkg-check', '-s', action='store_true', dest='skip_payload_check', - help='Skip checking of pkg existence. Useful' - 'when pkgs aren\'t on the same server' + help='Skip checking of pkg existence. Useful ' + 'when pkgs aren\'t on the same server ' 'as pkginfo, catalogs and manifests.') parser.add_option('--repo_url', '--repo-url', help='Optional repo URL that takes precedence ' From b1e94a072b9b5403f0d75fa63b2e0b73803c881a Mon Sep 17 00:00:00 2001 From: Clayton Burlison Date: Wed, 6 Jun 2018 15:46:18 -0500 Subject: [PATCH 8/9] fix: Remove OpenSSL import in keychain.py (#856) In Mojave 10.14 Apple has updated the openssl version to a newer release of LibreSSL (2.6.4) that no longer contains the proper Symbols for SSLv3. This causes import errors in the OpenSSL module that ships with 10.14. To resolve the import errors we now shell out to openssl to get the client cert common name. This has been hand tested with the following macOS/openssl versions: - 10.12 - 0.9.8zh 14 Jan 2016 - 10.13 - LibreSSL 2.2.7 - 10.14b1 - LibreSSL 2.6.4 Fix: #855 --- code/client/munkilib/keychain.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/code/client/munkilib/keychain.py b/code/client/munkilib/keychain.py index 521ae3fc..c294cedb 100644 --- a/code/client/munkilib/keychain.py +++ b/code/client/munkilib/keychain.py @@ -27,7 +27,6 @@ import base64 import hashlib import os import subprocess -from OpenSSL.crypto import load_certificate, FILETYPE_PEM from . import display from . import osutils @@ -151,11 +150,16 @@ def get_client_cert_common_name(): cert_info = get_munki_client_cert_info() client_cert_path = cert_info['client_cert_path'] if client_cert_path and os.path.exists(client_cert_path): - fileobj = open(client_cert_path) - data = fileobj.read() - fileobj.close() - x509 = load_certificate(FILETYPE_PEM, data) - common_name = x509.get_subject().commonName + cmd = ['/usr/bin/openssl', 'x509', '-noout', '-subject', '-in', + client_cert_path] + proc = subprocess.Popen(cmd, + bufsize=-1, stdout=subprocess.PIPE, + stderr=subprocess.PIPE, stdin=subprocess.PIPE) + (out, err) = proc.communicate() + if out: + for i in out.split('/'): + if i.startswith('CN='): + common_name = i[3:].rstrip() return common_name From 9e0d4e0426d241f4068dcef45e8834996a85d045 Mon Sep 17 00:00:00 2001 From: Greg Neagle Date: Mon, 18 Jun 2018 11:26:54 -0700 Subject: [PATCH 9/9] More strict validation of permissions when copying directories from a disk image. --- code/client/munkilib/installer/dmg.py | 28 +++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/code/client/munkilib/installer/dmg.py b/code/client/munkilib/installer/dmg.py index 29169faa..c79d3394 100644 --- a/code/client/munkilib/installer/dmg.py +++ b/code/client/munkilib/installer/dmg.py @@ -167,6 +167,14 @@ def validate_source_and_destination(mountpoint, item): "Error removing existing %s" % full_destpath) return (retcode, None, None) + if os.path.isdir(source_itempath): + # source item is a directory (application bundle counts as a dir!) + # to prevent a security vulnerability, recreate the destination item + # directory with 0700 mode to prevent other processes that may have + # write access to the parent directory from writing their own payloads + # to this directory + os.makedirs(full_destpath, 0o0700) + return (0, source_itempath, full_destpath) @@ -184,10 +192,24 @@ def copy_items_from_mountpoint(mountpoint, itemlist): if errorcode: return errorcode + # one last permissions check before we copy + if os.path.isdir(destination_path): + mode = os.stat(destination_path).st_mode & 0o7777 + # destination path that is a directory should have set the mode + # to 0700. if mode doesn't match, something insecure is happening + if mode != 0o0700: + display.display_error( + "Error copying %s to %s: destination path is insecure." + % (source_path, destination_path)) + return -1 + # validation passed, OK to copy display.display_status_minor( "Copying %s to %s" % (os.path.basename(source_path), destination_path)) + + # copy the file or directory, removing the quarantine xattr and + # preserving HFS+ compression retcode = subprocess.call(["/usr/bin/ditto", "--noqtn", source_path, destination_path]) if retcode: @@ -195,6 +217,12 @@ def copy_items_from_mountpoint(mountpoint, itemlist): "Error copying %s to %s" % (source_path, destination_path)) return retcode + # if destination is a directory, set the mode to that of the source + # directory (since we set it to 0700 earlier for copying safety) + if os.path.isdir(destination_path): + source_mode = os.stat(source_path).st_mode & 0o7777 + os.chmod(destination_path, source_mode) + # remove com.apple.quarantine xattr since `man ditto` lies and doesn't # seem to actually always remove it remove_quarantine(destination_path)