From 8e8a324fa6af33cd046ceed93ea6681e340c3b54 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Mon, 25 Oct 2021 14:23:19 +0300 Subject: [PATCH 01/11] Add files via upload --- .../CWE/CWE-377/InsecureTemporaryFile.cpp | 14 +++ .../CWE/CWE-377/InsecureTemporaryFile.qhelp | 22 +++++ .../CWE/CWE-377/InsecureTemporaryFile.ql | 98 +++++++++++++++++++ 3 files changed, 134 insertions(+) create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.cpp create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.cpp b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.cpp new file mode 100644 index 000000000000..a45b3fd3dfef --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.cpp @@ -0,0 +1,14 @@ +... + fp = fopen("/tmp/name.tmp","w"); // BAD +... + char filename = tmpnam(NULL); + fp = fopen(filename,"w"); // BAD +... + + strcat (filename, "/tmp/name.XXXXXX"); + fd = mkstemp(filename); + if ( fd < 0 ) { + return error; + } + fp = fdopen(fd,"w") // GOOD +... diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp new file mode 100644 index 000000000000..3c488cf1cc59 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp @@ -0,0 +1,22 @@ + + + +

Working with a file, without checking its existence and its rights, as well as working with names that can be predicted, may not be safe. Requires the attention of developers.

+ + + +

The following example demonstrates erroneous and corrected work with file.

