nasa / nasa/CF

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

Open
#484 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
C
Stars
120
Forks
77
Avg merge
7d 18h
Merged PRs (30d)
3

Description

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;
+ }

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read CF_CFDP_GetMoveTarget() in fsw/src/cf_cfdp.c, focusing on the dest_buf and dest_size boundary cases. Reproduce the zero-size call described in the issue, then verify that invalid buffers are not returned and that normal formatting and truncation behavior remain intact.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
networking
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.