Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions avs/runtime/freestanding/src/string/str.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ size_t strlen(const char *s) {
return (size_t)(p - s);
}

__attribute__((deprecated("strcpy() is unsafe: no bounds checking. Use strlcpy() instead.")))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Placing __attribute__((deprecated(...))) on the function definition in str.c is ineffective for warning external callers. In C, deprecation warnings are triggered based on the function declaration visible at the call site. Since other source files include the header file (e.g., string.h or linxisa_libc.h) which does not have this attribute, they will not receive any compiler warnings when calling strcpy.

To make the deprecation warning effective, please add the __attribute__((deprecated(...))) attribute to the declaration of strcpy in avs/runtime/freestanding/include/string.h (and any other relevant header files).

char *strcpy(char *dest, const char *src) {
char *d = dest;
while ((*d++ = *src++));
Expand Down
82 changes: 82 additions & 0 deletions tests/test_invariant_str.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#include <check.h>
#include <stdlib.h>
#include <string.h>

/* Import the actual strcpy from the freestanding runtime */
extern char *strcpy(char *dest, const char *src);

#define DEST_SIZE 16
#define GUARD_BYTE 0xAA

START_TEST(test_strcpy_buffer_overflow_detection)
{
/* Invariant: Buffer reads/writes must never exceed declared destination length */
const char *payloads[] = {
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", /* 2.5x overflow (40 chars) */
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", /* 10x overflow (160 chars) */
"AAAAAAAAAAAAAAA", /* Boundary: exactly fits (15 chars + null) */
"short" /* Valid input */
};
int num_payloads = sizeof(payloads) / sizeof(payloads[0]);

for (int i = 0; i < num_payloads; i++) {
/* Allocate buffer with guard bytes to detect overflow */
unsigned char *mem = malloc(DEST_SIZE + 16);
ck_assert_ptr_nonnull(mem);

/* Fill entire region with guard bytes */
memset(mem, GUARD_BYTE, DEST_SIZE + 16);

char *dest = (char *)(mem + 8); /* Buffer in middle */
size_t src_len = strlen(payloads[i]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The test allocates a fixed-size buffer of DEST_SIZE + 16 (32 bytes) and writes payloads of up to 160 characters into it using strcpy. This causes a massive out-of-bounds write on the heap, which will corrupt heap metadata and likely crash the test runner with a segmentation fault or abort.

To prevent heap corruption and ensure the test runs safely, we should dynamically allocate the buffer based on the actual payload length, ensuring there is always enough allocated space to hold the entire copied string plus the guard bytes.

        size_t src_len = strlen(payloads[i]);
        size_t max_len = (src_len > DEST_SIZE) ? src_len : DEST_SIZE;
        size_t alloc_size = 8 + max_len + 8;
        unsigned char *mem = malloc(alloc_size);
        ck_assert_ptr_nonnull(mem);
        memset(mem, GUARD_BYTE, alloc_size);
        char *dest = (char *)(mem + 8);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orbisai0security can you address code review comments?


/* Call the vulnerable function */
strcpy(dest, payloads[i]);

/* Check if overflow occurred by examining guard bytes after buffer */
if (src_len >= DEST_SIZE) {
/* Overflow WILL occur with unbounded strcpy - this test documents the vulnerability */
/* A safe implementation would truncate or reject; unbounded strcpy corrupts memory */
ck_assert_msg(mem[DEST_SIZE + 8] == GUARD_BYTE,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The assertion checks if the guard byte is equal to GUARD_BYTE when src_len >= DEST_SIZE. However, since strcpy is unbounded and will overflow the buffer, the guard byte will be overwritten (corrupted). Therefore, this assertion will always fail, causing the test suite to fail on CI.

If the goal of this test is to document that the vulnerability indeed exists (as indicated by the comment on line 38), the assertion should check that the guard byte was corrupted (i.e., != GUARD_BYTE).

            ck_assert_msg(mem[DEST_SIZE + 8] != GUARD_BYTE,

"Buffer overflow detected: guard byte corrupted for payload %d (len=%zu)",
i, src_len);
} else {
/* Valid input should not overflow */
ck_assert_msg(mem[DEST_SIZE + 8] == GUARD_BYTE,
"Unexpected overflow for valid payload %d", i);
}

free(mem);
}
}
END_TEST

Suite *security_suite(void)
{
Suite *s;
TCase *tc_core;

s = suite_create("Security");
tc_core = tcase_create("Core");

tcase_add_test(tc_core, test_strcpy_buffer_overflow_detection);
suite_add_tcase(s, tc_core);

return s;
}

int main(void)
{
int number_failed;
Suite *s;
SRunner *sr;

s = security_suite();
sr = srunner_create(s);

srunner_run_all(sr, CK_NORMAL);
number_failed = srunner_ntests_failed(sr);
srunner_free(sr);

return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
}
Loading