Unsafe Handling of Zero-Length Destination Buffer in CF_CFDP_GetMoveTarget() Can Trigger Out-of-Bounds Read

Leggi CF_CFDP_GetMoveTarget() in fsw/src/cf_cfdp.c, concentrandoti sui casi limite di dest_buf e dest_size. Riproduci la chiamata con dimensione zero descritta nell’issue, quindi verifica che non vengano restituiti buffer non validi e che il normale comportamento di formattazione e troncamento rimanga invariato.

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
CF_CFDP_GetMoveTarget() may return dest_buf even when dest_size == 0.

In that case, the function calls snprintf(dest_buf, dest_size, "%s/%s", ...), but with a zero-sized destination buffer no bytes are written, so dest_buf is not guaranteed to contain a terminating '\0'. The function still returns dest_buf as though it were a valid C string.

If a caller later treats the returned pointer as a normal string (for example with %s, strlen, logging helpers, or path/file APIs), this can trigger undefined behavior and may result in an out-of-bounds read.

This appears to be a boundary-condition / robustness issue with potential security relevance, rather than a confirmed high-severity vulnerability by itself.

To Reproduce

1. Use logic equivalent to CF_CFDP_GetMoveTarget() with a non-empty dest_dir.
2. Pass a valid dest_buf pointer with dest_size == 0.
3. Observe that the function returns dest_buf.
4. Observe that no bytes were written to the buffer, so it is not guaranteed to be a NUL-terminated C string.
5. Pass the returned pointer to a string-consuming API such as printf("%s", ...), strlen(), or similar.
6. Depending on surrounding memory contents and runtime environment, this can trigger undefined behavior and may read past the object boundary.

A minimal reproduction is shown below:

#include <stdio.h>
#include <string.h>
#include <stddef.h>

/* minimal reproduction of logic in CF_CFDP_GetMoveTarget() */
const char *GetMoveTarget(const char *dest_dir, const char *subject_file, char *dest_buf, size_t dest_size)
{
    const char *result;
    const char *filename;
    int         dest_path_len;

    result = NULL;
    if (dest_dir != NULL && dest_dir[0] != 0)
    {
        filename = strrchr(subject_file, '/');
        if (filename == NULL)
        {
            filename = subject_file;
        }
        else
        {
            ++filename;
        }

        dest_path_len = snprintf(dest_buf, dest_size, "%s/%s", dest_dir, filename);

        if (dest_path_len >= (int)dest_size && dest_size > 2)
        {
            dest_buf[dest_size - 2] = '$';
        }

        result = dest_buf;
    }

    return result;
}

int main(void)
{
    char buf0[1] = { 'X' };
    const char *r0 = GetMoveTarget("d", "longname", buf0, 0);

    /* r0 is returned, but buf0 was not written and is not guaranteed
       to be a valid NUL-terminated string */
    printf("returned=%p\n", (void *)r0);
    printf("bounded print: %.8s\n", r0); /* still undefined behavior */

    return 0;
}

Expected behavior
The function should not return a pointer that is not guaranteed to reference a valid NUL-terminated C string.

Safer behavior would be one of:

• return NULL when dest_buf == NULL or dest_size == 0, or
• ensure that any non-NULL returned buffer is always initialized as a valid empty string when dest_size > 0.

For example, a defensive fix could:

• return NULL if dest_buf == NULL or dest_size == 0
• initialize dest_buf[0] = '\0' before formatting when dest_size

Code Analysis
Location 1: fsw/src/cf_cfdp.c (lines 2209-2242)
CF_CFDP_GetMoveTarget() returns dest_buf even when dest_size is 0, which may yield a non-NUL-terminated string

Suggested Fix:

- const char *CF_CFDP_GetMoveTarget(const char *dest_dir, const char *subject_file, char *dest_buf, size_t dest_size)
- {
-     const char *result;
-     const char *filename;
-     int         dest_path_len;
- 
-     result = NULL;
-     if (dest_dir != NULL && dest_dir[0] != 0)
-     {
-         filename = strrchr(subject_file, '/');
-         if (filename == NULL)
-         {
-             filename = subject_file; /* not in a dir */
-         }
-         else
-         {
-             ++filename;
-         }
- 
-         dest_path_len = snprintf(dest_buf, dest_size, "%s/%s", dest_dir, filename);
-         if (dest_path_len >= dest_size && dest_size > 2)
-         {
-             /* Mark character before zero terminator to indicate truncation */
-             dest_buf[dest_size - 2] = CF_FILENAME_TRUNCATED;
- 
-             /* Send event describing that the path would be truncated */
-             CFE_EVS_SendEvent(CF_EID_INF_CFDP_BUF_EXCEED, CFE_EVS_EventType_INFORMATION,
-                               "CF: destination has been truncated to %s", dest_buf);
-         }
- 
-         result = dest_buf;
-     }
- 
-     return result;
- }
+ const char *CF_CFDP_GetMoveTarget(const char *dest_dir, const char *subject_file, char *dest_buf, size_t dest_size)
+ {
+     const char *result;
+     const char *filename;
+     int         dest_path_len;
+ 
+     result = NULL;
+ 
+     /* dest_buf must be a writable buffer with space for a NUL terminator */
+     if (dest_buf == NULL || dest_size == 0)
+     {
+         return NULL;
+     }
+ 
+     /* Ensure a safe empty string regardless of snprintf outcome */
+     dest_buf[0] = 0;
+ 
+     if (dest_dir != NULL && dest_dir[0] != 0)
+     {
+         filename = strrchr(subject_file, '/');
+         if (filename == NULL)
+         {
+             filename = subject_file; /* not in a dir */
+         }
+         else
+         {
+             ++filename;
+         }
+ 
+         dest_path_len = snprintf(dest_buf, dest_size, "%s/%s", dest_dir, filename);
+         if (dest_path_len >= (int)dest_size && dest_size > 2)
+         {
+             /* Mark character before zero terminator to indicate truncation */
+             dest_buf[dest_size - 2] = CF_FILENAME_TRUNCATED;
+ 
+             /* Send event describing that the path would be truncated */
+             CFE_EVS_SendEvent(CF_EID_INF_CFDP_BUF_EXCEED, CFE_EVS_EventType_INFORMATION,
+                               "CF: destination has been truncated to %s", dest_buf);
+         }
+ 
+         result = dest_buf;
+     }
+ 
+     return result;
+ }