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

Factors out XPC server message acceptance logic #39

Merged
merged 2 commits into from
Jan 9, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
70 changes: 70 additions & 0 deletions Sources/SecureXPC/Server/SecureMessageAcceptor.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
//
// SecureMessageAcceptor.swift
// SecureXPC
//
// Created by Josh Kaplan on 2022-01-06
//

import Foundation

/// Accepts messages which meet the provided code signing requirements.
///
/// Uses undocumented functionality prior to macOS 11.
internal struct SecureMessageAcceptor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be behind a protocol, so you can make an "always allow" implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally had it written that way, but it ended up with quite a bit more code across multiple files so it didn't seem worth the complexity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. If we need it in the future, we can just do an "extract interface" refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, can absolutely refactor in the future if warranted

/// At least one of these code signing requirements must be met in order for the message to be accepted
internal let requirements: [SecRequirement]

func acceptMessage(connection: xpc_connection_t, message: xpc_object_t) -> Bool {
// Get the code representing the client
var code: SecCode?
if #available(macOS 11, *) { // publicly documented, but only available since macOS 11
SecCodeCreateWithXPCMessage(message, SecCSFlags(), &code)
} else { // private undocumented function: xpc_connection_get_audit_token, available on prior versions of macOS
if var auditToken = xpc_connection_get_audit_token(connection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you add an else case, then code will be initialized in all code paths, removing the need for it to be optional.

Better yet, you should just flip this with a guard as well: guard var auditToken = ... else { return false }

Copy link
Contributor Author

@jakaplan jakaplan Jan 7, 2022

Choose a reason for hiding this comment

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

My understanding is code needs to be optional because both SecCodeCreateWithXPCMessage and SecCodeCopyGuestWithAttributes expect a parameter of type UnsafeMutablePointer<SecCode?> which can't be satisfied with a non-optional type. Is there a way to make this work?

Incorporated feedback on second point about guard statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

AH I didn't know that was the expected param type. Usually optional promotion happens, but not in the case of using & to generate a temporary pointer. You'd have to check, maybe it does do optional promotion in that case too, IDR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it's not allowed. Xcode outputs:

Cannot convert value of type 'UnsafeMutablePointer' to expected argument type 'UnsafeMutablePointer<SecCode?>'

let tokenData = NSData(bytes: &auditToken, length: MemoryLayout.size(ofValue: auditToken))
let attributes = [kSecGuestAttributeAudit : tokenData] as NSDictionary
SecCodeCopyGuestWithAttributes(nil, attributes, SecCSFlags(), &code)
}
}

// Accept message if code is valid and meets any of the client requirements
var accept = false
if let code = code {
for requirement in self.requirements {
if SecCodeCheckValidity(code, SecCSFlags(), requirement) == errSecSuccess {
accept = true
}
}
}

return accept
Copy link
Contributor

Choose a reason for hiding this comment

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

This for loop is approximating contains(where:) (but without the short-circuiting behaviour)

Suggested change
// Accept message if code is valid and meets any of the client requirements
var accept = false
if let code = code {
for requirement in self.requirements {
if SecCodeCheckValidity(code, SecCSFlags(), requirement) == errSecSuccess {
accept = true
}
}
}
return accept
guard let code = code else { return false }
return self.requirements.contains(where: { SecCodeCheckValidity(code, SecCSFlags(), $0) == errSecSuccess }

}

