src: fix libssh2_store_*() for >u32 inputs

[vszakats]

_libssh2_store_str() and _libssh2_store_bignum2_bytes() accept
inputs of size_t max, store the size as 32-bit unsigned integer, then
store the complete input buffer.

With inputs larger than UINT_MAX this means the stored size is smaller
than the data that follows it.

This patch truncates the stored data to the stored size, and now returns
a boolean with false if the stored length differs from the requested
one. Also add assert()s for this condition.

This is still not a correct fix, as we now dump consistent, but still
truncated data which is not what the caller wants. In future steps we'll
need to update all callers that might pass large data to this function
to check the return value and handle an error, or make sure to not call
this function with more than UINT_MAX bytes of data.

Ref: c3bcdd8 (2010-04-17)
Ref: ed439a2 (2022-09-29)

Closes #1025

[vszakats]

@MichaelBuckley Would you mind taking a look at this update for _libssh2_store_bignum2_bytes()?

[willco007]

Looking at this more, should these API just fail if len is greater than uint32? Blindly truncating the values is almost assuredly going to break something a bit further down the road.

[vszakats]

I agree they should. Though it would be a substantial update, there are 95 callers for _libssh2_store_str(). The 2 callers for the other function is less of an problem. (UPDATE: They are all low-level places, so the changes necessary are non-trivial, even for the 2 callers.)

As a first step I've updated them to return non-zero if the stored len matches the input.

[willco007]

Having them return non-void is a good first step.

[MichaelBuckley]

I've only thought about this for a few minutes, so I could be wrong, but would it not make more sense for all of the _libssh2_store_* functions to all use uint32_t instead of size_t? It does push the burden of ensuring that the string fits within UINT_MAX bytes to the call sites, but having the functions return a failure at runtime if it doesn't means that the call sites have to deal with it anyway. By updating the functions to take uint32_t instead, it's not only more explicit that string lengths are limited to 32-bits, but we'll also get compiler warnings about potential truncation instead of runtime errors.

[vszakats]

@MichaelBuckley That's also an option. As I understand, we're heading to support size_t for all sizes in the API, so most callers will have to deal with this issue on way or another. Moving to uint32_t with these APIs would mean truncating silently on the caller side. Which also isn't ideal, unless those callers previously made sure that any large input is rejected early or handled correctly down the line. Also with _libssh2_store_bignum2_bytes() specifically, it might accept UINT_MAX - 1 in some cases (due to extraByte), so an uint32_t might still overflow.

[bagder]

I think this should check for len > UINT_MAX and return a FALSE if so, as then it can't store the string properly. Could possibly also do with an assert() to get caught better in a debug build.

[vszakats]

I've added assert()s, and added returning FALSE earlier, though it still stores as much as it can, as no caller checks for the return value. I'm not ready adding these checks as it involves digging into low-level logic and risks adding leaks and worse bugs.

Earlier I added quite a few casts for _libssh2_store_u32() calls in 02f2700. The potential problem with these is similar, the caller code might continue to assume full length, while the length stored on the wire is u32.

[vszakats]

I've so far added a return value and assert() to both functions. The data is still stored, but limited to UINT_MAX. Ultimately I think this cannot be fully fixed without touching all callers that may pass large data to these functions, but I'd suggest tackling those in separate commits.

Any objections for committing this, or suggestions what else to improve before?

[rmsh1216]

Is this issue fixed?

[vszakats]

No, these functions now correctly return an error in case of an overflow, but callers are not checking for errors yet.