Factors out XPC server message acceptance logic #39
Conversation
Factors out the security logic into a SecureMessageAcceptor struct.
|
@amomchilov when you have the time, would appreciate you taking a look. I went with a compositional approach over inheritance as it seemed simpler to me. |
| 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) { |
There was a problem hiding this comment.
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 }
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Unfortunately it's not allowed. Xcode outputs:
Cannot convert value of type 'UnsafeMutablePointer' to expected argument type 'UnsafeMutablePointer<SecCode?>'
| // 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 |
There was a problem hiding this comment.
This for loop is approximating contains(where:) (but without the short-circuiting behaviour)
| // 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 } |
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Had the same thought while refactoring this, but didn't want to make such a change as part of this PR.
| /// Accepts messages which meet the provided code signing requirements. | ||
| /// | ||
| /// Uses undocumented functionality prior to macOS 11. | ||
| internal struct SecureMessageAcceptor { |
There was a problem hiding this comment.
Should this be behind a protocol, so you can make an "always allow" implementation?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fair enough. If we need it in the future, we can just do an "extract interface" refactor.
There was a problem hiding this comment.
Yup, can absolutely refactor in the future if warranted
|
Meta point: I would suggest:
|
jakaplan
changed the title
Factors out the security logic into a SecureMessageAcceptor struct.
Resolves #36