-
Notifications
You must be signed in to change notification settings - Fork 35
Bring in additional rules from internal repo (both experimental and recommended.) #216
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.
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
Open
NateD-MSFT
wants to merge
3
commits into
development
Choose a base branch
from
user/nated-msft/2603/stl-rule-expansion
base: development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| #include <stdio.h> | ||
| int main(int argc, char** argv) { | ||
| for(int i = 1; i < argc; ++i) { | ||
| printf(argv[i]); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| #include <stdio.h> | ||
| int main(int argc, char** argv) { | ||
| for(int i = 1; i < argc; ++i) { | ||
| printf("%s", argv[i]); | ||
| } | ||
| } |
13 changes: 13 additions & 0 deletions
13
src/microsoft/Likely Bugs/Format/NonConstantFormat-2-bad.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| void log_with_timestamp(const char* message) { | ||
| struct tm now; | ||
| time(&now); | ||
| printf("[%s] ", asctime(now)); | ||
| printf(message); | ||
| } | ||
|
|
||
| int main(int argc, char** argv) { | ||
| log_with_timestamp("Application is starting...\n"); | ||
| /* ... */ | ||
| log_with_timestamp("Application is closing...\n"); | ||
| return 0; | ||
| } | ||
16 changes: 16 additions & 0 deletions
16
src/microsoft/Likely Bugs/Format/NonConstantFormat-2-good.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| void log_with_timestamp(const char* message, ...) { | ||
| va_list args; | ||
| va_start(args, message); | ||
| struct tm now; | ||
| time(&now); | ||
| printf("[%s] ", asctime(now)); | ||
| vprintf(message, args); | ||
| va_end(args); | ||
| } | ||
|
|
||
| int main(int argc, char** argv) { | ||
| log_with_timestamp("%s is starting...\n", argv[0]); | ||
| /* ... */ | ||
| log_with_timestamp("%s is closing...\n", argv[0]); | ||
| return 0; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| void log_with_timestamp(const char* message) { | ||
| struct tm now; | ||
| time(&now); | ||
| printf("[%s] %s", asctime(now), message); | ||
| } | ||
|
|
||
| int main(int argc, char** argv) { | ||
| log_with_timestamp("Application is starting...\n"); | ||
| /* ... */ | ||
| log_with_timestamp("Application is closing...\n"); | ||
| return 0; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| # Non-constant format string | ||
| The `printf` function, related functions like `sprintf` and `fprintf`, and other functions built atop `vprintf` all accept a format string as one of their arguments. When such format strings are literal constants, it is easy for the programmer (and static analysis tools) to verify that the format specifiers (such as `%s` and `%02x`) in the format string are compatible with the trailing arguments of the function call. When such format strings are not literal constants, it is more difficult to maintain the program: programmers (and static analysis tools) must perform non-local data-flow analysis to deduce what values the format string argument might take. | ||
|
|
||
|
|
||
| ## Recommendation | ||
| If the argument passed as a format string is meant to be a plain string rather than a format string, then pass `%s` as the format string, and pass the original argument as the sole trailing argument. | ||
|
|
||
| If the argument passed as a format string is a parameter to the enclosing function, then consider redesigning the enclosing function's API to be less brittle. | ||
|
|
||
|
|
||
| ## Example | ||
| The following program is meant to echo its command line arguments: | ||
|
|
||
|
|
||
| ```c | ||
| #include <stdio.h> | ||
| int main(int argc, char** argv) { | ||
| for(int i = 1; i < argc; ++i) { | ||
| printf(argv[i]); | ||
| } | ||
| } | ||
| ``` | ||
| The above program behaves as expected in most cases, but breaks when one of its command line arguments contains a percent character. In such cases, the behavior of the program is undefined: it might echo garbage, it might crash, or it might give a malicious attacker root access. One way of addressing the problem is to use a constant `%s` format string, as in the following program: | ||
|
|
||
|
|
||
| ```c | ||
| #include <stdio.h> | ||
| int main(int argc, char** argv) { | ||
| for(int i = 1; i < argc; ++i) { | ||
| printf("%s", argv[i]); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Example | ||
| The following program defines a `log_with_timestamp` function: | ||
|
|
||
|
|
||
| ```c | ||
| void log_with_timestamp(const char* message) { | ||
| struct tm now; | ||
| time(&now); | ||
| printf("[%s] ", asctime(now)); | ||
| printf(message); | ||
| } | ||
|
|
||
| int main(int argc, char** argv) { | ||
| log_with_timestamp("Application is starting...\n"); | ||
| /* ... */ | ||
| log_with_timestamp("Application is closing...\n"); | ||
| return 0; | ||
| } | ||
| ``` | ||
| In the code that is visible, the reader can verify that `log_with_timestamp` is never called with a log message containing a percent character, but even if all current calls are correct, this presents an ongoing maintenance burden to ensure that newly-introduced calls don't contain percent characters. As in the previous example, one solution is to make the log message a trailing argument of the function call: | ||
|
|
||
|
|
||
| ```c | ||
| void log_with_timestamp(const char* message) { | ||
| struct tm now; | ||
| time(&now); | ||
| printf("[%s] %s", asctime(now), message); | ||
| } | ||
|
|
||
| int main(int argc, char** argv) { | ||
| log_with_timestamp("Application is starting...\n"); | ||
| /* ... */ | ||
| log_with_timestamp("Application is closing...\n"); | ||
| return 0; | ||
| } | ||
| ``` | ||
| An alternative solution is to allow `log_with_timestamp` to accept format arguments: | ||
|
|
||
|
|
||
| ```c | ||
| void log_with_timestamp(const char* message, ...) { | ||
| va_list args; | ||
| va_start(args, message); | ||
| struct tm now; | ||
| time(&now); | ||
| printf("[%s] ", asctime(now)); | ||
| vprintf(message, args); | ||
| va_end(args); | ||
| } | ||
|
|
||
| int main(int argc, char** argv) { | ||
| log_with_timestamp("%s is starting...\n", argv[0]); | ||
| /* ... */ | ||
| log_with_timestamp("%s is closing...\n", argv[0]); | ||
| return 0; | ||
| } | ||
| ``` | ||
| In this formulation, the non-constant format string to `printf` has been replaced with a non-constant format string to `vprintf`. The analysis will no longer consider the body of `log_with_timestamp` to be a problem, and will instead check that every call to `log_with_timestamp` passes a constant format string. | ||
|
|
||
|
|
||
| ## References | ||
| * CERT C Coding Standard: [FIO30-C. Exclude user input from format strings](https://www.securecoding.cert.org/confluence/display/c/FIO30-C.+Exclude+user+input+from+format+strings). | ||
| * M. Howard, D. Leblanc, J. Viega, *19 Deadly Sins of Software Security: Programming Flaws and How to Fix Them*. | ||
| * Common Weakness Enumeration: [CWE-134](https://cwe.mitre.org/data/definitions/134.html). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
| <overview> | ||
| <p>The <code>printf</code> function, related functions like <code>sprintf</code> and <code>fprintf</code>, | ||
| and other functions built atop <code>vprintf</code> all accept a format string as one of their arguments. | ||
| When such format strings are literal constants, it is easy for the programmer (and static analysis tools) | ||
| to verify that the format specifiers (such as <code>%s</code> and <code>%02x</code>) in the format string | ||
| are compatible with the trailing arguments of the function call. When such format strings are not literal | ||
| constants, it is more difficult to maintain the program: programmers (and static analysis tools) must | ||
| perform non-local data-flow analysis to deduce what values the format string argument might take.</p> | ||
|
|
||
| </overview> | ||
| <recommendation> | ||
| <p>If the argument passed as a format string is meant to be a plain string rather than a format string, | ||
| then pass <code>%s</code> as the format string, and pass the original argument as the sole trailing | ||
| argument.</p> | ||
|
|
||
| <p>If the argument passed as a format string is a parameter to the enclosing function, then consider | ||
| redesigning the enclosing function's API to be less brittle.</p> | ||
|
|
||
| </recommendation> | ||
| <example> | ||
| <p>The following program is meant to echo its command line arguments:</p> | ||
| <sample src="NonConstantFormat-1-bad.c" /> | ||
| <p>The above program behaves as expected in most cases, but breaks when one of its command line arguments | ||
| contains a percent character. In such cases, the behavior of the program is undefined: it might echo | ||
| garbage, it might crash, or it might give a malicious attacker root access. One way of addressing | ||
| the problem is to use a constant <code>%s</code> format string, as in the following program:</p> | ||
| <sample src="NonConstantFormat-1-good.c" /> | ||
|
|
||
| </example> | ||
| <example> | ||
| <p>The following program defines a <code>log_with_timestamp</code> function:</p> | ||
| <sample src="NonConstantFormat-2-bad.c" /> | ||
| <p>In the code that is visible, the reader can verify that <code>log_with_timestamp</code> is never called | ||
| with a log message containing a percent character, but even if all current calls are correct, this presents | ||
| an ongoing maintenance burden to ensure that newly-introduced calls don't contain percent characters. As | ||
| in the previous example, one solution is to make the log message a trailing argument of the function call:</p> | ||
| <sample src="NonConstantFormat-2-ok.c" /> | ||
| <p>An alternative solution is to allow <code>log_with_timestamp</code> to accept format arguments:</p> | ||
| <sample src="NonConstantFormat-2-good.c" /> | ||
| <p>In this formulation, the non-constant format string to <code>printf</code> has been replaced with | ||
| a non-constant format string to <code>vprintf</code>. The analysis will no longer consider the body of | ||
| <code>log_with_timestamp</code> to be a problem, and will instead check that every call to | ||
| <code>log_with_timestamp</code> passes a constant format string.</p> | ||
|
|
||
| </example> | ||
| <references> | ||
|
|
||
|
|
||
| <li>CERT C Coding | ||
| Standard: <a href="https://www.securecoding.cert.org/confluence/display/c/FIO30-C.+Exclude+user+input+from+format+strings">FIO30-C. Exclude user input from format strings</a>.</li> | ||
| <li>M. Howard, D. Leblanc, J. Viega, <i>19 Deadly Sins of Software Security: Programming Flaws and How to Fix Them</i>.</li> | ||
|
|
||
|
|
||
| </references> | ||
| </qhelp> |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.