[SUGGESTION] Prevent implicit conversion of negative values when passed to unsigned parameters in certain functions

[bluetarpmedia]

Suggestion

Cppfront could prevent implicit conversion of negative values that are passed to unsigned parameters in well-known, memory-related functions, with the goal of eliminating a certain class of security vulnerabilities (see below).

Details

Many memory-related functions have size/length/count parameters of type size_t (or similar), e.g.

(1) void* memcpy(void* dest, const void* src, std::size_t count);
(2) std::vector::reserve(size_type new_cap);
(3) std::string::resize(size_type count);
(4) LPVOID VirtualAlloc(
  [in, optional] LPVOID lpAddress,
  [in]           SIZE_T dwSize,
  [in]           DWORD  flAllocationType,
  [in]           DWORD  flProtect
);

A frequent source of security vulnerabilities (see below) is when a negative signed value is passed as the argument and implicitly converted to a (very large) unsigned value.

Cppfront could have a built-in list of well-known, memory-related functions (memcpy, memset, memmove, malloc, calloc, std::vector members, std::string members, VirtualAlloc, VirtualAllocEx, etc) along with the position(s) of their size/length/count unsigned parameters.

Cppfront could substitute all arguments passed to these size parameters for these well-known functions with a helper function which tests (at runtime) if the value is negative, and if so, calls std::terminate.

For example, Cppfront would transform this Cpp2 code:

get_length: () -> int;  // some function returning signed value

other_process : HANDLE = get_other_process();
length := get_length();
buffer : *void = VirtualAllocEx(other_process, nullptr, length, MEM_COMMIT, PAGE_READWRITE);

into something like:

HANDLE other_process {get_other_process()}; 
void* buffer { VirtualAllocEx(other_process, nullptr, cpp2::safe_unsigned_argument<SIZE_T>(length), MEM_COMMIT, PAGE_READWRITE) }; 

where cpp2::safe_unsigned_argument is something like:

template <typename ResultT, typename T>
constexpr ResultT safe_unsigned_argument(T t)
{
    if constexpr (std::is_unsigned_v<T>) {
        return static_cast<ResultT>(t);
    }

    if (t < 0) {
      assert(!"Negative value will be unsafely converted to an unsigned value.");
      std::terminate();
    }

    return static_cast<ResultT>(t);
}

This will require each documented parameter in the well-known memory functions to have their size type also documented, for use in the template return type ResultT. E.g.

safe_unsigned_argument<size_t>(length)                    // for memcpy
safe_unsigned_argument<std::vector::size_type>(length)    // for std::vector::resize / reserve
safe_unsigned_argument<SIZE_T>(length)                    // for VirtualAlloc

Will your feature suggestion eliminate X% of security vulnerabilities of a given kind in current C++ code?

Yes, this feature will eliminate security vulnerabilities caused by a negative signed value being implicitly converted to unsigned when passed to the size/length/count parameter of well-known (and frequently-used) memory functions. These vulnerabilities can cause stack overflow, buffer overflow and/or denial of service.

Example 4 in CWE-195: Signed to Unsigned Conversion Error has an example of the type of security vulnerability:

char* processNext(char* strm) {
  char buf[512];
  short len = *(short*) strm;   // <-- Untrusted user-input; may be negative
  strm += sizeof(len);
  if (len <= 512) {             // <-- Signed comparison (no warnings); branch is taken with negative `len`
    memcpy(buf, strm, len);     // <-- `len` implicitly converted to size_t, negative value becomes
    process(buf);               //       very large unsigned value causing stack overflow of `buf`
    return strm + len;
  }
  else {
    return -1;
  }
}

See also Example 4 in CWE-131: Incorrect Calculation of Buffer Size.

These CVEs are examples of security vulnerabilities that this feature would eliminate:

