d.mon: Fix Unbounded Source Buffer in main.c file of d.mon module #4260

ShubhamDesai · 2024-08-30T20:31:52Z

This Pull Request addresses the unbounded source buffer issue identified by Coverity Scan (CID: 1270265).

Changes Implemented:

Monitor Name Length Check:
Defined a maximum length for the monitor name (mon) as 256 characters using the MONITOR_NAME_LIMIT macro.
Introduced checks to ensure that the mon variable, which stores the monitor name, does not exceed this defined length.
The check if (mon && strlen(mon) < MONITOR_NAME_LIMIT) ensures that mon is not NULL and that its length does not exceed MONITOR_NAME_LIMIT.

wenzeslaus

While this may address the unbounded source buffer issue, it seems to me that it will just lead to another warning. The mon string goes into functions like list_cmd and somewhere there it is put to another buffer which may or may not be large enough. Maybe the issue needs to be addressed there. I checked this issue in Coverity Scan and it seems to be able to take you there.

ShubhamDesai · 2024-09-02T00:10:44Z

While this may address the unbounded source buffer issue, it seems to me that it will just lead to another warning. The mon string goes into functions like list_cmd and somewhere there it is put to another buffer which may or may not be large enough. Maybe the issue needs to be addressed there. I checked this issue in Coverity Scan and it seems to be able to take you there.

To address this, I have now added two checks:

Primary Check in the Main File: I implemented a length check for mon directly in the main file to ensure it is within safe limits before being passed to other functions.

Additional Checks in Individual Functions: In each of the individual functions, the length of name is also checked to provide an extra layer of safety.

For these checks, instead of using a random number, I used GPATH_MAX as the baseline. The MONITOR_NAME_LIMIT is specifically calculated as GPATH_MAX - strlen("/MONITORS/") - 1 because the "/MONITORS/" directory path is used in one of the functions to create a temporary directory. This calculation ensures that the actual monitor name length is within safe limits, considering the directory structure.

So if the checks pass in the main file still there will be additional checks inside the functions

wenzeslaus · 2024-09-03T13:02:35Z

display/d.mon/stop.c

+ if (strlen(name) >= GPATH_MAX) {
+ G_fatal_error(_("Monitor name <%s> is too long."), name);
+ }
+


Here, it is not constructing any strings, so a check is not needed.

Removed. Done

wenzeslaus · 2024-09-03T13:03:56Z

display/d.mon/main.c

@@ -176,7 +178,7 @@ int main(int argc, char *argv[])
 if (list_flag->answer)
 G_warning(_("Flag -%c ignored"), list_flag->key);
 mon = G_getenv_nofatal("MONITOR");
