Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
153 changes: 153 additions & 0 deletions dannytheway_personal/unbounded-copy-to-stack-buffer.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
#include <cstdio>
#define BUFFER_SIZE 1024

void test_001() {
char buf[BUFFER_SIZE];
// ruleid: unbounded-copy-to-stack-buffer
if (gets(buf) == NULL) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Semgrep identified an issue in your code:
The gets() function reads a line from stdin into the provided buffer
until either a terminating newline or EOF. This terminating newline or
EOF is replaced with a null byte '\0'. No check for buffer overruns are
performed so it is recommended to use fgets() instead. Do note
that some platforms will continue reading data after a '\0' is encountered.

Usage of fgets() is not recommended for reading binary based files or inputs,
instead the read or fread functions should be used.

For more information please see: https://linux.die.net/man/3/fgets

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by _getts-1.

You can view more details about this finding in the Semgrep AppSec Platform.

// ...
}
}

void test_002() {
char buf[BUFFER_SIZE];
// ruleid: unbounded-copy-to-stack-buffer
if (std::gets(buf) == NULL) {
// ...
}
}

void test_003(void) {
char buf[BUFFER_SIZE];
// ruleid: unbounded-copy-to-stack-buffer
if (1 != fscanf(stdin, "%s", buf)) {
// ...
}
}

void test_003_valist(void) {
char buf[BUFFER_SIZE];
// ruleid: unbounded-copy-to-stack-buffer
if (1 != fscanf(stdin, "%d%d%s", 1, 2, buf)) {
// ...
}
}

void test_003_stmt(void) {
char buf[BUFFER_SIZE];
// ruleid: unbounded-copy-to-stack-buffer
fscanf(stdin, "%d%d%s", 1, 2, buf);
}

void test_004(void) {
char buf[BUFFER_SIZE];
// ok: unbounded-copy-to-stack-buffer
if (1 != fscanf(stdin, "%d", buf)) {
// ...
}
}

void test_005(const char *name) {
char filename[128];
// ruleid: unbounded-copy-to-stack-buffer
sprintf(filename, "%s", name);
}

void test_006(const char *name) {
char filename[128];
// ok: unbounded-copy-to-stack-buffer
sprintf(filename, "%d", name);
}

void test_007(wchar_t *name) {
char filename[128];
// ruleid: unbounded-copy-to-stack-buffer
fwscanf(stdin, L"foo%sbar", filename);
}

void foo() {
char buf[64];
// ruleid: unbounded-copy-to-stack-buffer
gets(buf);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Semgrep identified an issue in your code:
The gets() function reads a line from stdin into the provided buffer
until either a terminating newline or EOF. This terminating newline or
EOF is replaced with a null byte '\0'. No check for buffer overruns are
performed so it is recommended to use fgets() instead. Do note
that some platforms will continue reading data after a '\0' is encountered.

Usage of fgets() is not recommended for reading binary based files or inputs,
instead the read or fread functions should be used.

For more information please see: https://linux.die.net/man/3/fgets

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by _getts-1.

You can view more details about this finding in the Semgrep AppSec Platform.

}

void foo() {
char buf[64];
// ok: unbounded-copy-to-stack-buffer
fgets(buf, 63, stdin);
}

void foo() {
char buf[128];
int n;
// ruleid: unbounded-copy-to-stack-buffer
fscanf(stdin, "%dfoo%sbar", n, buf);
// ruleid: unbounded-copy-to-stack-buffer
fscanf(stdin, "%sbar", n, buf);
// ruleid: unbounded-copy-to-stack-buffer
fscanf(stdin, "foo%s", n, buf);
}

void foo() {
char buf[128];
int n;
// ok: unbounded-copy-to-stack-buffer
fscanf(stdin, "%dfoo%10sbar", n, buf);
}

void foo() {
char buf[128];
int n;
// todoruleid: unbounded-copy-to-stack-buffer
fscanf(stdin, "%dfoo%1024sbar", n, buf);
}

void foo() {
char buf[128];
// ruleid: unbounded-copy-to-stack-buffer
fscanf(stdin, "%sfoo%%s", buf);

// We don't scan all '%' characters so we don't understand that
// '%%%s' means "literal '%', followed by '%s'"
// todook: unbounded-copy-to-stack-buffer
fscanf(stdin, "%sfoo%%%s", buf);
}

void foo() {
char buf[64];
va_list ap;
va_start(ap, buf);
// ruleid: unbounded-copy-to-stack-buffer
vscanf("%s", ap);
va_end(ap);
}

