Reported by Matheus Mariano, a Brazilian software developer, a programming error was discovered in Apple’s most recent operating system, High Sierra, that exposed passwords of encrypted volumes as password hints. A serious bug that quickly made the headlines in technology websites everywhere.

The dreaded password hint bug: Here, “dontdisplaythis” is the actual password.

Apple was prompt to provide macOS High Sierra Supplemental Update to customers via the App Store, and ensured that every distribution of High Sierra in their servers included this update.

I decided to apply a binary diffing technique to the update to learn more about the root cause of this bug and hypothesize about how the defect could have been prevented.

Inspecting the 51MB package, we can see that there are changes in the Disk Utility and Keychain Access apps, and also in related frameworks and command line tools:

This post will focus only on the password hint bug, so our first step is to extract Applications/Utilities/Disk Utility.app/Contents/MacOS/Disk Utility  and to compare it with the same binary from a stock macOS 10.13 High Sierra. For this, I have written an Emacs extension that launches IDA whenever I load a Mach-O file in a buffer, generates a SQL database with information about the decompiled functions, loads the patched binary, and finally outputs a diff generated by Diaphora. This technique is useful for deconstructing binaries that have been updated by a minor patch release because there are usually just a few changes and common heuristics work well.

The diff between both versions of the Disk Utility binary revealed no differences in the decompilation:

That usually means that the only substantial changes reside in one of the linked frameworks. The most interesting one for this investigation is StorageKit, a private Apple framework that exposes APFS functionality to Disk Utility. It has two parts: a client library and a daemon, storagekitd. The client connects to the daemon using an Apple standard XPC mechanism. The daemon executes the operations (represented as subclasses of NSOperation) that the client demands. Here’s an interesting usage of StorageKit inside Disk Utility:

Reference to a StorageKit structure from controller code in Disk Utility.

This is part of the code that runs when you add a new APFS volume from the Disk Utility interface (concretely, the controller responsible for managing the new volume sheet).

Diffing StorageKit provided much more interesting results:

​​​[SKHelperClient addChildVolumeToAPFSContainer:name:caseSensitive:minSize:maxSize:password:passwordHint:progressBlock:completionBlock:] was one of the functions modified by the supplemental update. Inspecting the differences in decompilation revealed the actual bug:

In the picture above, the old, vulnerable, StorageKit is diff’d against the updated one. Removed lines removed are depicted in red, added lines in green, and changes in yellow. The above function basically creates an instance of NSMutableDictionary (Cocoa’s representation of a hash table) and fills it with information about the volume. This dictionary is passed to addChildVolumeToAPFSContainer:optionsDictionary:handlingProgressForOperationUUID:completionBlock: as the optionsDictionary argument.

The most interesting keys in the dictionary are kSKAPFSDiskPasswordOption and kSKAPFSDiskPasswordHintOption, which are responsible for storing the password and the password hint, respectively. The bug is that the same variable, which contains the password, (represented in the decompilation as the same virtual register, v50) was used as value for both keys in the dictionary, meaning that the clear password was incorrectly sent as a password hint via XPC. In reconstructed Objective-C code, the bug would be something like this:

NSMutableDictionary *optionsDictionary = [NSMutableDictionary alloc] init];
[...]
optionsDictionary[kSKAPFSDiskPasswordOption] = password;

optionsDictionary[kSKAPFSDiskPasswordHintOption] = password;

Here’s the corrected function from the supplemental update:

Note that the correct variables for the password and the password hint are set.

This is an example of a common category of bugs where code with a common structure is copied and pasted but the developer forgets to make every required modification and consequently there’s a fatal change in behavior. If you are curious, this blog post shows you more examples of “Last Line Effect” bugs in open source software.

It’s important to emphasize that, although this particular dictionary is not stored anywhere (it’s simply used to pack the information that is sent to storagekitd), the fact that the password was sent incorrectly as password hint meant that storagekitd trusted its client and stored it as clear text, thinking it was a password hint.

Why did the bug not reproduce when using the command line?

This is a common question. Apparently, Disk Utility and command line diskutil use different code paths.  StorageKit does not appear as a direct dependency of diskutil, or in the transitive closure of its dependencies. Here’s otool -L output:

/usr/lib/libcsfde.dylib (compatibility version 1.0.0, current version 1.0.0)/usr/lib/libcsfde.dylib (compatibility version 1.0.0, current version 1.0.0) /usr/lib/libCoreStorage.dylib (compatibility version 1.0.0, current version 1.0.0) /System/Library/Frameworks/Foundation.framework/Versions/C/Foundation (compatibility version 300.0.0, current version 1443.14.0) /System/Library/Frameworks/IOKit.framework/Versions/A/IOKit (compatibility version 1.0.0, current version 275.0.0) /System/Library/PrivateFrameworks/DiskManagement.framework/Versions/A/DiskManagement (compatibility version 1.0.0, current version 1.0.0) /System/Library/Frameworks/DiscRecording.framework/Versions/A/DiscRecording (compatibility version 1.0.0, current version 1.0.0) /usr/lib/libncurses.5.4.dylib (compatibility version 5.4.0, current version 5.4.0) /System/Library/Frameworks/DiskArbitration.framework/Versions/A/DiskArbitration (compatibility version 1.0.0, current version 1.0.0) /usr/lib/libicucore.A.dylib (compatibility version 1.0.0, current version 59.1.0) /usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0) /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1252.0.0) /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1443.13.0)

This duplication in what’s more or less the same functionality, while sometimes justified, certainly increases the opportunity for bugs.

How could this have been prevented?

There’s two engineering practices that help with bugs like this (but do not eradicate them completely):

Unit testing

Unit testing is the practice of creating software tests that exercise a single unit in a computer program, where “unit” is typically a class or module. Effective unit testing requires sensing outputs reliably and asserting that they are expected, so side effects from functions complicate unit testing a bit. In this particular bug, the side effect is the communication with the XPC service, so separating the logic that creates the dictionary from the part that communicates with the service would help. When a software design is not easily testable, companies rely excessively on manual testing, which is not a very effective way of testing, given the high number of combinations that is typical in modern software (did the QA engineer test setting a password *and* a password hint?, easily forgettable on a tight deadline).

Code review

Code review is the practice of reviewing code before or after it lands the main development branch in a software project. Code reviews should always be small, so that the reviewer’s attention is focused and can suggest better improvements and even spot bugs like this. A “last line” bug can easily be ignored if it’s part of a huge code review.

Conclusion

An unfortunate bug in macOS High Sierra stained a bit its generally well-received debut, and from this root-case analysis we can learn what happened exactly and how good software development practices (including testable design and strict code reviews) can help reduce the chance that this kind of problems happen again in the future.