RFC: C++ Buffer Hardening

[jankorous]

RFC: C++ Buffer Hardening

We aim to improve security of critical C++ codebases. In order to do that we plan to work on two ideas.

1. Hardened C++ Standard Library
2. C++ Safe Buffers Programming Model and Adoption Tooling

Hardened libc++ aims to make C++ Standard Library interfaces generally more secure.

C++ Safe Buffers Programming Model together with Hardened libc++ provide runtime mitigation of out-of-bounds memory access. The adoption tooling will automatize code migration to this new programming model.

Hardened C++ Standard Library

by Louis Dionne (@ldionne)

We plan to implement a hardened mode of libc++ in which several cases of undefined behavior are caught and turned into assertion failures instead. For example, accessing a std::span or a std::vector outside of its bounds would abort the program, and so would accessing an empty std::optional. This can be done while staying Standards-conforming because undefined behavior implies that the library can do whatever it wants, which includes aborting.

These additional runtime checks will be grouped into various categories that can be controlled separately. The intent is that a vendor shipping libc++ on their platform can decide to pick which checks to enable in the library that they ship (if at all), depending on their desired level of safety. To reiterate, the end goal is for the shipped library to enable these checks in production — this is not a “debug only” feature, although it will eventually replace the long-broken “Debug mode”.

Indeed, the current libc++ “Debug mode” has several issues that explain why it has never been shipped by some vendors: it is slow, it requires a global database with a lock, and it is ABI-affecting despite the fact that its original design was done entirely to avoid that. Finally, we also realized recently that the current “Debug mode” was fundamentally incompatible with constexpr, which makes it non viable going forward.

Instead, we will explicitly allow some of the new checks to affect the ABI, which will make it possible to implement them more efficiently. For example, we may need to slightly tweak the representation of some data structures to include additional information to inform the checks. Categories of checks that are ABI-affecting will only be controllable by vendors at the time of configuring the library, while other checks will be controllable by users in their own code. ABI-affecting checks will also be encoded in the mangled names of library entities to reduce the risk for accidental mismatches.

Finally, libc++ already implements some of this proposal. Indeed, we recently introduced a safe mode where one can enable the most basic category of checks, i.e. those that do not have an effect on the ABI or on the complexity of operations. The assertion handler can also be customized to meet the needs of various users, similarly to how operator new can be customized via weak linking.

C++ Safe Buffers

by
Artem Dergachev (@NoQ)
Jan Korous (@jankorous)
Malavika Samak (@malavikasamak)
Rashmi Mudduluru (@t-rasmud)
Ziqing Luo (@ziqingluo-90)

Programming Model

We plan to introduce Safe Buffers Programming Model under which any pointer arithmetic is considered unsafe and clang warns about it. While this can be potentially useful for security-critical C projects its main application are C++ codebases in which pointer arithmetic can be transformed to use Hardened libc++ facilities such as std::array, std::vector and std::span that will be bounds-checked at runtime and potential exploits turned into traps.

Our plan is to emit a warning every time an unsafe operation is performed on a raw pointer (largely similar to clang-tidy checks for bounds safety such as clang-tidy - cppcoreguidelines-pro-bounds-pointer-arithmetic — Extra Clang Tools 16.0.0git documentation).

We plan the warnings to be directly in the compiler, being off-by-default (most likely outside of -Wall as well). This allows us to reach all maintainers of the targeted projects without forcing them to use other clang tools or to regress their build times. In particular, we expect our users to keep the warnings enabled even after they’ve updated all their code, to avoid regressing back to the old ways in the new code. This way these warnings basically form a hardened language mode. That said, we plan to have most of our machinery live in libAnalysis, from which it can be accessed by arbitrary tools or generalized to other use cases.

We are not addressing temporal memory safety violations such as use-after-free / lifetime bugs.

Adoption Tooling

In C++ mode we will also emit fixits that replace one or more variables of raw pointer type with standard containers. Such fixits would also update other uses of these variables if a standard container isn’t an exact drop-in replacement of a raw pointer; for example, if(array_pointer) may be converted to if(span.data()).

The fixits are targeting not only local variables but also parameter variables of pointer type. When we suggest replacing a parameter variable with, say, a std::span, the fixit would keep the old prototype as an overload but mark it deprecated with a custom attribute. Calls to functions wearing that attribute will be treated as unsafe operations in other functions which would in turn cause us to emit a warning at the call site. This way our fixes can spread across function boundaries or even across translation unit boundaries.

