Conversation
Signed-off-by: Scott R. Shinn <scott@atomicorp.com>
There was a problem hiding this comment.
Pull request overview
This pull request addresses issue #2056, which appears to be a crash caused by null pointer dereferences when processing malformed syscheck messages with incomplete checksums. The fix adds null checks and default values for MD5 and SHA1 hash strings, and includes a regression test to verify the fix.
Changes:
- Added null pointer checks in syscheck.c to handle malformed checksum inputs that don't contain MD5/SHA1 values
- Created a regression test to reproduce the crash scenario with malformed input
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/analysisd/decoders/syscheck.c | Adds null pointer checks for newfilemd5 and newfilesha1, setting them to "Unknown" when null to prevent crashes |
| src/tests/regressions/issue_2056_repro_crash.c | New regression test that reproduces the crash scenario with malformed checksum inputs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DecodeSyscheck(&lf); | ||
| printf("Survived SHA256 Input!\n"); | ||
| fflush(stdout); | ||
|
|
There was a problem hiding this comment.
Memory leak in the second test case. The test allocates memory for lf.log via strdup at line 52, but never frees it before the function returns. The cleanup section for test case 1 (lines 44-47) includes proper freeing, but test case 2 lacks this cleanup.
| /* Cleanup Test Case 2 resources */ | |
| free(lf.log); | |
| if (lf.full_log) { | |
| free(lf.full_log); | |
| } |
| printf("Attempting to call DecodeSyscheck with malformed input (Regression check)...\n"); | ||
| fflush(stdout); | ||
| DecodeSyscheck(&lf); | ||
| printf("Survived Malformed Input!\n"); | ||
| fflush(stdout); | ||
|
|
||
| /* Cleanup Test Case 1 resources */ | ||
| free(lf.log); | ||
| if(lf.full_log) free(lf.full_log); | ||
| /* Note: other fields leaked for simplicity, just zeroing struct for next test */ | ||
| memset(&lf, 0, sizeof(Eventinfo)); | ||
|
|
||
| /* Test Case 2: Full Checksum with SHA256 */ | ||
| /* Format: 1:2:3:4:MD5:SHA1:SHA256 */ | ||
| char *input_msg_full = "ignore:ignore:ignore:ignore:MYMD5:MYSHA1:MYSHA256 /tmp/testfile_sha256"; | ||
| lf.log = strdup(input_msg_full); | ||
| lf.location = "localhost"; | ||
|
|
||
| printf("Attempting to call DecodeSyscheck with SHA256 input...\n"); | ||
| fflush(stdout); | ||
| DecodeSyscheck(&lf); | ||
| printf("Survived SHA256 Input!\n"); | ||
| fflush(stdout); |
There was a problem hiding this comment.
Test lacks proper assertions and error checking. The test calls DecodeSyscheck but doesn't verify the return value or check if the function behaves correctly. It only checks that the program doesn't crash. A proper test should verify that the function returns the expected result and that output fields are set correctly, especially since this is meant to test that the fix properly handles malformed input.
| @@ -0,0 +1,62 @@ | |||
| #include "shared.h" | |||
There was a problem hiding this comment.
Missing copyright header. All other test files in src/tests include a copyright header at the top (e.g., test_os_crypto.c:1-8, test_shared.c:1-8). This file should include the same copyright notice for consistency with the codebase convention.
| int main() | ||
| { | ||
| /* Initialize Syscheck DB (Allocate sdb.syscheck_dec) */ | ||
| SyscheckInit(); | ||
|
|
||
| Eventinfo lf; | ||
| memset(&lf, 0, sizeof(Eventinfo)); | ||
|
|
||
| /* Initialize Mock Config */ | ||
| Config.syscheck_alert_new = 1; /* Crucial to trigger the vulnerable path */ | ||
|
|
||
| /* Malformed input: Fewer than 6 tokens in checksum part */ | ||
| /* Format: checksum filename */ | ||
| /* Checksum expected: c_sum:md5:sha1... based on ":" parsing */ | ||
| /* We provide a short checksum string */ | ||
| char *input_msg = "badchecksum:1234:short /tmp/testfile"; | ||
|
|
||
| /* Setup Eventinfo */ | ||
| lf.log = strdup(input_msg); | ||
| lf.location = "localhost"; | ||
|
|
||
| printf("Attempting to call DecodeSyscheck with malformed input (Regression check)...\n"); | ||
| fflush(stdout); | ||
| DecodeSyscheck(&lf); | ||
| printf("Survived Malformed Input!\n"); | ||
| fflush(stdout); | ||
|
|
||
| /* Cleanup Test Case 1 resources */ | ||
| free(lf.log); | ||
| if(lf.full_log) free(lf.full_log); | ||
| /* Note: other fields leaked for simplicity, just zeroing struct for next test */ | ||
| memset(&lf, 0, sizeof(Eventinfo)); | ||
|
|
||
| /* Test Case 2: Full Checksum with SHA256 */ | ||
| /* Format: 1:2:3:4:MD5:SHA1:SHA256 */ | ||
| char *input_msg_full = "ignore:ignore:ignore:ignore:MYMD5:MYSHA1:MYSHA256 /tmp/testfile_sha256"; | ||
| lf.log = strdup(input_msg_full); | ||
| lf.location = "localhost"; | ||
|
|
||
| printf("Attempting to call DecodeSyscheck with SHA256 input...\n"); | ||
| fflush(stdout); | ||
| DecodeSyscheck(&lf); | ||
| printf("Survived SHA256 Input!\n"); | ||
| fflush(stdout); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
The test file doesn't follow the established testing convention used in this codebase. All other test files in src/tests (test_os_crypto.c, test_os_net.c, test_os_regex.c, test_os_xml.c, test_os_zlib.c, test_shared.c) use the Check unit testing framework with START_TEST/END_TEST macros, test suites, and proper test runners. This regression test should be refactored to follow the same pattern for consistency.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 participant
No description provided.