Hammerspoon / hammerspoon

Staggeringly powerful macOS desktop automation with Lua
http://www.hammerspoon.org
MIT License
11.86k stars 578 forks source link

Harden Hammerspoon security model: configuration management #2514

Open dge8 opened 3 years ago

dge8 commented 3 years ago

Hi team, new-ish Hammerspoon user here. I’ve been running it for a few weeks and installed/coded a few Spoons to replace a number of small utilities I used to use, and I’m loving it, but some potential security vulnerabilities occurred to me around the handling of configuration and module loading. Obviously please correct me if there’s something I’ve missed or misunderstood.

The current design of Hammerspoon appears to provide numerous opportunities for the configuration to be modified by an otherwise unprivileged user process (or malicious actor if a machine is left unlocked), and thereby - silently - gain effective programmatic access to Accessibility controls, the clipboard, network, and any other privileges the user might have granted Hammerspoon e.g. the camera and microphone. It seems to me that an app with the privileges and functionality of Hammerspoon ought to prevent silent configuration changes via something like the standard macOS escalation dialog, in line with how apps like Safari/Keychain/etc. require a user’s password to view password entries.

Based on the current configuration model and Lua's package.path, three vectors occur to me:

Vector 1: Silent modification of config files in ~/.hammerspoon Possible mitigation: Introduce a config locking mechanism similar to apps like Tunnelblick, where a “locked” version of the config is copied to a second location requiring escalation to write. Each time the configuration is reloaded, every file in .hammerspoon is then compared against its locked version, and if the config has changed the user is offered the option to a) revert to the saved configuration or b) re-lock the changed configuration, requiring the user to enter their password. For convenience while developing it should be possible to disable this for a time, perhaps via an option on the Hammerspoon status menu (maybe a binary “Lock configuration”/“Unlock configuration”, or via a submenu “Configuration reloading”>”Automatic”/“Manual”/“Locked”).

Vector 2: Shadowing extensions by introducing similarly-named files into /usr/local/[share,lib]/lua Possible mitigation: Remove /usr/local from package.path by default. I assume this will break the setups of users using e.g. Homebrew modules, but users can manually add It back in through init.lua or a Preferences checkbox could be added with a suitable warning.

Vector 3: Modifying extensions directly inside Hammerspoon.app Possible mitigation: Hammerspoon should check on startup that the bundle and its subtree is not writeable by any user (via ownership and/or any ACLs) and if it is, provide an escalation dialog to repair permissions. This will have the minor but not unusual side-effect that the update process will need to escalate permissions. Incidentally this seems like the sort of thing code signing should prevent (?), but as far as I can tell on my Mojave system macOS only checks the bundle integrity upon first run.

I’m happy to have a shot at developing these mitigations myself, although I’m fairly new to Swift/ObjC and Lua so would appreciate any pointers.

Thoughts and comments?

asmagill commented 3 years ago

First I want to acknowledge all of these are valid concerns from a security perspective. I don't want to discourage you (or anyone else who reads this or joins the conversation) from possibly implementing these and submitting them as pull requests for consideration, but I do have some thoughts on why we haven't done this yet.

It shouldn't be necessary for me to say this, but just to be clear -- these are my thoughts and my thoughts alone. I don't know exactly what the other major contributors think, beyond what they have stated in the Hammerspoon issue posts before, and I don't know if they even still maintain or believe what they have previously posted. These are my thoughts alone and do not reflect the Hammerspoon team, other contributors, or even the primary application maintainer.

As once posted in these issues somewhere, installing Hammerspoon on your system is a security issue all by itself. It is not sandboxed, and by the very nature of some of the things it makes possible for you to do with your system, probably can't be. You can easily create a key-logger, interfere with (or replace) user interactions with other applications, trigger actions within other applications without direct user intervention... and that's just off the top of my head. Some of these Apple has put in provisions that other application developers can use to mitigate or prevent some of (enabling secure input in Terminal, for example, mitigates within Terminal against a key-logger built with Hammerspoon, limits the kinds of events we can inject or interfere with, etc. because we don't allow Hammerspoon to run event-taps as root (you'll see why I mention root in a moment). Even without elevated access, we can do some nasty things that should frighten anyone who is actively worried about true system security... one of my external modules (not included in core) can completely disable any force-touch trackpad in a way that requires Hammerspoon (if you know the correct commands) to undo or a complete system reboot. And that's without elevated access.

All of the vectors you outline require elevated privileges (root), something we have actively avoided even to the extent of recommending other applications when Hammerspoon can't quite do what the user wants (Karabiner Elements can do things with hotkeys and key remapping that we simply can't, for example).