/// Wrapper around the private undocumented function `void xpc_connection_get_audit_token(xpc_connection_t, audit_token_t *)`.
///
/// The private undocumented function will attempt to be dynamically loaded and then invoked. If no function exists with this name `nil` will be returned. If
/// the function does exist, but does not match the expected signature, the process calling this function is expected to crash. However, because this is only
/// called on older versions of macOS which are expected to have a stable non-changing API this is very unlikely to occur.
///
/// - Parameters:
/// - _: The connection for which the audit token will be retrieved for.
/// - Returns: The audit token or `nil` if the function could not be called.
private func xpc_connection_get_audit_token(_ connection: xpc_connection_t) -> audit_token_t? {
// Attempt to dynamically load the function
guard let handle = dlopen(nil, RTLD_LAZY) else {
return nil
}
defer { dlclose(handle) }
guard let sym = dlsym(handle, "xpc_connection_get_audit_token") else {
return nil
}
typealias functionSignature = @convention(c) (xpc_connection_t, UnsafeMutablePointer<audit_token_t>) -> Void
Comment on lines +49 to +58
Copy link
Contributor

Choose a reason for hiding this comment

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

This gets called on every message. It's probably worth caching the resulting function, thought that would be best done in a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had the same thought while refactoring this, but didn't want to make such a change as part of this PR.

let function = unsafeBitCast(sym, to: functionSignature.self)

// Call the function
var token = audit_token_t()
function(connection, &token)

return token
}
}
12 changes: 7 additions & 5 deletions Sources/SecureXPC/Server/XPCAnonymousServer.swift
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//
// XPCAnonymousServer.swift
//
// SecureXPC
//
// Created by Alexander Momchilov on 2021-11-28.
//
Expand All @@ -9,9 +9,13 @@ import Foundation