We plan to produce a reasonable fixit in the majority of cases to ease adoption. This means that we’re considering fairly sophisticated and novel machinery that analyzes all uses of one or more related variables to discover which container is appropriate and what other code changes will be necessary. We also do not promise that the fixit is going to compile; in some cases we plan to leave placeholders for the user to fill in, such as the size parameter for std::span. Later we may try to improve our machinery to discover the size of the span automatically but we believe that even without that feature the fixits are going to save our users a lot of time.

Static Analyzer Checker

We are also considering a path-sensitive clang static analyzer checker that warns if std::span is constructed from a container that has smaller size than specified in the span’s constructor. Such checker is self-contained and useful on its own, if everything goes well it’ll be enabled by default for all users. But it will play nicely with this project as it’ll protect the users from accidentally introducing new bugs while refactoring their code.

Comments and suggestions are welcome!

[tschuett]

+2…

Two comments:
const char* → string_view does not work in all C++ standards.

Is there room for a rusty unsafe

UNSAFE() {
  // pointer arithmetic
}

’ where unsafe is maybe an attribute?!?

[Xazax-hun]

Great initiative! I really love it!

@jankorous

The intent is that a vendor shipping libc++ on their platform can decide to pick which checks to enable in the library that they ship (if at all), depending on their desired level of safety.

Sometimes the desired level of safety of a platform vs a particular application might differ. Are there good ways for distributions to bundle multiple versions of libc++ and let applications link against the version they prefer? Or are applications stuck with static linking if they want to override the platform’s default? As a user, my ideal experience would be something like -fsafe-libcpp that would ensure that my application would only work with the safe variant of libc++.

@jankorous

Our plan is to emit a warning every time an unsafe operation is performed on a raw pointer (largely similar to clang-tidy checks for bounds safety such as clang-tidy - cppcoreguidelines-pro-bounds-pointer-arithmetic — Extra Clang Tools 16.0.0git documentation).

What does “largely similar” mean? What are the differences? I think the core guidelines are promoting the use of bound safe types over raw pointers so adding fixits to the existing core guidelines checks would be in line with the guidelines. Is there any specific reason why you are doing something in parallel? Is the only reason because you want to implement these checks directly in the compiler? Would it make sense to make the tidy warnings aliases to compiler warnings?

@jankorous

When we suggest replacing a parameter variable with, say, a std::span, the fixit would keep the old prototype as an overload but mark it deprecated with a custom attribute.

There is also gsl::span where the main motivation was to make it bounds checked and the library users can set a handler if they want to recover from an out of bounds error. (E.g., skipping the operation that triggered it, but continuing with other unrelated work.) Would you be open to support gsl::span as well (could be controlled by a flag)?

[kcc]

Yes! Please ship it!

[NoQ]

@tschuett

const char* → string_view does not work in all C++ standards.

Yes, and std::span is very new, like C++20. Yes, this proposal implies that the users are willing to adopt a fresh C++ standard, that’s a major requirement.

@tschuett

Is there room for a rusty unsafe

UNSAFE() {
  // pointer arithmetic
}

’ where unsafe is maybe an attribute?!?

Given that the functionality is supposed to be built as clang warnings, you can always do a

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunsafe-buffer-usage"
// pointer arithmetic
#pragma clang diagnostic pop

Which you can wrap in macros for easier use:

UNSAFE_BUFFERS_USAGE_BEGIN
// pointer arithmetic
UNSAFE_BUFFERS_USAGE_END

In a similar manner, you can use these pragmas to opt into the checking for portions of the file even when you aren’t passing the flag globally, so you can adopt containers incrementally.

While we haven’t decided on that yet, we may implement a better facility than these pragmas. In particular, we don’t like how these pragma-based opt-ins and opt-outs interact out of the box with all this push-pop dynamic.

[jankorous]

@Xazax-hun

There is also gsl::span where the main motivation was to make it bounds checked and the library users can set a handler if they want to recover from an out of bounds error. (E.g., skipping the operation that triggered it, but continuing with other unrelated work.) Would you be open to support gsl::span as well (could be controlled by a flag)?

Our primary target is hardened libc++ because that’s what is used in our software stack. At the same time I can totally imagine someone taking the machinery we’ll bring up and adding a different code transformation strategy that doesn’t target C++ Standard Library at all.

It is tempting to say that we’ll just change the namespace of span based on a flag because gsl::span really does look like a drop-in replacement for std::span in this context.

The only difference between gsl::span and std::span is that gsl::span strictly enforces runtime bounds checking. Any violations of the bounds check results in termination of the program.

GitHub - microsoft/GSL: Guidelines Support Library

However, we plan to also use other types such as std::array, std::vector or std::span<T>::iterator and it doesn’t look like GSL has direct analogy for all of these.

[DenisYaroshevskiy]

I don’t see why would this be a good idea.
The performance overhead is real and is easily measured: Quick C++ Benchmarks