While the vectors you mention can all be mostly (entirely?) controlled through targeted helper applications so that, in theory, they can only do specific things, it's not an area of macOS expertise that I've heard anyone mention that they have experience with... I certainly don't, and I don't want to be even peripherally responsible if someone finds that there is a bug in one of our helper applications that allows the user to maintain elevated access to implement other things that we had not intended. If I personally were commissioned to write an application where security was of high import, I would either subcontract that portion to someone with experience in secure macOS programming or at the very least make sure someone with experience in the field looked very closely at any of my code that ran as root or with any type of elevated privilege.

We don't have a hard release schedule, or even a process in place when we need to get bug fixes out quickly. If you've observed the transition from version 0.9.78 to (hopefully soon) 0.9.81 that we're currently in the middle of, you'll notice that errors and unintended changes do occur. That's not something I want in anything that can run with elevated privileges... and if it does happen, how quickly can we guarantee a fix gets released, even if it's a simple one liner to fix?

As to specifics...

Addressing Vector 1 would kind of defeat the way most of our users (up to this point) have used Hammerspoon... as an ever evolving combination of helpers and tricks that they can use, adjust, and modify at will... having to re-enter my password everytime I make even a one line change to anything within my config directory would very quickly turn me off... sometimes I have to tweak it 20 or 30 times to get something just the way I want it. I can see it having more appeal to @latenitefilms whose CommandPost is, as I understand it, a "self contained solution" in and of itself... perhaps he doesn't want his all of his users tweaking away at will, but Hammerspoon, as originally conceived, and as far as I currently know still, appeals to a different type of user. If that's changing, I'm currently unaware of it.

Addressing Vector 2 would make it more difficult for developers to slip in alternate versions of core modules during development... it takes me less than a minute to recompile a specific module, but 10-15 to recompile Hammerspoon in its entirety (old mac, I know) and when debugging problems or adding new features that need testing before inclusion, this time adds up. Even so, with hs._coresetup being loaded automatically by Hammerspoon before my init.lua file does, there are modules that I still can't override this way because hs._coresetup requires them before I can apply my path changes (unless I make other changes I have to remember to remove before pushing out the code). Granted this probably affects developers and "early adopters/testers" more than our regular users, but it's a concern of mine.

Addressing Vector 3 would have made some of the tweaks people have made in the last week while helping us track down some intermittent issues more difficult... I have asked people to modify their modules within the Hammerspoon application itself so that they didn't have to download and rebuild Hammerspoon from scratch themselves... especially given that we don't have a set release schedule, this can be the difference between someone fixing an issue themselves and waiting a week or more before we can offer a formal release, just to find out if it does in fact fix their problem.

Again, I don't say any of this to discourage you, if you think these are important enough to address yourself or solicit on here, then go for it. This is a community project which means that the community's wishes can affect the direction development goes.

In another type of application, I might even want some or all of these (I use Tunnelblick myself)... But as Hammerspoon is about experimenting and pushing the boundaries of what we're "supposed" to be able to do with our Macs... I'm just not sure the tradeoffs are worth it, in my opinion.

As always, however, I will go with the consensus, and if we do want to pursue this, would strongly urge that someone with secure Mac application coding experience head it... which is certainly not me at present.

von-forks commented 3 years ago

Responding to @asmagill: "All of the vectors you outline require elevated privileges (root)..."

I agree with you that Vectors 2 and 3 seem to not require root, so I'm not seeing an escalation path there. But I believe Vector 1 (Silent modification of config files in ~/.hammerspoon) does not require root and can be done with standard user privileges (I modify my Hammerspoon configuration all the time without being root - is my installation unusual?)

This is important since I believe Hammerspoon will often run with more than standard user privileges. For example, I suspect many of us give it access via OSX dialogs to control other apps, record the screen, read keyboard events, etc. So it doesn't run as root, but with more than standard user privileges.

Hence since Hammerspoon's configuration is code that Hammerspoon runs, being able to modify files in ~/.hammerspoon/ provides a privilege escalation path from normal user privileges to Hammerspoon's elevated privileges

Assuming that analysis is right, I would prioritize Vector 1 and something such as @dge9 describes sounds like a good course.

asmagill commented 3 years ago

Vector 1: Silent modification of config files in ~/.hammerspoon Possible mitigation: Introduce a config locking mechanism similar to apps like Tunnelblick, where a “locked” version of the config is copied to a second location requiring escalation to write. Each time the configuration is reloaded, every file in .hammerspoon is then compared against its locked version