+ + + + + +
  • + CERT C Coding Standard: + CON33-C. Avoid race conditions when using library functions. +
    • + +     +     diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql new file mode 100644 index 000000000000..83314d3cb79e --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql @@ -0,0 +1,98 @@ +/** + * @name Find insecure work with the file name. + * @description The file can be used to influence the correct operation of the program. + * @kind problem + * @id cpp/insecure-work-with-file-name + * @problem.severity warning + * @precision medium + * @tags correctness + * security + * external/cwe/cwe-377 + */ + +import cpp +import semmle.code.cpp.valuenumbering.GlobalValueNumbering + +/** Holds for a function `f` that has an argument at index `apos` used to read the file. */ +predicate numberArgumentRead(Function f, int apos) { + f.hasGlobalOrStdName("fgets") and apos = 2 + or + f.hasGlobalOrStdName("fread") and apos = 3 + or + f.hasGlobalOrStdName("read") and apos = 0 + or + f.hasGlobalOrStdName("fscanf") and apos = 0 +} + +/** Holds for a function `f` that has an argument at index `apos` used to write to file */ +predicate numberArgumentWrite(Function f, int apos) { + f.hasGlobalOrStdName("fprintf") and apos = 0 + or + f.hasGlobalOrStdName("fputs") and apos = 1 + or + f.hasGlobalOrStdName("write") and apos = 0 + or + f.hasGlobalOrStdName("fwrite") and apos = 3 + or + f.hasGlobalOrStdName("fflush") and apos = 0 +} + +from FunctionCall fc, string msg +where + ( + fc.getTarget().hasGlobalOrStdName("tmpnam") or + fc.getTarget().hasGlobalOrStdName("tmpnam_s") or + fc.getTarget().hasGlobalOrStdName("tmpnam_r") + ) and + not exists(FunctionCall fctmp | + fctmp.getTarget().hasGlobalOrStdName("mktemp") or + fctmp.getTarget().hasGlobalOrStdName("mkstemp") or + fctmp.getTarget().hasGlobalOrStdName("mkstemps") or + fctmp.getTarget().hasGlobalOrStdName("mkdtemp") + ) and + msg = + "Finding the name of a file that does not exist does not mean that it will not be exist at the next operation." + or + ( + fc.getTarget().hasGlobalOrStdName("fopen") or + fc.getTarget().hasGlobalOrStdName("open") + ) and + fc.getNumberOfArguments() = 2 and + exists(FunctionCall fctmp, int i | + numberArgumentWrite(fctmp.getTarget(), i) and + globalValueNumber(fc) = globalValueNumber(fctmp.getArgument(i)) + ) and + not exists(FunctionCall fctmp, int i | + numberArgumentRead(fctmp.getTarget(), i) and + globalValueNumber(fc) = globalValueNumber(fctmp.getArgument(i)) + ) and + exists(FunctionCall fctmp | + ( + fctmp.getTarget().hasGlobalOrStdName("strcat") or + fctmp.getTarget().hasGlobalOrStdName("strcpy") + ) and + globalValueNumber(fc.getArgument(0)) = globalValueNumber(fctmp.getAnArgument()) + or + fctmp.getTarget().hasGlobalOrStdName("getenv") and + globalValueNumber(fc.getArgument(0)) = globalValueNumber(fctmp) + or + ( + fctmp.getTarget().hasGlobalOrStdName("asprintf") or + fctmp.getTarget().hasGlobalOrStdName("vasprintf") or + fctmp.getTarget().hasGlobalOrStdName("xasprintf") or + fctmp.getTarget().hasGlobalOrStdName("xvasprintf ") + ) and + exists(Variable vrtmp | + vrtmp = fc.getArgument(0).(VariableAccess).getTarget() and + vrtmp = fctmp.getArgument(0).(AddressOfExpr).getAddressable().(Variable) and + not vrtmp instanceof Field + ) + ) and + not exists(FunctionCall fctmp | + fctmp.getTarget().hasGlobalOrStdName("umask") or + fctmp.getTarget().hasGlobalOrStdName("fchmod") or + fctmp.getTarget().hasGlobalOrStdName("chmod") + ) and + msg = + "Сreating a file for writing without evaluating its existence and setting permissions can be unsafe." +select fc, msg From 3f3988ce1c187a5ac84bbc1ca416837bd4e4f156 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Mon, 25 Oct 2021 14:24:35 +0300 Subject: [PATCH 02/11] Add files via upload --- .../semmle/tests/InsecureTemporaryFile.expected | 1 + .../semmle/tests/InsecureTemporaryFile.qlref | 1 + .../Security/CWE/CWE-377/semmle/tests/test.cpp | 16 ++++++++++++++++ 3 files changed, 18 insertions(+) create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.qlref create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/test.cpp diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected new file mode 100644 index 000000000000..3dc08c29827b --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected @@ -0,0 +1 @@ +| test.cpp:11:20:11:25 | call to tmpnam | Finding the name of a file that does not exist does not mean that it will not be exist at the next operation. | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.qlref b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.qlref new file mode 100644 index 000000000000..beec38ab5dc6 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/test.cpp new file mode 100644 index 000000000000..aa7f16bdd001 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/test.cpp @@ -0,0 +1,16 @@ +typedef int FILE; +#define NULL (0) +FILE *fopen(char *filename, const char *mode); +char * tmpnam(char * name); +int fprintf(FILE *fp,const char *fmt, ...); +int fclose(FILE *stream); + +int main(int argc, char *argv[]) +{ + FILE *fp; + char *filename = tmpnam(NULL); // BAD + fp = fopen(filename,"w"); + fprintf(fp,"%s\n","data to file"); + fclose(fp); + return 0; +} From 235a3ec23289d15c276dba3b3d75d6ac1be721c4 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Thu, 28 Oct 2021 10:34:42 +0300 Subject: [PATCH 03/11] Update InsecureTemporaryFile.qhelp --- .../Security/CWE/CWE-377/InsecureTemporaryFile.qhelp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp index 3c488cf1cc59..248394525d80 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.qhelp @@ -5,7 +5,8 @@

    Working with a file, without checking its existence and its rights, as well as working with names that can be predicted, may not be safe. Requires the attention of developers.

    - +     +

    The following example demonstrates erroneous and corrected work with file.

    From 432fc7445552d3d256e3890f84da031e1113c50a Mon Sep 17 00:00:00 2001 From: ihsinme Date: Thu, 28 Oct 2021 10:37:01 +0300 Subject: [PATCH 04/11] Apply suggestions from code review Co-authored-by: Mathias Vorreiter Pedersen --- .../Security/CWE/CWE-377/InsecureTemporaryFile.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql index 83314d3cb79e..e039f7a7e626 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql @@ -1,8 +1,8 @@ /** - * @name Find insecure work with the file name. + * @name Insecure generation of filenames. * @description The file can be used to influence the correct operation of the program. * @kind problem - * @id cpp/insecure-work-with-file-name + * @id cpp/insecure-generation-of-filename * @problem.severity warning * @precision medium * @tags correctness From 2574aa89805e65f8115a009f0dd38ad49df98529 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Thu, 28 Oct 2021 10:51:48 +0300 Subject: [PATCH 05/11] Update InsecureTemporaryFile.ql From c8a4a8b9651773fcdfe832c0d49b02af9bdfa3c2 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Fri, 29 Oct 2021 09:44:43 +0300 Subject: [PATCH 06/11] Update InsecureTemporaryFile.ql --- .../experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql index e039f7a7e626..0b7458d407d7 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql @@ -39,6 +39,7 @@ predicate numberArgumentWrite(Function f, int apos) { from FunctionCall fc, string msg where + // search for functions for generating a name, without a guarantee of the absence of a file during the period of work with it. ( fc.getTarget().hasGlobalOrStdName("tmpnam") or fc.getTarget().hasGlobalOrStdName("tmpnam_s") or @@ -53,6 +54,7 @@ where msg = "Finding the name of a file that does not exist does not mean that it will not be exist at the next operation." or + // finding places to work with a file without setting permissions, but with predictable names. ( fc.getTarget().hasGlobalOrStdName("fopen") or fc.getTarget().hasGlobalOrStdName("open") From cedc5fd743d8545e8f84c8f8907015661eef5123 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Fri, 5 Nov 2021 09:42:06 +0300 Subject: [PATCH 07/11] Update InsecureTemporaryFile.ql --- .../experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql index 0b7458d407d7..b15f0546212b 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql @@ -96,5 +96,5 @@ where fctmp.getTarget().hasGlobalOrStdName("chmod") ) and msg = - "Сreating a file for writing without evaluating its existence and setting permissions can be unsafe." + "Creating a file for writing without evaluating its existence and setting permissions can be unsafe." select fc, msg From 55fe01018f2b1bd25cb8c26f47cb3ddff7379bab Mon Sep 17 00:00:00 2001 From: ihsinme Date: Tue, 9 Nov 2021 09:33:33 +0300 Subject: [PATCH 08/11] Update InsecureTemporaryFile.ql --- .../experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql index b15f0546212b..aa910b3c8c7b 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql @@ -1,6 +1,6 @@ /** * @name Insecure generation of filenames. - * @description The file can be used to influence the correct operation of the program. + * @description Using a predictable filename when creating a temporary file can lead to an attacker-controlled input. * @kind problem * @id cpp/insecure-generation-of-filename * @problem.severity warning From 289d58745a3d39b68669e3747fb9ff615a7d5a73 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Wed, 10 Nov 2021 09:22:03 +0300 Subject: [PATCH 09/11] Update InsecureTemporaryFile.ql --- .../CWE/CWE-377/InsecureTemporaryFile.ql | 26 ++++++++++++++----- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql index aa910b3c8c7b..f08f7ef61f55 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-377/InsecureTemporaryFile.ql @@ -46,10 +46,16 @@ where fc.getTarget().hasGlobalOrStdName("tmpnam_r") ) and not exists(FunctionCall fctmp | - fctmp.getTarget().hasGlobalOrStdName("mktemp") or - fctmp.getTarget().hasGlobalOrStdName("mkstemp") or - fctmp.getTarget().hasGlobalOrStdName("mkstemps") or - fctmp.getTarget().hasGlobalOrStdName("mkdtemp") + ( + fctmp.getTarget().hasGlobalOrStdName("mktemp") or + fctmp.getTarget().hasGlobalOrStdName("mkstemp") or + fctmp.getTarget().hasGlobalOrStdName("mkstemps") or + fctmp.getTarget().hasGlobalOrStdName("mkdtemp") + ) and + ( + fc.getBasicBlock().getASuccessor*() = fctmp.getBasicBlock() or + fctmp.getBasicBlock().getASuccessor*() = fc.getBasicBlock() + ) ) and msg = "Finding the name of a file that does not exist does not mean that it will not be exist at the next operation." @@ -91,9 +97,15 @@ where ) ) and not exists(FunctionCall fctmp | - fctmp.getTarget().hasGlobalOrStdName("umask") or - fctmp.getTarget().hasGlobalOrStdName("fchmod") or - fctmp.getTarget().hasGlobalOrStdName("chmod") + ( + fctmp.getTarget().hasGlobalOrStdName("umask") or + fctmp.getTarget().hasGlobalOrStdName("fchmod") or + fctmp.getTarget().hasGlobalOrStdName("chmod") + ) and + ( + fc.getBasicBlock().getASuccessor*() = fctmp.getBasicBlock() or + fctmp.getBasicBlock().getASuccessor*() = fc.getBasicBlock() + ) ) and msg = "Creating a file for writing without evaluating its existence and setting permissions can be unsafe." From 7514fe2b45d005d1b51f5d4e86998821d11d61d7 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Wed, 10 Nov 2021 09:22:58 +0300 Subject: [PATCH 10/11] Update test.cpp --- .../CWE/CWE-377/semmle/tests/test.cpp | 54 ++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/test.cpp index aa7f16bdd001..ceffdf11fa9e 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/test.cpp +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/test.cpp @@ -1,11 +1,16 @@ typedef int FILE; #define NULL (0) FILE *fopen(char *filename, const char *mode); +FILE *fdopen(int handle, char *mode); char * tmpnam(char * name); +int mkstemp(char * name); +char * strcat(char *str1, const char *str2); +int umask(int pmode); +int chmod(char * filename,int pmode); int fprintf(FILE *fp,const char *fmt, ...); int fclose(FILE *stream); -int main(int argc, char *argv[]) +int funcTest1() { FILE *fp; char *filename = tmpnam(NULL); // BAD @@ -14,3 +19,50 @@ int main(int argc, char *argv[]) fclose(fp); return 0; } + +int funcTest2() +{ + FILE *fp; + int fd; + char filename[80]; + strcat (filename, "/tmp/name.XXXXXX"); + fd = mkstemp(filename); + if ( fd < 0 ) { + return 1; + } + fp = fdopen(fd,"w"); // GOOD + return 0; +} + +int funcTest3() +{ + FILE *fp; + char filename[80]; + strcat(filename, "/tmp/tmp.name"); + fp = fopen(filename,"w"); // BAD + fprintf(fp,"%s\n","data to file"); + fclose(fp); + return 0; +} + +int funcTest4() +{ + FILE *fp; + char filename[80]; + umask(0022); + strcat(filename, "/tmp/tmp.name"); + fp = fopen(filename,"w"); // GOOD + chmod(filename,0666); + fprintf(fp,"%s\n","data to file"); + fclose(fp); + return 0; +} + +int main(int argc, char *argv[]) +{ + funcTest1(); + funcTest2(); + funcTest3(); + funcTest4(); + return 0; +} From a0448240aaca449dc1ca97fb12c2d4fa793691c1 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Wed, 10 Nov 2021 09:23:51 +0300 Subject: [PATCH 11/11] Update InsecureTemporaryFile.expected --- .../CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected index 3dc08c29827b..b0632edcd2a2 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-377/semmle/tests/InsecureTemporaryFile.expected @@ -1 +1,2 @@ -| test.cpp:11:20:11:25 | call to tmpnam | Finding the name of a file that does not exist does not mean that it will not be exist at the next operation. | +| test.cpp:16:20:16:25 | call to tmpnam | Finding the name of a file that does not exist does not mean that it will not be exist at the next operation. | +| test.cpp:42:8:42:12 | call to fopen | Creating a file for writing without evaluating its existence and setting permissions can be unsafe. |