Skip to content

changes in file-access-before-action in c/cpp #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

changes in file-access-before-action in c/cpp #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
82 changes: 82 additions & 0 deletions rules/c/security/file-stat-before-action-c.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
id: file-stat-before-action-c
language: c
severity: warning
message: >-
A check is done with `stat` and then the file is used. There is no
guarantee that the status of the file has not changed since the call to
`stat` which may allow attackers to bypass permission checks.
note: >-
[CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition
[REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
utils:
match_fopen_identifier:
kind: identifier
regex: unlink|fopen|freopen|remove|rename|access|open|stat|lstat|unlink|mkdir|rmdir|chdir|folly::readFile|folly::writeFile|folly::writeFileAtomic|folly::writeFileAtomicNoThrow|folly::File
all:
- inside:
kind: call_expression
inside:
stopBy: end
kind: expression_statement
inside:
kind: compound_statement
inside:
kind: if_statement
has:
stopBy: end
kind: call_expression
all:
- has:
kind: identifier
regex: ^(fstatat|_fstatat)$
- has:
stopBy: neighbor
kind: argument_list
all:
- has:
stopBy: neighbor
kind: identifier
- has:
stopBy: neighbor
kind: call_expression
all:
- has:
stopBy: neighbor
kind: field_expression
- has:
stopBy: neighbor
kind: argument_list

match_fopen_identifier_2:
kind: identifier
regex: unlink|fopen|freopen|remove|rename|access|open|stat|lstat|unlink|mkdir|rmdir|chdir|folly::readFile|folly::writeFile|folly::writeFileAtomic|folly::writeFileAtomicNoThrow|folly::File
all:
- inside:
kind: call_expression
inside:
stopBy: end
kind: expression_statement
inside:
kind: compound_statement
inside:
kind: if_statement
has:
stopBy: end
kind: call_expression
all:
- has:
stopBy: neighbor
kind: identifier
regex: "^stat|_stat|lstat|_lstat$"
- has:
stopBy: neighbor
kind: argument_list
has:
stopBy: neighbor
kind: call_expression

rule:
any:
- matches: match_fopen_identifier
- matches: match_fopen_identifier_2
82 changes: 82 additions & 0 deletions rules/cpp/security/file-stat-before-action-cpp.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
id: file-stat-before-action-cpp
language: cpp
severity: warning
message: >-
A check is done with `stat` and then the file is used. There is no
guarantee that the status of the file has not changed since the call to
`stat` which may allow attackers to bypass permission checks.
note: >-
[CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition
[REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
utils:
match_fopen_identifier:
kind: identifier
regex: unlink|fopen|freopen|remove|rename|access|open|stat|lstat|unlink|mkdir|rmdir|chdir|folly::readFile|folly::writeFile|folly::writeFileAtomic|folly::writeFileAtomicNoThrow|folly::File
all:
- inside:
kind: call_expression
inside:
stopBy: end
kind: expression_statement
inside:
kind: compound_statement
inside:
kind: if_statement
has:
stopBy: end
kind: call_expression
all:
- has:
kind: identifier
regex: ^(fstatat|_fstatat)$
- has:
stopBy: neighbor
kind: argument_list
all:
- has:
stopBy: neighbor
kind: identifier
- has:
stopBy: neighbor
kind: call_expression
all:
- has:
stopBy: neighbor
kind: field_expression
- has:
stopBy: neighbor
kind: argument_list

match_fopen_identifier_2:
kind: identifier
regex: unlink|fopen|freopen|remove|rename|access|open|stat|lstat|unlink|mkdir|rmdir|chdir|folly::readFile|folly::writeFile|folly::writeFileAtomic|folly::writeFileAtomicNoThrow|folly::File
all:
- inside:
kind: call_expression
inside:
stopBy: end
kind: expression_statement
inside:
kind: compound_statement
inside:
kind: if_statement
has:
stopBy: end
kind: call_expression
all:
- has:
stopBy: neighbor
kind: identifier
regex: "^stat|_stat|lstat|_lstat$"
- has:
stopBy: neighbor
kind: argument_list
has:
stopBy: neighbor
kind: call_expression

rule:
any:
- matches: match_fopen_identifier
- matches: match_fopen_identifier_2
150 changes: 150 additions & 0 deletions tests/__snapshots__/file-stat-before-action-c-snapshot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
id: file-stat-before-action-c
snapshots:
? |
if (stat(file.c_str(), &buf) == 0)
{

// Open the file for reading
// ruleid: file-stat-before-action
fp = fopen(file.c_str(), "r");
if (fp == NULL)
{
char message[2560];
sprintf(message, "File '%s' Cound Not be Opened", file.c_str());
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typo in error message.

There's a typo in the error message: "Cound" should be "Could".

Apply this diff to fix the typo:

-             sprintf(message, "File '%s' Cound Not be Opened", file.c_str());
+             sprintf(message, "File '%s' Could Not be Opened", file.c_str());
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
sprintf(message, "File '%s' Cound Not be Opened", file.c_str());
sprintf(message, "File '%s' Could Not be Opened", file.c_str());

// DISPLAY_MSG_ERROR( this, message, "GetFileContents", "System" );
throw message;
Comment on lines +12 to +15
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using safer string handling methods.

The code uses C-style string handling with sprintf, which can be prone to buffer overflow issues. Consider using safer alternatives like snprintf or C++ string handling methods.

Here's an example using snprintf:

char message[2560];
snprintf(message, sizeof(message), "File '%s' Could Not be Opened", file.c_str());

Or using C++ string handling:

std::string message = "File '" + file + "' Could Not be Opened";
throw message;

}

// Read the file
MvString s, ss;
while (fgets(data, sizeof(data), fp) != (char *)0)
{
s = data;
s.trimBoth();
if (s.compare(0, 5, "GROUP") == 0)
{
// size_t t = s.find_last_of( ":" );
size_t t = s.find(":");
if (t != string::npos)
{
ss = s.substr(t + 1).c_str();
ss.trimBoth();
ss = ss.substr(1, ss.length() - 3).c_str();
group_list.push_back(ss);
}
}
}

// Close the file
fclose(fp);
}
: labels:
- source: fopen
style: primary
start: 123
end: 128
- source: stat
style: secondary
start: 4
end: 8
- source: file.c_str()
style: secondary
start: 9
end: 21
- source: (file.c_str(), &buf)
style: secondary
start: 8
end: 28
- source: stat(file.c_str(), &buf)
style: secondary
start: 4
end: 28
- source: |-
if (stat(file.c_str(), &buf) == 0)
{

// Open the file for reading
// ruleid: file-stat-before-action
fp = fopen(file.c_str(), "r");
if (fp == NULL)
{
char message[2560];
sprintf(message, "File '%s' Cound Not be Opened", file.c_str());
// DISPLAY_MSG_ERROR( this, message, "GetFileContents", "System" );
throw message;
}

// Read the file
MvString s, ss;
while (fgets(data, sizeof(data), fp) != (char *)0)
{
s = data;
s.trimBoth();
if (s.compare(0, 5, "GROUP") == 0)
{
// size_t t = s.find_last_of( ":" );
size_t t = s.find(":");
if (t != string::npos)
{
ss = s.substr(t + 1).c_str();
ss.trimBoth();
ss = ss.substr(1, ss.length() - 3).c_str();
group_list.push_back(ss);
}
}
}

// Close the file
fclose(fp);
}
style: secondary
start: 0
end: 989
- source: |-
{

// Open the file for reading
// ruleid: file-stat-before-action
fp = fopen(file.c_str(), "r");
if (fp == NULL)
{
char message[2560];
sprintf(message, "File '%s' Cound Not be Opened", file.c_str());
// DISPLAY_MSG_ERROR( this, message, "GetFileContents", "System" );
throw message;
}

// Read the file
MvString s, ss;
while (fgets(data, sizeof(data), fp) != (char *)0)
{
s = data;
s.trimBoth();
if (s.compare(0, 5, "GROUP") == 0)
{
// size_t t = s.find_last_of( ":" );
size_t t = s.find(":");
if (t != string::npos)
{
ss = s.substr(t + 1).c_str();
ss.trimBoth();
ss = ss.substr(1, ss.length() - 3).c_str();
group_list.push_back(ss);
}
}
}

// Close the file
fclose(fp);
}
style: secondary
start: 36
end: 989
- source: fp = fopen(file.c_str(), "r");
style: secondary
start: 118
end: 148
- source: fopen(file.c_str(), "r")
style: secondary
start: 123
end: 147
43 changes: 43 additions & 0 deletions tests/c/file-stat-before-action-c-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
id: file-stat-before-action-c
valid:
- |

invalid:
- |
if (stat(file.c_str(), &buf) == 0)
{

// Open the file for reading
// ruleid: file-stat-before-action
fp = fopen(file.c_str(), "r");
if (fp == NULL)
{
char message[2560];
sprintf(message, "File '%s' Cound Not be Opened", file.c_str());
// DISPLAY_MSG_ERROR( this, message, "GetFileContents", "System" );
Comment on lines +15 to +17
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Replace sprintf with a safer alternative.

The use of sprintf can lead to buffer overflows if the resulting string is longer than the buffer. Consider using snprintf instead, which allows you to specify the maximum number of characters to write:

snprintf(message, sizeof(message), "File '%s' Could Not be Opened", file.c_str());

This change will ensure that the buffer is not overflowed, improving the safety of the code.

throw message;
}

// Read the file
MvString s, ss;
while (fgets(data, sizeof(data), fp) != (char *)0)
{
s = data;
s.trimBoth();
if (s.compare(0, 5, "GROUP") == 0)
{
// size_t t = s.find_last_of( ":" );
size_t t = s.find(":");
if (t != string::npos)
{
ss = s.substr(t + 1).c_str();
ss.trimBoth();
ss = ss.substr(1, ss.length() - 3).c_str();
group_list.push_back(ss);
}
}
}

// Close the file
fclose(fp);
}
Loading