As Tnunelblick is explicitly mentioned, I assumed a mechanism similar to the one used by such and it does use the super user root to make a copy of the configuration and save it somewhere else, owned by the root user. This is the reason that upon any update to the application, you have to provide your user password twice: once for the updated application itself, and once again for the re-duplication of the config file required by the fresh running of a new version of the application.

But that aside, if you want Hammerspoon to make a copy of the "correct" configuration to compare against, what prevents me from silently modifying that copy at the same time I modify ~/.hammerspoon. Without privilege escalation, I do not see a reliable foolproof way to create a locked backup that can be relied upon any more than the original, which is the whole point of Vector 1.

However, it is possible I am mistaken or not aware of something and if you do see such an approach, then by all means feel free to submit your findings, or better yet a pull request with the necessary changes.

latenitefilms commented 3 years ago

Great post @dge9! Whilst I agree with everything you've said, and I'd love to see Hammerspoon be as secure as possible, as @asmagill notes, I'm not exactly sure how you'd do it - but certainly up for discussing it and exploring options!

The main issue I see is that a malicious user/script doesn't even need to modify the configuration files - they could just change the MJConfigFile property within the ~/Library/Preferences/org.hammerspoon.Hammerspoon.plist file, and point to a different config location. Or, even easier, they could just turn on AppleScript support, and then trigger Lua code from AppleScript.

Someone please correct me if I'm wrong, but I don't think there's any easy way to protect this property list, unless you sandbox the app?

Maybe one possibility is that you could have a secure Core Data store (or just SQLite), where the key is stored in your Keychain, which contains Hammerspoon's settings, and a checksum of your configuration files, so that Hammerspoon can alert you to when those configuration files are altered? This means that when you first install Hammerspoon, you'd be required to supply it a password, and then you could enable an option that alerts you if the configuration has changed? This feature could be off by default, so it wouldn't annoy long-term Hammerspoon users - but the option would be there for people how are worried about security on their system.

Related:

dge8 commented 3 years ago

Thanks all for your responses.

Firstly, as far as I can tell, none of the three proposed attack vectors require elevation on my Mac: Vector 1: .hammerspoon is in my user’s home directory and owned by my user, this one seems fairly straightforward and applicable to all Hammerspoon users Vector 2: I have Homebrew installed, which creates /usr/local/lib and /usr/local/share and chown’s them to my user. This may not be the case for all Hammerspoon users, but it seems to me that the overlap between Hammerspoon users and Homebrew users is likely to be significant. Vector 3: I installed Hammerspoon using the instructions at hammerspoon.org: “Download […] and then drag the application to /Applications/“ which leaves ownership on the app bundle with the installing user account. Hence I suspect it is likely to be the case for most Hammerspoon users. Alternatively, am I missing something here about macOS security? I’m running Mojave 10.14.6, do the new security features in Catalina prevent apps from accessing folders owned by the current user via e.g. Full Disk Access? What about a command-line app or something installed via e.g. Homebrew?

@latenitefilms, thanks for the excellent Keychain suggestion, and I had a read through your links. The encrypted Core Data store is an interesting and very flexible idea, although in the current context (and partly motivated by a desire for simple mitigations within my own limited Swift abilities), the following thought process occurs to me:

@asmagill, I appreciate your reluctance to add any code or helpers to Hammerspoon that run with root privileges. Following @latenitefilms Keychain suggestion I think it should be possible to develop mitigations to all three that do not require root. Your concerns regarding convenience of development and troubleshooting are also valid, hence along with the mitigations themselves I have tried to propose what seem to me to be the most natural UI/config elements allowing them to be disabled by those that require it.

Amended proposals:

Mitigation to vector 1: Introduce a password locking mechanism based on a secured hash of configuration files in ~/.hammerspoon (and/or wherever MJConfigFile points to). Configuration restore functionality can be implemented by copying the config to ~/.hammerspoon/.backup/ (or ~/.hammerspoon-backup) every lock (and obviously checking the hashes of the backup before restoring too). The hash is secured by storing in the Keychain and is therefore (in theory) accessible only to Hammerspoon. No password required, only clicking to confirm a prompt (which, incidentally, makes it vulnerable to other apps with Accessibility privileges. Perhaps a password is a good idea?). As I originally indicated, and to address @asmagill’s usability concerns, I think the best user experience here would not to have always-on locking, but to implement a status menu item allowing configuration locking to be temporarily disabled for development purposes, and then re-enabled. Another possibility is a timer that locks the configuration after 1 hour (or 6, or 24) of no modifications.