• CVE-2021-33316 -- buffer overflow or invalid memory access caused by negative value passed to memcpy
• CVE-2021-33315 -- as above
• CVE-2016-8729 -- exploitable memory corruption and code execution caused by negative value passed to memset
• CVE-2021-37669 -- denial of service caused by negative value passed to std::vector::resize
• CVE-2021-37661 -- denial of service caused by negative value passed to std::vector::reserve
• There are many more CVEs that appear with a simple search of memcpy negative or "memset" negative" but I haven't verified them since I felt the above was sufficient.

Will your feature suggestion automate or eliminate X% of current C++ guidance literature?

Best practice guidance is to enable a high level of warnings in the compiler (and ideally to treat warnings as errors).
For example see:

• Sutter & Alexandrescu, C++ Coding Standards: 101 Rules, Guidelines, and Best Practices (Item 1. Compile cleanly at high warning levels)
• Jason Turner, C++ Weekly - Ep 353 - Implicit Conversions Are Evil

However, in my tests this was not sufficient to produce a warning on the signed/unsigned conversion in all cases. Given this Cpp1 code:

int get_length();
int main()
{
    std::vector<unsigned char> source(100);
    std::vector<unsigned char> dest(100);
    int len = get_length();
    memcpy(dest.data(), source.data(), len);
}

• GCC 12.2 warns on the len conversion with -Wsign-conversion but I couldn't find any other way to make it warn (e.g. not with -Wconversion, -Wextra or -Wall)
• Clang 15 warns on the conversion with -Wsign-conversion (or -Wconversion)
• MSVC 19.33 did not produce a warning even with /W4. This is because the relevant warning C4365 is off by default. The warning can be enabled with /W4 /w44365.

This feature will automate this best practice for all users even if they have not enabled the relevant warning in their compiler, which as per the above is not always obvious or trivial.

Describe alternatives you've considered

An alternative for the cpp2::safe_unsigned_argument helper would be to avoid an explicit return type:

template <typename T>
constexpr auto safe_unsigned_argument(T t)
{
    if constexpr (std::is_unsigned_v<T>) {
        return t;
    }

    if (t < 0) {
      assert(!"Negative value will be unsafely converted to an unsigned value.");
      std::terminate();
    }

    return std::make_unsigned_t<T>(t);
}

This would have the benefit that Cppfront would not need to document the type for each size/count/length parameter in the list of well-known functions. However, it seemed a better design to be explicit in this case.

[jcanizales]

I think a better solution is to just ban that implicit conversion in all cases (which might already be the case in cppfront?), force the user to make it explicit.

[bluetarpmedia]

I think a better solution is to just ban that implicit conversion in all cases (which might already be the case in cppfront?), force the user to make it explicit.

Cppfront currently doesn't prevent implicit conversions, e.g.

main : () -> int = {
    len := -1;
    buf : u32 = len;
}

emits:

[[nodiscard]] auto main() -> int{
    auto len {-1}; 
    cpp2::u32 buf {len}; 
}

But this suggested feature is specifically about preventing negative values being implicitly converted when passed as the size/length/count parameters (unsigned) to well-known and frequently-used memory functions which are the repeated source of CVEs (as per above).

The following is a minimal, contrived example in Cpp2 code, which is currently permitted. For a realistic example, just translate any of the above CVEs into Cpp2 code:

main : () -> int = {
    src : std::vector<u8> = (100);
    dst : std::vector<u8> = (100);
    len := -1;
    std::memcpy(dst.data(), src.data(), len);
}