Apparently optimising it out is not something current compilers can do.
While I appreciate the security concern - introducing a 20% overhead everywhere for security goes against the language.

It has also been tried - Microsoft had “secure STL” levels. And people hated it. For example didn’t use iterators in vectors, instead realying on accessing data.

Consider how this will look on a big company level:

• We introduce a new policy - we now have perfomance overhead on every array access.
• C++ developers hate those things - you get into constant hacks around it.
• Developers will be massively unhappy.
• There will be more justified advice “don’t use standard library - it’s slow”

[NoQ]

@Xazax-hun

What does “largely similar” mean? What are the differences? I think the core guidelines are promoting the use of bound safe types over raw pointers so adding fixits to the existing core guidelines checks would be in line with the guidelines. Is there any specific reason why you are doing something in parallel? Is the only reason because you want to implement these checks directly in the compiler? Would it make sense to make the tidy warnings aliases to compiler warnings?

Our current thinking is that these very simple checkers already did a great job at identifying individual unsafe operations, there’s not much we want to change there.

Our main deliverable on top of that is fixits, like massive fixits. Once fully implemented, our fixit work would constitute over 95% of the tool’s complexity. So our solution absorbs these checks, and it’s so much larger than these checks, that it doesn’t really matter whether we reuse them or build from scratch.

Another feature that we’ll have on top of these clang-tidy checks is that we’re planning annotations for function boundaries: “this function accepts a pointer and uses it unsafely” (so, not necessarily obvious from the parameter type). This allows the users to adopt bounds-safe containers incrementally, say one class at a time, or one translation unit at a time, without breaking source compatibility. We also consider automatic fixits in situations where the code is using a function with such attribute while a bounds-safe variant is already provided (similar to how [[deprecated]] may suggest a drop-in replacement).

So, yes, our current strategy is to implement the core of our analysis in libAnalysis, which can be imported by either the compiler or clang at any time. This naturally allows us to absorb the existing clang-tidy check.

But we do prefer our tool to be delivered in the form of compiler warnings because that’s the easiest way to reach all users. Static analyzer and clang-tidy are a separate tool they need to opt into, so it comes with an extra cost and it’s much harder to enforce. Compiler warnings are naturally enforceable.

[jankorous]

I understand where you are coming from and agree that this approach does have both benefits and costs. Different projects have different requirements and consequently different trade-offs might or might not make sense. That is why we plan the new diagnostics to be off by-default and excluded from -Wall. That way no project gets affected unless it explicitly opts-in.

Yes, this feature is targeted towards security critical projects and is likely not generally applicable. For such projects security might be more important than performance.

[DenisYaroshevskiy]

Do you see this enabled company wide? Because then you will have all of the problems I described. As a flag to be able to develop some special libraries - maybe. Though those libraries are still written in C++ for some reason - presumably they wanted there performance.

Do you have some open source project that want this enabled?

[jankorous]

We do have internal use-cases but I unfortunately can’t comment on the details at this time.

[kcc]

I can totally see the compile-time enforcement be enabled company wide and the run-time checks to be selectively enabled per-binary, based on security/performance trade-offs.

One important thing (if I understand the RFC correctly): if you enforce these rules at compile time they don’t cost you any performance unless you also enable the run-time checks. But enforcing the rules allows you to enable the checks with a compile-time switch at any moment. You can also enable the checks by default for all non-production builds in order to fix more bugs there. Note: some of these checks will find bugs that no other mainstream tool finds today, e.g. AddressSanitizerIntraObjectOverflow · google/sanitizers Wiki · GitHub

[DenisYaroshevskiy]

I will say this: please be careful:

Things I’d expect this to lead to:

• bifrication of the community
• quiet sabotage: c++ developers hate inefficiencies
• prelifiration of hand written version of standard library that does not do those checks. (we had seen time and time again when inefficient standard library lead to that)
• more pain to everyone.

[DenisYaroshevskiy]

compile time
What I understood is - at compile time enforce that you are not using sertain constracts. Tests are always runtime

[J-Montgomery]

To clarify the runtime aspects, this proposal would not a replace the default libc++. It would provide an optional alternative libc++ that would provide greater guarantees in certain edge cases. There are many industries where C++ is in common use (e.g. automotive, aerospace) that would greatly benefit from a hardened runtime environment at the cost of some performance. The security side is nice too.

With respect to the proposal itself, is there any relationship between this and TR24772-10? The intent significantly overlaps and allowing the TR to guide both the detections and the adoption tooling suggestions seems like an obvious choice. It could also go a long way to helping end users understand what is and isn’t detected, an issue that’s long plagued sanitizers.

[NoQ]

@kcc

if you enforce these rules at compile time they don’t cost you any performance unless you also enable the run-time checks.