Mitigation to vector 2: Following @asmagill’s comment about init.lua being run after hs._coresetup and therefore too late, I can think of two alternatives:

Mitigation to vector 3: To avoid elevating Hammerspoon code, this vector can be pretty well mitigated by building/distributing Hammerspoon.app into an Installer package instead of the raw application bundle in a ZIP, and just setting the correct root:wheel permissions in the package and enabling elevation when installing. Then the elevation is then handled entirely (and safely) by Installer.app. Incidentally there’s nothing stopping users adjusting code inside Hammerspoon.app for a quick fix in this instance, they just need to elevate to do so.

Other potential vector: AppleScript? I’m not yet familiar with the AppleScript integrations with Hammerspoon.

If these mitigations seem agreeable, I’m happy to have a go at implementing them and creating a PR(s).

latenitefilms commented 3 years ago

The only preference key that (currently) affects Lua code execution is MJConfigFile

No, as explained above, you can turn on AppleScript support via the property list property HSAppleScriptEnabledKey. Once enabled, you can use AppleScript to trigger any Lua code.

You'd also need to add similar protections to hs.ipc.

There may also be others I haven't thought of off the top of my head.

Introduce ~/.hammerspoon/.preload.lua or similar, which is loaded before hs._coresetup, and append to package.path there. This seems like it would be the easier option, at the slight cost of further fragmenting the Lua configuration. This file is then included in the config hash.

This already happens - have a look at setup.lua.

However, it is possible the paths could be setup in Objective-C land instead here.

To avoid elevating Hammerspoon code, this vector can be pretty well mitigated by building/distributing Hammerspoon.app into an Installer package instead of the raw application bundle in a ZIP, and just setting the correct root:wheel permissions in the package and enabling elevation when installing. Then the elevation is then handled entirely (and safely) by Installer.app. Incidentally there’s nothing stopping users adjusting code inside Hammerspoon.app for a quick fix in this instance, they just need to elevate to do so.

This also then makes updates slightly more annoying, because each time you update you'll need to run the installer again.

Do you need any special permissions to remove the users permissions for the Hammerspoon app, and add permissions for "wheel" to be "Read Only". Could Hammerspoon just adjust permissions of itself on first run?

Note to self - this is what the permissions look like comparing Hammerspoon, and the App Store for example:

Screen Shot 2020-09-28 at 10 02 55 pm

If these mitigations seem agreeable, I’m happy to have a go at implementing them and creating a PR(s).

Initially, a good starting point might be to make a Hammerspoon extension that handles Keychain (hs.keychain)? That way it can be used for this purposes, but also allows Hammerspoon users easy access to Keychain as well for their own purposes.

As the maintainer of this community project, it would also be good to get @cmsj's thoughts on this, because at the end of the day he'll have the final say in terms of merging - but I know he's crazy busy at the moment due to all the COVID craziness in the world.

asmagill commented 3 years ago

Re Vector 1, I still maintain that I can modify the backup unless it's stored under a different username (e.g. root) which requires elevated privileges. I will agree that implementing a hash stored in the Keychain is a good way to determine whether or not the configuration (or backup if you go that far) has been modified... but at the end, without privilege elevation I see no way of keeping a "pristine" copy that Hammerspoon can absolutely rely on as being the original. At best (and it's not a horrible best, I admit) we can achieve the point where Hammerspoon can prompt at launch/reload with a dialog along the lines of "Your configuration has changed. Continue with new configuration or quit Hammerspoon?"

Of course some of our config dirs are pretty large, so hashing all of its entities might be time consuming... I'd be curious if someone knows of a quick way we can test this to get a feel for how long this might delay a restart/launch.

