Rules - dont-call-system c/cpp #17
Conversation
WalkthroughThis pull request introduces new static analysis rules for both C and C++ programming languages, specifically warning against the use of the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
rules/c/security/dont-call-system-c.yml (2)
4-12: Approve message and note with minor formatting suggestion.The message and note provide clear, actionable advice and valuable context. However, the formatting of the note could be improved for better readability.
Consider reformatting the note section for better readability:
note: >- [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection'). [REFERENCES] - https://owasp.org/Top10/A03_2021-Injection
1-26: Excellent rule implementation with a minor suggestion.This rule is well-structured and comprehensive. It effectively identifies calls to the
systemfunction, provides clear guidance on alternatives, and includes relevant security context. Great job!To further enhance this rule, consider adding examples of both valid and invalid code snippets in a comment section. This would help developers quickly understand what the rule is checking for and how to comply with it.
Here's an example of how you might add code snippets:
# Add this section at the end of the file examples: - name: Invalid use of system language: c code: | #include <stdlib.h> int main() { system("ls -l"); // Invalid: Using system function return 0; } - name: Valid alternative using execve language: c code: | #include <unistd.h> int main() { char *args[] = {"ls", "-l", NULL}; execve("/bin/ls", args, NULL); // Valid: Using execve return 0; }rules/cpp/security/dont-call-system-cpp.yml (1)
4-12: Consider enhancing the warning message with explicit security risks.The current message and note provide good information about why
systemshould be avoided and suggest alternatives. To make the warning more impactful, consider explicitly mentioning the security risks associated with usingsystem.Here's a suggested improvement for the message:
message: >- Don't call `system`. It's a high-level wrapper that allows for stacking multiple commands. Always prefer a more restrictive API such as calling - `execve` from the `exec` family. + `execve` from the `exec` family. Using `system` can lead to command + injection vulnerabilities if user input is not properly sanitized.tests/__snapshots__/dont-call-system-c-snapshot.yml (1)
1-41: LGTM! Consider adding inline comments for clarity.The overall structure and purpose of this snapshot file are appropriate for testing the
dont-call-system-crule. It includes both a "bad" example (test_002) and a "better but still problematic" example (test_001), which should effectively test the rule's implementation.Consider adding inline comments within the code snippets to explicitly state the purpose of each function (e.g., "Unsafe usage" for
test_002and "More careful but still problematic usage" fortest_001). This would enhance the clarity and educational value of the snapshot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- rules/c/security/dont-call-system-c.yml (1 hunks)
- rules/cpp/security/dont-call-system-cpp.yml (1 hunks)
- tests/snapshots/dont-call-system-c-snapshot.yml (1 hunks)
- tests/snapshots/dont-call-system-cpp-snapshot.yml (1 hunks)
- tests/c/dont-call-system-c-test.yml (1 hunks)
- tests/cpp/dont-call-system-cpp-test.yml (1 hunks)
🧰 Additional context used
🔇 Additional comments (21)
rules/c/security/dont-call-system-c.yml (3)
1-3: LGTM: Rule metadata is well-defined.The rule id is descriptive, the language is correctly specified, and the severity level is appropriate for this type of security warning.
13-23: LGTM: PATTERN_SYSTEM is well-defined.The pattern correctly identifies call expressions to the
systemfunction. The use ofstopBy: neighborand checking for both the identifier and argument list ensures accurate matching.
24-26: LGTM: Rule section is concise and effective.The rule correctly uses the PATTERN_SYSTEM to identify calls to the
systemfunction. The structure is simple and effective for this type of static analysis rule.rules/cpp/security/dont-call-system-cpp.yml (3)
1-3: LGTM: Rule metadata is well-defined.The rule id is descriptive, the language is correctly set to C++, and the warning severity is appropriate for this security-related issue.
24-26: LGTM: Rule definition is concise and correct.The rule section effectively utilizes the PATTERN_SYSTEM defined in the utils section to identify calls to the
systemfunction.
1-26: Overall, this is a well-implemented and valuable static analysis rule.The
dont-call-system-cpprule effectively targets the use of the potentially dangeroussystemfunction in C++ code. It provides clear guidance to developers and includes relevant references to security standards. With the suggested improvements, this rule will be even more robust and informative.Great job on addressing this important security concern!
tests/c/dont-call-system-c-test.yml (4)
4-7: LGTM: Valid test case demonstrates safe alternative tosystem()This test case correctly illustrates how to handle system-related functionality without using the
system()function. It uses astorerobject to store binary data from a system clock, which is a safe alternative.
10-16: Security vulnerability: Unsafe use ofsystem()functionThis test case correctly demonstrates an invalid usage of the
system()function:
- It uses
system()with a command string that includes user input, which is a severe security risk (command injection vulnerability).- There's no input validation or sanitization before using the input in the command.
- No error checking is performed on the
snprintf()orsystem()calls.To address these issues in real code:
- Avoid
system()altogether. Use safer alternatives likeexecve()family of functions.- If
system()must be used, ensure thorough input validation and sanitization.- Always check return values of functions like
snprintf()andsystem()for errors.
17-34: Improved but still unsafe use ofsystem()functionThis test case correctly demonstrates an invalid usage of the
system()function, albeit with improved error handling:
- It still uses
system()with a command string that includes user input, which remains a severe security risk (command injection vulnerability).- The error handling for
snprintf()andsystem()calls is a good practice, but it doesn't mitigate the core security issue.While the error handling is an improvement over
test_002, to address the security concerns in real code:
- Avoid
system()altogether. Use safer alternatives likeexecve()family of functions.- If
system()must be used (strongly discouraged), ensure thorough input validation and sanitization in addition to error handling.- Consider using a whitelist of allowed commands instead of directly incorporating user input into the command string.
1-34: Effective test cases for "dont-call-system-c" ruleThese test cases effectively demonstrate the intent of the "dont-call-system-c" rule:
- The valid case (
test_003) shows a safe alternative to usingsystem().- The invalid cases (
test_001andtest_002) illustrate unsafe uses ofsystem(), both with and without error handling.This comprehensive approach helps developers understand:
- How to correctly handle system-related functionality without
system().- Why
system()is dangerous even with proper error handling.- The importance of avoiding
system()altogether in security-sensitive contexts.Consider adding more valid cases to showcase other safe alternatives to
system(), providing developers with a broader range of secure options.tests/cpp/dont-call-system-cpp-test.yml (4)
3-7: Appropriate valid test case for the rule.This test case correctly demonstrates a valid usage that doesn't involve the
system()function. Instead, it usesClocks->system(), which likely refers to a system clock, andstorer->store_binary()for data storage. This aligns well with the rule's objective of avoidingsystem()calls.
9-16: Appropriate invalid test case demonstrating unsafesystem()usage.This test case correctly illustrates an invalid usage of the
system()function. It constructs a command string using user input and executes it without any error checking, which is precisely the kind of unsafe practice the rule aims to prevent. This example effectively demonstrates why usingsystem()can lead to security vulnerabilities.
17-34: Appropriate invalid test case demonstratingsystem()usage with error handling.This test case effectively illustrates why using
system()is discouraged, even when implemented with error handling. Despite the careful approach with error checking for bothsnprintfandsystemcalls, the fundamental security risk of executing system commands with user input remains. This example reinforces the rule's importance by showing that even seemingly careful uses ofsystem()are considered invalid.
1-34: Comprehensive test cases effectively illustrate the rule.The test cases in this file provide a well-rounded set of examples for the
dont-call-system-cpprule:
- The valid case demonstrates a safe alternative to using
system().- The first invalid case shows a basic misuse of
system()without error handling.- The second invalid case illustrates that even with proper error handling, using
system()is still considered unsafe.This combination effectively communicates the rule's intent and covers different scenarios developers might encounter. It reinforces the importance of avoiding
system()calls in all situations, contributing to better code security.tests/__snapshots__/dont-call-system-c-snapshot.yml (2)
4-10: Confirm iftest_002is intended as a negative example.The
test_002function demonstrates several unsafe practices:
- No error checking for the
snprintfcall.- No input validation or sanitization, potentially allowing command injection.
- No error checking for the
systemcall.These issues align with the purpose of the
dont-call-system-crule. Please confirm if this function is intentionally written as a negative example to trigger the rule.
11-28: Confirm iftest_001is intended as a more careful but still problematic example.The
test_001function demonstrates more careful coding practices:
- It checks for buffer overflow and negative return values from
snprintf.- It includes error handling for both
snprintfandsystemcalls.However, it still uses the
systemfunction, which is the subject of thedont-call-system-crule. This seems to be intentionally written as a more careful but still problematic example. Please confirm if this interpretation is correct.tests/__snapshots__/dont-call-system-cpp-snapshot.yml (5)
1-41: LGTM: Overall structure of the snapshot file is correct.The file structure follows the expected format for a snapshot test file. The id
dont-call-system-cppcorrectly matches the rule being tested, and the inclusion of labels for highlighting specific code parts is beneficial for visualization.
4-10: LGTM:test_002function serves its purpose as a test case.This function demonstrates a straightforward use of
systemwithout error checking, which is appropriate for testing thedont-call-system-cpprule. It provides a clear example of code that should trigger the rule.
11-28: LGTM:test_001function provides a more robust test case.This function demonstrates a more careful use of
systemwith proper error checking for bothsnprintfandsystem. The error handling placeholders are appropriate for a test case. This example is valuable as it shows that the rule should still trigger even with added safety measures.
29-41: LGTM: Labels are well-defined and useful.The labels are correctly structured and provide useful highlighting for the
systemcall and its components. The use of primary and secondary styles allows for differentiated highlighting, which is helpful for visualizing the critical parts of the code that the rule is meant to catch.
1-41: Overall, excellent snapshot test file for thedont-call-system-cpprule.This snapshot file is well-structured and includes appropriate test cases for the
dont-call-system-cpprule. It covers both a simple case (test_002) and a more robust case (test_001) ofsystemusage, which will help ensure the rule catches various scenarios. The inclusion of labels for highlighting is a nice touch for improved visualization of the critical code parts.Great job on creating a comprehensive test file that will help maintain the quality of the static analysis rule.
Summary by CodeRabbit
New Features
systemfunction, promoting safer coding practices.systemcalls.Bug Fixes
systemfunction usage, ensuring better adherence to security guidelines.