11
29
Fork
You've already forked ipmitool
33

ipmievd: Eliminate a possible command line overrun #27

Merged
Contributor
Copy link

While analyzing the source code while eliminating compile time
warnings I noticed there is a possible buffer overrun while parsing
the ipmievd command line arguments.

The compile time warnings flagged the direct use of 'argv' in the
strncpy function call. That same line uses an unbounded strlen()
function call to read the argv array. It is possible for a well
crafted pidfile parameter to cause unexpected behavior.

The code change here eliminates a compiler warning about using argv
directly in the strncpy function call.
It also removes the buffer overrun by only reading the same number of
characters as can be stored in the pidfile array.
It also removes a compile time warning about using a -C2X level
structure initialization feature (i.e. recv = {}).

Signed-off-by: Johnathan Mantey johnathanx.mantey@intel.com

While analyzing the source code while eliminating compile time warnings I noticed there is a possible buffer overrun while parsing the ipmievd command line arguments. The compile time warnings flagged the direct use of 'argv' in the strncpy function call. That same line uses an unbounded strlen() function call to read the argv array. It is possible for a well crafted pidfile parameter to cause unexpected behavior. The code change here eliminates a compiler warning about using argv directly in the strncpy function call. It also removes the buffer overrun by only reading the same number of characters as can be stored in the pidfile array. It also removes a compile time warning about using a -C2X level structure initialization feature (i.e. recv = {}). Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
src/ipmievd.c Outdated
@ -732,2 +733,4 @@
}
else if (strcasecmp(argv[i], "pidfile=") == 0) {
char *pidArg = argv[i]+sizeof("pidfile=")-1;
char pidArgLen = strnlen(pidArg, 63);
First-time contributor
Copy link

I am kind of partial to sizeof(array) rather than the integer size of the array. That way if it changes in the future, everything is coherent.

size_t pidArgLen = strnlen(pidArg, sizeof(pidfile)-1);
memset(pidfile, 0, sizeof(pidfile);
strncpy(pidfile, pidArg, pidArgLen);

I am kind of partial to sizeof(array) rather than the integer size of the array. That way if it changes in the future, everything is coherent. size_t pidArgLen = strnlen(pidArg, sizeof(pidfile)-1); memset(pidfile, 0, sizeof(pidfile); strncpy(pidfile, pidArg, pidArgLen);
Author
Contributor
Copy link

Updated per your suggestion. Thank you.

Updated per your suggestion. Thank you.
Howitzer105mm force-pushed bugfix/26-eliminate-possible-ipmid-command-line-overrun from de5631f862 to 293fde17df 2024年02月13日 00:28:04 +01:00 Compare
vmauery approved these changes 2024年02月13日 00:41:22 +01:00
Dismissed
src/ipmievd.c Outdated
@ -423,3 +423,3 @@
{
struct ipmi_addr addr;
struct ipmi_recv recv = {};
struct ipmi_recv recv;

As in another PR:

  1. Don't mix different changes in one commit
  2. Please leave this struct init as is, update the compilation flags instead to satisfy clang if needed
As in another PR: 1. Don't mix different changes in one commit 2. Please leave this struct init as is, update the compilation flags instead to satisfy clang if needed
Author
Contributor
Copy link

removed.

removed.
src/ipmievd.c Outdated
@ -702,3 +703,3 @@
struct sigaction act;
memset(pidfile, 0, 64);
memset(pidfile, 0, sizeof(pidfile));

This one is good! While at it, I would also change the global pidfile to static.

This one is good! While at it, I would also change the global `pidfile` to `static`.
Author
Contributor
Copy link

Made pidfile static.

Made pidfile static.
src/ipmievd.c Outdated
@ -734,3 +735,1 @@
memset(pidfile, 0, 64);
strncpy(pidfile, argv[i]+8,
__min(strlen((const char *)(argv[i]+8)), 63));
const char *pidArg = argv[i]+sizeof("pidfile=")-1;

No, please never do this! Don't copy&paste strings (or whatever values). Define a constant (preferably) or at least a macro.

No, please never do this! Don't copy&paste strings (or whatever values). Define a constant (preferably) or at least a macro.
Author
Contributor
Copy link

I don't quite understand your point.
Original code:
memset(pidfile, 0, 64);
strncpy(pidfile, argv[i]+8,
__min(strlen((const char *)(argv[i]+8)), 63));
// sets array to 0
// performs an unbounded strlen traversal, which if it doesn't crash gets truncated to 63 chars
// then performs a string copy

New code:
const char *pidArg = argv[i]+sizeof("pidfile=")-1;
size_t pidArgLen = strnlen(pidArg, sizeof(pidfile)-1);
memset(pidfile, 0, sizeof(pidfile));
strncpy(pidfile, pidArg, pidArgLen);

// points to the first char after "pidfile="
// does a length bounded check of the strings length
// clears the pidfile contents, same as before
// performs a string copy, same as before, except it is prevented from exceeding 63 chars.

What "this" is to "never be done"?

I don't quite understand your point. Original code: memset(pidfile, 0, 64); strncpy(pidfile, argv[i]+8, __min(strlen((const char *)(argv[i]+8)), 63)); // sets array to 0 // performs an unbounded strlen traversal, which if it doesn't crash gets truncated to 63 chars // then performs a string copy New code: const char *pidArg = argv[i]+sizeof("pidfile=")-1; size_t pidArgLen = strnlen(pidArg, sizeof(pidfile)-1); memset(pidfile, 0, sizeof(pidfile)); strncpy(pidfile, pidArg, pidArgLen); // points to the first char after "pidfile=" // does a length bounded check of the strings length // clears the pidfile contents, same as before // performs a string copy, same as before, except it is prevented from exceeding 63 chars. What "this" is to "never be done"?

I see that you've fixed it now. Thank you. And please excuse me if I sometimes sound harsh, rude or anything like that. English isn't my native language and I may be missing some cultural aspects. No offense is ever intended and I'm really grateful for your contribution.

I see that you've fixed it now. Thank you. And please excuse me if I sometimes sound harsh, rude or anything like that. English isn't my native language and I may be missing some cultural aspects. No offense is ever intended and I'm really grateful for your contribution.
src/ipmievd.c Outdated
@ -736,1 +735,3 @@
__min(strlen((const char *)(argv[i]+8)), 63));
const char *pidArg = argv[i]+sizeof("pidfile=")-1;
size_t pidArgLen = strnlen(pidArg, sizeof(pidfile)-1);
memset(pidfile, 0, sizeof(pidfile));

Isn't this excess? We've already cleared the whole pidfile array before in line 705. Why do it again? strncpy() will anyway either copy the NUL terminator from the original string (if it's shorter than 63 bytes) or will write 63 bytes, and then the 64th NUL byte that we've created with memset in line 705 will serve as a terminator.

Isn't this excess? We've already cleared the whole `pidfile` array before in line 705. Why do it again? `strncpy()` will anyway either copy the NUL terminator from the original string (if it's shorter than 63 bytes) or will write 63 bytes, and then the 64th NUL byte that we've created with `memset` in line 705 will serve as a terminator.
Author
Contributor
Copy link

If this is excess now, it was excess before.
Do you want this changed?

If this is excess now, it was excess before. Do you want this changed?
While analyzing the source code while eliminating compile time
warnings I noticed there is a possible buffer overrun while parsing
the ipmievd command line arguments.
The compile time warnings flagged the direct use of 'argv' in the
strncpy function call. That same line uses an unbounded strlen()
function call to read the argv array. It is possible for a well
crafted pidfile parameter to cause unexpected behavior.
The code change here eliminates a compiler warning about using argv
directly in the strncpy function call.
It also removes the buffer overrun by only reading the same number of
characters as can be stored in the pidfile array.
It also removes a compile time warning about using a -C2X level
structure initialization feature (i.e. recv = {}).
Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

Howitzer105mm changed title from (削除) Eliminate a possible ipmievd command line overrun (削除ここまで) to ipmievd: Eliminate a possible command line overrun 2024年02月21日 22:48:31 +01:00
vmauery requested changes 2024年02月23日 00:18:05 +01:00
Dismissed
src/ipmievd.c Outdated
@ -734,3 +734,1 @@
memset(pidfile, 0, 64);
strncpy(pidfile, argv[i]+8,
__min(strlen((const char *)(argv[i]+8)), 63));
const char *pidArg = argv[i]+sizeof("pidfile=")-1;
First-time contributor
Copy link

I think Alexander was talking about the "pidfile=" string. It is used as part of the strcasecmp() and then again in the sizeof(). I think he wants a #define for that string.

I think Alexander was talking about the "pidfile=" string. It is used as part of the strcasecmp() and then again in the sizeof(). I think he wants a #define for that string.

Exactly. Thanks.

Exactly. Thanks.
src/ipmievd.c Outdated
@ -737,0 +739,4 @@
sizeof(pidfile) - 1);
return (-1);
}
memset(pidfile, 0, sizeof(pidfile));
First-time contributor
Copy link

And this can be removed. Sure it was there before, but there is no need for it.

And this can be removed. Sure it was there before, but there is no need for it.
While analyzing the source code while eliminating compile time
warnings I noticed there is a possible buffer overrun while parsing
the ipmievd command line arguments.
The compile time warnings flagged the direct use of 'argv' in the
strncpy function call. That same line uses an unbounded strlen()
function call to read the argv array. It is possible for a well
crafted pidfile parameter to cause unexpected behavior.
The code change here eliminates a compiler warning about using argv
directly in the strncpy function call.
It also removes the buffer overrun by only reading the same number of
characters as can be stored in the pidfile array.
It also removes a compile time warning about using a -C2X level
structure initialization feature (i.e. recv = {}).
Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
Author
Contributor
Copy link

Thanks for clarifying Vernon.

Thanks for clarifying Vernon.
vmauery requested changes 2024年02月23日 20:27:41 +01:00
Dismissed
src/ipmievd.c Outdated
@ -84,3 +84,3 @@
#define WARNING_THRESHOLD 80
#define DEFAULT_PIDFILE _PATH_RUN "ipmievd.pid"
char pidfile[64];
#define PIDFILE_OPT_LEN (sizeof("pidfile=") - 1)
First-time contributor
Copy link

this is still a copy of the string "pidfile="

#define PIDFILE_OPT "pidfile="
#define PIDFILE_OPT_LEN (sizeof(PIDFILE_OPT)-1)

And then down below in the strcasecmp, use PIDFILE_OPT again.

this is still a copy of the string "pidfile=" #define PIDFILE_OPT "pidfile=" #define PIDFILE_OPT_LEN (sizeof(PIDFILE_OPT)-1) And then down below in the strcasecmp, use PIDFILE_OPT again.

You're right, but at least this way we only have a single literal. The compiler will most probably merge the strings and they won't occupy extra space. Anyway, my initial concern wasn't much about the space but rather about the potential difference in the copy-pasted literals. That looks mitigated now, and that's fine by me.

You're right, but at least this way we only have a single literal. The compiler will most probably merge the strings and they won't occupy extra space. Anyway, my initial concern wasn't much about the space but rather about the potential difference in the copy-pasted literals. That looks mitigated now, and that's fine by me.
Author
Contributor
Copy link

Sure, but now the parsing code is non-uniform:

stcasecmp ("timeout=")
strcasecmp("daemon")
strcasecmp(PIDFILE_OPT)

Why would you do this?

Sure, but now the parsing code is non-uniform: stcasecmp ("timeout=") strcasecmp("daemon") strcasecmp(PIDFILE_OPT) Why would you do this?
First-time contributor
Copy link

Sure, but now the parsing code is non-uniform:

stcasecmp ("timeout=")
strcasecmp("daemon")
strcasecmp(PIDFILE_OPT)

Why would you do this?

So changing the string in one place doesn't lead to a bug in the other.

> Sure, but now the parsing code is non-uniform: > > stcasecmp ("timeout=") > strcasecmp("daemon") > strcasecmp(PIDFILE_OPT) > > Why would you do this? So changing the string in one place doesn't lead to a bug in the other. 
While analyzing the source code while eliminating compile time
warnings I noticed there is a possible buffer overrun while parsing
the ipmievd command line arguments.
The compile time warnings flagged the direct use of 'argv' in the
strncpy function call. That same line uses an unbounded strlen()
function call to read the argv array. It is possible for a well
crafted pidfile parameter to cause unexpected behavior.
The code change here eliminates a compiler warning about using argv
directly in the strncpy function call.
It also removes the buffer overrun by only reading the same number of
characters as can be stored in the pidfile array.
It also removes a compile time warning about using a -C2X level
structure initialization feature (i.e. recv = {}).
Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>

Sure, but now the parsing code is non-uniform:

stcasecmp ("timeout=")
strcasecmp("daemon")
strcasecmp(PIDFILE_OPT)

Why would you do this?

If literals for the other options are also used multiple times, then it would be nice to replace them with macros too. Not necessarily in this PR though.

> Sure, but now the parsing code is non-uniform: > > stcasecmp ("timeout=") > strcasecmp("daemon") > strcasecmp(PIDFILE_OPT) > > Why would you do this? If literals for the other options are also used multiple times, then it would be nice to replace them with macros too. Not necessarily in this PR though.

Another small notice/hint. With codeberg (as well as with GItHub and modern Bitbucket) you don't need to stack the amendments as separate commits. You may use git commit --amend and then git push -f to replace your PR's branch. When you have just a single commit like in this PR, it doesn't make much difference, but when there are multiple meaningful (not amendment) commits in a single PR, then squashing them up together in your work tree makes it easer for me to just 'rebase and fast-forward' your changes when merging a PR.

Another small notice/hint. With codeberg (as well as with GItHub and modern Bitbucket) you don't need to stack the amendments as separate commits. You may use `git commit --amend` and then `git push -f` to replace your PR's branch. When you have just a single commit like in this PR, it doesn't make much difference, but when there are multiple meaningful (not amendment) commits in a single PR, then squashing them up together in your work tree makes it easer for me to just 'rebase and fast-forward' your changes when merging a PR.
Sign in to join this conversation.
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
IPMITool/ipmitool!27
Reference in a new issue
IPMITool/ipmitool
No description provided.
Delete branch "Howitzer105mm/ipmitool:bugfix/26-eliminate-possible-ipmid-command-line-overrun"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?