From 63e562296e8ec6b6a4cb34f2cd27f542110ca58d Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 16 Jan 2023 14:25:49 -0500 Subject: [PATCH 01/10] Declarations7: add RULE-8-12 --- .vscode/tasks.json | 1 + ...lueImplicitEnumerationConstantNotUnique.ql | 38 +++++++++++++++++++ ...licitEnumerationConstantNotUnique.expected | 1 + ...ImplicitEnumerationConstantNotUnique.qlref | 1 + c/misra/test/rules/RULE-8-12/test.c | 4 ++ .../cpp/exclusions/c/Declarations7.qll | 26 +++++++++++++ .../cpp/exclusions/c/RuleMetadata.qll | 3 ++ rule_packages/c/Declarations7.json | 24 ++++++++++++ rules.csv | 2 +- 9 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 c/misra/src/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.ql create mode 100644 c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.expected create mode 100644 c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.qlref create mode 100644 c/misra/test/rules/RULE-8-12/test.c create mode 100644 cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll create mode 100644 rule_packages/c/Declarations7.json diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 97bc7c4800..8763b730cf 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -208,6 +208,7 @@ "Declarations2", "Declarations3", "Declarations4", + "Declarations7", "Exceptions1", "Exceptions2", "Expressions", diff --git a/c/misra/src/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.ql b/c/misra/src/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.ql new file mode 100644 index 0000000000..0772da9b05 --- /dev/null +++ b/c/misra/src/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.ql @@ -0,0 +1,38 @@ +/** + * @id c/misra/value-implicit-enumeration-constant-not-unique + * @name RULE-8-12: Within an enumerator list, the value of an implicitly-specified enumeration constant shall be unique + * @description Using an implicitly specified enumeration constant that is not unique (with respect + * to an explicitly specified constant) can lead to unexpected program behaviour. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/misra/id/rule-8-12 + * correctness + * readability + * external/misra/obligation/required + */ + +import cpp +import codingstandards.c.misra + +/** + * An `EnumConstant` that has an implicitly specified value: + * `enum e { explicit = 1, implicit }` + */ +class ImplicitlySpecifiedEnumConstant extends EnumConstant { + ImplicitlySpecifiedEnumConstant() { + //implicitly specified have an initializer with location: `file://:0:0:0:0` + not this.getInitializer().getLocation().getFile() = this.getFile() + } +} + +from EnumConstant exp, ImplicitlySpecifiedEnumConstant imp +where + not isExcluded(exp, Declarations7Package::valueImplicitEnumerationConstantNotUniqueQuery()) and + not isExcluded(imp, Declarations7Package::valueImplicitEnumerationConstantNotUniqueQuery()) and + not exp = imp and + imp.getValue() = exp.getValue() and + imp.getDeclaringEnum() = exp.getDeclaringEnum() and + //can technically be the same declared enum across multiple headers but those are not relevant to this rule + imp.getFile() = exp.getFile() +select imp, "Nonunique value of enum constant compared to $@", exp, exp.getName() diff --git a/c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.expected b/c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.expected new file mode 100644 index 0000000000..b0e9365975 --- /dev/null +++ b/c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.expected @@ -0,0 +1 @@ +| test.c:3:18:3:19 | c4 | Nonunique value of enum constant compared to $@ | test.c:3:22:3:23 | c5 | c5 | diff --git a/c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.qlref b/c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.qlref new file mode 100644 index 0000000000..e43c765d37 --- /dev/null +++ b/c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.qlref @@ -0,0 +1 @@ +rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-8-12/test.c b/c/misra/test/rules/RULE-8-12/test.c new file mode 100644 index 0000000000..c4875b0ba2 --- /dev/null +++ b/c/misra/test/rules/RULE-8-12/test.c @@ -0,0 +1,4 @@ +enum e {c = 3}; // COMPLIANT +enum e1 {c1 = 3, c2}; // COMPLIANT +enum e3 {c3 = 3, c4, c5 = 4}; // NON_COMPLIANT +enum e4 {c6 = 3, c7, c8, c9 = 6}; // COMPLIANT \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll new file mode 100644 index 0000000000..cbcd0b2550 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll @@ -0,0 +1,26 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype Declarations7Query = TValueImplicitEnumerationConstantNotUniqueQuery() + +predicate isDeclarations7QueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `valueImplicitEnumerationConstantNotUnique` query + Declarations7Package::valueImplicitEnumerationConstantNotUniqueQuery() and + queryId = + // `@id` for the `valueImplicitEnumerationConstantNotUnique` query + "c/misra/value-implicit-enumeration-constant-not-unique" and + ruleId = "RULE-8-12" and + category = "required" +} + +module Declarations7Package { + Query valueImplicitEnumerationConstantNotUniqueQuery() { + //autogenerate `Query` type + result = + // `Query` type for `valueImplicitEnumerationConstantNotUnique` query + TQueryC(TDeclarations7PackageQuery(TValueImplicitEnumerationConstantNotUniqueQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index d85b3e0407..fcc5e50f05 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -17,6 +17,7 @@ import Declarations1 import Declarations2 import Declarations3 import Declarations4 +import Declarations7 import Expressions import IO1 import IO2 @@ -57,6 +58,7 @@ newtype TCQuery = TDeclarations2PackageQuery(Declarations2Query q) or TDeclarations3PackageQuery(Declarations3Query q) or TDeclarations4PackageQuery(Declarations4Query q) or + TDeclarations7PackageQuery(Declarations7Query q) or TExpressionsPackageQuery(ExpressionsQuery q) or TIO1PackageQuery(IO1Query q) or TIO2PackageQuery(IO2Query q) or @@ -97,6 +99,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat isDeclarations2QueryMetadata(query, queryId, ruleId, category) or isDeclarations3QueryMetadata(query, queryId, ruleId, category) or isDeclarations4QueryMetadata(query, queryId, ruleId, category) or + isDeclarations7QueryMetadata(query, queryId, ruleId, category) or isExpressionsQueryMetadata(query, queryId, ruleId, category) or isIO1QueryMetadata(query, queryId, ruleId, category) or isIO2QueryMetadata(query, queryId, ruleId, category) or diff --git a/rule_packages/c/Declarations7.json b/rule_packages/c/Declarations7.json new file mode 100644 index 0000000000..7629709cd5 --- /dev/null +++ b/rule_packages/c/Declarations7.json @@ -0,0 +1,24 @@ +{ + "MISRA-C-2012": { + "RULE-8-12": { + "properties": { + "obligation": "required" + }, + "queries": [ + { + "description": "Using an implicitly specified enumeration constant that is not unique (with respect to an explicitly specified constant) can lead to unexpected program behaviour.", + "kind": "problem", + "name": "Within an enumerator list, the value of an implicitly-specified enumeration constant shall be unique", + "precision": "very-high", + "severity": "error", + "short_name": "ValueImplicitEnumerationConstantNotUnique", + "tags": [ + "correctness", + "readability" + ] + } + ], + "title": "Within an enumerator list, the value of an implicitly-specified enumeration constant shall be unique" + } + } +} \ No newline at end of file diff --git a/rules.csv b/rules.csv index 1e299ae44c..5f4880955d 100644 --- a/rules.csv +++ b/rules.csv @@ -657,7 +657,7 @@ c,MISRA-C-2012,RULE-8-8,Yes,Required,,,The static storage class specifier shall c,MISRA-C-2012,RULE-8-9,Yes,Advisory,,,An object should be defined at block scope if its identifier only appears in a single function,M3-4-1,Declarations,Medium, c,MISRA-C-2012,RULE-8-10,Yes,Required,,,An inline function shall be declared with the static storage class,,Declarations,Medium, c,MISRA-C-2012,RULE-8-11,Yes,Advisory,,,"When an array with external linkage is declared, its size should be explicitly specified",,Declarations,Medium, -c,MISRA-C-2012,RULE-8-12,Yes,Required,,,"Within an enumerator list, the value of an implicitly-specified enumeration constant shall be unique",,Declarations,Medium, +c,MISRA-C-2012,RULE-8-12,Yes,Required,,,"Within an enumerator list, the value of an implicitly-specified enumeration constant shall be unique",,Declarations7,Medium, c,MISRA-C-2012,RULE-8-13,Yes,Advisory,,,A pointer should point to a const-qualified type whenever possible,,Pointers1,Medium, c,MISRA-C-2012,RULE-8-14,Yes,Required,,,The restrict type qualifier shall not be used,,Banned,Easy, c,MISRA-C-2012,RULE-9-1,Yes,Mandatory,,,The value of an object with automatic storage duration shall not be read before it has been set,,InvalidMemory,Medium, From 2c971a634bfa79dab0930e2433238dd7d56ace0d Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 16 Jan 2023 14:32:27 -0500 Subject: [PATCH 02/10] Declarations7: reformat testcase RULE-8-12 --- .../ValueImplicitEnumerationConstantNotUnique.expected | 2 +- c/misra/test/rules/RULE-8-12/test.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.expected b/c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.expected index b0e9365975..55abb72b57 100644 --- a/c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.expected +++ b/c/misra/test/rules/RULE-8-12/ValueImplicitEnumerationConstantNotUnique.expected @@ -1 +1 @@ -| test.c:3:18:3:19 | c4 | Nonunique value of enum constant compared to $@ | test.c:3:22:3:23 | c5 | c5 | +| test.c:3:19:3:20 | c4 | Nonunique value of enum constant compared to $@ | test.c:3:23:3:24 | c5 | c5 | diff --git a/c/misra/test/rules/RULE-8-12/test.c b/c/misra/test/rules/RULE-8-12/test.c index c4875b0ba2..349bb7867c 100644 --- a/c/misra/test/rules/RULE-8-12/test.c +++ b/c/misra/test/rules/RULE-8-12/test.c @@ -1,4 +1,4 @@ -enum e {c = 3}; // COMPLIANT -enum e1 {c1 = 3, c2}; // COMPLIANT -enum e3 {c3 = 3, c4, c5 = 4}; // NON_COMPLIANT -enum e4 {c6 = 3, c7, c8, c9 = 6}; // COMPLIANT \ No newline at end of file +enum e { c = 3 }; // COMPLIANT +enum e1 { c1 = 3, c2 }; // COMPLIANT +enum e3 { c3 = 3, c4, c5 = 4 }; // NON_COMPLIANT +enum e4 { c6 = 3, c7, c8, c9 = 6 }; // COMPLIANT \ No newline at end of file From 712d76eb8b2e070661d2d1f7ec3436c869d38083 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 17 Jan 2023 11:28:35 -0500 Subject: [PATCH 03/10] Declarations7: add RULE-18-8 --- .../RULE-18-8/VariableLengthArrayTypesUsed.ql | 55 +++++++++++++++++++ .../VariableLengthArrayTypesUsed.expected | 6 ++ .../VariableLengthArrayTypesUsed.qlref | 1 + c/misra/test/rules/RULE-18-8/test.c | 15 +++++ .../cpp/exclusions/c/Declarations7.qll | 20 ++++++- rule_packages/c/Declarations7.json | 20 +++++++ rules.csv | 2 +- 7 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql create mode 100644 c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.expected create mode 100644 c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.qlref create mode 100644 c/misra/test/rules/RULE-18-8/test.c diff --git a/c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql b/c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql new file mode 100644 index 0000000000..c1930ff70d --- /dev/null +++ b/c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql @@ -0,0 +1,55 @@ +/** + * @id c/misra/variable-length-array-types-used + * @name RULE-18-8: Variable-length array types shall not be used + * @description Using a variable length array can lead to unexpected or undefined program behaviour. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/misra/id/rule-18-8 + * correctness + * readability + * external/misra/obligation/required + */ + +import cpp +import codingstandards.c.misra + +predicate partOfConstantExpr(MacroInvocation i) { + exists(Expr e | + e.isConstant() and + not i.getExpr() = e and + i.getExpr().getParent+() = e + ) +} + +/** + * A variable length array (VLA) + * ie an array where the size + * is not an integer constant expression + */ +class VariableLengthArray extends VariableDeclarationEntry { + VariableLengthArray() { + //VLAs will not have: static/extern specifiers (compilation error) + not this.hasSpecifier("static") and + not this.hasSpecifier("extern") and + //VLAs are not allowed to be initialized + not this.getDeclaration().hasInitializer() and + exists(ArrayType a | + //a.hasArraySize() does not catch multidimensional VLAs like a[1][] + a.toString().matches("%[]%") and + this.getUnspecifiedType() = a and + //variable length array is one declared in block or function prototype + ( + this.getDeclaration().getParentScope() instanceof Function or + this.getDeclaration().getParentScope() instanceof BlockStmt + ) + ) + } +} + +from VariableLengthArray v +where + not isExcluded(v, Declarations7Package::variableLengthArrayTypesUsedQuery()) and + //an exception, argv in : int main(int argc, char *argv[]) + not v.getDeclaration().getParentScope().(Function).hasName("main") +select v, "Variable length array declared." diff --git a/c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.expected b/c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.expected new file mode 100644 index 0000000000..24bf35a90d --- /dev/null +++ b/c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.expected @@ -0,0 +1,6 @@ +WARNING: Unused predicate partOfConstantExpr (/Users/knewbury/Desktop/GITHUB/coding-standards/codeql-coding-standards/c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql:17,11-29) +| test.c:3:19:3:20 | definition of pa | Variable length array declared. | +| test.c:6:7:6:8 | definition of a1 | Variable length array declared. | +| test.c:7:7:7:8 | definition of a2 | Variable length array declared. | +| test.c:8:7:8:8 | definition of a3 | Variable length array declared. | +| test.c:14:20:14:21 | definition of pa | Variable length array declared. | diff --git a/c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.qlref b/c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.qlref new file mode 100644 index 0000000000..9193742acd --- /dev/null +++ b/c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.qlref @@ -0,0 +1 @@ +rules/RULE-18-8/VariableLengthArrayTypesUsed.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-18-8/test.c b/c/misra/test/rules/RULE-18-8/test.c new file mode 100644 index 0000000000..3a0a040f6d --- /dev/null +++ b/c/misra/test/rules/RULE-18-8/test.c @@ -0,0 +1,15 @@ +#define TEST 1 + +void f(int n, int pa[1][n]) { // NON_COMPLIANT + int a[1]; // COMPLIANT + int x = 1; + int a1[1 + x]; // NON_COMPLIANT - not integer constant expr + int a2[n]; // NON_COMPLIANT + int a3[1][n]; // NON_COMPLIANT + int a4[] = {1}; // COMPLIANT - not a VLA + int a5[TEST]; // COMPLIANT + int a6[1 + 1]; // COMPLIANT +} + +void f1(int n, int pa[n]) { // NON_COMPLIANT +} \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll index cbcd0b2550..62bd618396 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll @@ -3,9 +3,20 @@ import cpp import RuleMetadata import codingstandards.cpp.exclusions.RuleMetadata -newtype Declarations7Query = TValueImplicitEnumerationConstantNotUniqueQuery() +newtype Declarations7Query = + TVariableLengthArrayTypesUsedQuery() or + TValueImplicitEnumerationConstantNotUniqueQuery() predicate isDeclarations7QueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `variableLengthArrayTypesUsed` query + Declarations7Package::variableLengthArrayTypesUsedQuery() and + queryId = + // `@id` for the `variableLengthArrayTypesUsed` query + "c/misra/variable-length-array-types-used" and + ruleId = "RULE-18-8" and + category = "required" + or query = // `Query` instance for the `valueImplicitEnumerationConstantNotUnique` query Declarations7Package::valueImplicitEnumerationConstantNotUniqueQuery() and @@ -17,6 +28,13 @@ predicate isDeclarations7QueryMetadata(Query query, string queryId, string ruleI } module Declarations7Package { + Query variableLengthArrayTypesUsedQuery() { + //autogenerate `Query` type + result = + // `Query` type for `variableLengthArrayTypesUsed` query + TQueryC(TDeclarations7PackageQuery(TVariableLengthArrayTypesUsedQuery())) + } + Query valueImplicitEnumerationConstantNotUniqueQuery() { //autogenerate `Query` type result = diff --git a/rule_packages/c/Declarations7.json b/rule_packages/c/Declarations7.json index 7629709cd5..c938366a62 100644 --- a/rule_packages/c/Declarations7.json +++ b/rule_packages/c/Declarations7.json @@ -1,5 +1,25 @@ { "MISRA-C-2012": { + "RULE-18-8": { + "properties": { + "obligation": "required" + }, + "queries": [ + { + "description": "Using a variable length array can lead to unexpected or undefined program behaviour.", + "kind": "problem", + "name": "Variable-length array types shall not be used", + "precision": "very-high", + "severity": "error", + "short_name": "VariableLengthArrayTypesUsed", + "tags": [ + "correctness", + "readability" + ] + } + ], + "title": "Variable-length array types shall not be used" + }, "RULE-8-12": { "properties": { "obligation": "required" diff --git a/rules.csv b/rules.csv index 5f4880955d..e4bb168cd8 100644 --- a/rules.csv +++ b/rules.csv @@ -726,7 +726,7 @@ c,MISRA-C-2012,RULE-18-4,Yes,Advisory,,,"The +, -, += and -= operators should no c,MISRA-C-2012,RULE-18-5,Yes,Advisory,,,Declarations should contain no more than two levels of pointer nesting,A5-0-3,Pointers1,Import, c,MISRA-C-2012,RULE-18-6,Yes,Required,,,The address of an object with automatic storage shall not be copied to another object that persists after the first object has ceased to exist,M7-5-2,Pointers1,Import, c,MISRA-C-2012,RULE-18-7,Yes,Required,,,Flexible array members shall not be declared,,Declarations,Medium, -c,MISRA-C-2012,RULE-18-8,Yes,Required,,,Variable-length array types shall not be used,,Declarations,Medium, +c,MISRA-C-2012,RULE-18-8,Yes,Required,,,Variable-length array types shall not be used,,Declarations7,Medium, c,MISRA-C-2012,RULE-19-1,Yes,Mandatory,,,An object shall not be assigned or copied to an overlapping object,M0-2-1,Contracts,Hard, c,MISRA-C-2012,RULE-19-2,Yes,Advisory,,,The union keyword should not be used,A9-5-1,Banned,Import, c,MISRA-C-2012,RULE-20-1,Yes,Advisory,,,#include directives should only be preceded by preprocessor directives or comments,M16-0-1,Preprocessor1,Import, From 2a1ac4012845463d413b599600bb7e8eaeb00f37 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 17 Jan 2023 12:34:18 -0500 Subject: [PATCH 04/10] Declarations7: fix unused leftover predicate RULE-18-8 --- .../src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql | 8 -------- 1 file changed, 8 deletions(-) diff --git a/c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql b/c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql index c1930ff70d..00d02cdc02 100644 --- a/c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql +++ b/c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql @@ -14,14 +14,6 @@ import cpp import codingstandards.c.misra -predicate partOfConstantExpr(MacroInvocation i) { - exists(Expr e | - e.isConstant() and - not i.getExpr() = e and - i.getExpr().getParent+() = e - ) -} - /** * A variable length array (VLA) * ie an array where the size From b7b7ea754883a8acedfb18288efab2a11c50fba6 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 24 Jan 2023 10:26:46 -0500 Subject: [PATCH 05/10] Declarations7: omit DCL36-C and add DCL39-C --- ...nformationLeakageAcrossTrustBoundariesC.md | 18 ++ ...nformationLeakageAcrossTrustBoundariesC.ql | 22 +++ ...ationLeakageAcrossTrustBoundariesC.testref | 1 + ...nformationLeakageAcrossBoundaries.expected | 18 ++ .../InformationLeakageAcrossBoundaries.ql | 2 + .../arrays.c | 62 +++++++ .../interprocedural.c | 107 +++++++++++ .../multilayer.c | 36 ++++ .../informationleakageacrossboundaries/test.c | 167 ++++++++++++++++++ ...InformationLeakageAcrossTrustBoundaries.ql | 12 +- ...ormationLeakageAcrossTrustBoundaries.qlref | 1 - ...mationLeakageAcrossTrustBoundaries.testref | 1 + .../cpp/exclusions/c/Declarations7.qll | 17 ++ .../InformationLeakageAcrossBoundaries.qll | 17 ++ ...icSpecifierFunctionRedeclarationShared.qll | 9 +- ...formationLeakageAcrossBoundaries.expected} | 0 .../InformationLeakageAcrossBoundaries.ql | 2 + ...ationLeakageAcrossTrustBoundaries.expected | 20 +++ .../arrays.cpp | 0 .../inheritance.cpp | 0 .../interprocedural.cpp | 0 .../multilayer.cpp | 0 .../test.cpp | 0 rule_packages/c/Declarations7.json | 22 +++ rule_packages/cpp/Uninitialized.json | 1 + rules.csv | 4 +- 26 files changed, 529 insertions(+), 10 deletions(-) create mode 100644 c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md create mode 100644 c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.ql create mode 100644 c/cert/test/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.testref create mode 100644 c/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.expected create mode 100644 c/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.ql create mode 100644 c/common/test/rules/informationleakageacrossboundaries/arrays.c create mode 100644 c/common/test/rules/informationleakageacrossboundaries/interprocedural.c create mode 100644 c/common/test/rules/informationleakageacrossboundaries/multilayer.c create mode 100644 c/common/test/rules/informationleakageacrossboundaries/test.c delete mode 100644 cpp/cert/test/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.qlref create mode 100644 cpp/cert/test/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.testref create mode 100644 cpp/common/src/codingstandards/cpp/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.qll rename cpp/{cert/test/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.expected => common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.expected} (100%) create mode 100644 cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.ql create mode 100644 cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossTrustBoundaries.expected rename cpp/{cert/test/rules/DCL55-CPP => common/test/rules/informationleakageacrossboundaries}/arrays.cpp (100%) rename cpp/{cert/test/rules/DCL55-CPP => common/test/rules/informationleakageacrossboundaries}/inheritance.cpp (100%) rename cpp/{cert/test/rules/DCL55-CPP => common/test/rules/informationleakageacrossboundaries}/interprocedural.cpp (100%) rename cpp/{cert/test/rules/DCL55-CPP => common/test/rules/informationleakageacrossboundaries}/multilayer.cpp (100%) rename cpp/{cert/test/rules/DCL55-CPP => common/test/rules/informationleakageacrossboundaries}/test.cpp (100%) diff --git a/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md b/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md new file mode 100644 index 0000000000..58cc3bd15b --- /dev/null +++ b/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md @@ -0,0 +1,18 @@ +# DCL39-C: Avoid information leakage when passing a structure across a trust boundary + +This query implements the CERT-C rule DCL39-C: + +> Avoid information leakage when passing a structure across a trust boundary + + +## CERT + +** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** + +## Implementation notes + +None + +## References + +* CERT-C: [DCL39-C: Avoid information leakage when passing a structure across a trust boundary](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.ql b/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.ql new file mode 100644 index 0000000000..7a8a0e6d46 --- /dev/null +++ b/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.ql @@ -0,0 +1,22 @@ +/** + * @id c/cert/information-leakage-across-trust-boundaries-c + * @name DCL39-C: Avoid information leakage when passing a structure across a trust boundary + * @description Passing a structure with uninitialized fields or padding bytes can cause information + * to be unintentionally leaked. + * @kind problem + * @precision medium + * @problem.severity error + * @tags external/cert/id/dcl39-c + * security + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.rules.informationleakageacrossboundaries.InformationLeakageAcrossBoundaries + +class InformationLeakageAcrossTrustBoundariesCQuery extends InformationLeakageAcrossBoundariesSharedQuery { + InformationLeakageAcrossTrustBoundariesCQuery() { + this = Declarations7Package::informationLeakageAcrossTrustBoundariesCQuery() + } +} diff --git a/c/cert/test/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.testref b/c/cert/test/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.testref new file mode 100644 index 0000000000..394150a10b --- /dev/null +++ b/c/cert/test/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.testref @@ -0,0 +1 @@ +c/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.ql \ No newline at end of file diff --git a/c/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.expected b/c/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.expected new file mode 100644 index 0000000000..e4a9a1cee3 --- /dev/null +++ b/c/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.expected @@ -0,0 +1,18 @@ +| arrays.c:11:20:11:21 | wa | 'wa' may leak information from {elements of a[...] (arrays.c:7)}. Path: wa (arrays.c:11) --> & ... (arrays.c:12) | +| arrays.c:33:22:33:23 | wa | 'wa' may leak information from {elements of elements of a[...][...] (arrays.c:29)}. Path: wa (arrays.c:33) --> & ... (arrays.c:34) | +| arrays.c:57:22:57:23 | wa | 'wa' may leak information from {WithPointer (arrays.c:52)}. Path: wa (arrays.c:57) --> & ... (arrays.c:59) | +| interprocedural.c:37:9:37:9 | p | 'p' may leak information from {y (interprocedural.c:8)}. Path: p (interprocedural.c:37) --> past assign_x (interprocedural.c:32) --> & ... (interprocedural.c:39) | +| interprocedural.c:104:9:104:9 | p | 'p' may leak information from {x (interprocedural.c:7), y (interprocedural.c:8)}. Path: p (interprocedural.c:104) --> overwrite_after_leak(...) (interprocedural.c:96) --> p (interprocedural.c:97) | +| multilayer.c:16:10:16:10 | s | 's' may leak information from {b (multilayer.c:12)}. Path: s (multilayer.c:16) --> & ... (multilayer.c:18) | +| multilayer.c:29:10:29:10 | s | 's' may leak information from {b (multilayer.c:12), x (multilayer.c:7)}. Path: s (multilayer.c:29) --> & ... (multilayer.c:30) | +| multilayer.c:34:8:34:8 | s | 's' may leak information from {struct (multilayer.c:6)}. Path: s (multilayer.c:34) --> & ... (multilayer.c:35) | +| test.c:12:12:12:12 | s | 's' may leak information from {y (test.c:8)}. Path: s (test.c:12) --> & ... (test.c:14) | +| test.c:18:12:18:12 | s | 's' may leak information from {x (test.c:7)}. Path: s (test.c:18) --> & ... (test.c:20) | +| test.c:24:12:24:12 | s | 's' may leak information from {x (test.c:7), y (test.c:8)}. Path: s (test.c:24) --> & ... (test.c:25) | +| test.c:36:12:36:12 | s | 's' may leak information from {y (test.c:8)}. Path: s (test.c:36) --> & ... (test.c:38) | +| test.c:43:12:43:12 | s | 's' may leak information from {x (test.c:7)}. Path: s (test.c:43) --> & ... (test.c:47) | +| test.c:58:12:58:12 | s | 's' may leak information from {x (test.c:7), y (test.c:8)}. Path: s (test.c:58) --> & ... (test.c:59) | +| test.c:64:12:64:12 | s | 's' may leak information from {y (test.c:8)}. Path: s (test.c:64) --> & ... (test.c:66) | +| test.c:112:16:112:16 | s | 's' may leak information from {buf (test.c:92)}. Path: s (test.c:112) --> & ... (test.c:115) | +| test.c:128:12:128:12 | s | 's' may leak information from {x (test.c:7), y (test.c:8)}. Path: s (test.c:128) --> & ... (test.c:132) | +| test.c:157:22:157:22 | s | 's' may leak information from {2 to 2 bytes of padding in has_padding (test.c:151)}. Path: s (test.c:157) --> & ... (test.c:160) | diff --git a/c/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.ql b/c/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.ql new file mode 100644 index 0000000000..4e603a2e36 --- /dev/null +++ b/c/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.informationleakageacrossboundaries.InformationLeakageAcrossBoundaries diff --git a/c/common/test/rules/informationleakageacrossboundaries/arrays.c b/c/common/test/rules/informationleakageacrossboundaries/arrays.c new file mode 100644 index 0000000000..875af58934 --- /dev/null +++ b/c/common/test/rules/informationleakageacrossboundaries/arrays.c @@ -0,0 +1,62 @@ +#include +#include + +unsigned long copy_to_user(void *to, const void *from, unsigned long n); + +struct WithArray { + int a[2]; +}; + +void forget_array() { + struct WithArray wa; + copy_to_user(0, &wa, sizeof wa); // NON_COMPLIANT +} + +void write_partial_array() { + struct WithArray wa; + wa.a[0] = 1; + copy_to_user(0, &wa, sizeof wa); // NON_COMPLIANT[FALSE NEGATIVE] +} + +void write_full_array() { + struct WithArray wa; + wa.a[0] = 1; + wa.a[1] = 1; + copy_to_user(0, &wa, sizeof wa); // COMPLIANT +} + +struct WithArray2D { + int a[2][1]; +}; + +void forget_array2d() { + struct WithArray2D wa; + copy_to_user(0, &wa, sizeof wa); // NON_COMPLIANT +} + +void write_partial_array2d() { + struct WithArray2D wa; + wa.a[0][0] = 1; + copy_to_user(0, &wa, sizeof wa); // NON_COMPLIANT[FALSE NEGATIVE] +} + +void write_full_array2d() { + struct WithArray2D wa; + wa.a[0][0] = 1; + wa.a[1][0] = 1; + copy_to_user(0, &wa, sizeof wa); // COMPLIANT +} + +// A pointer field allows mostly the same syntactic operations as an array +// field, but the semantics are completely different. +struct WithPointer { + int *a; +}; + +void pointer_array_expression() { + struct WithPointer wa; + wa.a[0] = 1; + copy_to_user(0, &wa, sizeof wa); // NON_COMPLIANT +} + +// TODO: test a struct in an array \ No newline at end of file diff --git a/c/common/test/rules/informationleakageacrossboundaries/interprocedural.c b/c/common/test/rules/informationleakageacrossboundaries/interprocedural.c new file mode 100644 index 0000000000..e03d5fcc6e --- /dev/null +++ b/c/common/test/rules/informationleakageacrossboundaries/interprocedural.c @@ -0,0 +1,107 @@ +#include +#include + +unsigned long copy_to_user(void *to, const void *from, unsigned long n); + +typedef struct _point { + int x; + int y; +} point; + +void callee1(point *p) { + p->y = 1; + copy_to_user(0, p, sizeof(point)); // COMPLIANT +} + +void caller1() { + point p; + p.x = 1; + callee1(&p); +} + +void callee2(point *p) { + memset(p, 0, sizeof(point)); + copy_to_user(0, p, sizeof(point)); // COMPLIANT +} + +void caller2() { + point p; + callee2(&p); +} + +void assign_x(point *p, int value) { p->x = value; } + +void zero_y(point *p) { memset(&p->y, 0, sizeof(p->y)); } + +void call_to_overwrite_x() { + point p; + assign_x(&p, 1); + copy_to_user(0, &p, sizeof p); // NON_COMPLIANT +} + +void call_to_overwrite_both() { + point p; + assign_x(&p, 1); + zero_y(&p); + copy_to_user(0, &p, sizeof p); // COMPLIANT +} + +void zero_y_and_loop(point *p) { + int i; + memset(&p->y, 0, sizeof(p->y)); + for (i = 0; i < 10; i++) { + p->y++; + } +} + +void call_zero_y_and_loop() { + point p; + zero_y_and_loop(&p); + assign_x(&p, 1); + copy_to_user(0, &p, sizeof p); // COMPLIANT +} + +int zero_y_or_fail(point *p) { + if (p->x < 0) { + return 0; + } + p->y = 0; + return 1; +} + +void call_zero_y_or_fail(int i) { + point p; + p.x = i; + if (!zero_y_or_fail(&p)) { + return; + } + copy_to_user(0, &p, sizeof p); // COMPLIANT +} + +int zero_y_proxy(point *p) { + if (p->x) { + zero_y(p); + } else { + zero_y(p); + } +} + +void call_zero_y_proxy() { + point p; + zero_y_proxy(&p); + assign_x(&p, 1); + copy_to_user(0, &p, sizeof p); // COMPLIANT +} + +void overwrite_after_leak(point *p) { + copy_to_user(0, p, sizeof(*p)); // NON_COMPLIANT + + p->x = 0; + p->y = 0; +} + +void call_overwrite_after_leak(void) { + point p; + overwrite_after_leak(&p); + copy_to_user(0, &p, sizeof p); // COMPLIANT +} \ No newline at end of file diff --git a/c/common/test/rules/informationleakageacrossboundaries/multilayer.c b/c/common/test/rules/informationleakageacrossboundaries/multilayer.c new file mode 100644 index 0000000000..7fad75429f --- /dev/null +++ b/c/common/test/rules/informationleakageacrossboundaries/multilayer.c @@ -0,0 +1,36 @@ +#include +#include + +unsigned long copy_to_user(void *to, const void *from, unsigned long n); + +typedef struct { + int x; +} intx; + +typedef struct { + intx a; + intx b; +} intxab; + +void forget_y() { + intxab s; + s.a.x = 1; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT (y) +} + +void set_both() { + intxab s; + s.a.x = 1; + memset(&s.b, 0, sizeof s.b); + copy_to_user(0, &s, sizeof s); // COMPLIANT +} + +void set_none() { + intxab s; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT (both) +} + +void set_none_intx() { + intx s; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT (x) +} \ No newline at end of file diff --git a/c/common/test/rules/informationleakageacrossboundaries/test.c b/c/common/test/rules/informationleakageacrossboundaries/test.c new file mode 100644 index 0000000000..f17ca8fb87 --- /dev/null +++ b/c/common/test/rules/informationleakageacrossboundaries/test.c @@ -0,0 +1,167 @@ +#include +#include + +unsigned long copy_to_user(void *to, const void *from, unsigned long n); + +typedef struct { + int x; + int y; +} MyStruct; + +void forget_y() { + MyStruct s; + s.x = 1; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT (y) +} + +void forget_x() { + MyStruct s; + s.y = 1; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT (x) +} + +void forget_both() { + MyStruct s; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT (x, y) +} + +void init_both() { + MyStruct s; + s.x = 1; + s.y = 1; + copy_to_user(0, &s, sizeof s); // COMPLIANT +} + +void init_after() { + MyStruct s; + s.x = 1; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT + s.y = 1; +} + +void init_other() { + MyStruct s, t; + s.y = 1; + t.x = 1; + t.y = 1; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT (x) + copy_to_user(0, &t, sizeof t); // COMPLIANT +} + +void zero_memory() { + MyStruct s; + memset(&s, 0, sizeof s); + copy_to_user(0, &s, sizeof s); // COMPLIANT +} + +void zero_memory_after() { + MyStruct s; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT + memset(&s, 0, sizeof s); +} + +void zero_field() { + MyStruct s; + memset(&s.x, 0, sizeof s.x); + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT (y) +} + +void overwrite_with_zeroed() { + MyStruct s, t; + memset(&t, 0, sizeof t); + s = t; + copy_to_user(0, &s, sizeof s); // COMPLIANT +} + +void overwrite_struct_with_uninit() { + MyStruct s, t; + s = t; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT[FALSE NEGATIVE] +} + +void overwrite_field_with_uninit() { + MyStruct s; + int x; + s.x = x; + s.y = 1; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT[FALSE NEGATIVE] +} + +typedef struct { + size_t length; + char buf[128]; +} PascalString; + +void zero_array() { + PascalString s; + memset(s.buf, 0, sizeof s.buf); + s.length = 0; + copy_to_user(0, &s, sizeof s); // COMPLIANT +} + +void zero_array_by_ref() { + PascalString s; + memset(&s.buf, 0, sizeof s.buf); + s.length = 0; + copy_to_user(0, &s, sizeof s); // COMPLIANT +} + +char *strcpy(char *dst, const char *src); + +void use_strcpy() { + PascalString s; + strcpy(s.buf, "Hello, World"); // does not zero rest of s.buf + s.length = strlen(s.buf); + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT (buf) +} + +void *malloc(size_t size); + +void heap_memory() { + MyStruct *s; + s = (MyStruct *)malloc(sizeof(*s)); + s->x = 1; + copy_to_user(0, s, sizeof(*s)); // NON_COMPLIANT[FALSE NEGATIVE] +} + +void conditional_memset(int b) { + MyStruct s; + if (b) { + memset(&s, 0, sizeof s); + } + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT +} + +void memset_field() { + MyStruct s; + memset(&s.x, 0, sizeof(s.x)); + s.y = 1; + copy_to_user(0, &s, sizeof s); // COMPLIANT +} + +const static int one = 1; +void zero_if_true() { + MyStruct s; + if (one) { + memset(&s, 0, sizeof s); + } + copy_to_user(0, &s, sizeof s); // COMPLIANT +} + +struct has_padding { + short s; + int i; +}; + +void forget_padding() { + struct has_padding s; + s.s = 1; + s.i = 1; + copy_to_user(0, &s, sizeof s); // NON_COMPLIANT +} + +void remember_padding() { + struct has_padding s; + memset(&s, 0, sizeof s); + copy_to_user(0, &s, sizeof s); // COMPLIANT +} \ No newline at end of file diff --git a/cpp/cert/src/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.ql b/cpp/cert/src/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.ql index 68b4ae5e3c..e3061a0314 100644 --- a/cpp/cert/src/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.ql +++ b/cpp/cert/src/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.ql @@ -13,10 +13,10 @@ import cpp import codingstandards.cpp.cert -import codingstandards.cpp.trustboundary.UninitializedField +import codingstandards.cpp.rules.informationleakageacrossboundaries.InformationLeakageAcrossBoundaries -from Element e, string alertMessage -where - not isExcluded(e, UninitializedPackage::informationLeakageAcrossTrustBoundariesQuery()) and - uninitializedFieldQuery(e, alertMessage) -select e, alertMessage +class InformationLeakageAcrossTrustBoundariesQuery extends InformationLeakageAcrossBoundariesSharedQuery { + InformationLeakageAcrossTrustBoundariesQuery() { + this = UninitializedPackage::informationLeakageAcrossTrustBoundariesQuery() + } +} diff --git a/cpp/cert/test/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.qlref b/cpp/cert/test/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.qlref deleted file mode 100644 index 7fd5774344..0000000000 --- a/cpp/cert/test/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.qlref +++ /dev/null @@ -1 +0,0 @@ -rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.ql \ No newline at end of file diff --git a/cpp/cert/test/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.testref b/cpp/cert/test/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.testref new file mode 100644 index 0000000000..44035e2ee4 --- /dev/null +++ b/cpp/cert/test/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.testref @@ -0,0 +1 @@ +cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.ql \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll index 62bd618396..facb651573 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Declarations7.qll @@ -4,10 +4,20 @@ import RuleMetadata import codingstandards.cpp.exclusions.RuleMetadata newtype Declarations7Query = + TInformationLeakageAcrossTrustBoundariesCQuery() or TVariableLengthArrayTypesUsedQuery() or TValueImplicitEnumerationConstantNotUniqueQuery() predicate isDeclarations7QueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `informationLeakageAcrossTrustBoundariesC` query + Declarations7Package::informationLeakageAcrossTrustBoundariesCQuery() and + queryId = + // `@id` for the `informationLeakageAcrossTrustBoundariesC` query + "c/cert/information-leakage-across-trust-boundaries-c" and + ruleId = "DCL39-C" and + category = "rule" + or query = // `Query` instance for the `variableLengthArrayTypesUsed` query Declarations7Package::variableLengthArrayTypesUsedQuery() and @@ -28,6 +38,13 @@ predicate isDeclarations7QueryMetadata(Query query, string queryId, string ruleI } module Declarations7Package { + Query informationLeakageAcrossTrustBoundariesCQuery() { + //autogenerate `Query` type + result = + // `Query` type for `informationLeakageAcrossTrustBoundariesC` query + TQueryC(TDeclarations7PackageQuery(TInformationLeakageAcrossTrustBoundariesCQuery())) + } + Query variableLengthArrayTypesUsedQuery() { //autogenerate `Query` type result = diff --git a/cpp/common/src/codingstandards/cpp/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.qll b/cpp/common/src/codingstandards/cpp/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.qll new file mode 100644 index 0000000000..f33d5ac87c --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.qll @@ -0,0 +1,17 @@ +/** + * Provides a library which includes a `problems` predicate for reporting.... + */ + +import cpp +import codingstandards.cpp.Customizations +import codingstandards.cpp.Exclusions +import codingstandards.cpp.trustboundary.UninitializedField + +abstract class InformationLeakageAcrossBoundariesSharedQuery extends Query { } + +Query getQuery() { result instanceof InformationLeakageAcrossBoundariesSharedQuery } + +query predicate problems(Element e, string message) { + uninitializedFieldQuery(e, message) and + not isExcluded(e, getQuery()) +} diff --git a/cpp/common/src/codingstandards/cpp/rules/missingstaticspecifierfunctionredeclarationshared/MissingStaticSpecifierFunctionRedeclarationShared.qll b/cpp/common/src/codingstandards/cpp/rules/missingstaticspecifierfunctionredeclarationshared/MissingStaticSpecifierFunctionRedeclarationShared.qll index 43c1821e2e..60889ed86b 100644 --- a/cpp/common/src/codingstandards/cpp/rules/missingstaticspecifierfunctionredeclarationshared/MissingStaticSpecifierFunctionRedeclarationShared.qll +++ b/cpp/common/src/codingstandards/cpp/rules/missingstaticspecifierfunctionredeclarationshared/MissingStaticSpecifierFunctionRedeclarationShared.qll @@ -21,5 +21,12 @@ query predicate problems( not redeclaration.hasSpecifier("static") and fde != redeclaration and message = "The redeclaration of $@ with internal linkage misses the static specifier." and - msgpiece = "function" + msgpiece = "function" + and + ( + fde.getFile().getAbsolutePath() < redeclaration.getFile().getAbsolutePath() + or + fde.getFile().getAbsolutePath() = redeclaration.getFile().getAbsolutePath() and + fde.getLocation().getStartLine() < redeclaration.getLocation().getStartLine() + ) } diff --git a/cpp/cert/test/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.expected b/cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.expected similarity index 100% rename from cpp/cert/test/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.expected rename to cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.expected diff --git a/cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.ql b/cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.ql new file mode 100644 index 0000000000..4e603a2e36 --- /dev/null +++ b/cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.informationleakageacrossboundaries.InformationLeakageAcrossBoundaries diff --git a/cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossTrustBoundaries.expected b/cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossTrustBoundaries.expected new file mode 100644 index 0000000000..4f703cff78 --- /dev/null +++ b/cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossTrustBoundaries.expected @@ -0,0 +1,20 @@ +cpp/common/test/rules/informationleakageacrossboundaries/| arrays.cpp:11:20:11:21 | wa | 'wa' may leak information from {elements of a[...] (arrays.cpp:7)}. Path: wa (arrays.cpp:11) --> & ... (arrays.cpp:12) | +| arrays.cpp:33:22:33:23 | wa | 'wa' may leak information from {elements of elements of a[...][...] (arrays.cpp:29)}. Path: wa (arrays.cpp:33) --> & ... (arrays.cpp:34) | +| arrays.cpp:57:22:57:23 | wa | 'wa' may leak information from {WithPointer (arrays.cpp:52)}. Path: wa (arrays.cpp:57) --> & ... (arrays.cpp:59) | +| inheritance.cpp:19:14:19:14 | s | 's' may leak information from {i (inheritance.cpp:7)}. Path: s (inheritance.cpp:19) --> & ... (inheritance.cpp:21) | +| inheritance.cpp:32:14:32:14 | s | 's' may leak information from {0 to 4 bytes of padding in ptrDerived (inheritance.cpp:14)}. Path: s (inheritance.cpp:32) --> & ... (inheritance.cpp:35) | +| interprocedural.cpp:37:9:37:9 | p | 'p' may leak information from {y (interprocedural.cpp:8)}. Path: p (interprocedural.cpp:37) --> past assign_x (interprocedural.cpp:32) --> & ... (interprocedural.cpp:39) | +| interprocedural.cpp:104:9:104:9 | p | 'p' may leak information from {x (interprocedural.cpp:7), y (interprocedural.cpp:8)}. Path: p (interprocedural.cpp:104) --> overwrite_after_leak(...) (interprocedural.cpp:96) --> p (interprocedural.cpp:97) | +| multilayer.cpp:16:10:16:10 | s | 's' may leak information from {b (multilayer.cpp:12)}. Path: s (multilayer.cpp:16) --> & ... (multilayer.cpp:18) | +| multilayer.cpp:29:10:29:10 | s | 's' may leak information from {b (multilayer.cpp:12), x (multilayer.cpp:7)}. Path: s (multilayer.cpp:29) --> & ... (multilayer.cpp:30) | +| multilayer.cpp:34:8:34:8 | s | 's' may leak information from {intx (multilayer.cpp:6)}. Path: s (multilayer.cpp:34) --> & ... (multilayer.cpp:35) | +| test.cpp:12:12:12:12 | s | 's' may leak information from {y (test.cpp:8)}. Path: s (test.cpp:12) --> & ... (test.cpp:14) | +| test.cpp:18:12:18:12 | s | 's' may leak information from {x (test.cpp:7)}. Path: s (test.cpp:18) --> & ... (test.cpp:20) | +| test.cpp:24:12:24:12 | s | 's' may leak information from {x (test.cpp:7), y (test.cpp:8)}. Path: s (test.cpp:24) --> & ... (test.cpp:25) | +| test.cpp:36:12:36:12 | s | 's' may leak information from {y (test.cpp:8)}. Path: s (test.cpp:36) --> & ... (test.cpp:38) | +| test.cpp:43:12:43:12 | s | 's' may leak information from {x (test.cpp:7)}. Path: s (test.cpp:43) --> & ... (test.cpp:47) | +| test.cpp:58:12:58:12 | s | 's' may leak information from {x (test.cpp:7), y (test.cpp:8)}. Path: s (test.cpp:58) --> & ... (test.cpp:59) | +| test.cpp:64:12:64:12 | s | 's' may leak information from {y (test.cpp:8)}. Path: s (test.cpp:64) --> & ... (test.cpp:66) | +| test.cpp:112:16:112:16 | s | 's' may leak information from {buf (test.cpp:92)}. Path: s (test.cpp:112) --> & ... (test.cpp:115) | +| test.cpp:128:12:128:12 | s | 's' may leak information from {x (test.cpp:7), y (test.cpp:8)}. Path: s (test.cpp:128) --> & ... (test.cpp:132) | +| test.cpp:157:22:157:22 | s | 's' may leak information from {2 to 2 bytes of padding in has_padding (test.cpp:151)}. Path: s (test.cpp:157) --> & ... (test.cpp:160) | diff --git a/cpp/cert/test/rules/DCL55-CPP/arrays.cpp b/cpp/common/test/rules/informationleakageacrossboundaries/arrays.cpp similarity index 100% rename from cpp/cert/test/rules/DCL55-CPP/arrays.cpp rename to cpp/common/test/rules/informationleakageacrossboundaries/arrays.cpp diff --git a/cpp/cert/test/rules/DCL55-CPP/inheritance.cpp b/cpp/common/test/rules/informationleakageacrossboundaries/inheritance.cpp similarity index 100% rename from cpp/cert/test/rules/DCL55-CPP/inheritance.cpp rename to cpp/common/test/rules/informationleakageacrossboundaries/inheritance.cpp diff --git a/cpp/cert/test/rules/DCL55-CPP/interprocedural.cpp b/cpp/common/test/rules/informationleakageacrossboundaries/interprocedural.cpp similarity index 100% rename from cpp/cert/test/rules/DCL55-CPP/interprocedural.cpp rename to cpp/common/test/rules/informationleakageacrossboundaries/interprocedural.cpp diff --git a/cpp/cert/test/rules/DCL55-CPP/multilayer.cpp b/cpp/common/test/rules/informationleakageacrossboundaries/multilayer.cpp similarity index 100% rename from cpp/cert/test/rules/DCL55-CPP/multilayer.cpp rename to cpp/common/test/rules/informationleakageacrossboundaries/multilayer.cpp diff --git a/cpp/cert/test/rules/DCL55-CPP/test.cpp b/cpp/common/test/rules/informationleakageacrossboundaries/test.cpp similarity index 100% rename from cpp/cert/test/rules/DCL55-CPP/test.cpp rename to cpp/common/test/rules/informationleakageacrossboundaries/test.cpp diff --git a/rule_packages/c/Declarations7.json b/rule_packages/c/Declarations7.json index c938366a62..d02db1e68a 100644 --- a/rule_packages/c/Declarations7.json +++ b/rule_packages/c/Declarations7.json @@ -1,4 +1,26 @@ { + "CERT-C": { + "DCL39-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Passing a structure with uninitialized fields or padding bytes can cause information to be unintentionally leaked.", + "kind": "problem", + "name": "Avoid information leakage when passing a structure across a trust boundary", + "precision": "medium", + "severity": "error", + "short_name": "InformationLeakageAcrossTrustBoundariesC", + "shared_implementation_short_name": "InformationLeakageAcrossBoundaries", + "tags": [ + "security" + ] + } + ], + "title": "Avoid information leakage when passing a structure across a trust boundary" + } + }, "MISRA-C-2012": { "RULE-18-8": { "properties": { diff --git a/rule_packages/cpp/Uninitialized.json b/rule_packages/cpp/Uninitialized.json index 86a5b97115..03b5de5d3b 100644 --- a/rule_packages/cpp/Uninitialized.json +++ b/rule_packages/cpp/Uninitialized.json @@ -39,6 +39,7 @@ "precision": "medium", "severity": "error", "short_name": "InformationLeakageAcrossTrustBoundaries", + "shared_implementation_short_name": "InformationLeakageAcrossBoundaries", "tags": [ "security" ] diff --git a/rules.csv b/rules.csv index 94befb29ae..15625c5fba 100644 --- a/rules.csv +++ b/rules.csv @@ -500,10 +500,10 @@ c,CERT-C,CON41-C,Yes,Rule,,,Wrap functions that can fail spuriously in a loop,CO c,CERT-C,CON43-C,OutOfScope,Rule,,,Do not allow data races in multithreaded code,,,, c,CERT-C,DCL30-C,Yes,Rule,,,Declare objects with appropriate storage durations,,Declarations,Hard, c,CERT-C,DCL31-C,Yes,Rule,,,Declare identifiers before using them,,Declarations1,Medium, -c,CERT-C,DCL36-C,Yes,Rule,,,Do not declare an identifier with conflicting linkage classifications,,Declarations,Medium, +c,CERT-C,DCL36-C,No,Rule,,,Do not declare an identifier with conflicting linkage classifications,,,, c,CERT-C,DCL37-C,Yes,Rule,,,Do not declare or define a reserved identifier,,Declarations1,Easy, c,CERT-C,DCL38-C,Yes,Rule,,,Use the correct syntax when declaring a flexible array member,,Declarations2,Easy, -c,CERT-C,DCL39-C,Yes,Rule,,,Avoid information leakage when passing a structure across a trust boundary,,Declarations,Hard, +c,CERT-C,DCL39-C,Yes,Rule,,,Avoid information leakage when passing a structure across a trust boundary,,Declarations7,Hard, c,CERT-C,DCL40-C,Yes,Rule,,,Do not create incompatible declarations of the same function or object,,Declarations2,Hard, c,CERT-C,DCL41-C,Yes,Rule,,,Do not declare variables inside a switch statement before the first case label,,Declarations2,Medium, c,CERT-C,ENV30-C,Yes,Rule,,,Do not modify the object referenced by the return value of certain functions,RULE-21-19,Contracts1,Medium, From 1fc8306944c4e57395057d6b4bb5c8ff4a4217a3 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 24 Jan 2023 10:33:52 -0500 Subject: [PATCH 06/10] Declarations7: add missing help file DCL39-C --- ...nformationLeakageAcrossTrustBoundariesC.md | 279 +++++++++++++++++- 1 file changed, 277 insertions(+), 2 deletions(-) diff --git a/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md b/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md index 58cc3bd15b..74a1d8d3be 100644 --- a/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md +++ b/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md @@ -5,9 +5,284 @@ This query implements the CERT-C rule DCL39-C: > Avoid information leakage when passing a structure across a trust boundary -## CERT -** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** +## Description + +The C Standard, 6.7.2.1, discusses the layout of structure fields. It specifies that non-bit-field members are aligned in an [implementation-defined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior) manner and that there may be padding within or at the end of a structure. Furthermore, initializing the members of the structure does not guarantee initialization of the padding bytes. The C Standard, 6.2.6.1, paragraph 6 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], states + +> When a value is stored in an object of structure or union type, including in a member object, the bytes of the object representation that correspond to any padding bytes take unspecified values. + + +Additionally, the storage units in which a bit-field resides may also have padding bits. For an object with automatic storage duration, these padding bits do not take on specific values and can contribute to leaking sensitive information. + +When passing a pointer to a structure across a trust boundary to a different trusted domain, the programmer must ensure that the padding bytes and bit-field storage unit padding bits of such a structure do not contain sensitive information. + +## Noncompliant Code Example + +This noncompliant code example runs in kernel space and copies data from `arg` to user space. However, padding bytes may be used within the structure, for example, to ensure the proper alignment of the structure members. These padding bytes may contain sensitive information, which may then be leaked when the data is copied to user space. + +```cpp +#include + +struct test { + int a; + char b; + int c; +}; + +/* Safely copy bytes to user space */ +extern int copy_to_user(void *dest, void *src, size_t size); + +void do_stuff(void *usr_buf) { + struct test arg = {.a = 1, .b = 2, .c = 3}; + copy_to_user(usr_buf, &arg, sizeof(arg)); +} + +``` + +## Noncompliant Code Example (memset()) + +The padding bytes can be explicitly initialized by calling `memset()`: + +```cpp +#include + +struct test { + int a; + char b; + int c; +}; + +/* Safely copy bytes to user space */ +extern int copy_to_user(void *dest, void *src, size_t size); + +void do_stuff(void *usr_buf) { + struct test arg; + + /* Set all bytes (including padding bytes) to zero */ + memset(&arg, 0, sizeof(arg)); + + arg.a = 1; + arg.b = 2; + arg.c = 3; + + copy_to_user(usr_buf, &arg, sizeof(arg)); +} + +``` +However, a conforming compiler is free to implement `arg.b = 2` by setting the low-order bits of a register to 2, leaving the high-order bits unchanged and containing sensitive information. Then the platform copies all register bits into memory, leaving sensitive information in the padding bits. Consequently, this implementation could leak the high-order bits from the register to a user. + +## Compliant Solution + +This compliant solution serializes the structure data before copying it to an untrusted context: + +```cpp +#include +#include + +struct test { + int a; + char b; + int c; +}; + +/* Safely copy bytes to user space */ +extern int copy_to_user(void *dest, void *src, size_t size); + +void do_stuff(void *usr_buf) { + struct test arg = {.a = 1, .b = 2, .c = 3}; + /* May be larger than strictly needed */ + unsigned char buf[sizeof(arg)]; + size_t offset = 0; + + memcpy(buf + offset, &arg.a, sizeof(arg.a)); + offset += sizeof(arg.a); + memcpy(buf + offset, &arg.b, sizeof(arg.b)); + offset += sizeof(arg.b); + memcpy(buf + offset, &arg.c, sizeof(arg.c)); + offset += sizeof(arg.c); + /* Set all remaining bytes to zero */ + memset(buf + offset, 0, sizeof(arg) - offset); + + copy_to_user(usr_buf, buf, offset /* size of info copied */); +} +``` +This code ensures that no uninitialized padding bytes are copied to unprivileged users. **Important:** The structure copied to user space is now a packed structure and the `copy_to_user()` function (or other eventual user) would need to unpack it to recreate the original padded structure. + +## Compliant Solution (Padding Bytes) + +Padding bytes can be explicitly declared as fields within the structure. This solution is not portable, however, because it depends on the [implementation](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation) and target memory architecture. The following solution is specific to the x86-32 architecture: + +```cpp +#include +#include + +struct test { + int a; + char b; + char padding_1, padding_2, padding_3; + int c; +}; + +/* Safely copy bytes to user space */ +extern int copy_to_user(void *dest, void *src, size_t size); + +void do_stuff(void *usr_buf) { + /* Ensure c is the next byte after the last padding byte */ + static_assert(offsetof(struct test, c) == + offsetof(struct test, padding_3) + 1, + "Structure contains intermediate padding"); + /* Ensure there is no trailing padding */ + static_assert(sizeof(struct test) == + offsetof(struct test, c) + sizeof(int), + "Structure contains trailing padding"); + struct test arg = {.a = 1, .b = 2, .c = 3}; + arg.padding_1 = 0; + arg.padding_2 = 0; + arg.padding_3 = 0; + copy_to_user(usr_buf, &arg, sizeof(arg)); +} + +``` +The C Standard `static_assert()` macro accepts a constant expression and an [error message](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-error). The expression is evaluated at compile time and, if false, the compilation is terminated and the error message is output. (See [DCL03-C. Use a static assertion to test the value of a constant expression](https://wiki.sei.cmu.edu/confluence/display/c/DCL03-C.+Use+a+static+assertion+to+test+the+value+of+a+constant+expression) for more details.) The explicit insertion of the padding bytes into the `struct` should ensure that no additional padding bytes are added by the compiler and consequently both static assertions should be true. However, it is necessary to validate these assumptions to ensure that the solution is correct for a particular implementation. + +## Compliant Solution (Structure Packing—GCC) + +GCC allows specifying declaration attributes using the keyword `__attribute__((__packed__))`. When this attribute is present, the compiler will not add padding bytes for memory alignment unless an explicit alignment specifier for a structure member requires the introduction of padding bytes. + +```cpp +#include + +struct test { + int a; + char b; + int c; +} __attribute__((__packed__)); + +/* Safely copy bytes to user space */ +extern int copy_to_user(void *dest, void *src, size_t size); + +void do_stuff(void *usr_buf) { + struct test arg = {.a = 1, .b = 2, .c = 3}; + copy_to_user(usr_buf, &arg, sizeof(arg)); +} + +``` + +## Compliant Solution (Structure Packing—Microsoft Visual Studio) + +Microsoft Visual Studio supports `#pragma pack()` to suppress padding bytes \[[MSDN](http://msdn.microsoft.com/en-us/library/2e70t5y1(v=vs.110).aspx)\]. The compiler adds padding bytes for memory alignment, depending on the current packing mode, but still honors the alignment specified by `__declspec(align())`. In this compliant solution, the packing mode is set to 1 in an attempt to ensure all fields are given adjacent offsets: + +```cpp +#include + +#pragma pack(push, 1) /* 1 byte */ +struct test { + int a; + char b; + int c; +}; +#pragma pack(pop) + +/* Safely copy bytes to user space */ +extern int copy_to_user(void *dest, void *src, size_t size); + +void do_stuff(void *usr_buf) { + struct test arg = {1, 2, 3}; + copy_to_user(usr_buf, &arg, sizeof(arg)); +} + +``` +The `pack` pragma takes effect at the first `struct` declaration after the pragma is seen. + +## Noncompliant Code Example + +This noncompliant code example also runs in kernel space and copies data from `struct test` to user space. However, padding bits will be used within the structure due to the bit-field member lengths not adding up to the number of bits in an `unsigned` object. Further, there is an unnamed bit-field that causes no further bit-fields to be packed into the same storage unit. These padding bits may contain sensitive information, which may then be leaked when the data is copied to user space. For instance, the uninitialized bits may contain a sensitive kernel space pointer value that can be trivially reconstructed by an attacker in user space. + +```cpp +#include + +struct test { + unsigned a : 1; + unsigned : 0; + unsigned b : 4; +}; + +/* Safely copy bytes to user space */ +extern int copy_to_user(void *dest, void *src, size_t size); + +void do_stuff(void *usr_buf) { + struct test arg = { .a = 1, .b = 10 }; + copy_to_user(usr_buf, &arg, sizeof(arg)); +} +``` + +## Compliant Solution + +Padding bits can be explicitly declared, allowing the programmer to specify the value of those bits. When explicitly declaring all of the padding bits, any unnamed bit-fields of length `0` must be removed from the structure because the explicit padding bits ensure that no further bit-fields will be packed into the same storage unit. + +```cpp +#include +#include +#include + +struct test { + unsigned a : 1; + unsigned padding1 : sizeof(unsigned) * CHAR_BIT - 1; + unsigned b : 4; + unsigned padding2 : sizeof(unsigned) * CHAR_BIT - 4; +}; +/* Ensure that we have added the correct number of padding bits. */ +static_assert(sizeof(struct test) == sizeof(unsigned) * 2, + "Incorrect number of padding bits for type: unsigned"); + +/* Safely copy bytes to user space */ +extern int copy_to_user(void *dest, void *src, size_t size); + +void do_stuff(void *usr_buf) { + struct test arg = { .a = 1, .padding1 = 0, .b = 10, .padding2 = 0 }; + copy_to_user(usr_buf, &arg, sizeof(arg)); +} +``` +This solution is not portable, however, because it depends on the [implementation](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation) and target memory architecture. The explicit insertion of padding bits into the `struct` should ensure that no additional padding bits are added by the compiler. However, it is still necessary to validate these assumptions to ensure that the solution is correct for a particular implementation. For instance, the DEC Alpha is an example of a 64-bit architecture with 32-bit integers that allocates 64 bits to a storage unit. + +In addition, this solution assumes that there are no integer padding bits in an `unsigned int`. The portable version of the width calculation from [INT35-C. Use correct integer precisions](https://wiki.sei.cmu.edu/confluence/display/c/INT35-C.+Use+correct+integer+precisions) cannot be used because the bit-field width must be an integer constant expression. + +From this situation, it can be seen that special care must be taken because no solution to the bit-field padding issue will be 100% portable. + +Risk Assessment + +Padding units might contain sensitive data because the C Standard allows any padding to take [unspecified values](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-unspecifiedvalue). A pointer to such a structure could be passed to other functions, causing information leakage. + +
Rule Severity Likelihood Remediation Cost Priority Level
DCL39-C Low Unlikely High P1 L3
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 function-argument-with-padding Partially checked
Axivion Bauhaus Suite 7.2.0 CertC-DCL39 Detects composite structures with padding, in particular those passed to trust boundary routines.
CodeSonar 7.2p0 MISC.PADDING.POTB Padding Passed Across a Trust Boundary
Helix QAC 2022.4 DF4941, DF4942, DF4943
Klocwork 2022.4 PORTING.STORAGE.STRUCT
Parasoft C/C++test 2022.2 CERT_C-DCL39-a A pointer to a structure should not be passed to a function that can copy data to the user space
Polyspace Bug Finder R2022b CERT C: Rule DCL39-C Checks for information leak via structure padding
PRQA QA-C 9.7 4941, 4942, 4943
PRQA QA-C++ 4.4 4941, 4942, 4943
RuleChecker 22.04 function-argument-with-padding Partially checked
+ + +## Related Vulnerabilities + +Numerous vulnerabilities in the Linux Kernel have resulted from violations of this rule. [CVE-2010-4083](http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2010-4083) describes a [vulnerability](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) in which the `semctl()` system call allows unprivileged users to read uninitialized kernel stack memory because various fields of a `semid_ds struct` declared on the stack are not altered or zeroed before being copied back to the user. + +[CVE-2010-3881](http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2010-3881) describes a vulnerability in which structure padding and reserved fields in certain data structures in `QEMU-KVM` were not initialized properly before being copied to user space. A privileged host user with access to `/dev/kvm` could use this [flaw](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-securityflaw) to leak kernel stack memory to user space. + +[CVE-2010-3477](http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2010-3477) describes a kernel information leak in `act_police` where incorrectly initialized structures in the traffic-control dump code may allow the disclosure of kernel memory to user space applications. + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+DCL39-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
CERT C Secure Coding Standard DCL03-C. Use a static assertion to test the value of a constant expression Prior to 2018-01-12: CERT: Unspecified Relationship
+ + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] 6.2.6.1, "General" 6.7.2.1, "Structure and Union Specifiers"
\[ Graff 2003 \]
\[ Sun 1993 \]
+ ## Implementation notes From ea3fb53161aadb90fef7545eac5e334a85483fd6 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 24 Jan 2023 10:39:03 -0500 Subject: [PATCH 07/10] Declarations7: fix format help file DCL39-C --- .../rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md | 1 - 1 file changed, 1 deletion(-) diff --git a/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md b/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md index 74a1d8d3be..978b6d85d7 100644 --- a/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md +++ b/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md @@ -5,7 +5,6 @@ This query implements the CERT-C rule DCL39-C: > Avoid information leakage when passing a structure across a trust boundary - ## Description The C Standard, 6.7.2.1, discusses the layout of structure fields. It specifies that non-bit-field members are aligned in an [implementation-defined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior) manner and that there may be padding within or at the end of a structure. Furthermore, initializing the members of the structure does not guarantee initialization of the padding bytes. The C Standard, 6.2.6.1, paragraph 6 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], states From 6fbdbfbf0d894a172d0267d2e32021da0ce3d6a6 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Wed, 25 Jan 2023 12:32:14 -0500 Subject: [PATCH 08/10] Declarations7: fix RULE-18-8 expected file --- .../test/rules/RULE-18-8/VariableLengthArrayTypesUsed.expected | 1 - 1 file changed, 1 deletion(-) diff --git a/c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.expected b/c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.expected index 24bf35a90d..e9721ce642 100644 --- a/c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.expected +++ b/c/misra/test/rules/RULE-18-8/VariableLengthArrayTypesUsed.expected @@ -1,4 +1,3 @@ -WARNING: Unused predicate partOfConstantExpr (/Users/knewbury/Desktop/GITHUB/coding-standards/codeql-coding-standards/c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql:17,11-29) | test.c:3:19:3:20 | definition of pa | Variable length array declared. | | test.c:6:7:6:8 | definition of a1 | Variable length array declared. | | test.c:7:7:7:8 | definition of a2 | Variable length array declared. | From 1f9e4e7a7cf2959722c80cc6c3a2f98c3ab832e9 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Wed, 25 Jan 2023 12:41:20 -0500 Subject: [PATCH 09/10] Declarations7: fix accidental addition to unrelated query --- ...MissingStaticSpecifierFunctionRedeclarationShared.qll | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/rules/missingstaticspecifierfunctionredeclarationshared/MissingStaticSpecifierFunctionRedeclarationShared.qll b/cpp/common/src/codingstandards/cpp/rules/missingstaticspecifierfunctionredeclarationshared/MissingStaticSpecifierFunctionRedeclarationShared.qll index 60889ed86b..43c1821e2e 100644 --- a/cpp/common/src/codingstandards/cpp/rules/missingstaticspecifierfunctionredeclarationshared/MissingStaticSpecifierFunctionRedeclarationShared.qll +++ b/cpp/common/src/codingstandards/cpp/rules/missingstaticspecifierfunctionredeclarationshared/MissingStaticSpecifierFunctionRedeclarationShared.qll @@ -21,12 +21,5 @@ query predicate problems( not redeclaration.hasSpecifier("static") and fde != redeclaration and message = "The redeclaration of $@ with internal linkage misses the static specifier." and - msgpiece = "function" - and - ( - fde.getFile().getAbsolutePath() < redeclaration.getFile().getAbsolutePath() - or - fde.getFile().getAbsolutePath() = redeclaration.getFile().getAbsolutePath() and - fde.getLocation().getStartLine() < redeclaration.getLocation().getStartLine() - ) + msgpiece = "function" } From cae74a9e9aac2f9f51f738e1f06df7a6080748f2 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Fri, 27 Jan 2023 12:12:06 -0500 Subject: [PATCH 10/10] Declarations7: add implementation notes and lib description also rm accidental leftover expected file --- ...nformationLeakageAcrossTrustBoundariesC.md | 2 +- ...InformationLeakageAcrossTrustBoundaries.md | 2 +- .../InformationLeakageAcrossBoundaries.qll | 2 +- ...ationLeakageAcrossTrustBoundaries.expected | 20 ------------------- rule_packages/c/Declarations7.json | 5 ++++- rule_packages/cpp/Uninitialized.json | 5 ++++- 6 files changed, 11 insertions(+), 25 deletions(-) delete mode 100644 cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossTrustBoundaries.expected diff --git a/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md b/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md index 978b6d85d7..cdc62493a1 100644 --- a/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md +++ b/c/cert/src/rules/DCL39-C/InformationLeakageAcrossTrustBoundariesC.md @@ -285,7 +285,7 @@ Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+D ## Implementation notes -None +The rule does not detect cases where fields may have uninitialized padding but are initialized via an initializer. ## References diff --git a/cpp/cert/src/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.md b/cpp/cert/src/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.md index a4f9891f53..29231d4809 100644 --- a/cpp/cert/src/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.md +++ b/cpp/cert/src/rules/DCL55-CPP/InformationLeakageAcrossTrustBoundaries.md @@ -310,7 +310,7 @@ Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/cpluspl ## Implementation notes -None +The rule does not detect cases where fields may have uninitialized padding but are initialized via an initializer. ## References diff --git a/cpp/common/src/codingstandards/cpp/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.qll b/cpp/common/src/codingstandards/cpp/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.qll index f33d5ac87c..16d8fd47ec 100644 --- a/cpp/common/src/codingstandards/cpp/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.qll +++ b/cpp/common/src/codingstandards/cpp/rules/informationleakageacrossboundaries/InformationLeakageAcrossBoundaries.qll @@ -1,5 +1,5 @@ /** - * Provides a library which includes a `problems` predicate for reporting.... + * Provides a library which includes a `problems` predicate for reporting potential information leakage across trust boundaries, relating to uninitialized memory in structs. */ import cpp diff --git a/cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossTrustBoundaries.expected b/cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossTrustBoundaries.expected deleted file mode 100644 index 4f703cff78..0000000000 --- a/cpp/common/test/rules/informationleakageacrossboundaries/InformationLeakageAcrossTrustBoundaries.expected +++ /dev/null @@ -1,20 +0,0 @@ -cpp/common/test/rules/informationleakageacrossboundaries/| arrays.cpp:11:20:11:21 | wa | 'wa' may leak information from {elements of a[...] (arrays.cpp:7)}. Path: wa (arrays.cpp:11) --> & ... (arrays.cpp:12) | -| arrays.cpp:33:22:33:23 | wa | 'wa' may leak information from {elements of elements of a[...][...] (arrays.cpp:29)}. Path: wa (arrays.cpp:33) --> & ... (arrays.cpp:34) | -| arrays.cpp:57:22:57:23 | wa | 'wa' may leak information from {WithPointer (arrays.cpp:52)}. Path: wa (arrays.cpp:57) --> & ... (arrays.cpp:59) | -| inheritance.cpp:19:14:19:14 | s | 's' may leak information from {i (inheritance.cpp:7)}. Path: s (inheritance.cpp:19) --> & ... (inheritance.cpp:21) | -| inheritance.cpp:32:14:32:14 | s | 's' may leak information from {0 to 4 bytes of padding in ptrDerived (inheritance.cpp:14)}. Path: s (inheritance.cpp:32) --> & ... (inheritance.cpp:35) | -| interprocedural.cpp:37:9:37:9 | p | 'p' may leak information from {y (interprocedural.cpp:8)}. Path: p (interprocedural.cpp:37) --> past assign_x (interprocedural.cpp:32) --> & ... (interprocedural.cpp:39) | -| interprocedural.cpp:104:9:104:9 | p | 'p' may leak information from {x (interprocedural.cpp:7), y (interprocedural.cpp:8)}. Path: p (interprocedural.cpp:104) --> overwrite_after_leak(...) (interprocedural.cpp:96) --> p (interprocedural.cpp:97) | -| multilayer.cpp:16:10:16:10 | s | 's' may leak information from {b (multilayer.cpp:12)}. Path: s (multilayer.cpp:16) --> & ... (multilayer.cpp:18) | -| multilayer.cpp:29:10:29:10 | s | 's' may leak information from {b (multilayer.cpp:12), x (multilayer.cpp:7)}. Path: s (multilayer.cpp:29) --> & ... (multilayer.cpp:30) | -| multilayer.cpp:34:8:34:8 | s | 's' may leak information from {intx (multilayer.cpp:6)}. Path: s (multilayer.cpp:34) --> & ... (multilayer.cpp:35) | -| test.cpp:12:12:12:12 | s | 's' may leak information from {y (test.cpp:8)}. Path: s (test.cpp:12) --> & ... (test.cpp:14) | -| test.cpp:18:12:18:12 | s | 's' may leak information from {x (test.cpp:7)}. Path: s (test.cpp:18) --> & ... (test.cpp:20) | -| test.cpp:24:12:24:12 | s | 's' may leak information from {x (test.cpp:7), y (test.cpp:8)}. Path: s (test.cpp:24) --> & ... (test.cpp:25) | -| test.cpp:36:12:36:12 | s | 's' may leak information from {y (test.cpp:8)}. Path: s (test.cpp:36) --> & ... (test.cpp:38) | -| test.cpp:43:12:43:12 | s | 's' may leak information from {x (test.cpp:7)}. Path: s (test.cpp:43) --> & ... (test.cpp:47) | -| test.cpp:58:12:58:12 | s | 's' may leak information from {x (test.cpp:7), y (test.cpp:8)}. Path: s (test.cpp:58) --> & ... (test.cpp:59) | -| test.cpp:64:12:64:12 | s | 's' may leak information from {y (test.cpp:8)}. Path: s (test.cpp:64) --> & ... (test.cpp:66) | -| test.cpp:112:16:112:16 | s | 's' may leak information from {buf (test.cpp:92)}. Path: s (test.cpp:112) --> & ... (test.cpp:115) | -| test.cpp:128:12:128:12 | s | 's' may leak information from {x (test.cpp:7), y (test.cpp:8)}. Path: s (test.cpp:128) --> & ... (test.cpp:132) | -| test.cpp:157:22:157:22 | s | 's' may leak information from {2 to 2 bytes of padding in has_padding (test.cpp:151)}. Path: s (test.cpp:157) --> & ... (test.cpp:160) | diff --git a/rule_packages/c/Declarations7.json b/rule_packages/c/Declarations7.json index d02db1e68a..cd3b3e6b18 100644 --- a/rule_packages/c/Declarations7.json +++ b/rule_packages/c/Declarations7.json @@ -15,7 +15,10 @@ "shared_implementation_short_name": "InformationLeakageAcrossBoundaries", "tags": [ "security" - ] + ], + "implementation_scope": { + "description": "The rule does not detect cases where fields may have uninitialized padding but are initialized via an initializer." + } } ], "title": "Avoid information leakage when passing a structure across a trust boundary" diff --git a/rule_packages/cpp/Uninitialized.json b/rule_packages/cpp/Uninitialized.json index 03b5de5d3b..019987eef4 100644 --- a/rule_packages/cpp/Uninitialized.json +++ b/rule_packages/cpp/Uninitialized.json @@ -42,7 +42,10 @@ "shared_implementation_short_name": "InformationLeakageAcrossBoundaries", "tags": [ "security" - ] + ], + "implementation_scope": { + "description": "The rule does not detect cases where fields may have uninitialized padding but are initialized via an initializer." + } } ], "title": "Avoid information leakage when passing a class object across a trust boundary"