Yes, pretty much, unless you look at it with a lot more scrutiny. But there’s also relatively little security benefit of that alone, so I don’t expect this to be useful unless the user already plans to use the hardened library at least occasionally.

@kcc

You can also enable the checks by default for all non-production builds in order to fix more bugs there.

Yes, the hardened C++ standard library can be used independently as a “sanitizer”; I think @ldionne will follow up with more explanations on that in the near future.

So the hardened libc++ is useful on its own, but adoption tooling probably isn’t.

@DenisYaroshevskiy

I will say this: please be careful:

Things I’d expect this to lead to:

• bifrication of the community
• quiet sabotage: c++ developers hate inefficiencies
• prelifiration of hand written version of standard library that does not do those checks. (we had seen time and time again when inefficient standard library lead to that)
• more pain to everyone.

I agree, I don’t think this solution can be applied without active consent from the developers. Not just for performance reasons, but also because it is they who will have to spend time rewriting their code to use bounds-safe collections. It’s definitely not intended to be something that you can simply throw at the problem and expect results for free.

It also doesn’t need to be company-wide, but it has to deal with libc++ ABI concerns which somewhat limits quantization. So it could be applied, say, per-executable.

[waffles_the_dog]

You are correct in that bounds checking can have negative performance impacts. However we know that these impacts can be minimized (Many years ago I was able to make WebKit’s Vector class bounds check every access with no performance impact in real code).

I do agree that the current codegen for things like std::vector are suboptimal, but the trade offs have to be considered carefully. Saying “you can never regress a microbenchmark” is tantamount to saying “you can never have libc++ be memory safe”, which is simply unreasonable.

• We introduce a new policy - we now have perfomance overhead on every array access.

That sounds like work for a resourceful engineer
Yes it may turn out that a bound check is required and no optimizer can remove it, but in that case we have to ask “is this code actually correct in its assumptions?”. At a fundamental level if an all seeing optimizer cannot get rid of a bounds check, then you the programmer also can’t safely assume it is unnecessary.

• C++ developers hate those things - you get into constant hacks around it.

They also hate 8pm/Saturday calls about exploited security bugs. Again everything is about trade offs.

• Developers will be massively unhappy.

This is a claim made without justification. We have seen a large amount of developer commentary for other languages saying that they love having guaranteed memory safety. Those languages have either the same performance cost of these checks or they have better
implementations of the relevant structures. If the former we have evidence that developers are ok with the cost, if the latter we have engineering work to do.

• There will be more justified advice “don’t use standard library - it’s slow”

Fundamentally there is nothing stopping a developer from writing their own insecure code in C++ if they really want to, what we are proposing is that we think that someone using the quintessential “modern c++” should at least attempt to not be trivially memory unsafe.

[DenisYaroshevskiy]

Let me mention that if you make some reasonable “UNSAFE” that gives me back my standard library I would object much less. Not “now stop using standard library and go to pointers” but the same standard library.

[waffles_the_dog]

@DenisYaroshevskiy What behavior changes are you concerned about? The behaviour changes being proposed are all currently UB so a program using them would be incorrect.

Out of curiousity - as the quick-bench output was so far out of wack with my experience, and historical numbers about perf cost of bounds checking - I ran the test cases locally, both as written and again with a much larger test vector to try and reduce any misc. variance and the two versions in the quick-bench example run in identical time. An additional version using an explicit if (i >= arr.size()) __builtin_trap(); also took identical time:

ReduceWithAt_mean                        31412 ns        31392 ns           10
ReduceWithAt_median                      31364 ns        31352 ns           10
ReduceNormal_mean                        31383 ns        31363 ns           10
ReduceNormal_median                      31357 ns        31343 ns           10
ReduceWithMinimalBoundsCheck_mean        31392 ns        31381 ns           10
ReduceWithMinimalBoundsCheck_median      31382 ns        31372 ns           10

There is a large amount of unfounded/cargo-cult fear of bounds checks that I simply do not believe is warranted on modern hardware.

[hansw2000]

This is very interesting.

If I may, I’d like to sneak in a feature request here:

libc++ already implements some of this proposal. Indeed, we recently introduced a safe mode 18 where one can enable the most basic category of checks, i.e. those that do not have an effect on the ABI or on the complexity of operations. The assertion handler can also be customized to meet the needs of various users, similarly to how operator new can be customized via weak linking.

We’d like to override the assertion handler at compile-time so it can be easily inlined, rather than overriding the weak symbol at link-time. That way we could use something with smaller code size than a function call.

I’m also a little bit concerned about the build-time impact of replacing native types with STL types everywhere. The standard library headers are large and growing larger every day. Massively increasing the use of these could be expensive.