Re Vector 2, if it comes down to it, I can modify setup.lua (I'd actually forgotten about this) with special paths which is actually run even before hs._coresetup. This file is found in the Application at /Applications/Hammerspoon.app/Contents/Resources/setup.lua so its protection would fall under Vector 3.

Vector 3, I'm not opposed to the idea of an installer, but I also have absolutely zero experience with building one. I'd be a little concerned about those of us who mainly run developer builds (i.e. build Hammerspoon ourselves so that we're running the current master branch, which is often a little ahead of the releases)... there are a few differences in the developer build: some additional debugging flags are set in Xcode, the Sparkle framework is disabled, some of us run with a self-signed developer certificate (to avoid repeatedly resetting Accessibility) while others run with no certificate at all, probably some other differences I'm not remembering at the moment... how might an installer handle these variances? Or would an installer not be used at all in this case? At any rate, questions we'd need answers to. Ultimately, @cmsj would be the one who would have to be on board with going the installer route as he handles the release builds.

If you don't want Hammerspoon to use IPC, don't load the hs.ipc module. Of course, this prevents the use of the hs shell command, so it's a trade off. It's been long enough that I think we could remove the "fallback" mode for hs.ipc (When hs.ipc was rewritten a while back, we had to include a "simple" mode for fallback because we couldn't be certain of which version of the shell command users had installed) which means it may be possible to add a challenge-response system so that only the hs command (or other authorized externals) can use the IPC channel, but that still means anyone who can execute shell scripts as the user could run Hammerspoon commands while it's running... really, the Safest™ approach here is to just not load hs.ipc at all.

I've never used the AppleTalk interface and even mostly avoid the modules that call out to osascript... can't comment on it.

I'm sure there are others... remember, the original purpose behind Hammerspoon/Mjolnir/Hydra was "pushing the envelope" of what was usually hidden by Apple from most power users... true security has never been a motivating factor... it will have to be a series of trade offs and ultimately, if you're really concerned... don't run Hammerspoon. I certainly wouldn't in a strictly regimented corporate environment (or, at best, would create a custom version with a good 75% or more of the modules removed entirely).

latenitefilms commented 3 years ago

This is an interesting thread of someone trying to protect something similar (just with html files instead of lua):

https://stackoverflow.com/questions/35042903/how-do-i-get-an-osx-folder-checksum-in-objective-c

latenitefilms commented 3 years ago

FWIW, running find CONTENTS -type f -exec shasum {} \; | sort -k 2 | shasum on the CommandPost code (which is probably the most complex Hammerspoon configuration out there) takes a loooooong time to calculate the hash - several minutes on my MacBook Pro (15-inch, 2017).

latenitefilms commented 3 years ago

Thinking about it further, and reading about other Mac and iOS developers trying to protect assets in their "Resources" folder, I'm not sure there is a good way to really protect Hammerspoon scripts, without having a helper app that has root privileges, which as @asmagill has explained - we definitely don't want to do, as that potentially opens up even more dangerous vectors. Given this, my suggestion would just be to offer some ideas in the Wiki, about how a user can protect their own Hammerspoon configuration directory via macOS's built-in permissions. Maybe also update the FAQ accordingly?

Having said that... I guess it would technically be possible to copy the contents of your Lua scripts into a secure SQlite database - and use that as the password protected datastore (using Keychain to store the password). This would probably be ok for small Hammerspoon configurations - but would start to be a main bottleneck once you start dealing with hundreds of Lua files like CommandPost has.

Given you'd need to check for changes to the scripts each time you reload Hammerspoon, I think most users would basically turn off this feature, as it would slow things down - so in practise, I'm not sure how useful or secure it would actually be.

As always, open to ideas and suggestions!

asmagill commented 3 years ago

@latenitefilms I'm curious how long this takes for you (you'll need to stick it in a module somewhere, or I'll upload it later, but don't have time ATM:

    static int extras_directorySHA512(lua_State *L) {
        LuaSkin *skin = [LuaSkin sharedWithState:L] ;
        [skin checkArgs:LS_TSTRING, LS_TBREAK] ;
        NSString *path = [skin toNSObjectAtIndex:1] ;
        path = path.stringByExpandingTildeInPath.stringByResolvingSymlinksInPath ;

        NSURL                 *dirURL  = [NSURL fileURLWithPath:path isDirectory:YES] ;
        NSFileManager         *sFM     = [NSFileManager defaultManager] ;
        NSDirectoryEnumerator *dirEnum = [sFM enumeratorAtURL:dirURL
                                   includingPropertiesForKeys:@[
                                                                 NSURLIsRegularFileKey,
                                                                 NSURLPathKey
                                                               ]
                                                      options:0
                                                 errorHandler:nil] ;

        NSMutableArray *pathsOfInterest = [NSMutableArray array] ;
        for (NSURL *fileURL in dirEnum) {
            NSNumber *isRegularFile = nil ;
            [fileURL getResourceValue:&isRegularFile forKey:NSURLIsRegularFileKey error:nil] ;
            if (isRegularFile.boolValue) {
                NSString *filePath = nil ;
                [fileURL getResourceValue:&filePath forKey:NSURLPathKey error:nil] ;
                [pathsOfInterest addObject:filePath] ;
            }
        }

        // ensure consistent order
        [pathsOfInterest sortUsingSelector:@selector(compare:)] ;

        CC_SHA512_CTX context ;

        CC_SHA512_Init(&context) ;

        for (NSString *filePath in pathsOfInterest) {
            NSData *fileData = [NSData dataWithContentsOfFile:filePath] ;
            if (fileData) {
                CC_SHA512_Update(&context, fileData.bytes, (CC_LONG)fileData.length) ;
            } else {
                [skin logWarn:[NSString stringWithFormat:@"Unable to rertrieve contents of %@ for checksum", filePath]] ;
            }
        }

        unsigned char hash[CC_SHA512_DIGEST_LENGTH] ;
        CC_SHA512_Final(hash, &context) ;
        NSData *data = [NSMutableData dataWithBytes:hash length:CC_SHA512_DIGEST_LENGTH] ;
        NSMutableString *asString = [NSMutableString stringWithCapacity: CC_SHA512_DIGEST_LENGTH*2];
        const unsigned char *dataBytes = [data bytes];
        for (NSUInteger idx = 0; idx < CC_SHA512_DIGEST_LENGTH; ++idx) {
            [asString appendFormat:@"%02x", dataBytes[idx]];
        }

        [skin pushNSObject:asString] ;
        return 1 ;
    }

For me it's under a second, but my config while larger than some is no where near as large as your. A potential problem with it is the the directory enumerator doesn't follow symlink dirs (other than the starting dir, if its a link) but I don't find an obvious enumerator in the mac docs that does (though I'll dig more later), so that may be something we'd have to address ourselves if necessary (and for the record, my Spoons dir is a symlink).

The value does match what I get if I run find ~/.config/hammerspoon -type f -print | sort | xargs cat | shasum -a 512 from the terminal.

latenitefilms commented 3 years ago

It actually works quite fast for me too!

> local secondsSinceEpoch = require("hs.timer").secondsSinceEpoch
local startTime = secondsSinceEpoch()
local hash = hs.fs.directorySHA512("/path/to/commandpost/source")
local endTime = secondsSinceEpoch()
local result = endTime - startTime
print(string.format("Hash took %s seconds: %s", result, hash))
2020-10-01 09:38:01: Hash took 0.76828193664551 seconds: 79bb0a3685a903137c78dfefbe71b4cc2c69766742cf85a5ce56e7423b7f26b05571c3e5635c0eb1bb92f1e7f1bab421a397fd7813efd0e18fa40f0b9b0102eb

This function would actually be seriously useful for some other features in CommandPost - any chance we could add this to hs.fs sooner rather than later? I'm happy to do a pull request if you don't have time.

asmagill commented 3 years ago

I was thinking hs.hash, but hs.fs could work, too... preferences?

I want to expand on it a bit; right now it gives bogus results if you pass the path to a file rather than a directory, and I have some ideas about how to get around the symlink issue so optionally allowing the following of those, maybe offering a choice of which type of hash, etc.

But yes, I was thinking in terms of this being added in at some point.

latenitefilms commented 3 years ago

hs.hash sounds good to me!

dge8 commented 3 years ago

The only preference key that (currently) affects Lua code execution is MJConfigFile No, as explained above, you can turn on AppleScript support via the property list property HSAppleScriptEnabledKey. Once enabled, you can use AppleScript to trigger any Lua code.

Thanks for the clarification, didn't realise that was a PLIST property as well.

You'd also need to add similar protections to hs.ipc. There may also be others I haven't thought of off the top of my head.

I hadn't seen that extension, although based on the docs am I correct in understanding that it's not loaded by default? Three possibilities occur to me:

This already happens - have a look at setup.lua.

Re Vector 2, if it comes down to it, I can modify setup.lua (I'd actually forgotten about this) with special paths which is actually run even before hs._coresetup. This file is found in the Application at /Applications/Hammerspoon.app/Contents/Resources/setup.lua so its protection would fall under Vector 3.

Nice, that seems like a simple solution, adjusting setup.lua to remove /usr/local/[lib,share] paths by default, and those that need it can add it again manually. My only other question here is whether having ./?.lua;./?/init.lua early in package.path also represents a risk since an attacker could use it to shadow extensions in the app bundle? But that depends on how Lua adjusts the current working directory, which as long as it stays in ~/.hammerspoon/MJConfigFile seems like it would be protected by the config hash.

This also then makes updates slightly more annoying, because each time you update you'll need to run the installer again.

Sparkle can apparently handle package updates behind the scenes, with the exception of the user being prompted for their password.

Vector 3, I'm not opposed to the idea of an installer, but I also have absolutely zero experience with building one. I'd be a little concerned about those of us who mainly run developer builds (i.e. build Hammerspoon ourselves so that we're running the current master branch, which is often a little ahead of the releases)... there are a few differences in the developer build: some additional debugging flags are set in Xcode, the Sparkle framework is disabled, some of us run with a self-signed developer certificate (to avoid repeatedly resetting Accessibility) while others run with no certificate at all, probably some other differences I'm not remembering at the moment... how might an installer handle these variances? Or would an installer not be used at all in this case?

This slightly older StackOverflow thread goes into some detail using Apple's pkgbuild and productbuild tools that might be helpful. I've personally built many installer packages using this tool which also supports command-line operation, so presumably could be called as part of an Xcode build phase.

FWIW, running find CONTENTS -type f -exec shasum {} \; | sort -k 2 | shasum on the CommandPost code (which is probably the most complex Hammerspoon configuration out there) takes a loooooong time to calculate the hash - several minutes on my MacBook Pro (15-inch, 2017).

I noticed that for some reason it was much faster running through xargs than with find -exec, how long does it take you to run find ~/.hammerspoon -type f -not -name .DS_Store -print | sort | xargs shasum -a512 | shasum -a512 | cut -d" " -f1?

@asmagill Nice work with the hashing function, would it be desirable to also:

Initially, a good starting point might be to make a Hammerspoon extension that handles Keychain (hs.keychain)? That way it can be used for this purposes, but also allows Hammerspoon users easy access to Keychain as well for their own purposes.

Good idea, can have a look into it. Isn't/wasn't there a Spoon in the docs that offered some sort of Keychain integration? I can't find the code now, only the doc page.

Edit: quote formatting.

asmagill commented 3 years ago

My only other question here is whether having ./?.lua;./?/init.lua early in package.path also represents a risk since an attacker could use it to shadow extensions in the app bundle? But that depends on how Lua adjusts the current working directory, which as long as it stays in ~/.hammerspoon/MJConfigFile seems like it would be protected by the config hash.

Simple answer, yes, putting the user's config directory and the "current" directory versions before the application ones does mean that modules can be shadowed. This is usually what I do when doing work on an existing module.

Reversing this and locking down the contents of the Hammerspoon application Resources directory would mitigate this somewhat, but users could still modify the paths in their own configurations (or use the console) that would allow shadowing modules not already loaded by hs._coresetup.

As to the "current working directory", this can be changed with hs.fs.chdir.


Re the hashing algorithm, sounds like you want something that hashes the files individually then hashes the report of that?

Another option would be appending the file list to the data being hashed within the algorithm I threw together... even if sort order was the same, the block of text added at the end would be different.

I guess the questions I would ask then are:

  1. Is there a commonly accepted approach already in general usage for situations like this? Off the top of my head most of the ones I know about generally check the hash on a zip/tgz or image file, not on the contents of an already expanded directory tree.
  2. Which approach would be more generally useful for the situations you're thinking of @latenitefilms? Hashing file contents concatenated together (with or without the paths appended)? Or hashing a report of individual hashes for each file?
asmagill commented 3 years ago

Re package paths... I suppose you could wrap require (we already do, to capture information about loaded modules for crash logs) to silently prepend the core paths before loading, then remove them again. But I'd want a way to systematically disable this for development purposes.

latenitefilms commented 3 years ago

Regarding AppleScript support, another easy fix is that we could tweak it so that AppleScript support is only loaded/enabled if hs.osascript is loaded - this should be a relatively easy fix - but probably something that @asmagill should tackle rather than me. In retrospect, when thinking about security, adding the AppleScript functions into hs.osascript.applescript would have made more sense than having it as hs.allowAppleScript().

I noticed that for some reason it was much faster running through xargs than with find -exec, how long does it take you to run find ~/.hammerspoon -type f -not -name .DS_Store -print | sort | xargs shasum -a512 | shasum -a512 | cut -d" " -f1?

It runs instantly - but I do get a xargs: unterminated quote error - so not sure if this is causing it to not work correctly?

Which approach would be more generally useful for the situations you're thinking of @latenitefilms? Hashing file contents concatenated together (with or without the paths appended)? Or hashing a report of individual hashes for each file?

In CommandPost, we're currently using a bunch of tricks (comparing folder size and/or modification date) to detect if a directory contents has changed, so that we can see, for example, when new plugins and effects are added to Final Cut Pro. If there's no new plugins/effects, we can use a cached version, otherwise, we re-scan the directory to get all the required metadata (which is painfully slow). In theory, if this hash technique is fast, we can dramatically speed up the initial check.

Here's our plugin scanning code for future reference.

dge8 commented 3 years ago

@latenitefilms Some of your filenames will have quotes in them, try this one: find ~/.hammerspoon -type f -not -name .DS_Store -print0 | sort -z | xargs -0 shasum -a512 | shasum -a512 | cut -d" " -f1

Would it be feasible to remove the preference/PLIST property for AppleScript entirely in favour of a Lua toggle (hs.osascript.applescript.enable() or otherwise)? What about removing the MJConfigFile property and instead setting the directory in setup.lua instead? Then org.hammerspoon.Hammerspoon.plist doesn't contain any preferences that can affect Lua execution.

Reversing this and locking down the contents of the Hammerspoon application Resources directory would mitigate this somewhat, but users could still modify the paths in their own configurations (or use the console) that would allow shadowing modules not already loaded by hs._coresetup.

If the user wants to deliberately shadow Hammerspoon extensions/modules, that seems fine to me, as long as it's not possible for a hypothetical attacker to write a file somewhere that serves to shadow an extension/module without the user being aware. For example, I was trying to work out if, say, a module silently changed the working directory using hs.fs.chdir or otherwise and ./?.lua is in package.path, then an extension could be silently shadowed by placing it in that directory. One solution is just making sure that no Hammerspoon-included extensions change the working directory. Another is to remove ./?.lua;./?/init.lua from package.path but I suspect this will break the loading of multi-file modules which require other (relative) Lua files from within their init.lua.

Re package paths... I suppose you could wrap require (we already do, to capture information about loaded modules for crash logs) to silently prepend the core paths before loading, then remove them again. But I'd want a way to systematically disable this for development purposes.

What about moving the core paths permanently ahead of ./?.lua;./?/init.lua but still behind the user config paths?

Re the hashing algorithm, sounds like you want something that hashes the files individually then hashes the report of that?

My understanding is that this option is the most secure, otherwise you have the potential for funny scenarios (albeit probably ineffective as threat vectors) where e.g. swapping the contents of two files would leave the hash unchanged. As a bonus, generating the report can be its own function, which is useful in its own right for e.g. seeing exactly which files have changed, and then add another simple hash function to hash the report.

asmagill commented 3 years ago

Without rewriting the package library in the lua source itself, you can't "permanently" do anything to package.path or package.cpath. And I have no idea what doing so might break, especially with respect to non-Hammerspoon modules, like those installed with LuaRocks (and we have a number of users that regularly use LuaRocks to install things).

latenitefilms commented 3 years ago

@latenitefilms Some of your filenames will have quotes in them, try this one: find ~/.hammerspoon -type f -not -name .DS_Store -print0 | sort -z | xargs -0 shasum -a512 | shasum -a512 | cut -d" " -f1

Awesome, thanks! That works. It's not instant - probably a second or two.

Would it be feasible to remove the preference/PLIST property for AppleScript entirely in favour of a Lua toggle (hs.osascript.applescript.enable() or otherwise)?

I think that's a good idea - depends on how strongly @cmsj & @asmagill feel about breaking backwards compatibility?

What about removing the MJConfigFile property and instead setting the directory in setup.lua instead? Then org.hammerspoon.Hammerspoon.plist doesn't contain any preferences that can affect Lua execution.

I'm not exactly sure how that would work? A lot of people have their Hammerspoon config in a place other than ~/.hammerspoon, so the path needs to be defined BEFORE the configuration is loaded?

asmagill commented 3 years ago

What about removing the MJConfigFile property and instead setting the directory in setup.lua instead? Then org.hammerspoon.Hammerspoon.plist doesn't contain any preferences that can affect Lua execution.

I'm not exactly sure how that would work? A lot of people have their Hammerspoon config in a place other than ~/.hammerspoon, so the path needs to be defined BEFORE the configuration is loaded?

That value is used within MJAppDelegate.m before the Lua state is even started for various purposes (determining if the test suite is running, determine if the user even has a configuration or needs to be prompted about the Getting Started guide, to determine where to store Spoons if such are double clicked on, probably others) and would likely require significant code changes to move it to another source... and we have a number of users who do in fact change it for various reasons, so moving it somewhere that required root (or sudo) would probably incur a lot of backlash, especially from those for whom this is the only developer like thing that they change.

gotham8x commented 2 years ago

I also concern about this right after installing Hammerspoon and granting permission to it. The software itself is safe, but it opens a potential issue for malicious programs to inject their scripts into ~/.hammerspoon folder. Currently I do temporary work-around by setting readonly permission to that folder, and lock it with chflags -R uchg ~/.hammerspoon.