void foo() {
char buf[64];
va_list ap;
va_start(ap, buf);
// ruleid: unbounded-copy-to-stack-buffer
vfscanf(stdin, "%s", ap);
va_end(ap);
}

void foo(char *str) {
char buf[0];
// ruleid: unbounded-copy-to-stack-buffer
sscanf(str, "%s", buf);

// todook: unbounded-copy-to-stack-buffer
sscanf("constant string", "%s", buf);

va_list ap;
va_start(ap, fmt);
// todoruleid: unbounded-copy-to-stack-buffer
vsscanf(str, "%s", buf);
va_end(ap);

va_list ap;
va_start(ap, fmt);
// ok: unbounded-copy-to-stack-buffer
vsscanf("constant string", "%s", buf);
va_end(ap);
}
120 changes: 120 additions & 0 deletions dannytheway_personal/unbounded-copy-to-stack-buffer.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
rules:
- id: unbounded-copy-to-stack-buffer
languages:
- cpp
severity: ERROR
message: The function `$FUN` does not impose any size limitation to what it writes
to `$BUF`. That may lead to a stack buffer overflow if there is no validation
on the size of the input.
patterns:
- pattern-inside: |-
$TY $BUF[$SIZE];
...
- pattern-either:
- patterns:
- pattern: <... $FUN($BUF, $STR) ...>
- metavariable-pattern:
metavariable: $FUN
patterns:
- pattern-either:
- pattern: strcpy
- pattern: std::strcpy
- focus-metavariable: $FUN
- patterns:
- pattern: <... $FUN($BUF) ...>
- metavariable-pattern:
metavariable: $FUN
patterns:
- pattern-either:
- pattern: gets
- pattern: std::gets
- focus-metavariable: $FUN
- patterns:
- pattern: <... $FUN($STDIN, "$FMT_STRING", ..., <... $BUF ...>) ...>
- metavariable-regex:
metavariable: $FMT_STRING
regex: ^(%l?s.*|.*[^%]%l?s.*)$
- metavariable-pattern:
metavariable: $FUN
patterns:
- pattern-either:
- pattern: fscanf
- pattern: std::fscanf
- pattern: sscanf
- pattern: std::sscanf
- pattern: fwscanf
- pattern: std::fwscanf
- pattern: swscanf
- pattern: std::swscanf
- focus-metavariable: $FUN
- patterns:
- pattern: <... $FUN($BUF, "$FMT_STRING", ...) ...>
- metavariable-regex:
metavariable: $FMT_STRING
regex: ^(%l?s.*|.*[^%]%l?s.*)$
- metavariable-pattern:
metavariable: $FUN
patterns:
- pattern-either:
- pattern: sprintf
- pattern: std::sprintf
- pattern: swprintf
- pattern: std::swprintf
- focus-metavariable: $FUN
- patterns:
- pattern: <... $FUN($SOURCE, "$FMT_STRING", $VA) ...>
- pattern-inside: |-
va_start($VA, $BUF);
...
<... $FUN($SOURCE, "$FMT_STRING", $VA) ...>;
- metavariable-regex:
metavariable: $FMT_STRING
regex: ^(%l?s.*|.*[^%]%l?s.*)$
- metavariable-pattern:
metavariable: $FUN
patterns:
- pattern-either:
- pattern: vfscanf
- pattern: std::vfscanf
- focus-metavariable: $FUN
- patterns:
- pattern: <... $FUN("$FMT_STRING", $VA) ...>
- pattern-inside: |-
va_start($VA, $BUF);
...
<... $FUN("$FMT_STRING", $VA) ...>;
- metavariable-regex:
metavariable: $FMT_STRING
regex: ^(%l?s.*|.*[^%]%l?s.*)$
- metavariable-pattern:
metavariable: $FUN
patterns:
- pattern-either:
- pattern: vscanf
- pattern: std::vscanf
- focus-metavariable: $FUN
metadata:
likelihood: LOW
impact: HIGH
confidence: MEDIUM
category: security
subcategory:
- vuln
cert:
- C
- C++
- L1
- STR31-C
cwe:
- 'CWE-120: Buffer Copy without Checking Size of Input (''Classic Buffer Overflow'')'
display-name: Buffer Overflow
functional-categories:
- memory::sink::buffer-overflow
references:
- https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
technology:
- cpp
license: Semgrep Rules License v1.0. For more details, visit semgrep.dev/legal/rules-license
vulnerability_class:
- Other
min-version: 1.59.0
Loading