Unless Herb intends to modify Cppfront and parse headers (and module interfaces?) in order to inspect function declarations (in order to determine if a signed value is passed to a function's unsigned parameter), the best solution I could think of was to have a well-defined list of 'dangerous' memory functions along with their size/length parameters, in order to then insert the safe_unsigned_argument helper function.

[hsutter]

Interesting idea. Let me noodle on this... I can think of a couple of ways to improve this, but I may sit on it for a while thinking about it first before coding something up.

I think a better solution is to just ban that implicit conversion in all cases (which might already be the case in cppfront?), force the user to make it explicit.

If it were a fully native compiler I'd do that (and not allow treating character types like char/char16_t/... as numeric, including ban arithmetic on them). At this point I don't have full control of the built-in types/aliases, but I have some ideas...

Unless Herb intends to [...] parse [today's C++] headers (and module interfaces?)

Jordan Peele well captured my reaction to that, and my 4-letter all-caps answer. :)

[hsutter]

Disclaimer: I dislike even mentioning a gross idea like wrapping fundamental types, it just screams "overhead!" But for the sake of discussion...

Just as a strawman to show potential semantics and to understand the costs/benefits, how far would something like this (Godbolt) go toward addressing this set of concerns, if hypothetically it were ever feasible to emit a signed fundamental type (e.g., int) as a wrapped one (e.g., Signed<int>?

[bluetarpmedia]

Nice work Herb, I like this approach a lot.

For the compile time cases I think the benefit definitely outweighs the cost. I’d rather language level errors than a separate static analysis tool, for example.

Personally I think eliminating these types of CVEs (as above) is also worth the runtime cost for the runtime case.

I can imagine the temptation to cast to remove the compile time error is very high, meaning it would continue to allow those types of CVEs.

[jcanizales]

Clever!

If one were writing that explicit conversion manually in today's C++, asserting before is the right thing to do. To fail fast on unsatisfied preconditions. I don't know if it's in a guideline somewhere, but it's definitely encouraged inside Google with glog's CHECK macros. So with that solution, the right thing would be the default ✔️

The runtime cost is optional, because one can disable the assertion with the NDEBUG macro. So if I know my value is non-negative, I don't have to pay for it. ✔️ ✔️

And if one really wants a reinterpret_cast instead, that's still always an option. ✔️ ✔️ ✔️

What's not to love?

[filipsajdak]

I assume that it is worth reusing what is already done and reuse is_narrowing_v<To, From> for casting operator:

    template<typename U>
        requires (std::is_integral_v<T> && std::is_integral_v<U> 
                  && !is_narrowing_v<U, T> )
    operator U() const { return t; }

https://godbolt.org/z/66h394bvd

[hsutter]

[Edited with additional info]
Quick ack on this, catching up:

What's not to love?

Thanks for the kind words! But some things to maybe not quite love:

• It loses deduction when the type is a function parameter: https://godbolt.org/z/5Woa48ff1 (link updated to include @filipsajdak's suggested requires)
• Supporting int& parameters probably requires a non-const overload that returns by reference (just added this to the above link)... not sure about the impact of adding indirections if this is used a lot.
• As mentioned above, in general I worry about efficiency of wrapping fundamental types. That never seems to go totally smoothly.

Those are some of the reasons I've left this open to keep thinking about, but haven't taken any action yet...

[bluetarpmedia]

It loses deduction when the type is a function parameter

Could this be solved by applying the wrapper to integer literals? Like transforming this:

sadly_this_does_not_work( -1 );

to:

sadly_this_does_not_work( Signed<int32_t>(-1) );

As mentioned above, in general I worry about efficiency of wrapping fundamental types. That never seems to go totally smoothly.

Yeah, I can imagine there would be concerns and push backs about this.

It seems to me that Cpp2 has a philosophy of "safety with performance" but "safety over performance -- if necessary". I'm thinking of bounds checking in particular.

But one of the nice aspects of your solution is that it only ("only...") introduces a bit of extra compile-time work for what should be the common case where the programmer is passing a signed integer to an argument of signed type. Any extra runtime work (and assembly code generated) only happens if the programmer is doing something that they probably didn't mean to (or didn't consider the corner cases).

[bluetarpmedia]

Supporting int& parameters probably requires a non-const overload that returns by reference (just added this to the above link)... not sure about the impact of adding indirections if this is used a lot.

I'm not sure if this a scenario you were thinking of but I created a test to compare passing int32_t or Signed<int32_t> to functions taking int32_t&.

https://godbolt.org/z/hzMhh4oYs

GCC and Clang both produced identical assembly.
MSVC differed slightly by adding 14 extra lines at the top, but was otherwise identical (apart from some changes to label names).

More details in the link.