From ea131ffa45ee1bb1e52c3832f17ea28d77815249 Mon Sep 17 00:00:00 2001 From: Nicolas Harraudeau <40498978+nicolas-harraudeau-sonarsource@users.noreply.github.com> Date: Mon, 22 Jun 2020 13:16:10 +0200 Subject: [PATCH] Add rule S3457 in asciidoc --- .gitignore | 7 ++++ README.md | 21 +++++++++++- rules/RSPEC_3457/c-family/compliant.c | 6 ++++ rules/RSPEC_3457/c-family/metadata.json | 6 ++++ rules/RSPEC_3457/c-family/noncompliant.c | 6 ++++ rules/RSPEC_3457/c-family/rule.adoc | 24 +++++++++++++ rules/RSPEC_3457/java/compliant.java | 31 +++++++++++++++++ rules/RSPEC_3457/java/metadata.json | 6 ++++ rules/RSPEC_3457/java/noncompliant.java | 32 ++++++++++++++++++ rules/RSPEC_3457/java/rule.adoc | 43 ++++++++++++++++++++++++ rules/RSPEC_3457/metadata.json | 18 ++++++++++ rules/RSPEC_3457/python/compliant.py | 10 ++++++ rules/RSPEC_3457/python/metadata.json | 3 ++ rules/RSPEC_3457/python/noncompliant.py | 10 ++++++ rules/RSPEC_3457/python/rule.adoc | 35 +++++++++++++++++++ rules/RSPEC_3457/see.adoc | 1 + 16 files changed, 258 insertions(+), 1 deletion(-) create mode 100644 .gitignore create mode 100644 rules/RSPEC_3457/c-family/compliant.c create mode 100644 rules/RSPEC_3457/c-family/metadata.json create mode 100644 rules/RSPEC_3457/c-family/noncompliant.c create mode 100644 rules/RSPEC_3457/c-family/rule.adoc create mode 100644 rules/RSPEC_3457/java/compliant.java create mode 100644 rules/RSPEC_3457/java/metadata.json create mode 100644 rules/RSPEC_3457/java/noncompliant.java create mode 100644 rules/RSPEC_3457/java/rule.adoc create mode 100644 rules/RSPEC_3457/metadata.json create mode 100644 rules/RSPEC_3457/python/compliant.py create mode 100644 rules/RSPEC_3457/python/metadata.json create mode 100644 rules/RSPEC_3457/python/noncompliant.py create mode 100644 rules/RSPEC_3457/python/rule.adoc create mode 100644 rules/RSPEC_3457/see.adoc diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000000..086716ef48 --- /dev/null +++ b/.gitignore @@ -0,0 +1,7 @@ +# generated files +*.html + +# compiled files +*.out +*.obj +*.pyc \ No newline at end of file diff --git a/README.md b/README.md index 14ee3b2d34..99287c73b5 100644 --- a/README.md +++ b/README.md @@ -1 +1,20 @@ -# rspec \ No newline at end of file +# rspec + + +Structure +--------- + +`rules` directory +- `RSPEC_****` directory for each rule + - a file per section shared between multiple languages: `main.adoc`, `compliant.adoc`, `noncompliant.adoc`, `exceptions.adoc`, `see.adoc` + - `metadata.json`: metadatas shared between laguage. They can be overridden. + - a directory per LANGUAGE: `java`, `c-family`, `python`... + - `rule.adoc`: root file used to generate the rule description for the specific language. It can include parts from `*.adoc` files defined in the parent directory. + - `metadata.json`: metadatas for the specific language. Each root key will completely override the key of the above `metadata.json` file. No "smart" merge takes place, which makes it easier to have in one glance the full value of a field. + - `compliant.py/java/...`, `noncompliant.py/java/...`: source code files containing compliant and noncompliant code examples. + +Metadata format +--------------- +The metadata match closely what plugins expect. +Additional fields: +* `qualityProfile`: quality profile(s) in which the rule should be registered. \ No newline at end of file diff --git a/rules/RSPEC_3457/c-family/compliant.c b/rules/RSPEC_3457/c-family/compliant.c new file mode 100644 index 0000000000..fa9d79d854 --- /dev/null +++ b/rules/RSPEC_3457/c-family/compliant.c @@ -0,0 +1,6 @@ +#include + +int main() { + printf("%d %d", 1, 2); // Compliant + printf("%-f", 1.2); // Compliant +} \ No newline at end of file diff --git a/rules/RSPEC_3457/c-family/metadata.json b/rules/RSPEC_3457/c-family/metadata.json new file mode 100644 index 0000000000..17c487b1dd --- /dev/null +++ b/rules/RSPEC_3457/c-family/metadata.json @@ -0,0 +1,6 @@ +{ + "tags": [ + "cert", + "confusing" + ] +} diff --git a/rules/RSPEC_3457/c-family/noncompliant.c b/rules/RSPEC_3457/c-family/noncompliant.c new file mode 100644 index 0000000000..38e596e48f --- /dev/null +++ b/rules/RSPEC_3457/c-family/noncompliant.c @@ -0,0 +1,6 @@ +#include + +int main() { + printf("%d", 1, 2); // Noncompliant; the second argument "2" is unused + printf("%0-f", 1.2); // Noncompliant; flag "0" is ignored because of "-" +} \ No newline at end of file diff --git a/rules/RSPEC_3457/c-family/rule.adoc b/rules/RSPEC_3457/c-family/rule.adoc new file mode 100644 index 0000000000..78fc22c178 --- /dev/null +++ b/rules/RSPEC_3457/c-family/rule.adoc @@ -0,0 +1,24 @@ +:source-highlighter: prettify +:source-language: c# +:source-indent: 0 + +Because `printf` format strings are interpreted at runtime, rather than validated by the compiler, they can contain errors that result in the wrong strings being created. This rule statically validates the correlation of `printf` format strings to their arguments. + +// TODO add a MACRO to replace reference to rule S2275 +The related rule S2275 is about errors that will create undefined behavior, while this rule is about errors that produce an unexpected string. + +== Exceptions +This rule will only work if the format string is provided as a string literal. + +== Noncompliant Code Example +---- +include::noncompliant.c[lines=4..5] +---- + +== Compliant Solution +---- +include::compliant.c[lines=4..5] +---- + +== See +include::../see.adoc[] \ No newline at end of file diff --git a/rules/RSPEC_3457/java/compliant.java b/rules/RSPEC_3457/java/compliant.java new file mode 100644 index 0000000000..679e29addc --- /dev/null +++ b/rules/RSPEC_3457/java/compliant.java @@ -0,0 +1,31 @@ +// TODO: Add includes + +public class Main +{ + public void main() { + String.format("First %s and then %s", "foo", "bar"); + String.format("Display %2$d and then %d", 1, 3); + String.format("Too many arguments %d %d", 1, 2); + String.format("First Line%n"); + String.format("Is myObject null ? %b", myObject == null); + String.format("value is %d", value); + String s = "string without arguments"; + + MessageFormat.format("Result {0}.", value); + MessageFormat.format("Result '{0}' = {0}", value); + MessageFormat.format("Result {0}.", myObject); + + java.util.Logger logger; + logger.log(java.util.logging.Level.SEVERE, "Result {0}.", myObject); + logger.log(java.util.logging.Level.SEVERE, "Result {0}'", 14); + logger.log(java.util.logging.Level.SEVERE, exception, () -> "Result " + param); + + org.slf4j.Logger slf4jLog; + org.slf4j.Marker marker; + slf4jLog.debug(marker, "message {}"); + slf4jLog.debug(marker, "message {}", 1); + + org.apache.logging.log4j.Logger log4jLog; + log4jLog.debug("message {}", 1); + } +} diff --git a/rules/RSPEC_3457/java/metadata.json b/rules/RSPEC_3457/java/metadata.json new file mode 100644 index 0000000000..17c487b1dd --- /dev/null +++ b/rules/RSPEC_3457/java/metadata.json @@ -0,0 +1,6 @@ +{ + "tags": [ + "cert", + "confusing" + ] +} diff --git a/rules/RSPEC_3457/java/noncompliant.java b/rules/RSPEC_3457/java/noncompliant.java new file mode 100644 index 0000000000..a377179986 --- /dev/null +++ b/rules/RSPEC_3457/java/noncompliant.java @@ -0,0 +1,32 @@ +// TODO: Add includes + +public class Main +{ + public void main() { + String.format("First {0} and then {1}", "foo", "bar"); //Noncompliant. Looks like there is a confusion with the use of {{java.text.MessageFormat}}, parameters "foo" and "bar" will be simply ignored here + String.format("Display %3$d and then %d", 1, 2, 3); //Noncompliant; the second argument '2' is unused + String.format("Too many arguments %d and %d", 1, 2, 3); //Noncompliant; the third argument '3' is unused + String.format("First Line\n"); //Noncompliant; %n should be used in place of \n to produce the platform-specific line separator + String.format("Is myObject null ? %b", myObject); //Noncompliant; when a non-boolean argument is formatted with %b, it prints true for any nonnull value, and false for null. Even if intended, this is misleading. It's better to directly inject the boolean value (myObject == null in this case) + String.format("value is " + value); // Noncompliant + String s = String.format("string without arguments"); // Noncompliant + + MessageFormat.format("Result '{0}'.", value); // Noncompliant; String contains no format specifiers. (quote are discarding format specifiers) + MessageFormat.format("Result {0}.", value, value); // Noncompliant; 2nd argument is not used + MessageFormat.format("Result {0}.", myObject.toString()); // Noncompliant; no need to call toString() on objects + + java.util.Logger logger; + logger.log(java.util.logging.Level.SEVERE, "Result {0}.", myObject.toString()); // Noncompliant; no need to call toString() on objects + logger.log(java.util.logging.Level.SEVERE, "Result.", new Exception()); // compliant, parameter is an exception + logger.log(java.util.logging.Level.SEVERE, "Result '{0}'", 14); // Noncompliant - String contains no format specifiers. + logger.log(java.util.logging.Level.SEVERE, "Result " + param, exception); // Noncompliant; Lambda should be used to differ string concatenation. + + org.slf4j.Logger slf4jLog; + org.slf4j.Marker marker; + slf4jLog.debug(marker, "message {}"); + slf4jLog.debug(marker, "message", 1); // Noncompliant - String contains no format specifiers. + + org.apache.logging.log4j.Logger log4jLog; + log4jLog.debug("message", 1); // Noncompliant - String contains no format specifiers. + } +} \ No newline at end of file diff --git a/rules/RSPEC_3457/java/rule.adoc b/rules/RSPEC_3457/java/rule.adoc new file mode 100644 index 0000000000..b284bbdb54 --- /dev/null +++ b/rules/RSPEC_3457/java/rule.adoc @@ -0,0 +1,43 @@ +:source-highlighter: prettify +:source-language: java +:source-indent: 0 + +Because `printf`-style format strings are interpreted at runtime, rather than validated by the compiler, they can contain errors that result in the wrong strings being created. This rule statically validates the correlation of `printf`-style format strings to their arguments when calling the `format(...)` methods of `java.util.Formatter`, `java.lang.String`, `java.io.PrintStream`, `MessageFormat`, and `java.io.PrintWriter` classes and the `printf(...)` methods of `java.io.PrintStream` or `java.io.PrintWriter` classes. + +== Noncompliant Code Example +---- +include::noncompliant.java[lines=6..12] +---- +---- +include::noncompliant.java[lines=14..16] +---- +---- +include::noncompliant.java[lines=18..22] +---- +---- +include::noncompliant.java[lines=24..27] +---- +---- +include::noncompliant.java[lines=29..30] +---- + +== Compliant Solution +[source,indent=0] +---- +include::compliant.java[lines=6..12] +---- +---- +include::compliant.java[lines=14..16] +---- +---- +include::compliant.java[lines=18..21] +---- +---- +include::compliant.java[lines=23..26] +---- +---- +include::compliant.java[lines=28..29] +---- + +== See +include::../see.adoc[] \ No newline at end of file diff --git a/rules/RSPEC_3457/metadata.json b/rules/RSPEC_3457/metadata.json new file mode 100644 index 0000000000..9172c5b6aa --- /dev/null +++ b/rules/RSPEC_3457/metadata.json @@ -0,0 +1,18 @@ +{ + "title": "Printf-style format strings should be used correctly", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "10min" + }, + "tags": [ + "confusing" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-3457", + "sqKey": "S3457", + "scope": "All", + "qualityProfile": "Sonar way" +} + \ No newline at end of file diff --git a/rules/RSPEC_3457/python/compliant.py b/rules/RSPEC_3457/python/compliant.py new file mode 100644 index 0000000000..acff8e4687 --- /dev/null +++ b/rules/RSPEC_3457/python/compliant.py @@ -0,0 +1,10 @@ +"Error %(message)s" % {"message": "something failed"} + +"Error: User {} has not been able to access {}".format("Alice", "MyFile") + +user = "Alice" +resource = "MyFile" +message = f"Error: User {user} has not been able to access {resource}" + +import logging +logging.error("Error: User %s has not been able to access %s", "Alice", "MyFile") \ No newline at end of file diff --git a/rules/RSPEC_3457/python/metadata.json b/rules/RSPEC_3457/python/metadata.json new file mode 100644 index 0000000000..f3207b28a0 --- /dev/null +++ b/rules/RSPEC_3457/python/metadata.json @@ -0,0 +1,3 @@ +{ + "title": "String formatting should be used correctly" +} diff --git a/rules/RSPEC_3457/python/noncompliant.py b/rules/RSPEC_3457/python/noncompliant.py new file mode 100644 index 0000000000..685093d799 --- /dev/null +++ b/rules/RSPEC_3457/python/noncompliant.py @@ -0,0 +1,10 @@ +"Error %(message)s" % {"message": "something failed", "extra": "some dead code"} # Noncompliant. Remove the unused argument "extra" or add a replacement field. + +"Error: User {} has not been able to access []".format("Alice", "MyFile") # Noncompliant. Remove 1 unexpected argument or add a replacement field. + +user = "Alice" +resource = "MyFile" +message = f"Error: User [user] has not been able to access [resource]" # Noncompliant. Add replacement fields or use a normal string instead of an f-string. + +import logging +logging.error("Error: User %s has not been able to access %s", "Alice") # Noncompliant. Add 1 missing argument. \ No newline at end of file diff --git a/rules/RSPEC_3457/python/rule.adoc b/rules/RSPEC_3457/python/rule.adoc new file mode 100644 index 0000000000..64a02a8405 --- /dev/null +++ b/rules/RSPEC_3457/python/rule.adoc @@ -0,0 +1,35 @@ +:source-highlighter: prettify +:source-language: python +:source-indent: 0 + +Formatting strings, either with the `%` operator or `str.format` method, requires a valid string and arguments matching this string's replacement fields. + +This also applies to loggers from the `logging` module. Internally they use `%-formatting`. The only difference is that they will log an error instead of raising an exception when provided arguments are invalid. + +Formatted string literals, also called "f-strings", are generally simpler to use, and any syntax mistake will fail at compile time. However it is easy to forget curly braces and it won't raise any error. + +This rule raises an issue when: +* A string formatted with `%` will not return the expected string because some arguments are not used. +* A string formatted with `str.format` will not return the expected string because some arguments are not used. +* An "f-string" doesn't contain any replacement field, which probably means that some curly braces are missing. +* Loggers will log an error because their message is not formatted properly. + +// TODO add a MACRO to replace reference to rule S2275 +Rule S2275 covers cases where formatting a string will raise an exception. + +== Noncompliant Code Example +---- +include::noncompliant.py[] +---- + +== Compliant Solution +---- +include::noncompliant.py[] +---- + +== See +* https://docs.python.org/3/library/string.html#format-string-syntax[Python documentation - Format String Syntax] +* https://docs.python.org/3/library/stdtypes.html#printf-style-string-formatting[Python documentation - printf-style String Formatting] +* https://docs.python.org/3/howto/logging.html#loggers[Python documentation - Loggers] +* https://docs.python.org/3/howto/logging-cookbook.html#using-particular-formatting-styles-throughout-your-application[Python documentation - Using particular formatting styles throughout your application] +* https://docs.python.org/3/reference/lexical_analysis.html#formatted-string-literals[Python documentation - Formatted string literals] \ No newline at end of file diff --git a/rules/RSPEC_3457/see.adoc b/rules/RSPEC_3457/see.adoc new file mode 100644 index 0000000000..80c769f33b --- /dev/null +++ b/rules/RSPEC_3457/see.adoc @@ -0,0 +1 @@ +* https://www.securecoding.cert.org/confluence/x/wQA1[CERT, FIO47-C.] - Use valid format strings \ No newline at end of file