add Feature/debugger detection #10
Conversation
Sources/DebuggerDetection.swift
Outdated
|
|
||
| let result = sysctl(&mib, u_int(mib.count), &info, &size, nil, 0) | ||
| if result != 0 { | ||
| fatalError("sysctl failed") |
There was a problem hiding this comment.
please remove fatalError calls from a production code
There was a problem hiding this comment.
Sure, I will change it to a false return at this point (no debugger recognized)?
Occurs if the sysctl call is not successful. Should not actually happen.
At this moment we cannot make a reliable statement about whether a debugger is running or not. It is not clear to me whether it would be better to return false or true. It is actually undefined at this point.
There was a problem hiding this comment.
return false at this point?
There was a problem hiding this comment.
yes, in this case we have only 2 options: assume no debugger is recognized (return false) or handle this case separately (ex. return nil). As the usecase should not be too often, it is fine to return here false.
There was a problem hiding this comment.
Hi @theDeniZ ,
I'm checking this SDK as an option to implement it into our apps, but before we do that, I'm tracking this PR as we already have (Apple's) debugger check in the old SDK.
Usually, if the result is not 0, that means that we can not determine whether a debugger is connected. In that case, we can assume that checking the debugger's connection is not reliable.
This leads me to be concerned about returning any value here.
If we look at Apple's implementation (https://developer.apple.com/library/archive/qa/qa1361/_index.html), they use assert to check that value. When the app is connected to the debugger (e.g., Xcode) and run from there, it's ok to have crash on that line, while on the production build (archive) this will be ignored.
TL;DR;
I would propose using the assert(result == 0, "Sysctl failed, check could not be reliable") instead of falsely returning false.
Returning an option value would lead to more extensive documentation and a more complex call site, where SDK callers would have to handle all those use cases they could easily miss. Throwing an error is also an option, but it may be overkill for this case.
There was a problem hiding this comment.
Looks to me like we are doing it with an optional bool.
I think this is a good solution for now, as an enum can always be added at a later stage of development without changing the framework's API too much. (If we want it then)
There was a problem hiding this comment.
Thank you all for the discussion 👍
There was a problem hiding this comment.
Yes. After a few internal discussions we are more comfortable with "nil" solution. Please include a good documentation for a consumer to reflect, what the "nil" return value could mean.
There was a problem hiding this comment.
I have pushed the changes.
isDebuggerDetected return type is now an Optional Bool.
There was a problem hiding this comment.
Please also note the following comment on AsyncStream.
https://github.com/EXXETA/iOS-Security-Toolkit/pull/10/files#r1932347230
It is currently not possible to cover the nil return of DebuggerDetection.threatDetected() via the AsyncStream. If you want to check for nil, you must use the static function isDebuggerDetected without AsyncStream.
no explicit Darwin import needed and therefore removed
Use of optional bool for public function isDebuggerDetected. This decision is the result of a joint discussion that took place in the associated PR EXXETA#10 : EXXETA#10 Motivation: No exception should be thrown. It should be communicated that something went wrong in the decision-making process and that no clear statement can be made. Decision: We now return nil in an unclear case. nil: The detection process did not produce a definitive result. This could happen due to system limitations, lack of required permissions, or other undefined conditions.
* add DebuggerDetection.swift * add DebuggerDetection to ThreatDetectionCenter * add debugger detection view to SecurityToolkitExample * add documentation for isDebuggerAttached * fix typo: isDebuggerAttached -> isDebuggerDetected * Comment made even more clear * hasTracerFlagSet() additionally made private * No explicit Darwin import needed and therefore rmd no explicit Darwin import needed and therefore removed * Use of optional bool for isDebuggerDetected return Use of optional bool for public function isDebuggerDetected. This decision is the result of a joint discussion that took place in the associated PR #10 : #10 Motivation: No exception should be thrown. It should be communicated that something went wrong in the decision-making process and that no clear statement can be made. Decision: We now return nil in an unclear case. nil: The detection process did not produce a definitive result. This could happen due to system limitations, lack of required permissions, or other undefined conditions.
No description provided.