Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: fix libssh2_store_*() for >u32 inputs #1025

Closed
wants to merge 4 commits into from
Closed

src: fix libssh2_store_*() for >u32 inputs #1025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5646f3
src: fix `libssh2_store_*()` for >u32 inputs
vszakats May 3, 2023
4b23b50
fixups
vszakats May 3, 2023
1a139d2
return failure if stored_len != len
vszakats May 3, 2023
b41cfca
add assert
vszakats May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions src/misc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
#endif

#include <errno.h>
#include <assert.h>

#ifdef WIN32
/* Force parameter type. */
Expand Down Expand Up @@ -254,37 +255,50 @@ void _libssh2_store_u32(unsigned char **buf, uint32_t value)

/* _libssh2_store_str
*/
void _libssh2_store_str(unsigned char **buf, const char *str, size_t len)
int _libssh2_store_str(unsigned char **buf, const char *str, size_t len)
{
_libssh2_store_u32(buf, (uint32_t)len);
if(len) {
memcpy(*buf, str, len);
*buf += len;
uint32_t len_stored = (uint32_t)len;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


_libssh2_store_u32(buf, len_stored);
if(len_stored) {
memcpy(*buf, str, len_stored);
*buf += len_stored;
}

assert(len_stored == len);
return len_stored == len;
}

/* _libssh2_store_bignum2_bytes
*/
void _libssh2_store_bignum2_bytes(unsigned char **buf,
const unsigned char *bytes,
size_t len)
int _libssh2_store_bignum2_bytes(unsigned char **buf,
const unsigned char *bytes,
size_t len)
{
int extraByte = 0;
uint32_t len_stored;
uint32_t extraByte;
const unsigned char *p;

for(p = bytes; len > 0 && *p == 0; --len, ++p) {}

extraByte = (len > 0 && (p[0] & 0x80) != 0);
_libssh2_store_u32(buf, (uint32_t)(len + extraByte));
len_stored = (uint32_t)len;
if(extraByte && len_stored == 0xffffffff)
len_stored--;
_libssh2_store_u32(buf, len_stored + extraByte);

if(extraByte) {
*buf[0] = 0;
*buf += 1;
}

if(len > 0) {
memcpy(*buf, p, len);
*buf += len;
if(len_stored) {
memcpy(*buf, p, len_stored);
*buf += len_stored;
}

assert(len_stored == len);
return len_stored == len;
}

/* Base64 Conversion */
Expand Down
8 changes: 4 additions & 4 deletions src/misc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,10 @@ uint32_t _libssh2_ntohu32(const unsigned char *buf);
libssh2_uint64_t _libssh2_ntohu64(const unsigned char *buf);
void _libssh2_htonu32(unsigned char *buf, uint32_t val);
void _libssh2_store_u32(unsigned char **buf, uint32_t value);
void _libssh2_store_str(unsigned char **buf, const char *str, size_t len);
void _libssh2_store_bignum2_bytes(unsigned char **buf,
const unsigned char *bytes,
size_t len);
int _libssh2_store_str(unsigned char **buf, const char *str, size_t len);
int _libssh2_store_bignum2_bytes(unsigned char **buf,
const unsigned char *bytes,
size_t len);
void *_libssh2_calloc(LIBSSH2_SESSION *session, size_t size);

struct string_buf *_libssh2_string_buf_new(LIBSSH2_SESSION *session);
Expand Down