- if (mon) {
+ if (mon && strlen(mon) < MONITOR_NAME_LIMIT) {


There is actually GNAME_MAX, so you that.

wenzeslaus · 2024-09-03T13:07:26Z

display/d.mon/list.c

+ if (strlen(name) >= GPATH_MAX) {
+ G_fatal_error(_("Monitor name <%s> is too long."), name);
+ }


The check should be done in get_path where the strings are concatenated. See also below for the issue with the check itself.

I initialized the available_space variable with GPATH_MAX - strlen(tmpdir) - 1, ensuring that it accounts for the initial size of tmpdir and the null terminator.
After each concatenation (such as "/", "MONITORS", and the monitor name), I subtracted the length of the appended string from available_space and checked whether there was enough space for the next operation.
This ensures that each part of the path can be appended safely without overflowing the buffer.

wenzeslaus · 2024-09-03T13:12:45Z

display/d.mon/list.c

+ if (strlen(tmpdir) >= GPATH_MAX) {
+ G_fatal_error(_("The path for monitor <%s> is too long."), name);
+ }


This type of check actually does not work. The point is not to limit the length of the string just because. The point is to not overflow the allocated buffer. You need to compute the length before using strcat, for strings which are too long, the behavior is undefined (see strcat doc).

same usage of variable available_space for calculating the buffer availability

display/d.mon/list.c

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

nilason

I have planned to create a new G_strlcat() (local wrapper/implementation of strlcat(), to accompany G_strlcpy()), but have not got the time for that yet.
That would make this fix much less complicated. Please hold on for that (or implement for the time being with posix strlcat() if possible).

nilason · 2024-09-11T16:21:26Z

I have planned to create a new G_strlcat() (local wrapper/implementation of strlcat(), to accompany G_strlcpy())

Addressed by #4304.

ShubhamDesai · 2024-09-11T23:03:08Z

I have planned to create a new G_strlcat() (local wrapper/implementation of strlcat(), to accompany G_strlcpy())

Addressed by #4304.

Thanks. I will use it

nilason · 2024-09-16T06:24:54Z

G_strlcat is now in place.

ShubhamDesai · 2024-09-17T01:10:22Z

G_strlcat is now in place.

Done.

nilason · 2024-09-17T08:16:05Z

G_temp_element() + "/" + "MONITORS" + "/" is guaranteed to fit into a buffer of size GPATH_MAX, so you can (void) the return value of the three first G_strlcat (in both get_path and list_files). The name variable is coming from user, so this should be checked (and G_fatal_error if truncated).
I don't see the need for the change of main.c.

Fix Unbounded Source Buffer

a1045f5

github-actions bot added C Related code is in C module display labels Aug 30, 2024

wenzeslaus reviewed Aug 31, 2024

View reviewed changes

Shubham Vasudeo Desai added 2 commits September 1, 2024 17:34

Added checks in file

c246b03

added checks

aee6c48

ShubhamDesai changed the title ~~display: Fix Unbounded Source Buffer in main.c file of d.mon module~~ d.mon: Fix Unbounded Source Buffer in main.c file of d.mon module Sep 1, 2024

wenzeslaus requested changes Sep 3, 2024

View reviewed changes

Shubham Vasudeo Desai added 3 commits September 5, 2024 19:52

Requested changes

1889258

Updated

523c36c

changes

05e9f80

github-actions bot reviewed Sep 6, 2024

View reviewed changes

display/d.mon/list.c Outdated Show resolved Hide resolved

display/d.mon/list.c Outdated Show resolved Hide resolved

ShubhamDesai and others added 4 commits September 5, 2024 20:45

Update display/d.mon/list.c

9dc9fb3

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Update display/d.mon/list.c

80c514e

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

updated code

f04df52

Merge remote-tracking branch 'origin/issue_41' into issue_41

df4e5cd

nilason requested changes Sep 9, 2024

View reviewed changes

nilason added this to the 8.5.0 milestone Sep 11, 2024

used G_strlcat

de760d8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

d.mon: Fix Unbounded Source Buffer in main.c file of d.mon module #4260

d.mon: Fix Unbounded Source Buffer in main.c file of d.mon module #4260

ShubhamDesai commented Aug 30, 2024

wenzeslaus left a comment

ShubhamDesai commented Sep 2, 2024

wenzeslaus Sep 3, 2024

ShubhamDesai Sep 6, 2024

wenzeslaus Sep 3, 2024

ShubhamDesai Sep 6, 2024

wenzeslaus Sep 3, 2024

ShubhamDesai Sep 6, 2024

wenzeslaus Sep 3, 2024

ShubhamDesai Sep 6, 2024

nilason left a comment

nilason commented Sep 11, 2024

ShubhamDesai commented Sep 11, 2024

nilason commented Sep 16, 2024

ShubhamDesai commented Sep 17, 2024

nilason commented Sep 17, 2024

d.mon: Fix Unbounded Source Buffer in main.c file of d.mon module #4260

Are you sure you want to change the base?

d.mon: Fix Unbounded Source Buffer in main.c file of d.mon module #4260

Conversation

ShubhamDesai commented Aug 30, 2024

wenzeslaus left a comment

Choose a reason for hiding this comment

ShubhamDesai commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilason left a comment

Choose a reason for hiding this comment

nilason commented Sep 11, 2024

ShubhamDesai commented Sep 11, 2024

nilason commented Sep 16, 2024

ShubhamDesai commented Sep 17, 2024

nilason commented Sep 17, 2024