[libcxx-commits] [PATCH] D155873: [libc++][hardening] Categorize more assertions.

Sat Jul 22 09:23:38 PDT 2023

Mordante added a comment.

In D155873#4523756 <https://reviews.llvm.org/D155873#4523756>, @ldionne wrote:

> In D155873#4523344 <https://reviews.llvm.org/D155873#4523344>, @Mordante wrote:
>
>> I really like this hardening work. I think it would be good to have bit more discussion and tweaking after LLVM 17 has branched. I would love it when a security expert can have a look at it. I expect they have a better view on what root causes for security issues are.
>
> FWIW, we're planning to review everything with security experts in the coming weeks.

Great to hear, thanks.

================
Comment at: libcxx/include/__tree:171
 {
-    _LIBCPP_ASSERT_UNCATEGORIZED(__x != nullptr, "Root node shouldn't be null");
+    _LIBCPP_ASSERT_INTERNAL(__x != nullptr, "Root node shouldn't be null");
     while (__x->__left_ != nullptr)
----------------
var-const wrote:
> Mordante wrote:
> > As mentioned in D155906 I really would like to have these kind of cheap internal sanity checks enabled in `_LIBCPP_ENABLE_HARDENED_MODE`. They are not user errors, but they still cause potential issues for users, including users in safety-critial places. When there's not enough time to get that done before LLVM 17, I'm happy to defer that to LLVM 18.
> Thanks for starting this discussion!
> 
> My current thinking has been to keep the hardened mode as lightweight as possible. I have a big concern about performance penalty blocking adoption; also, while I'm not 100% sure this can happen, my ideal would be if we could make the hardened mode the default at some point (of course, users would still be able to opt out and use the unchecked mode). Because of that, so far I have been generally erring on the side of performance and using the following criteria for enabling an assertion:
> 1. Is the failure directly triggered by user input?
> 2. Is the failure //directly// exploitable?
> 3. Is the failure condition relatively cheap to check?
> 
> Personally, I feel #2 is the most important distinction. Almost any error, including logic errors that don't trigger UB, can lead to very incorrect results and also contribute to a security vulnerability. I envision the hardened mode as only preventing errors that create a security vulnerability //directly// (e.g. an out-of-bounds read) -- the kind of errors where we can confidently say "if your program has one of those, it is vulnerable, regardless of any other context" (and thus we can justify the extra penalty for checking those).
> 
> From that perspective, it follows that checks for null pointers should not be a part of the hardened mode (FWIW, it was somewhat surprising to me, or at least I found it counterintuitive). From what I know (and I'd be happy to be corrected on this if I'm missing something), dereferencing a null pointer on any relatively modern OS leads to a crash and is not directly exploitable (it might contribute to a more complicated exploit arising from context, but so can many other types of failures).
> 
> The upside I'm hoping we get from limiting the number of checks we perform is wider adoption. The intention behind hardened mode is not that it provides complete safety or entirely eliminates UB -- rather, it makes your program measurably //safer// at a "negligible" cost; it relatively cheaply protects against the most catastrophic stuff. I'm hoping something like the 80/20 principle applies here (meaning that just a fraction of the cost that would be necessary to protect against all UB is enough to prevent 80% of exploitable vulnerabilities). The "perfect" hardened mode (aspirational, not realistic) is the mode where if you were to enable it, you would be guaranteed that an attacker could not use your program to achieve arbitrary code execution, //and// it's cheap enough to use in production.
> 
> We currently have the hardened mode and the debug mode. While I'd prefer to keep this as simple as possible and stick with the 2 modes, I'd be open to exploring adding a 3rd mode in-between those two that would contain more checks that the hardened mode while still aiming at some reasonable level of performance (which for the debug mode is almost a non-goal).
> 
> Tagging @ldionne as well in case he has a different perspective.
I'm not sure whether deferring a null pointer will always leads to a crash on a modern OS. I can imagine some cases where that might be an issue. Embedded systems may use less well know OSes. Are kernel drivers allowed to modify address 0 or with low values? (This assumes a nullptr is mapped to address 0, which is not required by the Standard.)

Maybe we should ask this to security experts. They probably can tell us how often deferring a nullptr may lead to real-world issues.

Even when it's not a security-issue I think it would be good to test a nullptr when not allowed. Deferring nullptr is known to be quite a large issue in both C and C++. I'm also open to a third mode. Maybe we should measure what the overhead for adding null checks is. When we annotate it with likelihood attribute telling the compiler to expect a valid pointer the branch predictor will always guess correctly. In that case the overhead should be low. The nullptr case will be more expense, but I in that case the performance is no longer relevant.

I think we can move forward this way. When we put everything in the right category we can measure the overhead of every category. That way we can have number of the performance overhead and can make an informed decision in this regard.

I think it would be good to discuss this on Discord/Discourse to get input of more people in the LLVM communicate. Again I'm fine to do that for LLVM 18 and keep the current way in LLVM 17.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155873/new/

https://reviews.llvm.org/D155873