internal class XPCAnonymousServer: XPCServer {
private let anonymousListenerConnection: xpc_connection_t

/// Determines if an incoming request can be handled based on the provided client requirements
private let messageAcceptor: SecureMessageAcceptor

internal override init() {
internal init(clientRequirements: [SecRequirement]) {
self.anonymousListenerConnection = xpc_connection_create(nil, nil)
self.messageAcceptor = SecureMessageAcceptor(requirements: clientRequirements)
super.init()

// Start listener for the new anonymous connection, all received events should be for incoming client connections
Expand All @@ -25,9 +29,7 @@ internal class XPCAnonymousServer: XPCServer {
}

internal override func acceptMessage(connection: xpc_connection_t, message: xpc_object_t) -> Bool {
// Anonymous service connections should only ever passed among trusted parties.
// TODO: add support for client security requirements https://github.com/trilemma-dev/SecureXPC/issues/36
true
self.messageAcceptor.acceptMessage(connection: connection, message: message)
}

/// Begins processing requests received by this XPC server and never returns.
Expand Down
66 changes: 6 additions & 60 deletions Sources/SecureXPC/Server/XPCMachServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@ internal class XPCMachServer: XPCServer, NonBlockingStartable {
private let machServiceName: String
/// Receives new incoming connections, created once the server is started.
private var listenerConnection: xpc_connection_t?
/// The code signing requirements a client must match in order for an incoming request to be handled.
private let clientRequirements: [SecRequirement]
/// Determines if an incoming request will be accepted based on the provided client requirements
private let messageAcceptor: SecureMessageAcceptor

/// This should only ever be called from `getXPCMachServer(...)` so that client requirement invariants are upheld.
private init(machServiceName: String, clientRequirements: [SecRequirement]) {
self.machServiceName = machServiceName
self.clientRequirements = clientRequirements
self.messageAcceptor = SecureMessageAcceptor(requirements: clientRequirements)
}

/// Cache of servers with the machServiceName as the key.
Expand Down Expand Up @@ -63,7 +63,8 @@ internal class XPCMachServer: XPCServer, NonBlockingStartable {

// Turn into sets so they can be compared without taking into account the order of requirements
let requirementsData = Set<Data>(try clientRequirements.map(requirementTransform))
let cachedRequirementsData = Set<Data>(try cachedServer.clientRequirements.map(requirementTransform))
let cachedRequirementsData = Set<Data>(try cachedServer.messageAcceptor.requirements
.map(requirementTransform))
guard requirementsData == cachedRequirementsData else {
throw XPCError.conflictingClientRequirements
}
Expand Down Expand Up @@ -177,29 +178,7 @@ internal class XPCMachServer: XPCServer, NonBlockingStartable {
}

internal override func acceptMessage(connection: xpc_connection_t, message: xpc_object_t) -> Bool {
// Get the code representing the client
var code: SecCode?
if #available(macOS 11, *) { // publicly documented, but only available since macOS 11
SecCodeCreateWithXPCMessage(message, SecCSFlags(), &code)
} else { // private undocumented function: xpc_connection_get_audit_token, available on prior versions of macOS
if var auditToken = xpc_connection_get_audit_token(connection) {
let tokenData = NSData(bytes: &auditToken, length: MemoryLayout.size(ofValue: auditToken))
let attributes = [kSecGuestAttributeAudit : tokenData] as NSDictionary
SecCodeCopyGuestWithAttributes(nil, attributes, SecCSFlags(), &code)
}
}

// Accept message if code is valid and meets any of the client requirements
var accept = false
if let code = code {
for requirement in self.clientRequirements {
if SecCodeCheckValidity(code, SecCSFlags(), requirement) == errSecSuccess {
accept = true
}
}
}

return accept
self.acceptMessage(connection: connection, message: message)
}

public override var endpoint: XPCServerEndpoint {
Expand All @@ -219,39 +198,6 @@ internal class XPCMachServer: XPCServer, NonBlockingStartable {
endpoint: endpoint
)
}

/// Wrapper around the private undocumented function `void xpc_connection_get_audit_token(xpc_connection_t, audit_token_t *)`.
///
/// The private undocumented function will attempt to be dynamically loaded and then invoked. If no function exists with this name `nil` will be returned. If
/// the function does exist, but does not match the expected signature, the process calling this function is expected to crash. However, because this is only
/// called on older versions of macOS which are expected to have a stable non-changing API this is very unlikely to occur.
///
/// - Parameters:
/// - _: The connection for which the audit token will be retrieved for.
/// - Returns: The audit token or `nil` if the function could not be called.
private func xpc_connection_get_audit_token(_ connection: xpc_connection_t) -> audit_token_t? {
typealias functionSignature = @convention(c) (xpc_connection_t, UnsafeMutablePointer<audit_token_t>) -> Void
let auditToken: audit_token_t?

// Attempt to dynamically load the function
if let handle = dlopen(nil, RTLD_LAZY) {
defer { dlclose(handle) }
if let sym = dlsym(handle, "xpc_connection_get_audit_token") {
let function = unsafeBitCast(sym, to: functionSignature.self)

// Call the function
var token = audit_token_t()
function(connection, &token)
auditToken = token
} else {
auditToken = nil
}
} else {
auditToken = nil
}

return auditToken
}
}

extension XPCMachServer: CustomDebugStringConvertible {
Expand Down
6 changes: 4 additions & 2 deletions Sources/SecureXPC/Server/XPCServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,10 @@ public class XPCServer {
try XPCServiceServer._forThisXPCService()
}

internal static func makeAnonymousService() -> XPCServer & NonBlockingStartable {
XPCAnonymousServer()
internal static func makeAnonymousService(
clientRequirements: [SecRequirement]
) -> XPCServer & NonBlockingStartable {
XPCAnonymousServer(clientRequirements: clientRequirements)
}

/// Provides a server for this helper tool if it was installed with
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,15 @@ import XCTest
class RoundTripIntegrationTest: XCTestCase {
var xpcClient: XPCClient! = nil

let anonymousServer = XPCServer.makeAnonymousService()
static let dummyRequirements: [SecRequirement] = {
var requirement: SecRequirement?
// This is a worthless security requirement which should always result in the connection being accepted
SecRequirementCreateWithString("info [CFBundleVersion] exists" as CFString, SecCSFlags(), &requirement)

return [requirement!]
}()

let anonymousServer = XPCServer.makeAnonymousService(clientRequirements: RoundTripIntegrationTest.dummyRequirements)

override func setUp() {
let endpoint = anonymousServer.endpoint
Expand Down