Bug 2165583

Summary: NetworkManager: unchecked return value in send_arps() in src/devices/nm-device.c
Product: [Other] Security Response Reporter: TEJ RATHI <trathi>
Component: vulnerabilityAssignee: Red Hat Product Security <security-response-team>
Status: CLOSED NOTABUG QA Contact:
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: unspecifiedCC: bgalvani, dfreiber, drow, jburrell, kyoshida, lrintel, nm-team, rkhan, rogbas, sukulkar, thaller, till, vkumar
Target Milestone: ---Keywords: Security
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2023-03-06 17:12:57 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 2165575    

Description TEJ RATHI 2023-01-30 13:19:34 UTC 
A vulnerability, which was classified as problematic, has been found in vicamo NetworkManager. Affected by this issue is the function send_arps of the file src/devices/nm-device.c. The manipulation leads to unchecked return value. The name of the patch is 4da19b89815cbf6e063e39bc33c04fe4b3f789df. It is recommended to apply a patch to fix this issue. VDB-217514 is the identifier assigned to this vulnerability.

Reference:
https://vuldb.com/?id.217514

Upstream patch:
https://github.com/vicamo/NetworkManager/commit/4da19b89815cbf6e063e39bc33c04fe4b3f789df
Comment 1 TEJ RATHI 2023-01-30 13:33:52 UTC 
Upstream Commit:

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/commit/4da19b89815cbf6e063e39bc33c04fe4b3f789df
Comment 3 Thomas Haller 2023-02-09 08:20:33 UTC 
similar to https://bugzilla.redhat.com/show_bug.cgi?id=2165582#c3 .


`g_spawn_async()` is a glib function. It has a certain API (expected behavior).

Like many glib functions, it can return a boolean to indicate success. It will also return a GError if (and only if) the function fails.

As such, unless `g_spawn_async()` had a bug where it violates it's API, the code:

```
    success = g_spawn_async(..., &error);
    if (!success) {
        ...
```
and
```
    g_spawn_async(..., &error);
    if (!error) {
        ...
```
is equivalent.



Note that g_spawn_async() does various assertions on the input, like
```
  g_return_val_if_fail (argv != NULL, FALSE);
```
so there are some code paths where the function fails but would *not* set the error output. These are assertion failures!! So indeed, checking for "success" appears to be slightly more robust However, see https://bugzilla.redhat.com/show_bug.cgi?id=2165582#c3, that we don't gain much of robustness. The caller likely will do
```
    success = g_spawn_async(..., &error);
    if (!success) {
        log("an error happend: %s", error->str); /* << oops. */
```

To be more robust against assertion failures, it would even require
```
    success = g_spawn_async(..., &error);
    if (!success) {
        log("an error happend: %s", error ? error->str : "unknown error");
```
but we usually don't do such things! We must call g_spawn_async() correctly in the first place, and not think what happens after we called it with invalid arguments.




Also, coverity didn't complain about that. It merely said, we used g_spawn_async() differently a number of times, this usage might be wrong. Which is a fine warning and something that the patch addressed (but there is no actual problem with either approaches).