Forum Stats

  • 3,824,872 Users
  • 2,260,435 Discussions
  • 7,896,336 Comments

Discussions

True-Positive (minor) Read-Beyond Array Bug

User_VA11D
User_VA11D Member Posts: 1 Green Ribbon

summary

There's a minor read-beyond the length of an array bug in 18.1.40 (found by standard gcc 8 compiler warnings). It's a true-positive read beyond an array, probably either static memory or heap memory. I don't think it's user-facing, though, as it's copying to an array that appears to be a null-terminated c-string.

bug

In the function inside src/repmgr/repmgr_net.c:

static int__repmgr_load_ssl_certificates(env, ssl_ctx)

on line 2546, the function declares:

const char *passwd = "";

while on line 2579 it does:

memcpy(rmgr_ssl_conf->repmgr_key_file_passwd, passwd, sizeof(passwd) + 1);

sizeof(passwd) is incorrect here, as const char *passwd = ""; is a pointer, not a C-style array. As such, sizeof(passwd) usually returns 8 on 64 bit platforms and 4 on 32 bit platforms, meaning this reads either 5 or 9 bytes in total from the string instead of the 2 bytes intended.

possible intention of code

The intention of the code was probably

memcpy(rmgr_ssl_conf->repmgr_key_file_passwd, passwd, strlen(passwd) + 1);

The strlen + 1 could also be replaced with a static 2 instead, if it impacts performance for some reason, but given how it statically knew the length of the string, I would assume gcc/other vendors would also replace the strlen with a constant 1.

probable reason for code

I think what the code is intending to do, though, is set the first character to be a null terminator... which isn't needed as on line 2578 it already memset the storage to 0, which is the same as '\0' assuming normal 8 bit bytes.

Line 2579 therefore most likely is dead code, and the passwd variable can most likely be removed entirely from the function, and the comment updated to reflect that the function dependency is expecting a null-terminated string.

meh

This is more of an annoyance as it's flagging in my gcc build and will probably flag when I go to run a static analyzer on my code later. I don't think it's exploitable, and it shouldn't crash cause it's merely a read rather than a write.

disclaimer

Note: This patch may have unintended consequences and no warranty is given. :D