diff --git a/c/cert/src/codeql-suites/cert-c-default.qls b/c/cert/src/codeql-suites/cert-c-default.qls new file mode 100644 index 0000000000..348d2f37ae --- /dev/null +++ b/c/cert/src/codeql-suites/cert-c-default.qls @@ -0,0 +1,10 @@ +- description: CERT C 2016 (Default) +- qlpack: codeql/cert-c-coding-standards +- include: + kind: + - problem + - path-problem + - external/cert/obligation/rule +- exclude: + tags contain: + - external/cert/default-disabled diff --git a/c/cert/src/codeql-suites/cert-c-recommendation.qls b/c/cert/src/codeql-suites/cert-c-recommendation.qls new file mode 100644 index 0000000000..59ac5e9c2d --- /dev/null +++ b/c/cert/src/codeql-suites/cert-c-recommendation.qls @@ -0,0 +1,10 @@ +- description: CERT C 2016 (Recommendations) +- qlpack: codeql/cert-c-coding-standards +- include: + kind: + - problem + - path-problem + - external/cert/obligation/recommendation +- exclude: + tags contain: + - external/cert/default-disabled diff --git a/c/cert/src/codeql-suites/cert-default.qls b/c/cert/src/codeql-suites/cert-default.qls index 1e11a0afca..c093b31fa7 100644 --- a/c/cert/src/codeql-suites/cert-default.qls +++ b/c/cert/src/codeql-suites/cert-default.qls @@ -1,9 +1,2 @@ -- description: CERT C 2016 (Default) -- qlpack: codeql/cert-c-coding-standards -- include: - kind: - - problem - - path-problem -- exclude: - tags contain: - - external/cert/default-disabled +- description: "DEPRECATED - CERT C 2016 - use cert-c-default.qls instead" +- import: codeql-suites/cert-c-default.qls diff --git a/c/cert/src/qlpack.yml b/c/cert/src/qlpack.yml index 631639301e..e3664d75b5 100644 --- a/c/cert/src/qlpack.yml +++ b/c/cert/src/qlpack.yml @@ -3,6 +3,7 @@ version: 2.44.0-dev description: CERT C 2016 suites: codeql-suites license: MIT +default-suite-file: codeql-suites/cert-c-default.qls dependencies: codeql/common-c-coding-standards: '*' codeql/cpp-all: 2.1.1 diff --git a/c/cert/src/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.md b/c/cert/src/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.md new file mode 100644 index 0000000000..a6014e1468 --- /dev/null +++ b/c/cert/src/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.md @@ -0,0 +1,166 @@ +# FIO03-C: Do not make assumptions about fopen() and file creation + +This query implements the CERT-C rule FIO03-C: + +> Do not make assumptions about fopen() and file creation + + +## Description + +The C `fopen()` function is used to open an existing file or create a new one. The C11 version of the `fopen()` and `fopen_s()` functions provides a mode flag, `x`, that provides the mechanism needed to determine if the file that is to be opened exists. Not using this mode flag can lead to a program overwriting or accessing an unintended file. + +## Noncompliant Code Example (fopen()) + +In this noncompliant code example, the file referenced by `file_name` is opened for writing. This example is noncompliant if the programmer's intent was to create a new file, but the referenced file already exists. + +```cpp +char *file_name; +FILE *fp; + +/* Initialize file_name */ + +fp = fopen(file_name, "w"); +if (!fp) { + /* Handle error */ +} + +``` + +## Noncompliant Code Example (fopen_s(), C11 Annex K) + +The C11 Annex K `fopen_s()` function is designed to improve the security of the `fopen()` function. Like the `fopen()` function, `fopen_s()` provides a mechanism to determine whether the file exists. See below for use of the exclusive mode flag. + +```cpp +char *file_name; +FILE *fp; + +/* Initialize file_name */ +errno_t res = fopen_s(&fp, file_name, "w"); +if (res != 0) { + /* Handle error */ +} + +``` + +## Compliant Solution (fopen_s(), C11 Annex K) + +The C Standard provides a new flag to address this problem. Subclause 7.21.5.3, paragraph 5 \[[ISO/IEC 9899:2011](https://www.securecoding.cert.org/confluence/display/seccode/AA.+Bibliography#AABibliography-ISOIEC9899-2011)\], states: + +> Opening a file with exclusive mode ('x' as the last character in the mode argument) fails if the file already exists or cannot be created. Otherwise, the file is created with exclusive (also known as non-shared) access to the extent that the underlying system supports exclusive access. + + +This option is also provided by the GNU C library \[[Loosemore 2007](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Loosemore07)\]. + +This compliant solution uses the `x` mode character to instruct `fopen_s()` to fail rather than open an existing file: + +```cpp +char *file_name; + +/* Initialize file_name */ +FILE *fp; +errno_t res = fopen_s(&fp, file_name, "wx"); +if (res != 0) { + /* Handle error */ +} + +``` +Use of this option allows for the easy remediation of legacy code. However, note that Microsoft Visual Studio 2012 and earlier do not support the `x` mode character \[[MSDN](http://msdn.microsoft.com/en-us/library/z5hh6ee9(v=vs.110).aspx)\]. + +## Compliant Solution (open(), POSIX) + +The `open()` function, as defined in the *Standard for Information Technology—Portable Operating System Interface (POSIX®), Base Specifications, Issue 7* \[[IEEE Std 1003.1:2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-IEEEStd1003.1-2013)\], is available on many platforms and provides finer control than `fopen()`. In particular, `open()` accepts the `O_CREAT` and `O_EXCL` flags. When used together, these flags instruct the `open()` function to fail if the file specified by `file_name` already exists. + +```cpp +char *file_name; +int new_file_mode; + +/* Initialize file_name and new_file_mode */ + +int fd = open(file_name, O_CREAT | O_EXCL | O_WRONLY, new_file_mode); +if (fd == -1) { + /* Handle error */ +} + +``` +Care should be taken when using `O_EXCL` with remote file systems because it does not work with NFS version 2. NFS version 3 added support for `O_EXCL` mode in `open()`. IETF RFC 1813 \[[Callaghan 1995](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Callaghan95)\] defines the `EXCLUSIVE` value to the `mode` argument of `CREATE`: + +> `EXCLUSIVE` specifies that the server is to follow exclusive creation semantics, using the verifier to ensure exclusive creation of the target. No attributes may be provided in this case, since the server may use the target file metadata to store the createverf3 verifier. + + +For examples of how to check for the existence of a file without opening it, see recommendation [FIO10-C. Take care when using the rename() function](https://wiki.sei.cmu.edu/confluence/display/c/FIO10-C.+Take+care+when+using+the+rename%28%29+function). + +## Compliant Solution (fdopen(), POSIX) + +For code that operates on `FILE` pointers and not file descriptors, the POSIX `fdopen()` function can be used to associate an open stream with the file descriptor returned by `open()`, as shown in this compliant solution \[[IEEE Std 1003.1:2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-IEEEStd1003.1-2013)\]: + +```cpp +char *file_name; +int new_file_mode; +FILE *fp; +int fd; + +/* Initialize file_name and new_file_mode */ + +fd = open(file_name, O_CREAT | O_EXCL | O_WRONLY, new_file_mode); +if (fd == -1) { + /* Handle error */ +} + +fp = fdopen(fd, "w"); +if (fp == NULL) { + /* Handle error */ +} + +``` + +## Compliant Solution (Windows) + +The Win32 API `[CreateFile()](http://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx)` allows a programmer to create or open a file depending on the flags passed in. Passing in the `CREATE_NEW` flag ensures the call fails if the file already exists. This compliant solution demonstrates how to open a file for reading and writing without sharing access to the file such that the call fails if the file already exists. + +```cpp +TCHAR *file_name; +HANDLE hFile = CreateFile(file_name, GENERIC_READ | GENERIC_WRITE, 0, 0, + CREATE_NEW, FILE_ATTRIBUTE_NORMAL, 0); +if (INVALID_HANDLE_VALUE == hFile) { + DWORD err = GetLastError(); + if (ERROR_FILE_EXISTS == err) { + /* Handle file exists error */ + } else { + /* Handle other error */ + } +} +``` + +## Risk Assessment + +The ability to determine whether an existing file has been opened or a new file has been created provides greater assurance that a file other than the intended file is not acted upon. + +<table> <tbody> <tr> <th> Recommendation </th> <th> Severity </th> <th> Likelihood </th> <th> Remediation Cost </th> <th> Priority </th> <th> Level </th> </tr> <tr> <td> FIO03-C </td> <td> Medium </td> <td> Probable </td> <td> High </td> <td> <strong>P4</strong> </td> <td> <strong>L3</strong> </td> </tr> </tbody> </table> + + +## Automated Detection + +<table> <tbody> <tr> <th> Tool </th> <th> Version </th> <th> Checker </th> <th> Description </th> </tr> <tr> <td> <a> Coverity </a> </td> <td> 6.5 </td> <td> <strong>OPEN_ARGS</strong> </td> <td> Fully implemented </td> </tr> <tr> <td> <a> Helix QAC </a> </td> <td> 2024.4 </td> <td> <strong>C5012</strong> </td> <td> </td> </tr> <tr> <td> <a> LDRA tool suite </a> </td> <td> 9.7.1 </td> <td> <strong>44 S</strong> </td> <td> Enhanced Enforcement </td> </tr> <tr> <td> <a> Polyspace Bug Finder </a> </td> <td> R2024b </td> <td> <a> CERT C: Rec. FIO03-C </a> </td> <td> Checks for file not opened in exclusive mode </td> </tr> </tbody> </table> + + +## Related Vulnerabilities + +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+FIO03-C). + +**Related Guidelines** + +<table> <tbody> <tr> <td> <a> SEI CERT C++ Coding Standard </a> </td> <td> <a> VOID FIO03-CPP. Do not make assumptions about fopen() and file creation </a> </td> </tr> <tr> <td> <a> ISO/IEC TR 24731-1:2007 </a> </td> <td> Section 6.5.2.1, "The <code>fopen_s</code> Function" </td> </tr> </tbody> </table> + + +## Bibliography + +<table> <tbody> <tr> <td> \[ <a> Callaghan 1995 </a> \] </td> <td> <a> IETF RFC 1813 NFS Version 3 Protocol Specification </a> </td> </tr> <tr> <td> \[ <a> IEEE Std 1003.1:2013 </a> \] </td> <td> System Interfaces: <code>open</code> </td> </tr> <tr> <td> \[ <a> ISO/IEC 9899:2011 </a> \] </td> <td> Subclause 7.21.5.3, "The <code>fopen</code> Function" Subclause K.3.5.2.1, "The <code>fopen_s</code> Function" </td> </tr> <tr> <td> \[ <a> Loosemore 2007 </a> \] </td> <td> <a> Section 12.3, "Opening Streams" </a> </td> </tr> <tr> <td> \[ <a> Seacord 2013 </a> \] </td> <td> Chapter 8, "File I/O" </td> </tr> </tbody> </table> + + +## Implementation notes + +None + +## References + +* CERT-C: [FIO03-C: Do not make assumptions about fopen() and file creation](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.ql b/c/cert/src/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.ql new file mode 100644 index 0000000000..cf5e5f3a48 --- /dev/null +++ b/c/cert/src/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.ql @@ -0,0 +1,37 @@ +/** + * @id c/cert/fopen-with-non-exclusive-file-creation-mode + * @name FIO03-C: Do not make assumptions about fopen() and file creation + * @description Usage of fopen() without the proper exclusion mode can lead to a program overwriting + * over accessing an unintended file. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/cert/id/fio03-c + * correctness + * security + * external/cert/obligation/recommendation + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.alertreporting.PrintExpr +import codingstandards.cpp.standardlibrary.FileAccess + +class PrettyPrintExpr extends Expr { + PrettyPrintExpr() { + exists(FOpenCall fopen | this.getParent*() = fopen.getMode()) and + ( + this instanceof BinaryOperation + or + this instanceof Literal + ) + } +} + +from FOpenCall fopen, string modeStr +where + not isExcluded(fopen, IO5Package::fopenWithNonExclusiveFileCreationModeQuery()) and + fopen.mayCreate() and + not fopen.isExclusiveMode() and + modeStr = PrintExpr<PrettyPrintExpr>::print(fopen.getMode()) +select fopen, "Call to create file with non-exclusive creation mode '" + modeStr + "'." diff --git a/c/cert/src/rules/FIO21-C/ExploitableTemporaryFileCreation.md b/c/cert/src/rules/FIO21-C/ExploitableTemporaryFileCreation.md new file mode 100644 index 0000000000..9ae3c60a4d --- /dev/null +++ b/c/cert/src/rules/FIO21-C/ExploitableTemporaryFileCreation.md @@ -0,0 +1,336 @@ +# FIO21-C: Do not create temporary files in shared directories + +This query implements the CERT-C rule FIO21-C: + +> Do not create temporary files in shared directories + + +## Description + +Programmers frequently create temporary files in directories that are writable by everyone (examples are `/tmp` and `/var/tmp` on UNIX and `%TEMP%` on Windows) and may be purged regularly (for example, every night or during reboot). + +Temporary files are commonly used for auxiliary storage for data that does not need to, or otherwise cannot, reside in memory and also as a means of communicating with other processes by transferring data through the file system. For example, one process will create a temporary file in a shared directory with a well-known name or a temporary name that is communicated to collaborating processes. The file then can be used to share information among these collaborating processes. + +This practice is dangerous because a well-known file in a shared directory can be easily hijacked or manipulated by an attacker. [Mitigation](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-mitigation) strategies include the following: + +1. Use other low-level IPC (interprocess communication) mechanisms such as sockets or shared memory. +1. Use higher-level IPC mechanisms such as remote procedure calls. +1. Use a secure directory or a jail that can be accessed only by application instances (ensuring that multiple instances of the application running on the same platform do not compete). +There are many different IPC mechanisms; some require the use of temporary files, and others do not. An example of an IPC mechanism that uses temporary files is the POSIX `mmap()` function. Berkeley Sockets, POSIX Local IPC Sockets, and System V Shared Memory do not require temporary files. Because the multiuser nature of shared directories poses an inherent security risk, the use of shared temporary files for IPC is discouraged. + +When two or more users or a group of users have write permission to a directory, the potential for deception is far greater than it is for shared access to a few files. Consequently, temporary files in shared directories must be + +* Created unpredictable file names +* Created with unique names +* Opened only if the file doesn't already exist (atomic open) +* Opened with exclusive access +* Opened with appropriate permissions +* Removed before the program exits +The following table lists common temporary file functions and their respective conformance to these criteria: + +Conformance of File Functions to Criteria for Temporary Files + +<table> <tbody> <tr> <th> </th> <th> <code>tmpnam</code> (C) </th> <th> <code>tmpnam_s</code> (Annex K) </th> <th> <code>tmpfile</code> (C/POSIX) </th> <th> <code>tmpfile_s</code> (Annex K) </th> <th> <code>mktemp</code> (POSIX) </th> <th> <code>mkstemp</code> (POSIX) </th> </tr> <tr> <td> <strong>Unpredictable Name</strong> </td> <td> Not portably </td> <td> Yes </td> <td> Not portably </td> <td> Yes </td> <td> Not portably </td> <td> Not portably </td> </tr> <tr> <td> <strong>Unique Name</strong> </td> <td> Yes </td> <td> Yes </td> <td> Yes </td> <td> Yes </td> <td> Yes </td> <td> Yes </td> </tr> <tr> <td> <strong>Atomic open</strong> </td> <td> No </td> <td> No </td> <td> Yes </td> <td> Yes </td> <td> No </td> <td> Yes </td> </tr> <tr> <td> <strong>Exclusive Access</strong> </td> <td> Possible </td> <td> Possible </td> <td> No </td> <td> If supported by OS </td> <td> Possible </td> <td> If supported by OS </td> </tr> <tr> <td> <strong>Appropriate Permissions</strong> </td> <td> Possible </td> <td> Possible </td> <td> If supported by OS \* </td> <td> If supported by OS </td> <td> Possible </td> <td> Not portably </td> </tr> <tr> <td> <strong>File Removed</strong> </td> <td> No </td> <td> No </td> <td> Yes\* </td> <td> Yes\* </td> <td> No </td> <td> No </td> </tr> </tbody> </table> +\* If the program terminates abnormally, this behavior is [implementation-defined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior). + + +Securely creating temporary files is error prone and dependent on the version of the C runtime library used, the operating system, and the file system. Code that works for a locally mounted file system, for example, may be vulnerable when used with a remotely mounted file system. Moreover, none of these functions are without problems. The only secure solution is to not create temporary files in shared directories. + +**Unique and Unpredictable File Names** + +Privileged programs that create temporary files in world-writable directories can be [exploited](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-exploit) to overwrite protected system files. An attacker who can predict the name of a file created by a privileged program can create a symbolic link (with the same name as the file used by the program) to point to a protected system file. Unless the privileged program is coded securely, the program will follow the symbolic link instead of opening or creating the file that it is supposed to be using. As a result, a protected system file to which the symbolic link points can be overwritten when the program is executed \[[HP 2003](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-HP03)\]. Unprivileged programs can be similarly exploited to overwrite protected user files. + +**Exclusive Access** + +Exclusive access grants unrestricted file access to the locking process while denying access to all other processes and eliminates the potential for a race condition on the locked region. (See *Secure Coding in C and* C++, Chapter 8**\[[Seacord 2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Seacord2013)\].) + +Files, or regions of files, can be locked to prevent two processes from concurrent access. Windows supports two types of file locks: + +* *Shared locks*, provided by `LockFile()`, prohibit all write access to the locked file region while allowing concurrent read access to all processes. +* *Exclusive locks*, provided by `LockFileEx()`, grant unrestricted file access to the locking process while denying access to all other processes. +In both cases, the lock is removed by calling `UnlockFile()`. + +Both shared locks and exclusive locks eliminate the potential for a race condition on the locked region. The exclusive lock is similar to a mutual exclusion solution, and the shared lock eliminates race conditions by removing the potential for altering the state of the locked file region (one of the required properties for a race). + +These Windows file-locking mechanisms are called *mandatory locks* because every process attempting to access a locked file region is subject to the restriction. Linux implements mandatory locks and advisory locks. An advisory lock is not enforced by the operating system, which severely diminishes its value from a security perspective. Unfortunately, the mandatory file lock in Linux is also largely impractical for the following reasons: + +* Mandatory locking works only on local file systems and does not extend to network file systems (such as NFS or AFS). +* File systems must be mounted with support for mandatory locking, and this is disabled by default. +* Locking relies on the group ID bit that can be turned off by another process (thereby defeating the lock). +**Removal before Termination** + +Removing temporary files when they are no longer required allows file names and other resources (such as secondary storage) to be recycled. In the case of [abnormal termination](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-abnormaltermination), there is no sure method that can guarantee the removal of orphaned files. For this reason, temporary file cleaner utilities, which are invoked manually by a system administrator or periodically run by a daemon to sweep temporary directories and remove old files, are widely used. However, these utilities are themselves vulnerable to file-based [exploits](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-exploit) and often require the use of shared directories. During normal operation, it is the responsibility of the program to ensure that temporary files are removed either explicitly or through the use of library routines, such as `tmpfile_s`, which guarantee temporary file deletion upon program termination. + +## Noncompliant Code Example (fopen()/open() with tmpnam()) + +This noncompliant code example creates a file with a hard-coded `file_name` (presumably in a shared directory such as `/tmp` or `C:\Temp`): + +```cpp +#include <stdio.h> + +void func(const char *file_name) { + FILE *fp = fopen(file_name, "wb+"); + if (fp == NULL) { + /* Handle error */ + } +} +``` +Because the name is hard coded and consequently neither unique nor unpredictable, an attacker need only replace a file with a symbolic link, and the target file referenced by the link is opened and truncated. + +This noncompliant code example attempts to remedy the problem by generating the file name at runtime using `tmpnam()`. The C `tmpnam()` function generates a string that is a valid file name and that is not the same as the name of an existing file. Files created using strings generated by the `tmpnam()` function are temporary in that their names should not collide with those generated by conventional naming rules for the [implementation](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation). The function is potentially capable of generating `TMP_MAX` different strings, but any or all of them may already be in use by existing files. + +```cpp +#include <stdio.h> + +void func(void) { + char file_name[L_tmpnam]; + FILE *fp; + + if (!tmpnam(file_name)) { + /* Handle error */ + } + + /* A TOCTOU race condition exists here */ + + fp = fopen(file_name, "wb+"); + if (fp == NULL) { + /* Handle error */ + } +} +``` +Because `tmpnam()` does not guarantee a unique name and `fopen()` does not provide a facility for an exclusive open, this code is still vulnerable. + +The next noncompliant code example attempts to remedy the problem by using the POSIX `open()` function and providing a mechanism to indicate whether an existing file has been opened for writing or a new file has been created \[[IEEE Std 1003.1:2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-IEEEStd1003.1-2013)\]. If the `O_CREAT` and `O_EXCL` flags are used together, the `open()` function fails when the file specified by `file_name` already exists. To prevent an existing file from being opened and truncated, include the flags `O_CREAT` and `O_EXCL` when calling `open()`: + +```cpp +#include <stdio.h> + +void func(void) { + char file_name[L_tmpnam]; + int fd; + + if (!(tmpnam(file_name))) { + /* Handle error */ + } + + /* A TOCTOU race condition exists here */ + + fd = open(file_name, O_WRONLY | O_CREAT | O_EXCL | O_TRUNC, + 0600); + if (fd < 0) { + /* Handle error */ + } +} +``` +This call to `open()` fails whenever `file_name` already exists, including when it is a symbolic link, but a temporary file is presumably still required. Additionally, the method used by `tmpnam()` to generate file names is not guaranteed to be unpredictable, which leaves room for an attacker to guess the file name ahead of time. + +Care should be observed when using `O_EXCL` with remote file systems because it does not work with NFS version 2. NFS version 3 added support for `O_EXCL` mode in `open()`; see IETF RFC 1813 \[[Callaghan 1995](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Callaghan95)\], particularly the `EXCLUSIVE` value to the `mode` argument of `CREATE`. + +Moreover, the `open()` function, as specified by the *Standard for Information Technology—Portable Operating System Interface (POSIX®), Base Specifications, Issue 7* \[[IEEE Std 1003.1:2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-IEEEStd1003.1-2013)\], does not include support for shared or exclusive locks. However, BSD systems support two additional flags that allow you to obtain these locks: + +* `O_SHLOCK`: Atomically obtain a shared lock. +* `O_EXLOCK`: Atomically obtain an exclusive lock. + +## Noncompliant Code Example (tmpnam_s()/open(), Annex K, POSIX) + +The C Standard function `tmpnam_s()` function generates a string that is a valid file name and that is not the same as the name of an existing file. It is almost identical to the `tmpnam()` function except for an added `maxsize` argument for the supplied buffer. + +```cpp +#define __STDC_WANT_LIB_EXT1__ +#include <stdio.h> + +void func(void) { + char file_name[L_tmpnam_s]; + int fd; + + if (tmpnam_s(file_name, L_tmpnam_s) != 0) { + /* Handle error */ + } + + /* A TOCTOU race condition exists here */ + + fd = open(file_name, O_WRONLY | O_CREAT | O_EXCL | O_TRUNC, + 0600); + if (fd < 0) { + /* Handle error */ + } +} +``` +Nonnormative text in the C Standard, subclause K.3.5.1.2 \[[ISO/IEC 9899:2011](https://www.securecoding.cert.org/confluence/display/seccode/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], also recommends the following: + +> Implementations should take care in choosing the patterns used for names returned by `tmpnam_s`. For example, making a thread id part of the names avoids the race condition and possible conflict when multiple programs run simultaneously by the same user generate the same temporary file names. + + +If implemented, the space for unique names is reduced and the predictability of the resulting names is increased. In general, Annex K does not establish any criteria for the predictability of names. For example, the name generated by the `tmpnam_s` function from Microsoft Visual Studio consists of a program-generated file name and, after the first call to `tmpnam_s()`, a file extension of sequential numbers in base 32 (.1-.1vvvvvu). + +## Noncompliant Code Example (mktemp()/open(), POSIX) + +The POSIX function `mktemp()` takes a given file name template and overwrites a portion of it to create a file name. The template may be any file name with exactly six X's appended to it (for example, `/tmp/temp.XXXXXX`). The six trailing X's are replaced with the current process number and/or a unique letter combination. + +```cpp +#include <stdio.h> +#include <stdlib.h> + +void func(void) { + char file_name[] = "tmp-XXXXXX"; + int fd; + + if (!mktemp(file_name)) { + /* Handle error */ + } + + /* A TOCTOU race condition exists here */ + + fd = open(file_name, O_WRONLY | O_CREAT | O_EXCL | O_TRUNC, + 0600); + if (fd < 0) { + /* Handle error */ + } +} +``` +The `mktemp()` function is marked "LEGACY" in the Open Group Base Specifications Issue 6 \[[Open Group 2004](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-OpenGroup04)\]. The manual page for `mktemp()` gives more detail: + +> Never use `mktemp()`. Some implementations follow BSD 4.3 and replace `XXXXXX` by the current process id and a single letter, so that at most 26 different names can be returned. Since on the one hand the names are easy to guess, and on the other hand there is a race between testing whether the name exists and opening the file, every use of `mktemp()` is a security risk. The race is avoided by `mkstemp(3)`. + + +## Noncompliant Code Example (tmpfile()) + +The `tmpfile()` function creates a temporary binary file that is different from any other existing file and that is automatically removed when it is closed or at program termination. + +It should be possible to open at least `TMP_MAX` temporary files during the lifetime of the program. (This limit may be shared with `tmpnam()`.) Subclause 7.21.4.4, paragraph 6, of the C Standard allows for the value of the macro `TMP_MAX` to be as small as 25. + +Most historic implementations provide only a limited number of possible temporary file names (usually 26) before file names are recycled. + +```cpp +#include <stdio.h> + +void func(void) { + FILE *fp = tmpfile(); + if (fp == NULL) { + /* Handle error */ + } +} +``` + +## Noncompliant Code Example (tmpfile_s(), Annex K) + +The `tmpfile_s()` function creates a temporary binary file that is different from any other existing file and is automatically removed when it is closed or at program termination. If the program [terminates abnormally](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-abnormaltermination), whether an open temporary file is removed is [implementation-defined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior). + +The file is opened for update with `"wb+"` mode, which means "truncate to zero length or create binary file for update." To the extent that the underlying system supports the concepts, the file is opened with exclusive (nonshared) access and has a file permission that prevents other users on the system from accessing the file. + +It should be possible to open at least `TMP_MAX_S` temporary files during the lifetime of the program. (This limit may be shared with `tmpnam_s()`.) The value of the macro `TMP_MAX_S` is required to be only 25 \[[ISO/IEC 9899:2011](https://www.securecoding.cert.org/confluence/display/seccode/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\]. + +The C Standard, subclause K3.5.1.2, paragraph 7, notes the following regarding the use of `tmpfile_s()` instead of `tmpnam_s()` \[[ISO/IEC 9899:2011](https://www.securecoding.cert.org/confluence/display/seccode/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\]: + +> After a program obtains a file name using the `tmpnam_s` function and before the program creates a file with that name, the possibility exists that someone else may create a file with that same name. To avoid this race condition, the `tmpfile_s` function should be used instead of `tmpnam_s` when possible. One situation that requires the use of the `tmpnam_s` function is when the program needs to create a temporary directory rather than a temporary file. + + +```cpp +#define __STDC_WANT_LIB_EXT1__ +#include <stdio.h> + +void func(void) { + FILE *fp; + + if (tmpfile_s(&fp)) { + /* Handle error */ + } +} +``` +The `tmpfile_s()` function should not be used with implementations that create temporary files in a shared directory, such as `/tmp` or `C:`, because the function does not allow the user to specify a directory in which the temporary file should be created. + +## Compliant Solution (mkstemp(), POSIX) + +The `mkstemp()` algorithm for selecting file names has shown to be immune to attacks. The `mkstemp()` function is available on systems that support the Open Group Base Specifications Issue 4, version 2 or later. + +A call to `mkstemp()` replaces the six `X`'s in the template string with six randomly selected characters and returns a file descriptor for the file (opened for reading and writing), as in this compliant solution: + +```cpp +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> + +extern int secure_dir(const char *sdn); + +void func(void) { + const char *sdn = "/home/usr1/"; + char sfn[] = "/home/usr1/temp-XXXXXX"; + FILE *sfp; + + if (!secure_dir(sdn)) { + /* Handle error */ + } + + int fd = mkstemp(sfn); + if (fd == -1) { + /* Handle error */ + } + + /* + * Unlink immediately to hide the file name. The race + * condition here is inconsequential if the file is created + * with exclusive permissions (glibc >= 2.0.7). + */ + if (unlink(sfn) == -1) { + /* Handle error */ + } + + sfp = fdopen(fd, "w+"); + if (sfp == NULL) { + close(fd); + /* Handle error */ + } + + /* Use temporary file */ + + fclose(sfp); /* Also closes fd */ +} +``` +This solution is not serially reusable, however, because the `mkstemp()` function replaces the `"XXXXXX"` in `template` the first time it is invoked. This is not a problem as long as `template` is reinitialized before calling `mkstemp()` again. If `template` is not reinitialized, the `mkstemp()` function will return `-1` and leave `template` unmodified because `template` did not contain six `X`'s. + +The Open Group Base Specification Issue 6 \[[Open Group 2004](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-OpenGroup04)\] does not specify the permissions the file is created with, so these are [implementation-defined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior). However, IEEE Std 1003.1, 2013 Edition \[[IEEE Std 1003.1:2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-IEEEStd1003.1-2013)\] specifies them as `S_IRUSR|S_IWUSR` (0600). + +This compliant solution invokes the user-defined function `secure_dir()` (such as the one defined in [FIO15-C. Ensure that file operations are performed in a secure directory](https://wiki.sei.cmu.edu/confluence/display/c/FIO15-C.+Ensure+that+file+operations+are+performed+in+a+secure+directory)) to ensure the temporary file resides in a secure directory. + +**Implementation Details** + +For GLIBC, versions 2.0.6 and earlier, the file is created with permissions 0666; for GLIBC, versions 2.0.7 and later, the file is created with permissions 0600. On NetBSD, the file is created with permissions 0600. This creates a security risk in that an attacker will have write access to the file immediately after creation. Consequently, programs need a private version of the `mkstemp()` function in which this issue is known to be fixed. + +In many older [implementations](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation), the name is a function of process ID and time, so it is possible for the attacker to predict the name and create a decoy in advance. FreeBSD changed the `mk*temp()` family to eliminate the process ID component of the file name and replace the entire field with base-62 encoded randomness. This raises the number of possible temporary files for the typical use of six `X`'s significantly, meaning that even `mktemp()` with six `X`'s is reasonably (probabilistically) secure against guessing except under frequent usage \[[Kennaway 2000](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Kennaway00)\]. + +## Exceptions + +**FIO21-C-EX1:** The Annex K `tmpfile_s()` function can be used if all the targeted implementations create temporary files in secure directories. + +## Risk Assessment + +Insecure temporary file creation can lead to a program accessing unintended files and permission escalation on local systems. + +<table> <tbody> <tr> <th> Recommendation </th> <th> Severity </th> <th> Likelihood </th> <th> Remediation Cost </th> <th> Priority </th> <th> Level </th> </tr> <tr> <td> FIO21-C </td> <td> Medium </td> <td> Probable </td> <td> Medium </td> <td> <strong>P8</strong> </td> <td> <strong>L2</strong> </td> </tr> </tbody> </table> +** Automated Detection** + + +<table> <tbody> <tr> <th> Tool </th> <th> Version </th> <th> Checker </th> <th> Description </th> </tr> <tr> <td> <a> CodeSonar </a> </td> <td> 9.0p0 </td> <td> <strong>BADFUNC.TEMP.\*</strong> <strong>BADFUNC.TMPFILE_S</strong> <strong>BADFUNC.TMPNAM_S</strong> </td> <td> A collection of checks that report uses of library functions associated with temporary file vulnerabilities Use of tmpfile_s Use of tmpnam_s </td> </tr> <tr> <td> <a> Compass/ROSE </a> </td> <td> </td> <td> </td> <td> Can detect violations of this recommendation. Specifically, Rose reports use of <code>tmpnam()</code> , <code>tmpnam_s()</code> , <code>tmpfile()</code> , and <code>mktemp()</code> </td> </tr> <tr> <td> <a> Coverity </a> </td> <td> 6.5 </td> <td> <strong>SECURE_TEMP</strong> </td> <td> Fully implemented </td> </tr> <tr> <td> <a> Helix QAC </a> </td> <td> 2024.4 </td> <td> <strong>C5016</strong> </td> <td> </td> </tr> <tr> <td> <a> LDRA tool suite </a> </td> <td> 9.7.1 </td> <td> <strong>44 S</strong> </td> <td> Enhanced enforcement </td> </tr> <tr> <td> <a> Parasoft C/C++test </a> </td> <td> 2024.2 </td> <td> <strong>CERT_C-FIO21-b</strong> </td> <td> Use secure temporary file name functions </td> </tr> <tr> <td> <a> Polyspace Bug Finder </a> </td> <td> R2024b </td> <td> <a> CERT C: Rec. FIO21-C </a> </td> <td> Checks for non-secure temporary file (rec. partially covered) </td> </tr> </tbody> </table> + + +## Related Vulnerabilities + +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+FIO21-C). + +## Related Guidelines + +<table> <tbody> <tr> <td> <a> CERT C Secure Coding Standard </a> </td> <td> <a> FIO15-C. Ensure that file operations are performed in a secure directory </a> </td> </tr> <tr> <td> <a> SEI CERT C++ Coding Standard </a> </td> <td> <a> VOID FIO19-CPP. Do not create temporary files in shared directories </a> </td> </tr> <tr> <td> <a> CERT Oracle Secure Coding Standard for Java </a> </td> <td> <a> FIO03-J. Remove temporary files before termination </a> </td> </tr> <tr> <td> <a> ISO/IEC TR 24772:2013 </a> </td> <td> Path Traversal \[EWR\] </td> </tr> <tr> <td> <a> MITRE CWE </a> </td> <td> <a> CWE-379 </a> , Creation of temporary file in directory with insecure permissions </td> </tr> </tbody> </table> + + +## Bibliography + +<table> <tbody> <tr> <td> \[ <a> HP 2003 </a> \] </td> <td> </td> </tr> <tr> <td> \[ <a> IEEE Std 1003.1:2013 </a> \] </td> <td> XSH, System Interfaces: <code>open</code> XSH, System Interfaces: <code>mkdopen</code> , <code> mksopen</code> </td> </tr> <tr> <td> \[ <a> ISO/IEC 9899:2011 </a> \] </td> <td> Subclause K.3.5.1.2, "The <code>tmpnam_s</code> Function" Subclause 7.21.4.4, "The <code>tmpnam</code> Function </td> </tr> <tr> <td> \[ <a> Kennaway 2000 </a> \] </td> <td> </td> </tr> <tr> <td> \[ <a> Open Group 2004 </a> \] </td> <td> <code><a>mkstemp()</a>mktemp()</code> <a> open() </a> </td> </tr> <tr> <td> \[ <a> Seacord 2013 </a> \] </td> <td> Chapter 3, "Pointer Subterfuge" Chapter 8, "File I/O" </td> </tr> <tr> <td> \[ <a> Viega 2003 </a> \] </td> <td> Section 2.1, "Creating Files for Temporary Use" </td> </tr> <tr> <td> \[ <a> Wheeler 2003 </a> \] </td> <td> <a> Chapter 7, "Structure Program Internals and Approach" </a> </td> </tr> </tbody> </table> + + +## Implementation notes + +None + +## References + +* CERT-C: [FIO21-C: Do not create temporary files in shared directories](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/FIO21-C/ExploitableTemporaryFileCreation.ql b/c/cert/src/rules/FIO21-C/ExploitableTemporaryFileCreation.ql new file mode 100644 index 0000000000..856732b2d2 --- /dev/null +++ b/c/cert/src/rules/FIO21-C/ExploitableTemporaryFileCreation.ql @@ -0,0 +1,22 @@ +/** + * @id c/cert/exploitable-temporary-file-creation + * @name FIO21-C: Do not create temporary files in shared directories + * @description Temporary files can be hijacked or manipulated by an attacker if not created in a + * secure manner. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/cert/id/fio21-c + * security + * external/cert/obligation/recommendation + */ + +import cpp +import codingstandards.c.cert + +from FunctionCall fc, string funcName +where + not isExcluded(fc, IO5Package::exploitableTemporaryFileCreationQuery()) and + funcName = fc.getTarget().getName() and + funcName in ["tmpfile", "tmpfile_s", "tmpnam", "tmpnam_s", "mktemp"] +select fc, "Unsafe temporary file creation function '" + funcName + "' called." diff --git a/c/cert/src/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.md b/c/cert/src/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.md new file mode 100644 index 0000000000..ffe1c45425 --- /dev/null +++ b/c/cert/src/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.md @@ -0,0 +1,336 @@ +# FIO21-C: Do not use predictably generated temporary file paths + +This query implements the CERT-C rule FIO21-C: + +> Do not create temporary files in shared directories + + +## Description + +Programmers frequently create temporary files in directories that are writable by everyone (examples are `/tmp` and `/var/tmp` on UNIX and `%TEMP%` on Windows) and may be purged regularly (for example, every night or during reboot). + +Temporary files are commonly used for auxiliary storage for data that does not need to, or otherwise cannot, reside in memory and also as a means of communicating with other processes by transferring data through the file system. For example, one process will create a temporary file in a shared directory with a well-known name or a temporary name that is communicated to collaborating processes. The file then can be used to share information among these collaborating processes. + +This practice is dangerous because a well-known file in a shared directory can be easily hijacked or manipulated by an attacker. [Mitigation](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-mitigation) strategies include the following: + +1. Use other low-level IPC (interprocess communication) mechanisms such as sockets or shared memory. +1. Use higher-level IPC mechanisms such as remote procedure calls. +1. Use a secure directory or a jail that can be accessed only by application instances (ensuring that multiple instances of the application running on the same platform do not compete). +There are many different IPC mechanisms; some require the use of temporary files, and others do not. An example of an IPC mechanism that uses temporary files is the POSIX `mmap()` function. Berkeley Sockets, POSIX Local IPC Sockets, and System V Shared Memory do not require temporary files. Because the multiuser nature of shared directories poses an inherent security risk, the use of shared temporary files for IPC is discouraged. + +When two or more users or a group of users have write permission to a directory, the potential for deception is far greater than it is for shared access to a few files. Consequently, temporary files in shared directories must be + +* Created unpredictable file names +* Created with unique names +* Opened only if the file doesn't already exist (atomic open) +* Opened with exclusive access +* Opened with appropriate permissions +* Removed before the program exits +The following table lists common temporary file functions and their respective conformance to these criteria: + +Conformance of File Functions to Criteria for Temporary Files + +<table> <tbody> <tr> <th> </th> <th> <code>tmpnam</code> (C) </th> <th> <code>tmpnam_s</code> (Annex K) </th> <th> <code>tmpfile</code> (C/POSIX) </th> <th> <code>tmpfile_s</code> (Annex K) </th> <th> <code>mktemp</code> (POSIX) </th> <th> <code>mkstemp</code> (POSIX) </th> </tr> <tr> <td> <strong>Unpredictable Name</strong> </td> <td> Not portably </td> <td> Yes </td> <td> Not portably </td> <td> Yes </td> <td> Not portably </td> <td> Not portably </td> </tr> <tr> <td> <strong>Unique Name</strong> </td> <td> Yes </td> <td> Yes </td> <td> Yes </td> <td> Yes </td> <td> Yes </td> <td> Yes </td> </tr> <tr> <td> <strong>Atomic open</strong> </td> <td> No </td> <td> No </td> <td> Yes </td> <td> Yes </td> <td> No </td> <td> Yes </td> </tr> <tr> <td> <strong>Exclusive Access</strong> </td> <td> Possible </td> <td> Possible </td> <td> No </td> <td> If supported by OS </td> <td> Possible </td> <td> If supported by OS </td> </tr> <tr> <td> <strong>Appropriate Permissions</strong> </td> <td> Possible </td> <td> Possible </td> <td> If supported by OS \* </td> <td> If supported by OS </td> <td> Possible </td> <td> Not portably </td> </tr> <tr> <td> <strong>File Removed</strong> </td> <td> No </td> <td> No </td> <td> Yes\* </td> <td> Yes\* </td> <td> No </td> <td> No </td> </tr> </tbody> </table> +\* If the program terminates abnormally, this behavior is [implementation-defined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior). + + +Securely creating temporary files is error prone and dependent on the version of the C runtime library used, the operating system, and the file system. Code that works for a locally mounted file system, for example, may be vulnerable when used with a remotely mounted file system. Moreover, none of these functions are without problems. The only secure solution is to not create temporary files in shared directories. + +**Unique and Unpredictable File Names** + +Privileged programs that create temporary files in world-writable directories can be [exploited](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-exploit) to overwrite protected system files. An attacker who can predict the name of a file created by a privileged program can create a symbolic link (with the same name as the file used by the program) to point to a protected system file. Unless the privileged program is coded securely, the program will follow the symbolic link instead of opening or creating the file that it is supposed to be using. As a result, a protected system file to which the symbolic link points can be overwritten when the program is executed \[[HP 2003](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-HP03)\]. Unprivileged programs can be similarly exploited to overwrite protected user files. + +**Exclusive Access** + +Exclusive access grants unrestricted file access to the locking process while denying access to all other processes and eliminates the potential for a race condition on the locked region. (See *Secure Coding in C and* C++, Chapter 8**\[[Seacord 2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Seacord2013)\].) + +Files, or regions of files, can be locked to prevent two processes from concurrent access. Windows supports two types of file locks: + +* *Shared locks*, provided by `LockFile()`, prohibit all write access to the locked file region while allowing concurrent read access to all processes. +* *Exclusive locks*, provided by `LockFileEx()`, grant unrestricted file access to the locking process while denying access to all other processes. +In both cases, the lock is removed by calling `UnlockFile()`. + +Both shared locks and exclusive locks eliminate the potential for a race condition on the locked region. The exclusive lock is similar to a mutual exclusion solution, and the shared lock eliminates race conditions by removing the potential for altering the state of the locked file region (one of the required properties for a race). + +These Windows file-locking mechanisms are called *mandatory locks* because every process attempting to access a locked file region is subject to the restriction. Linux implements mandatory locks and advisory locks. An advisory lock is not enforced by the operating system, which severely diminishes its value from a security perspective. Unfortunately, the mandatory file lock in Linux is also largely impractical for the following reasons: + +* Mandatory locking works only on local file systems and does not extend to network file systems (such as NFS or AFS). +* File systems must be mounted with support for mandatory locking, and this is disabled by default. +* Locking relies on the group ID bit that can be turned off by another process (thereby defeating the lock). +**Removal before Termination** + +Removing temporary files when they are no longer required allows file names and other resources (such as secondary storage) to be recycled. In the case of [abnormal termination](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-abnormaltermination), there is no sure method that can guarantee the removal of orphaned files. For this reason, temporary file cleaner utilities, which are invoked manually by a system administrator or periodically run by a daemon to sweep temporary directories and remove old files, are widely used. However, these utilities are themselves vulnerable to file-based [exploits](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-exploit) and often require the use of shared directories. During normal operation, it is the responsibility of the program to ensure that temporary files are removed either explicitly or through the use of library routines, such as `tmpfile_s`, which guarantee temporary file deletion upon program termination. + +## Noncompliant Code Example (fopen()/open() with tmpnam()) + +This noncompliant code example creates a file with a hard-coded `file_name` (presumably in a shared directory such as `/tmp` or `C:\Temp`): + +```cpp +#include <stdio.h> + +void func(const char *file_name) { + FILE *fp = fopen(file_name, "wb+"); + if (fp == NULL) { + /* Handle error */ + } +} +``` +Because the name is hard coded and consequently neither unique nor unpredictable, an attacker need only replace a file with a symbolic link, and the target file referenced by the link is opened and truncated. + +This noncompliant code example attempts to remedy the problem by generating the file name at runtime using `tmpnam()`. The C `tmpnam()` function generates a string that is a valid file name and that is not the same as the name of an existing file. Files created using strings generated by the `tmpnam()` function are temporary in that their names should not collide with those generated by conventional naming rules for the [implementation](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation). The function is potentially capable of generating `TMP_MAX` different strings, but any or all of them may already be in use by existing files. + +```cpp +#include <stdio.h> + +void func(void) { + char file_name[L_tmpnam]; + FILE *fp; + + if (!tmpnam(file_name)) { + /* Handle error */ + } + + /* A TOCTOU race condition exists here */ + + fp = fopen(file_name, "wb+"); + if (fp == NULL) { + /* Handle error */ + } +} +``` +Because `tmpnam()` does not guarantee a unique name and `fopen()` does not provide a facility for an exclusive open, this code is still vulnerable. + +The next noncompliant code example attempts to remedy the problem by using the POSIX `open()` function and providing a mechanism to indicate whether an existing file has been opened for writing or a new file has been created \[[IEEE Std 1003.1:2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-IEEEStd1003.1-2013)\]. If the `O_CREAT` and `O_EXCL` flags are used together, the `open()` function fails when the file specified by `file_name` already exists. To prevent an existing file from being opened and truncated, include the flags `O_CREAT` and `O_EXCL` when calling `open()`: + +```cpp +#include <stdio.h> + +void func(void) { + char file_name[L_tmpnam]; + int fd; + + if (!(tmpnam(file_name))) { + /* Handle error */ + } + + /* A TOCTOU race condition exists here */ + + fd = open(file_name, O_WRONLY | O_CREAT | O_EXCL | O_TRUNC, + 0600); + if (fd < 0) { + /* Handle error */ + } +} +``` +This call to `open()` fails whenever `file_name` already exists, including when it is a symbolic link, but a temporary file is presumably still required. Additionally, the method used by `tmpnam()` to generate file names is not guaranteed to be unpredictable, which leaves room for an attacker to guess the file name ahead of time. + +Care should be observed when using `O_EXCL` with remote file systems because it does not work with NFS version 2. NFS version 3 added support for `O_EXCL` mode in `open()`; see IETF RFC 1813 \[[Callaghan 1995](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Callaghan95)\], particularly the `EXCLUSIVE` value to the `mode` argument of `CREATE`. + +Moreover, the `open()` function, as specified by the *Standard for Information Technology—Portable Operating System Interface (POSIX®), Base Specifications, Issue 7* \[[IEEE Std 1003.1:2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-IEEEStd1003.1-2013)\], does not include support for shared or exclusive locks. However, BSD systems support two additional flags that allow you to obtain these locks: + +* `O_SHLOCK`: Atomically obtain a shared lock. +* `O_EXLOCK`: Atomically obtain an exclusive lock. + +## Noncompliant Code Example (tmpnam_s()/open(), Annex K, POSIX) + +The C Standard function `tmpnam_s()` function generates a string that is a valid file name and that is not the same as the name of an existing file. It is almost identical to the `tmpnam()` function except for an added `maxsize` argument for the supplied buffer. + +```cpp +#define __STDC_WANT_LIB_EXT1__ +#include <stdio.h> + +void func(void) { + char file_name[L_tmpnam_s]; + int fd; + + if (tmpnam_s(file_name, L_tmpnam_s) != 0) { + /* Handle error */ + } + + /* A TOCTOU race condition exists here */ + + fd = open(file_name, O_WRONLY | O_CREAT | O_EXCL | O_TRUNC, + 0600); + if (fd < 0) { + /* Handle error */ + } +} +``` +Nonnormative text in the C Standard, subclause K.3.5.1.2 \[[ISO/IEC 9899:2011](https://www.securecoding.cert.org/confluence/display/seccode/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], also recommends the following: + +> Implementations should take care in choosing the patterns used for names returned by `tmpnam_s`. For example, making a thread id part of the names avoids the race condition and possible conflict when multiple programs run simultaneously by the same user generate the same temporary file names. + + +If implemented, the space for unique names is reduced and the predictability of the resulting names is increased. In general, Annex K does not establish any criteria for the predictability of names. For example, the name generated by the `tmpnam_s` function from Microsoft Visual Studio consists of a program-generated file name and, after the first call to `tmpnam_s()`, a file extension of sequential numbers in base 32 (.1-.1vvvvvu). + +## Noncompliant Code Example (mktemp()/open(), POSIX) + +The POSIX function `mktemp()` takes a given file name template and overwrites a portion of it to create a file name. The template may be any file name with exactly six X's appended to it (for example, `/tmp/temp.XXXXXX`). The six trailing X's are replaced with the current process number and/or a unique letter combination. + +```cpp +#include <stdio.h> +#include <stdlib.h> + +void func(void) { + char file_name[] = "tmp-XXXXXX"; + int fd; + + if (!mktemp(file_name)) { + /* Handle error */ + } + + /* A TOCTOU race condition exists here */ + + fd = open(file_name, O_WRONLY | O_CREAT | O_EXCL | O_TRUNC, + 0600); + if (fd < 0) { + /* Handle error */ + } +} +``` +The `mktemp()` function is marked "LEGACY" in the Open Group Base Specifications Issue 6 \[[Open Group 2004](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-OpenGroup04)\]. The manual page for `mktemp()` gives more detail: + +> Never use `mktemp()`. Some implementations follow BSD 4.3 and replace `XXXXXX` by the current process id and a single letter, so that at most 26 different names can be returned. Since on the one hand the names are easy to guess, and on the other hand there is a race between testing whether the name exists and opening the file, every use of `mktemp()` is a security risk. The race is avoided by `mkstemp(3)`. + + +## Noncompliant Code Example (tmpfile()) + +The `tmpfile()` function creates a temporary binary file that is different from any other existing file and that is automatically removed when it is closed or at program termination. + +It should be possible to open at least `TMP_MAX` temporary files during the lifetime of the program. (This limit may be shared with `tmpnam()`.) Subclause 7.21.4.4, paragraph 6, of the C Standard allows for the value of the macro `TMP_MAX` to be as small as 25. + +Most historic implementations provide only a limited number of possible temporary file names (usually 26) before file names are recycled. + +```cpp +#include <stdio.h> + +void func(void) { + FILE *fp = tmpfile(); + if (fp == NULL) { + /* Handle error */ + } +} +``` + +## Noncompliant Code Example (tmpfile_s(), Annex K) + +The `tmpfile_s()` function creates a temporary binary file that is different from any other existing file and is automatically removed when it is closed or at program termination. If the program [terminates abnormally](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-abnormaltermination), whether an open temporary file is removed is [implementation-defined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior). + +The file is opened for update with `"wb+"` mode, which means "truncate to zero length or create binary file for update." To the extent that the underlying system supports the concepts, the file is opened with exclusive (nonshared) access and has a file permission that prevents other users on the system from accessing the file. + +It should be possible to open at least `TMP_MAX_S` temporary files during the lifetime of the program. (This limit may be shared with `tmpnam_s()`.) The value of the macro `TMP_MAX_S` is required to be only 25 \[[ISO/IEC 9899:2011](https://www.securecoding.cert.org/confluence/display/seccode/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\]. + +The C Standard, subclause K3.5.1.2, paragraph 7, notes the following regarding the use of `tmpfile_s()` instead of `tmpnam_s()` \[[ISO/IEC 9899:2011](https://www.securecoding.cert.org/confluence/display/seccode/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\]: + +> After a program obtains a file name using the `tmpnam_s` function and before the program creates a file with that name, the possibility exists that someone else may create a file with that same name. To avoid this race condition, the `tmpfile_s` function should be used instead of `tmpnam_s` when possible. One situation that requires the use of the `tmpnam_s` function is when the program needs to create a temporary directory rather than a temporary file. + + +```cpp +#define __STDC_WANT_LIB_EXT1__ +#include <stdio.h> + +void func(void) { + FILE *fp; + + if (tmpfile_s(&fp)) { + /* Handle error */ + } +} +``` +The `tmpfile_s()` function should not be used with implementations that create temporary files in a shared directory, such as `/tmp` or `C:`, because the function does not allow the user to specify a directory in which the temporary file should be created. + +## Compliant Solution (mkstemp(), POSIX) + +The `mkstemp()` algorithm for selecting file names has shown to be immune to attacks. The `mkstemp()` function is available on systems that support the Open Group Base Specifications Issue 4, version 2 or later. + +A call to `mkstemp()` replaces the six `X`'s in the template string with six randomly selected characters and returns a file descriptor for the file (opened for reading and writing), as in this compliant solution: + +```cpp +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> + +extern int secure_dir(const char *sdn); + +void func(void) { + const char *sdn = "/home/usr1/"; + char sfn[] = "/home/usr1/temp-XXXXXX"; + FILE *sfp; + + if (!secure_dir(sdn)) { + /* Handle error */ + } + + int fd = mkstemp(sfn); + if (fd == -1) { + /* Handle error */ + } + + /* + * Unlink immediately to hide the file name. The race + * condition here is inconsequential if the file is created + * with exclusive permissions (glibc >= 2.0.7). + */ + if (unlink(sfn) == -1) { + /* Handle error */ + } + + sfp = fdopen(fd, "w+"); + if (sfp == NULL) { + close(fd); + /* Handle error */ + } + + /* Use temporary file */ + + fclose(sfp); /* Also closes fd */ +} +``` +This solution is not serially reusable, however, because the `mkstemp()` function replaces the `"XXXXXX"` in `template` the first time it is invoked. This is not a problem as long as `template` is reinitialized before calling `mkstemp()` again. If `template` is not reinitialized, the `mkstemp()` function will return `-1` and leave `template` unmodified because `template` did not contain six `X`'s. + +The Open Group Base Specification Issue 6 \[[Open Group 2004](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-OpenGroup04)\] does not specify the permissions the file is created with, so these are [implementation-defined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior). However, IEEE Std 1003.1, 2013 Edition \[[IEEE Std 1003.1:2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-IEEEStd1003.1-2013)\] specifies them as `S_IRUSR|S_IWUSR` (0600). + +This compliant solution invokes the user-defined function `secure_dir()` (such as the one defined in [FIO15-C. Ensure that file operations are performed in a secure directory](https://wiki.sei.cmu.edu/confluence/display/c/FIO15-C.+Ensure+that+file+operations+are+performed+in+a+secure+directory)) to ensure the temporary file resides in a secure directory. + +**Implementation Details** + +For GLIBC, versions 2.0.6 and earlier, the file is created with permissions 0666; for GLIBC, versions 2.0.7 and later, the file is created with permissions 0600. On NetBSD, the file is created with permissions 0600. This creates a security risk in that an attacker will have write access to the file immediately after creation. Consequently, programs need a private version of the `mkstemp()` function in which this issue is known to be fixed. + +In many older [implementations](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation), the name is a function of process ID and time, so it is possible for the attacker to predict the name and create a decoy in advance. FreeBSD changed the `mk*temp()` family to eliminate the process ID component of the file name and replace the entire field with base-62 encoded randomness. This raises the number of possible temporary files for the typical use of six `X`'s significantly, meaning that even `mktemp()` with six `X`'s is reasonably (probabilistically) secure against guessing except under frequent usage \[[Kennaway 2000](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Kennaway00)\]. + +## Exceptions + +**FIO21-C-EX1:** The Annex K `tmpfile_s()` function can be used if all the targeted implementations create temporary files in secure directories. + +## Risk Assessment + +Insecure temporary file creation can lead to a program accessing unintended files and permission escalation on local systems. + +<table> <tbody> <tr> <th> Recommendation </th> <th> Severity </th> <th> Likelihood </th> <th> Remediation Cost </th> <th> Priority </th> <th> Level </th> </tr> <tr> <td> FIO21-C </td> <td> Medium </td> <td> Probable </td> <td> Medium </td> <td> <strong>P8</strong> </td> <td> <strong>L2</strong> </td> </tr> </tbody> </table> +** Automated Detection** + + +<table> <tbody> <tr> <th> Tool </th> <th> Version </th> <th> Checker </th> <th> Description </th> </tr> <tr> <td> <a> CodeSonar </a> </td> <td> 9.0p0 </td> <td> <strong>BADFUNC.TEMP.\*</strong> <strong>BADFUNC.TMPFILE_S</strong> <strong>BADFUNC.TMPNAM_S</strong> </td> <td> A collection of checks that report uses of library functions associated with temporary file vulnerabilities Use of tmpfile_s Use of tmpnam_s </td> </tr> <tr> <td> <a> Compass/ROSE </a> </td> <td> </td> <td> </td> <td> Can detect violations of this recommendation. Specifically, Rose reports use of <code>tmpnam()</code> , <code>tmpnam_s()</code> , <code>tmpfile()</code> , and <code>mktemp()</code> </td> </tr> <tr> <td> <a> Coverity </a> </td> <td> 6.5 </td> <td> <strong>SECURE_TEMP</strong> </td> <td> Fully implemented </td> </tr> <tr> <td> <a> Helix QAC </a> </td> <td> 2024.4 </td> <td> <strong>C5016</strong> </td> <td> </td> </tr> <tr> <td> <a> LDRA tool suite </a> </td> <td> 9.7.1 </td> <td> <strong>44 S</strong> </td> <td> Enhanced enforcement </td> </tr> <tr> <td> <a> Parasoft C/C++test </a> </td> <td> 2024.2 </td> <td> <strong>CERT_C-FIO21-b</strong> </td> <td> Use secure temporary file name functions </td> </tr> <tr> <td> <a> Polyspace Bug Finder </a> </td> <td> R2024b </td> <td> <a> CERT C: Rec. FIO21-C </a> </td> <td> Checks for non-secure temporary file (rec. partially covered) </td> </tr> </tbody> </table> + + +## Related Vulnerabilities + +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+FIO21-C). + +## Related Guidelines + +<table> <tbody> <tr> <td> <a> CERT C Secure Coding Standard </a> </td> <td> <a> FIO15-C. Ensure that file operations are performed in a secure directory </a> </td> </tr> <tr> <td> <a> SEI CERT C++ Coding Standard </a> </td> <td> <a> VOID FIO19-CPP. Do not create temporary files in shared directories </a> </td> </tr> <tr> <td> <a> CERT Oracle Secure Coding Standard for Java </a> </td> <td> <a> FIO03-J. Remove temporary files before termination </a> </td> </tr> <tr> <td> <a> ISO/IEC TR 24772:2013 </a> </td> <td> Path Traversal \[EWR\] </td> </tr> <tr> <td> <a> MITRE CWE </a> </td> <td> <a> CWE-379 </a> , Creation of temporary file in directory with insecure permissions </td> </tr> </tbody> </table> + + +## Bibliography + +<table> <tbody> <tr> <td> \[ <a> HP 2003 </a> \] </td> <td> </td> </tr> <tr> <td> \[ <a> IEEE Std 1003.1:2013 </a> \] </td> <td> XSH, System Interfaces: <code>open</code> XSH, System Interfaces: <code>mkdopen</code> , <code> mksopen</code> </td> </tr> <tr> <td> \[ <a> ISO/IEC 9899:2011 </a> \] </td> <td> Subclause K.3.5.1.2, "The <code>tmpnam_s</code> Function" Subclause 7.21.4.4, "The <code>tmpnam</code> Function </td> </tr> <tr> <td> \[ <a> Kennaway 2000 </a> \] </td> <td> </td> </tr> <tr> <td> \[ <a> Open Group 2004 </a> \] </td> <td> <code><a>mkstemp()</a>mktemp()</code> <a> open() </a> </td> </tr> <tr> <td> \[ <a> Seacord 2013 </a> \] </td> <td> Chapter 3, "Pointer Subterfuge" Chapter 8, "File I/O" </td> </tr> <tr> <td> \[ <a> Viega 2003 </a> \] </td> <td> Section 2.1, "Creating Files for Temporary Use" </td> </tr> <tr> <td> \[ <a> Wheeler 2003 </a> \] </td> <td> <a> Chapter 7, "Structure Program Internals and Approach" </a> </td> </tr> </tbody> </table> + + +## Implementation notes + +None + +## References + +* CERT-C: [FIO21-C: Do not create temporary files in shared directories](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.ql b/c/cert/src/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.ql new file mode 100644 index 0000000000..14d184a194 --- /dev/null +++ b/c/cert/src/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.ql @@ -0,0 +1,39 @@ +/** + * @id c/cert/possibly-predictable-temporary-file-creation + * @name FIO21-C: Do not use predictably generated temporary file paths + * @description Hardcoded or predictably generated temporary file paths can be hijacked or + * manipulated by an attacker if not created in a secure manner. + * @kind path-problem + * @precision medium + * @problem.severity warning + * @tags external/cert/id/fio21-c + * security + * external/cert/obligation/recommendation + */ + +import cpp +import codingstandards.c.cert +import semmle.code.cpp.dataflow.TaintTracking + +module TempDirTaintConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { + node.asExpr().(StringLiteral).getValue().matches(["/var/tmp/%", "/tmp/%", "C:\\\\Temp\\\\%"]) + } + + predicate isSink(DataFlow::Node node) { + exists(FunctionCall fc | fc.getArgument(0) = node.asExpr()) + } +} + +module TempDirTaint = TaintTracking::Global<TempDirTaintConfig>; + +import TempDirTaint::PathGraph + +from FunctionCall fc, string message, TempDirTaint::PathNode source, TempDirTaint::PathNode sink +where + not isExcluded(fc, IO5Package::possiblyPredictableTemporaryFileCreationQuery()) and + fc.getTarget().hasName("fopen") and + sink.getNode() = DataFlow::exprNode(fc.getArgument(0)) and + TempDirTaint::flow(source.getNode(), sink.getNode()) and + message = "Possible unsafe temporary file creation using 'fopen'." +select fc, source, sink, "Possible unsafe temporary file creation using 'fopen'." diff --git a/c/cert/src/rules/POS30-C/IncorrectNullTerminationOfReadlink.md b/c/cert/src/rules/POS30-C/IncorrectNullTerminationOfReadlink.md new file mode 100644 index 0000000000..6534bf0248 --- /dev/null +++ b/c/cert/src/rules/POS30-C/IncorrectNullTerminationOfReadlink.md @@ -0,0 +1,117 @@ +# POS30-C: Use the readlink() function properly + +This query implements the CERT-C rule POS30-C: + +> Use the readlink() function properly + + +## Description + +The `readlink()` function reads where a link points to. It makes **no** effort to null-terminate its second argument, `buffer`. Instead, it just returns the number of characters it has written. + +## Noncompliant Code Example + +If `len` is equal to `sizeof(buf)`, the null terminator is written 1 byte past the end of `buf`: + +```cpp +char buf[1024]; +ssize_t len = readlink("/usr/bin/perl", buf, sizeof(buf)); +buf[len] = '\0'; + +``` +An incorrect solution to this problem is to try to make `buf` large enough that it can always hold the result: + +```cpp +long symlink_max; +size_t bufsize; +char *buf; +ssize_t len; + +errno = 0; +symlink_max = pathconf("/usr/bin/", _PC_SYMLINK_MAX); +if (symlink_max == -1) { + if (errno != 0) { + /* handle error condition */ + } + bufsize = 10000; +} +else { + bufsize = symlink_max+1; +} + +buf = (char *)malloc(bufsize); +if (buf == NULL) { + /* handle error condition */ +} + +len = readlink("/usr/bin/perl", buf, bufsize); +buf[len] = '\0'; + +``` +This modification incorrectly assumes that the symbolic link cannot be longer than the value of `SYMLINK_MAX` returned by `pathconf()`. However, the value returned by `pathconf()` is out of date by the time `readlink()` is called, so the off-by-one buffer-overflow risk is still present because, between the two calls, the location of `/usr/bin/perl` can change to a file system with a larger `SYMLINK_MAX` value. Also, if `SYMLINK_MAX` is indeterminate (that is, if `pathconf()` returned `-1` without setting `errno`), the code uses an arbitrary large buffer size (10,000) that it hopes will be sufficient, but there is a small chance that `readlink()` can return exactly this size. + +An additional issue is that `readlink()` can return `-1` if it fails, causing an off-by-one underflow. + +## Compliant Solution + +This compliant solution ensures there is no overflow by reading in only `sizeof(buf)-1` characters. It also properly checks to see if an error has occurred: + +```cpp +enum { BUFFERSIZE = 1024 }; +char buf[BUFFERSIZE]; +ssize_t len = readlink("/usr/bin/perl", buf, sizeof(buf)-1); + +if (len != -1) { + buf[len] = '\0'; +} +else { + /* handle error condition */ +} + +``` + +## Risk Assessment + +Failing to properly null-terminate the result of `readlink()` can result in abnormal program termination and buffer-overflow vulnerabilities. + +<table> <tbody> <tr> <th> Rule </th> <th> Severity </th> <th> Likelihood </th> <th> Remediation Cost </th> <th> Priority </th> <th> Level </th> </tr> <tr> <td> POS30-C </td> <td> high </td> <td> probable </td> <td> medium </td> <td> <strong>P12</strong> </td> <td> <strong>L1</strong> </td> </tr> </tbody> </table> + + +## Automated Detection + +<table> <tbody> <tr> <th> Tool </th> <th> Version </th> <th> Checker </th> <th> Description </th> </tr> <tr> <td> <a> Astrée </a> </td> <td> 24.04 </td> <td> </td> <td> Supported: Can be checked with appropriate analysis stubs. </td> </tr> <tr> <td> <a> Axivion Bauhaus Suite </a> </td> <td> 7.2.0 </td> <td> <strong>CertC-POS30</strong> </td> <td> </td> </tr> <tr> <td> <a> CodeSonar </a> </td> <td> 9.0p0 </td> <td> <strong>LANG.MEM.BO</strong> <strong>LANG.MEM.TBA</strong> <strong>MISC.MEM.NTERM.CSTRING</strong> </td> <td> Buffer Overrun Tainted Buffer Access Unterminated C String </td> </tr> <tr> <td> <a> Compass/ROSE </a> </td> <td> </td> <td> </td> <td> </td> </tr> <tr> <td> <a> Coverity </a> </td> <td> 2017.07 </td> <td> <strong>READLINK</strong> </td> <td> Implemented </td> </tr> <tr> <td> <a> Helix QAC </a> </td> <td> 2024.4 </td> <td> <strong>C5033</strong> </td> <td> </td> </tr> <tr> <td> <a> Klocwork </a> </td> <td> 2024.4 </td> <td> <strong>ABV.GENERAL</strong> <strong>ABV.GENERAL.MULTIDIMENSION</strong> </td> <td> </td> </tr> <tr> <td> <a> Parasoft C/C++test </a> </td> <td> 2024.2 </td> <td> <strong>CERT_C-POS30-a</strong> <strong>CERT_C-POS30-b</strong> <strong>CERT_C-POS30-c</strong> </td> <td> Avoid overflow due to reading a not zero terminated string The values returned by functions 'read' and 'readlink' shall be used Use of possibly not null-terminated string with functions expecting null-terminated string </td> </tr> <tr> <td> <a> Polyspace Bug Finder </a> </td> <td> R2024b </td> <td> <a> CERT C: Rule POS30-C </a> </td> <td> Checks for misuse of readlink() (rule partially covered) </td> </tr> </tbody> </table> + + +## Related Vulnerabilities + +Search for vulnerabilities resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+POS30-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) + +<table> <tbody> <tr> <th> Taxonomy </th> <th> Taxonomy item </th> <th> Relationship </th> </tr> <tr> <td> <a> CWE 2.11 </a> </td> <td> <a> CWE-170 </a> , Improper null termination </td> <td> 2017-06-13: CERT: Rule subset of CWE </td> </tr> </tbody> </table> + + +## CERT-CWE Mapping Notes + +[Key here](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152408#HowthisCodingStandardisOrganized-CERT-CWEMappingNotes) for mapping notes + +**CWE-170 and POS30-C** + +CWE-170 = Union( POS30-C, list) where list = + +* Non-null terminated strings fed to functions other than POSIX readlink() + +## Bibliography + +<table> <tbody> <tr> <td> \[ <a> Ilja 2006 </a> \] </td> </tr> <tr> <td> \[ <a> Open Group 1997a </a> \] </td> </tr> <tr> <td> \[ <a> Open Group 2004 </a> \] </td> </tr> </tbody> </table> + + +## Implementation notes + +None + +## References + +* CERT-C: [POS30-C: Use the readlink() function properly](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/POS30-C/IncorrectNullTerminationOfReadlink.ql b/c/cert/src/rules/POS30-C/IncorrectNullTerminationOfReadlink.ql new file mode 100644 index 0000000000..e1b24f6649 --- /dev/null +++ b/c/cert/src/rules/POS30-C/IncorrectNullTerminationOfReadlink.ql @@ -0,0 +1,99 @@ +/** + * @id c/cert/incorrect-null-termination-of-readlink + * @name POS30-C: Use the readlink() function properly + * @description The readlink() function does not null terminate or leave a character at the end of + * the buffer argument, and must be handled properly. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/cert/id/pos30-c + * correctness + * security + * external/cert/obligation/rule + */ + +import cpp +import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.controlflow.Dominance +import semmle.code.cpp.controlflow.Guards +import codingstandards.c.cert +import codingstandards.cpp.StdFunctionOrMacro + +predicate isValidSize(Expr size) { size.(SubExpr).getAnOperand().getValue().toInt() = 1 } + +AssignExpr getNullTermination(ReadlinkCall readlink) { + exists(AssignExpr assign, ArrayExpr arrayExpr, Expr arrayBase, Expr arrayOffset | + assign.getLValue() = arrayExpr and + arrayExpr.getArrayBase() = arrayBase and + arrayExpr.getArrayOffset() = arrayOffset and + DataFlow::localFlow(DataFlow::exprNode(readlink.getExpr()), DataFlow::exprNode(arrayOffset)) and + DataFlow::localFlow(DataFlow::definitionByReferenceNodeFromArgument(readlink.getBufferArg()), + DataFlow::exprNode(arrayBase)) and + result = assign + ) +} + +predicate isErrorChecked(ReadlinkCall readlink, GuardCondition check, BasicBlock block) { + exists(Expr sizeExpr | + DataFlow::localFlow(DataFlow::exprNode(readlink.getExpr()), DataFlow::exprNode(sizeExpr)) and + ( + check.ensuresLt(sizeExpr, 0, block, false) + or + check.ensuresEq(sizeExpr, -1, block, false) + ) + ) +} + +predicate isErrorCheckedBeforeNullTermination(ReadlinkCall readlink, GuardCondition check) { + isErrorChecked(readlink, check, getNullTermination(readlink).getBasicBlock()) +} + +predicate usedBeforeNullTermination(ReadlinkCall readlink, Expr use, Expr nullTerm) { + nullTerm = getNullTermination(readlink) and + DataFlow::localFlow(DataFlow::definitionByReferenceNodeFromArgument(readlink.getBufferArg()), + DataFlow::exprNode(use)) and + not strictlyDominates(nullTerm, use) and + not use.getParent+() = nullTerm +} + +from + ReadlinkCall readlink, string message, Element extra1, string extraString1, Element extra2, + string extraString2 +where + not isExcluded(readlink.getElement(), IO5Package::incorrectNullTerminationOfReadlinkQuery()) and + ( + not isValidSize(readlink.getSizeArg()) and + message = "Call to readlink() with size argument that may not leave room for a null terminator." and + extra1 = readlink.getElement() and + extra2 = readlink.getElement() and + extraString1 = "" and + extraString2 = "" + ) + or + not exists(getNullTermination(readlink)) and + message = + "Call to readlink() not followed by an assignment that ensures a null terminated result string." and + extra1 = readlink.getElement() and + extra2 = readlink.getElement() and + extraString1 = "" and + extraString2 = "" + or + not isErrorChecked(readlink, _, _) and + message = "Call to readlink() not followed by a check to result error code." and + extra1 = readlink.getElement() and + extra2 = readlink.getElement() and + extraString1 = "" and + extraString2 = "" + or + not isErrorCheckedBeforeNullTermination(readlink, extra2) and + message = "Buffer passed to readlink() is $@ by result value before the result value is $@." and + extra1 = getNullTermination(readlink) and + isErrorChecked(readlink, extra2, _) and + extraString1 = "null terminated" and + extraString2 = "error checked" + or + usedBeforeNullTermination(readlink, extra1, extra2) and + message = "Buffer passed to readlink() is $@ before $@." and + extraString1 = "used" and + extraString2 = "null terminaton is added" +select readlink.getElement(), message, extra1, extraString1, extra2, extraString2 diff --git a/c/cert/test/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.expected b/c/cert/test/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.expected new file mode 100644 index 0000000000..9da2d5278b --- /dev/null +++ b/c/cert/test/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.expected @@ -0,0 +1,4 @@ +| test.c:10:14:10:18 | call to fopen | Call to create file with non-exclusive creation mode 'w'. | +| test.c:35:17:35:23 | call to fopen_s | Call to create file with non-exclusive creation mode 'w'. | +| test.c:52:12:52:15 | call to open | Call to create file with non-exclusive creation mode 'O_CREAT \| O_WRONLY'. | +| test.c:89:12:89:15 | call to open | Call to create file with non-exclusive creation mode 'O_CREAT \| O_WRONLY'. | diff --git a/c/cert/test/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.qlref b/c/cert/test/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.qlref new file mode 100644 index 0000000000..2fa53c04e4 --- /dev/null +++ b/c/cert/test/rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.qlref @@ -0,0 +1 @@ +rules/FIO03-C/FopenWithNonExclusiveFileCreationMode.ql \ No newline at end of file diff --git a/c/cert/test/rules/FIO03-C/test.c b/c/cert/test/rules/FIO03-C/test.c new file mode 100644 index 0000000000..bbb6a1089b --- /dev/null +++ b/c/cert/test/rules/FIO03-C/test.c @@ -0,0 +1,111 @@ +#define __STDC_WANT_LIB_EXT1__ +#include <fcntl.h> +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> + +void f1(void) { + // fopen() without the 'x' mode flag is non-compliant when creating a new + // file. + FILE *fp = fopen("/tmp/file", "w"); // NON-COMPLIANT + if (fp) { + fclose(fp); + } +} + +void f2(void) { + // fopen() in read mode doesn't require exclusive creation. + FILE *fp = fopen("/tmp/file", "r"); // COMPLIANT + if (fp) { + fclose(fp); + } +} + +void f3(void) { + // fopen() with the 'x' mode flag ensures exclusive creation. + FILE *fp = fopen("/tmp/file", "wx"); // COMPLIANT + if (fp) { + fclose(fp); + } +} + +void f4(void) { + // fopen_s() without the 'x' mode flag is non-compliant. + FILE *fp; + errno_t res = fopen_s(&fp, "/tmp/file", "w"); // NON-COMPLIANT + if (res == 0) { + fclose(fp); + } +} + +void f5(void) { + // fopen_s() with the 'x' mode flag ensures exclusive creation. + FILE *fp; + errno_t res = fopen_s(&fp, "/tmp/file", "wx"); // COMPLIANT + if (res == 0) { + fclose(fp); + } +} + +void f6(void) { + // open() without O_EXCL is non-compliant when creating a new file. + int fd = open("/tmp/file", O_CREAT | O_WRONLY, 0644); // NON-COMPLIANT + if (fd != -1) { + close(fd); + } +} + +void f7(void) { + // open() with O_EXCL ensures exclusive creation. + int fd = open("/tmp/file", O_CREAT | O_EXCL | O_WRONLY, 0644); // COMPLIANT + if (fd != -1) { + close(fd); + } +} + +void f8(void) { + // open() without O_CREAT doesn't require exclusive creation. + int fd = open("/tmp/file", O_WRONLY, 0644); // COMPLIANT + if (fd != -1) { + close(fd); + } +} + +void f9(void) { + // fdopen() can be used with a file descriptor created with O_EXCL. + int fd = open("/tmp/file", O_CREAT | O_EXCL | O_WRONLY, 0644); // COMPLIANT + if (fd != -1) { + FILE *fp = fdopen(fd, "w"); + if (fp) { + fclose(fp); + } else { + close(fd); + } + } +} + +void f10(void) { + // fdopen() without O_EXCL is non-compliant when creating a new file. + int fd = open("/tmp/file", O_CREAT | O_WRONLY, 0644); // NON-COMPLIANT + if (fd != -1) { + FILE *fp = fdopen(fd, "w"); + if (fp) { + fclose(fp); + } else { + close(fd); + } + } +} + +void f11(void) { + // fdopen() without O_CREAT doesn't require exclusive creation. + int fd = open("/tmp/file", O_WRONLY, 0644); // COMPLIANT + if (fd != -1) { + FILE *fp = fdopen(fd, "w"); + if (fp) { + fclose(fp); + } else { + close(fd); + } + } +} \ No newline at end of file diff --git a/c/cert/test/rules/FIO21-C/ExploitableTemporaryFileCreation.expected b/c/cert/test/rules/FIO21-C/ExploitableTemporaryFileCreation.expected new file mode 100644 index 0000000000..da51d68529 --- /dev/null +++ b/c/cert/test/rules/FIO21-C/ExploitableTemporaryFileCreation.expected @@ -0,0 +1,5 @@ +| test.c:18:3:18:8 | call to tmpnam | Unsafe temporary file creation function 'tmpnam' called. | +| test.c:25:3:25:10 | call to tmpnam_s | Unsafe temporary file creation function 'tmpnam_s' called. | +| test.c:31:3:31:9 | call to tmpfile | Unsafe temporary file creation function 'tmpfile' called. | +| test.c:42:3:42:11 | call to tmpfile_s | Unsafe temporary file creation function 'tmpfile_s' called. | +| test.c:50:3:50:8 | call to mktemp | Unsafe temporary file creation function 'mktemp' called. | diff --git a/c/cert/test/rules/FIO21-C/ExploitableTemporaryFileCreation.qlref b/c/cert/test/rules/FIO21-C/ExploitableTemporaryFileCreation.qlref new file mode 100644 index 0000000000..2df90f9df1 --- /dev/null +++ b/c/cert/test/rules/FIO21-C/ExploitableTemporaryFileCreation.qlref @@ -0,0 +1 @@ +rules/FIO21-C/ExploitableTemporaryFileCreation.ql \ No newline at end of file diff --git a/c/cert/test/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.expected b/c/cert/test/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.expected new file mode 100644 index 0000000000..189f1e0893 --- /dev/null +++ b/c/cert/test/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.expected @@ -0,0 +1,19 @@ +WARNING: module 'DataFlow' has been deprecated and may be removed in future (PossiblyPredictableTemporaryFileCreation.ql:18,38-46) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (PossiblyPredictableTemporaryFileCreation.ql:19,22-30) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (PossiblyPredictableTemporaryFileCreation.ql:23,20-28) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (PossiblyPredictableTemporaryFileCreation.ql:36,20-28) +WARNING: module 'TaintTracking' has been deprecated and may be removed in future (PossiblyPredictableTemporaryFileCreation.ql:28,23-36) +edges +| test.c:66:36:66:42 | /tmp/dir/ | test.c:67:21:67:22 | l1 | provenance | | +nodes +| test.c:9:9:9:19 | /tmp/file | semmle.label | /tmp/file | +| test.c:10:9:10:23 | /var/tmp/file | semmle.label | /var/tmp/file | +| test.c:11:9:11:24 | C:\\Temp\\file | semmle.label | C:\\Temp\\file | +| test.c:66:36:66:42 | /tmp/dir/ | semmle.label | /tmp/dir/ | +| test.c:67:21:67:22 | l1 | semmle.label | l1 | +subpaths +#select +| test.c:9:3:9:7 | call to fopen | test.c:9:9:9:19 | /tmp/file | test.c:9:9:9:19 | /tmp/file | Possible unsafe temporary file creation using 'fopen'. | +| test.c:10:3:10:7 | call to fopen | test.c:10:9:10:23 | /var/tmp/file | test.c:10:9:10:23 | /var/tmp/file | Possible unsafe temporary file creation using 'fopen'. | +| test.c:11:3:11:7 | call to fopen | test.c:11:9:11:24 | C:\\Temp\\file | test.c:11:9:11:24 | C:\\Temp\\file | Possible unsafe temporary file creation using 'fopen'. | +| test.c:67:15:67:19 | call to fopen | test.c:66:36:66:42 | /tmp/dir/ | test.c:67:21:67:22 | l1 | Possible unsafe temporary file creation using 'fopen'. | diff --git a/c/cert/test/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.qlref b/c/cert/test/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.qlref new file mode 100644 index 0000000000..e503a0ea26 --- /dev/null +++ b/c/cert/test/rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.qlref @@ -0,0 +1 @@ +rules/FIO21-C/PossiblyPredictableTemporaryFileCreation.ql \ No newline at end of file diff --git a/c/cert/test/rules/FIO21-C/test.c b/c/cert/test/rules/FIO21-C/test.c new file mode 100644 index 0000000000..f63d8d4f4b --- /dev/null +++ b/c/cert/test/rules/FIO21-C/test.c @@ -0,0 +1,73 @@ +#define __STDC_WANT_LIB_EXT1__ +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> + +void f1(void) { + // fopen with a constant path is not a compliant means of creating a + // temporary file (if we can infer it is a temporary file). + fopen("/tmp/file", "r"); // NON-COMPLIANT + fopen("/var/tmp/file", "r"); // NON-COMPLIANT + fopen("C:\\Temp\\file", "r"); // NON-COMPLIANT +} + +void f2(void) { + char buf[L_tmpnam]; + // tmpnam() doesn't guarantee a unique name and requires extra care to + // safely open a file with the name it returns. + tmpnam(buf); +} + +void f3(void) { + // tmpnam_s is not safer than tmpnam, it is just an option to set a size + // for the buffer. It is not a compliant means of creating a temporary file. + char buf[400]; + tmpnam_s(buf, sizeof(buf)); // NON-COMPLIANT +} + +void f4(void) { + // Most historical implementations of tmpfile() have a limited number of + // filenames before recycling them. + tmpfile(); // NON-COMPLIANT +} + +void f5(void) { + FILE *fp; + // tmpfile_s doesn't allow the user to specify a directory for the + // temporary file. + // + // An exception is allowed (FIO21-C-EX1) if all targeted implementations + // create temporary files in secure directories. However, we cannot detect + // this statically. + tmpfile_s(fp); // NON-COMPLIANT +} + +void f6(void) { + char path[] = "/tmp-XXXXXX"; + // Some implementations follow BSD 4.3 and generate very predictable names. + // There is also a race condition between the call to mkstemp and the open + // that can add a security risk. + mktemp(path); // NON-COMPLIANT +} + +void f7(void) { + char path[] = "/tmp-XXXXXX"; + // mkstemp is a compliant means of creating a temporary file. + mkstemp(path); // COMPLIANT +} + +#define PATHMAX 256 +#define TMPROOT "/tmp/dir/" +#define TMP "tmp/" +void f8(char *basename) { + // Test that we handle some basic cases of path concatenation. + char l1[PATHMAX]; + // We treat "/tmp/" as a strong sign of a temporary file. + snprintf(l1, sizeof(l1), "%s%s", TMPROOT, basename); + FILE *fd1 = fopen(l1, "r"); // NON-COMPLIANT + + char l2[PATHMAX]; + // Currently we draw the line here, don't report "tmp" without slashes. + snprintf(l2, sizeof(l2), "/%s%s", TMP, basename); + FILE *fd2 = fopen(l2, "r"); // NON-COMPLIANT[False negative] +} \ No newline at end of file diff --git a/c/cert/test/rules/POS30-C/IncorrectNullTerminationOfReadlink.expected b/c/cert/test/rules/POS30-C/IncorrectNullTerminationOfReadlink.expected new file mode 100644 index 0000000000..c678391f73 --- /dev/null +++ b/c/cert/test/rules/POS30-C/IncorrectNullTerminationOfReadlink.expected @@ -0,0 +1,24 @@ +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:29,5-13) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:29,25-33) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:29,65-73) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:30,5-13) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:30,25-33) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:31,7-15) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:38,5-13) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:38,25-33) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:38,65-73) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:53,3-11) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:53,23-31) +WARNING: module 'DataFlow' has been deprecated and may be removed in future (IncorrectNullTerminationOfReadlink.ql:54,5-13) +| test.c:7:17:7:24 | call to readlink | Call to readlink() not followed by a check to result error code. | test.c:7:17:7:24 | call to readlink | | test.c:7:17:7:24 | call to readlink | | +| test.c:7:17:7:24 | call to readlink | Call to readlink() with size argument that may not leave room for a null terminator. | test.c:7:17:7:24 | call to readlink | | test.c:7:17:7:24 | call to readlink | | +| test.c:13:17:13:24 | call to readlink | Call to readlink() not followed by a check to result error code. | test.c:13:17:13:24 | call to readlink | | test.c:13:17:13:24 | call to readlink | | +| test.c:13:17:13:24 | call to readlink | Call to readlink() not followed by an assignment that ensures a null terminated result string. | test.c:13:17:13:24 | call to readlink | | test.c:13:17:13:24 | call to readlink | | +| test.c:19:17:19:24 | call to readlink | Call to readlink() not followed by a check to result error code. | test.c:19:17:19:24 | call to readlink | | test.c:19:17:19:24 | call to readlink | | +| test.c:26:17:26:24 | call to readlink | Call to readlink() not followed by an assignment that ensures a null terminated result string. | test.c:26:17:26:24 | call to readlink | | test.c:26:17:26:24 | call to readlink | | +| test.c:46:17:46:24 | call to readlink | Buffer passed to readlink() is $@ by result value before the result value is $@. | test.c:48:3:48:17 | ... = ... | null terminated | test.c:49:7:49:15 | ... != ... | error checked | +| test.c:58:17:58:24 | call to readlink | Buffer passed to readlink() is $@ before $@. | test.c:66:11:66:13 | buf | used | test.c:62:7:62:21 | ... = ... | null terminaton is added | +| test.c:71:17:71:24 | call to readlink | Buffer passed to readlink() is $@ before $@. | test.c:73:11:73:13 | buf | used | test.c:78:3:78:17 | ... = ... | null terminaton is added | +| test.c:91:17:91:24 | call to readlink | Call to readlink() not followed by a check to result error code. | test.c:91:17:91:24 | call to readlink | | test.c:91:17:91:24 | call to readlink | | +| test.c:91:17:91:24 | call to readlink | Call to readlink() not followed by an assignment that ensures a null terminated result string. | test.c:91:17:91:24 | call to readlink | | test.c:91:17:91:24 | call to readlink | | +| test.c:91:17:91:24 | call to readlink | Call to readlink() with size argument that may not leave room for a null terminator. | test.c:91:17:91:24 | call to readlink | | test.c:91:17:91:24 | call to readlink | | diff --git a/c/cert/test/rules/POS30-C/IncorrectNullTerminationOfReadlink.qlref b/c/cert/test/rules/POS30-C/IncorrectNullTerminationOfReadlink.qlref new file mode 100644 index 0000000000..00d3ac00dc --- /dev/null +++ b/c/cert/test/rules/POS30-C/IncorrectNullTerminationOfReadlink.qlref @@ -0,0 +1 @@ +rules/POS30-C/IncorrectNullTerminationOfReadlink.ql \ No newline at end of file diff --git a/c/cert/test/rules/POS30-C/test.c b/c/cert/test/rules/POS30-C/test.c new file mode 100644 index 0000000000..053cfd5f37 --- /dev/null +++ b/c/cert/test/rules/POS30-C/test.c @@ -0,0 +1,93 @@ +#include <unistd.h> + +void use_str(char *buf); + +void f1() { + char buf[1024]; + ssize_t len = readlink("/file", buf, sizeof(buf)); // NON-COMPLIANT + buf[len] = '\0'; // NON-COMPLIANT +} + +void f2() { + char buf[1024]; + ssize_t len = readlink("/file", buf, sizeof(buf) - 1); // COMPLIANT + // NON-COMPLIANT: buffer not null-terminated +} + +void f3() { + char buf[1024]; + ssize_t len = readlink("/file", buf, sizeof(buf) - 1); // COMPLIANT + + buf[len] = '\0'; // NON-COMPLIANT: not guarded by error check +} + +void f4() { + char buf[1024]; + ssize_t len = readlink("/file", buf, sizeof(buf) - 1); // COMPLIANT + + if (len == -1) { + return; + } + + use_str(buf); // NON-COMPLIANT: buf is not null-terminated +} + +void f5() { + char buf[1024]; + ssize_t len = readlink("/file", buf, sizeof(buf) - 1); // COMPLIANT + + if (len != -1) { + buf[len] = '\0'; // COMPLIANT + } +} + +void f6() { + char buf[1024]; + ssize_t len = readlink("/file", buf, sizeof(buf) - 1); // COMPLIANT + + buf[len] = '\0'; // NON-COMPLIANT: len may be -1 + if (len != -1) { + return; + } + + use_str(buf); +} + +void f7() { + char buf[1024]; + ssize_t len = readlink("/file", buf, sizeof(buf) - 1); // COMPLIANT + + if (len < 10) { + if (len != -1) { + buf[len] = '\0'; + } + } + + use_str(buf); // NON-COMPLIANT: Null termination doesn't dominate use +} + +void f8() { + char buf[1024]; + ssize_t len = readlink("/file", buf, sizeof(buf) - 1); // COMPLIANT + + use_str(buf); // NON-COMPLIANT: buf used before null-termination + if (len == -1) { + return; + } + + buf[len] = '\0'; +} + +void f9() { + char buf[1024]; + ssize_t len = readlink("/file", buf, sizeof(buf) - 1); // COMPLIANT + + if (len >= 0) { + buf[len] = '\0'; // COMPLIANT + } +} + +void f10(char *buf, int size) { + ssize_t len = readlink( + "/file", buf, size); // NON-COMPLIANT: The size likely should be size - 1 +} \ No newline at end of file diff --git a/c/common/test/includes/standard-library/stdio.h b/c/common/test/includes/standard-library/stdio.h index 3604198c3e..6f0127c571 100644 --- a/c/common/test/includes/standard-library/stdio.h +++ b/c/common/test/includes/standard-library/stdio.h @@ -129,6 +129,14 @@ void setbuf(FILE *__restrict, char *__restrict); char *tmpnam(char *); FILE *tmpfile(void); +#ifdef __STDC_WANT_LIB_EXT1__ +#define errno_t int +errno_t fopen_s(FILE * restrict* restrict, const char * restrict, const char * restrict); +typedef size_t rsize_t; +errno_t tmpfile_s(FILE * restrict* restrict streamptr); +int tmpnam_s(char *, rsize_t); +#endif + #if defined(_POSIX_SOURCE) || defined(_POSIX_C_SOURCE) \ || defined(_XOPEN_SOURCE) || defined(_GNU_SOURCE) \ || defined(_BSD_SOURCE) diff --git a/change_notes/2025-04-09-new-cert-c-recommendation-query-suite.md b/change_notes/2025-04-09-new-cert-c-recommendation-query-suite.md new file mode 100644 index 0000000000..910433e351 --- /dev/null +++ b/change_notes/2025-04-09-new-cert-c-recommendation-query-suite.md @@ -0,0 +1,9 @@ + - The following query suites have been added or modified for CERT C: + - A new query suite has been created `cert-c-default.qls` to avoid confusion with the CERT C++ query suites. The `cert-default.qls` suite has been deprecated, and will be removed in a future releases, and is replaced by the `cert-c-default.qls` suite. + - The `cert-c-default.qls` suite has been specified as the default for the pack, and will include our most up-to-date coverage for CERT C. + - One new query suite, `cert-c-recommended.qls` has been added to enable running CERT recommendations (as opposed to rules) that will be added in the future. + - The default query suite, `cert-c-default.qls` has been set to exclude CERT recommendations (as opposed to rules) that will be added in the future. + - The following query suites have been added or modified for CERT C++: + - A new query suite has been created `cert-cpp-default.qls` to avoid confusion with the CERT C query suites. The `cert-default.qls` suite has been deprecated, and will be removed in a future releases, and is replaced by the `cert-cpp-default.qls` suite. + - The `cert-cpp-default.qls` suite has been specified as the default for the pack, and will include our most up-to-date coverage for CERT C. + - A new query suite has been created `cert-cpp-single-translation-unit.qls` to avoid confusion with the CERT C query suites. The `cert-single-translation-unit.qls` suite has been deprecated, and will be removed in a future releases, and is replaced by the `cert-cpp-single-translation-unit.qls` suite. \ No newline at end of file diff --git a/cpp/cert/src/codeql-suites/cert-cpp-default.qls b/cpp/cert/src/codeql-suites/cert-cpp-default.qls new file mode 100644 index 0000000000..e9211246b1 --- /dev/null +++ b/cpp/cert/src/codeql-suites/cert-cpp-default.qls @@ -0,0 +1,9 @@ +- description: CERT C++ 2016 (Default) +- qlpack: codeql/cert-cpp-coding-standards +- include: + kind: + - problem + - path-problem +- exclude: + tags contain: + - external/cert/default-disabled diff --git a/cpp/cert/src/codeql-suites/cert-cpp-single-translation-unit.qls b/cpp/cert/src/codeql-suites/cert-cpp-single-translation-unit.qls new file mode 100644 index 0000000000..2f09815e0d --- /dev/null +++ b/cpp/cert/src/codeql-suites/cert-cpp-single-translation-unit.qls @@ -0,0 +1,11 @@ +- description: CERT C++ 2016 (Single Translation Unit) +- qlpack: codeql/cert-cpp-coding-standards +- include: + kind: + - problem + - path-problem + tags contain: + - scope/single-translation-unit +- exclude: + tags contain: + - external/cert/default-disabled diff --git a/cpp/cert/src/codeql-suites/cert-default.qls b/cpp/cert/src/codeql-suites/cert-default.qls index e9211246b1..66599b60fb 100644 --- a/cpp/cert/src/codeql-suites/cert-default.qls +++ b/cpp/cert/src/codeql-suites/cert-default.qls @@ -1,9 +1,2 @@ -- description: CERT C++ 2016 (Default) -- qlpack: codeql/cert-cpp-coding-standards -- include: - kind: - - problem - - path-problem -- exclude: - tags contain: - - external/cert/default-disabled +- description: "DEPRECATED - CERT C++ 2016 - use cert-cpp-default.qls instead" +- import: codeql-suites/cert-cpp-default.qls \ No newline at end of file diff --git a/cpp/cert/src/codeql-suites/cert-single-translation-unit.qls b/cpp/cert/src/codeql-suites/cert-single-translation-unit.qls index 2f09815e0d..4966648394 100644 --- a/cpp/cert/src/codeql-suites/cert-single-translation-unit.qls +++ b/cpp/cert/src/codeql-suites/cert-single-translation-unit.qls @@ -1,11 +1,2 @@ -- description: CERT C++ 2016 (Single Translation Unit) -- qlpack: codeql/cert-cpp-coding-standards -- include: - kind: - - problem - - path-problem - tags contain: - - scope/single-translation-unit -- exclude: - tags contain: - - external/cert/default-disabled +- description: "DEPRECATED - CERT C++ 2016 (Single Translation Unit) - use cert-cpp-single-translation-unit.qls instead" +- import: codeql-suites/cert-cpp-single-translation-unit.qls \ No newline at end of file diff --git a/cpp/cert/src/qlpack.yml b/cpp/cert/src/qlpack.yml index f44646cdbe..97976d30cd 100644 --- a/cpp/cert/src/qlpack.yml +++ b/cpp/cert/src/qlpack.yml @@ -3,6 +3,7 @@ version: 2.44.0-dev description: CERT C++ 2016 suites: codeql-suites license: MIT +default-suite-file: codeql-suites/cert-cpp-default.qls dependencies: codeql/cpp-all: 2.1.1 codeql/common-cpp-coding-standards: '*' diff --git a/cpp/common/src/codingstandards/cpp/StdFunctionOrMacro.qll b/cpp/common/src/codingstandards/cpp/StdFunctionOrMacro.qll index 1067b7ad09..577655b87a 100644 --- a/cpp/common/src/codingstandards/cpp/StdFunctionOrMacro.qll +++ b/cpp/common/src/codingstandards/cpp/StdFunctionOrMacro.qll @@ -31,10 +31,27 @@ import cpp as cpp +private string underscoresPrefix() { result = "__" } + +private string c11Prefix() { result = "__c11_" } + private string atomicInit() { result = "atomic_init" } class AtomicInitCall = StdFunctionOrMacro<C11FunctionWrapperMacro, atomicInit/0>::Call; +private string readlink() { result = "readlink" } + +class ReadlinkCall extends StdFunctionOrMacro<UnderscoredFunctionWrapperMacro, readlink/0>::Call { + /** The first argument to readlink is the file path. */ + cpp::Expr getPathArg() { result = this.getArgument(0) } + + /** The second argument to readlink is the size of the buffer. */ + cpp::Expr getBufferArg() { result = this.getArgument(1) } + + /** The third argument to readlink is the size of the buffer. */ + cpp::Expr getSizeArg() { result = this.getArgument(2) } +} + /** Specify the name of your function as a predicate */ private signature string getName(); @@ -42,18 +59,20 @@ private signature string getName(); private signature module InferMacroExpansionArguments { bindingset[mi, argumentIdx] cpp::Expr inferArgument(cpp::MacroInvocation mi, int argumentIdx); + + /** Get an expression representing the result of this function call if it is a macro */ + bindingset[mi] + cpp::Expr inferExpr(cpp::MacroInvocation mi); } /** Assume macro `f(x, y, ...)` expands to `__c11_f(x, y, ...)`. */ private module C11FunctionWrapperMacro implements InferMacroExpansionArguments { - bindingset[mi, argumentIdx] - cpp::Expr inferArgument(cpp::MacroInvocation mi, int argumentIdx) { - exists(cpp::FunctionCall fc | - fc = mi.getExpr() and - fc.getTarget().hasName("__c11_" + mi.getMacroName()) and - result = mi.getExpr().(cpp::FunctionCall).getArgument(argumentIdx) - ) - } + import PrefixedFunctionWrapperMacro<c11Prefix/0> +} + +/** Assume macro `f(x, y, ...)` expands to `__f(x, y, ...)`. */ +private module UnderscoredFunctionWrapperMacro implements InferMacroExpansionArguments { + import PrefixedFunctionWrapperMacro<underscoresPrefix/0> } /** @@ -106,5 +125,54 @@ private module StdFunctionOrMacro<InferMacroExpansionArguments InferExpansion, g this = TStdMacroInvocation(_) and result = "Invocation of a standard function implemented as a macro" } + + /** + * Get the `Element` of this pseudo-call: either the FunctionCall or the MacroInvocation. + */ + cpp::Element getElement() { + this = TStdFunctionCall(result) or + this = TStdMacroInvocation(result) + } + + /** + * Get an expression that represents the call to the standard function or macro. + * + * In the case of a macro, the result is determined by the `InferMacroExpansionArguments` + * config module. + */ + cpp::Expr getExpr() { + this = TStdFunctionCall(result) + or + exists(MacroInvocation mi | + this = TStdMacroInvocation(mi) and + result = InferExpansion::inferExpr(mi) + ) + } + } +} + +/** Specify the name of your function as a predicate */ +private signature string getPrefix(); + +/** + * A module to generate a config for `StdFunctionOrMacro` based on a prefix satisfying the interface + * of `InferMacroExpansionArguments`. + * + * For instance, if an implementation uses `#define readlink(...) __foo_readlink(...)` then this module + * can be used to tell `StdFunctionOrMacro` to look for `__foo_readlink` based on the prefix `__foo_`. + */ +private module PrefixedFunctionWrapperMacro<getPrefix/0 getPfx> { + bindingset[mi, argumentIdx] + cpp::Expr inferArgument(cpp::MacroInvocation mi, int argumentIdx) { + result = getFunctionCall(mi).getArgument(argumentIdx) + } + + bindingset[mi] + cpp::Expr inferExpr(cpp::MacroInvocation mi) { result = getFunctionCall(mi) } + + bindingset[mi] + private cpp::FunctionCall getFunctionCall(cpp::MacroInvocation mi) { + result = mi.getExpr() and + result.getTarget().hasName(getPfx() + mi.getMacroName()) } } diff --git a/cpp/common/src/codingstandards/cpp/alertreporting/PrintExpr.qll b/cpp/common/src/codingstandards/cpp/alertreporting/PrintExpr.qll new file mode 100644 index 0000000000..14830c0118 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/alertreporting/PrintExpr.qll @@ -0,0 +1,45 @@ +import cpp + +signature class ShouldPrettyPrint extends Expr; + +module PrintExpr<ShouldPrettyPrint PrintExpr> { + bindingset[e] + string print(Expr e) { + if not e instanceof PrintExpr + then result = e.toString() + else + if exists(getMacroInvocationString(e)) + then result = getMacroInvocationString(e) + else + if e instanceof SupportedExpr + then result = e.(SupportedExpr).getExprString() + else result = e.toString() + } + + private string getMacroInvocationString(PrintExpr e) { + exists(MacroInvocation mi | + mi.getExpr() = e and + result = mi.getMacroName() + ) + } + + final class FinalPrintExpr = PrintExpr; + + abstract class SupportedExpr extends FinalPrintExpr { + abstract string getExprString(); + } + + final class FinalBinaryOperation = BinaryOperation; + + class PrintBinaryExpr extends SupportedExpr, FinalBinaryOperation { + override string getExprString() { + result = print(getLeftOperand()) + " " + getOperator() + " " + print(getRightOperand()) + } + } + + final class FinalLiteral = Literal; + + private class PrintLiteral extends SupportedExpr, FinalLiteral { + override string getExprString() { result = getValue() } + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/IO5.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/IO5.qll new file mode 100644 index 0000000000..964473ffbd --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/IO5.qll @@ -0,0 +1,78 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype IO5Query = + TFopenWithNonExclusiveFileCreationModeQuery() or + TPossiblyPredictableTemporaryFileCreationQuery() or + TExploitableTemporaryFileCreationQuery() or + TIncorrectNullTerminationOfReadlinkQuery() + +predicate isIO5QueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `fopenWithNonExclusiveFileCreationMode` query + IO5Package::fopenWithNonExclusiveFileCreationModeQuery() and + queryId = + // `@id` for the `fopenWithNonExclusiveFileCreationMode` query + "c/cert/fopen-with-non-exclusive-file-creation-mode" and + ruleId = "FIO03-C" and + category = "recommendation" + or + query = + // `Query` instance for the `possiblyPredictableTemporaryFileCreation` query + IO5Package::possiblyPredictableTemporaryFileCreationQuery() and + queryId = + // `@id` for the `possiblyPredictableTemporaryFileCreation` query + "c/cert/possibly-predictable-temporary-file-creation" and + ruleId = "FIO21-C" and + category = "recommendation" + or + query = + // `Query` instance for the `exploitableTemporaryFileCreation` query + IO5Package::exploitableTemporaryFileCreationQuery() and + queryId = + // `@id` for the `exploitableTemporaryFileCreation` query + "c/cert/exploitable-temporary-file-creation" and + ruleId = "FIO21-C" and + category = "recommendation" + or + query = + // `Query` instance for the `incorrectNullTerminationOfReadlink` query + IO5Package::incorrectNullTerminationOfReadlinkQuery() and + queryId = + // `@id` for the `incorrectNullTerminationOfReadlink` query + "c/cert/incorrect-null-termination-of-readlink" and + ruleId = "POS30-C" and + category = "rule" +} + +module IO5Package { + Query fopenWithNonExclusiveFileCreationModeQuery() { + //autogenerate `Query` type + result = + // `Query` type for `fopenWithNonExclusiveFileCreationMode` query + TQueryC(TIO5PackageQuery(TFopenWithNonExclusiveFileCreationModeQuery())) + } + + Query possiblyPredictableTemporaryFileCreationQuery() { + //autogenerate `Query` type + result = + // `Query` type for `possiblyPredictableTemporaryFileCreation` query + TQueryC(TIO5PackageQuery(TPossiblyPredictableTemporaryFileCreationQuery())) + } + + Query exploitableTemporaryFileCreationQuery() { + //autogenerate `Query` type + result = + // `Query` type for `exploitableTemporaryFileCreation` query + TQueryC(TIO5PackageQuery(TExploitableTemporaryFileCreationQuery())) + } + + Query incorrectNullTerminationOfReadlinkQuery() { + //autogenerate `Query` type + result = + // `Query` type for `incorrectNullTerminationOfReadlink` query + TQueryC(TIO5PackageQuery(TIncorrectNullTerminationOfReadlinkQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index ae035b9c5d..a2a5e925f8 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -46,6 +46,7 @@ import IO1 import IO2 import IO3 import IO4 +import IO5 import IntegerOverflow import InvalidMemory1 import InvalidMemory2 @@ -135,6 +136,7 @@ newtype TCQuery = TIO2PackageQuery(IO2Query q) or TIO3PackageQuery(IO3Query q) or TIO4PackageQuery(IO4Query q) or + TIO5PackageQuery(IO5Query q) or TIntegerOverflowPackageQuery(IntegerOverflowQuery q) or TInvalidMemory1PackageQuery(InvalidMemory1Query q) or TInvalidMemory2PackageQuery(InvalidMemory2Query q) or @@ -224,6 +226,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat isIO2QueryMetadata(query, queryId, ruleId, category) or isIO3QueryMetadata(query, queryId, ruleId, category) or isIO4QueryMetadata(query, queryId, ruleId, category) or + isIO5QueryMetadata(query, queryId, ruleId, category) or isIntegerOverflowQueryMetadata(query, queryId, ruleId, category) or isInvalidMemory1QueryMetadata(query, queryId, ruleId, category) or isInvalidMemory2QueryMetadata(query, queryId, ruleId, category) or diff --git a/cpp/common/src/codingstandards/cpp/standardlibrary/FileAccess.qll b/cpp/common/src/codingstandards/cpp/standardlibrary/FileAccess.qll index 58d93de1a9..cf33af3a8b 100644 --- a/cpp/common/src/codingstandards/cpp/standardlibrary/FileAccess.qll +++ b/cpp/common/src/codingstandards/cpp/standardlibrary/FileAccess.qll @@ -71,9 +71,9 @@ class FOpenCall extends FunctionCall { } Expr getMode() { - this.getTarget().hasGlobalName("open") and result = this.getArgument(1) - or - result = this.getArgument(getNumberOfArguments() - 1) + if this.getTarget().hasGlobalName("open") + then result = this.getArgument(1) + else result = this.getArgument(getNumberOfArguments() - 1) } // make predicate @@ -81,11 +81,29 @@ class FOpenCall extends FunctionCall { predicate isWriteMode() { this.getMode().getValue() = ["w", "a", "r+", "w+", "a+"] } + predicate mayCreate() { + not getTarget().hasGlobalName("fdopen") and + this.getMode().getValue().matches(["%w%", "%a%"]) + or + modeHasOFlag("O_CREAT") + } + + predicate isExclusiveMode() { + this.getMode().getValue().matches(["%x"]) + or + modeHasOFlag("O_EXCL") + } + predicate isReadOnlyMode() { this.isReadMode() and not this.isWriteMode() or + modeHasOFlag("O_RDONLY") + } + + /** Holds if the flag is present in a call such as `open(..., O_CREAT | O_RDONLY, ...)` */ + private predicate modeHasOFlag(string flag) { exists(MacroInvocation mi | - mi.getMacroName() = "O_RDONLY" and + mi.getMacroName() = flag and ( this.getMode() = mi.getExpr() or diff --git a/rule_packages/c/IO5.json b/rule_packages/c/IO5.json new file mode 100644 index 0000000000..73a93eaab3 --- /dev/null +++ b/rule_packages/c/IO5.json @@ -0,0 +1,74 @@ +{ + "CERT-C": { + "FIO03-C": { + "properties": { + "obligation": "recommendation" + }, + "queries": [ + { + "description": "Usage of fopen() without the proper exclusion mode can lead to a program overwriting over accessing an unintended file.", + "kind": "problem", + "name": "Do not make assumptions about fopen() and file creation", + "precision": "high", + "severity": "error", + "short_name": "FopenWithNonExclusiveFileCreationMode", + "tags": [ + "correctness", + "security" + ] + } + ], + "title": "Do not make assumptions about fopen() and file creation" + }, + "FIO21-C": { + "properties": { + "obligation": "recommendation" + }, + "queries": [ + { + "description": "Hardcoded or predictably generated temporary file paths can be hijacked or manipulated by an attacker if not created in a secure manner.", + "kind": "path-problem", + "name": "Do not use predictably generated temporary file paths", + "precision": "medium", + "severity": "warning", + "short_name": "PossiblyPredictableTemporaryFileCreation", + "tags": [ + "security" + ] + }, + { + "description": "Temporary files can be hijacked or manipulated by an attacker if not created in a secure manner.", + "kind": "problem", + "name": "Do not create temporary files in shared directories", + "precision": "very-high", + "severity": "error", + "short_name": "ExploitableTemporaryFileCreation", + "tags": [ + "security" + ] + } + ], + "title": "Do not create temporary files in shared directories" + }, + "POS30-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "The readlink() function does not null terminate or leave a character at the end of the buffer argument, and must be handled properly.", + "kind": "problem", + "name": "Use the readlink() function properly", + "precision": "high", + "severity": "error", + "short_name": "IncorrectNullTerminationOfReadlink", + "tags": [ + "correctness", + "security" + ] + } + ], + "title": "Use the readlink() function properly" + } + } +} \ No newline at end of file diff --git a/rules.csv b/rules.csv index 3f7961b630..68049625e6 100644 --- a/rules.csv +++ b/rules.csv @@ -515,6 +515,7 @@ c,CERT-C,ERR30-C,Yes,Rule,,,Take care when reading errno,M19-3-1,Contracts4,Hard c,CERT-C,ERR32-C,Yes,Rule,,,Do not rely on indeterminate values of errno,,Contracts5,Hard, c,CERT-C,ERR33-C,Yes,Rule,,,Detect and handle standard library errors,MEM52-CPP,Contracts5,Hard, c,CERT-C,ERR34-C,OutOfScope,Rule,,,Detect errors when converting a string to a number,,,, +c,CERT-C,EXP16-C,Yes,Recommendation,,,Do not compare function pointers to constant values,,Expressions2,Medium, c,CERT-C,EXP30-C,Yes,Rule,,,Do not depend on the order of evaluation for side effects,EXP50-CPP,SideEffects1,Easy, c,CERT-C,EXP32-C,Yes,Rule,,,Do not access a volatile object through a nonvolatile reference,,Pointers3,Easy, c,CERT-C,EXP33-C,Yes,Rule,,,Do not read uninitialized memory,EXP53-CPP,InvalidMemory1,Import, @@ -530,6 +531,8 @@ c,CERT-C,EXP44-C,Yes,Rule,,,"Do not rely on side effects in operands to sizeof, c,CERT-C,EXP45-C,Yes,Rule,,,Do not perform assignments in selection statements,M6-2-1,SideEffects1,Medium, c,CERT-C,EXP46-C,Yes,Rule,,,Do not use a bitwise operator with a Boolean-like operand,,Expressions,Easy, c,CERT-C,EXP47-C,OutOfScope,Rule,,,Do not call va_arg with an argument of the incorrect type,,,, +c,CERT-C,FIO03-C,Yes,Recommendation,,,Do not make assumptions about fopen() and file creation,,IO5,Hard, +c,CERT-C,FIO21-C,Yes,Recommendation,,,Do not create temporary files in shared directories,,IO5,Easy, c,CERT-C,FIO30-C,Yes,Rule,,,Exclude user input from format strings,A27-0-1,IO1,Import, c,CERT-C,FIO32-C,Yes,Rule,,,Do not perform operations on devices that are only appropriate for files,,IO3,Medium, c,CERT-C,FIO34-C,Yes,Rule,,,Distinguish between characters read from a file and EOF or WEOF,,IO1,Hard, @@ -569,7 +572,7 @@ c,CERT-C,MSC38-C,Yes,Rule,,,Do not treat a predefined identifier as an object if c,CERT-C,MSC39-C,Yes,Rule,,,Do not call va_arg() on a va_list that has an indeterminate value,,Contracts7,Hard, c,CERT-C,MSC40-C,Yes,Rule,,,Do not violate constraints,,Contracts,Very Hard, c,CERT-C,MSC41-C,OutOfScope,Rule,,,Never hard code sensitive information,,,, -c,CERT-C,POS30-C,OutOfScope,Rule,,,Use the readlink() function properly,,,, +c,CERT-C,POS30-C,Yes,Rule,,,Use the readlink() function properly,,IO5,Hard, c,CERT-C,POS34-C,OutOfScope,Rule,,,Do not call putenv() with a pointer to an automatic variable as the argument,,,, c,CERT-C,POS35-C,OutOfScope,Rule,,,Avoid race conditions while checking for the existence of a symbolic link,,,, c,CERT-C,POS36-C,OutOfScope,Rule,,,Observe correct revocation order while relinquishing privileges,,,, diff --git a/schemas/rule-package.schema.json b/schemas/rule-package.schema.json index b4f729afe2..f8c3f028e3 100644 --- a/schemas/rule-package.schema.json +++ b/schemas/rule-package.schema.json @@ -141,7 +141,8 @@ "obligation": { "type": "string", "enum": [ - "rule" + "rule", + "recommendation" ] } }, diff --git a/scripts/help/cert-help-extraction.py b/scripts/help/cert-help-extraction.py index 6bd1abccd5..f785b0955f 100755 --- a/scripts/help/cert-help-extraction.py +++ b/scripts/help/cert-help-extraction.py @@ -1,5 +1,6 @@ #!/usr/bin/env python3 from argparse import ArgumentParser +from typing import Generator import tempfile import re import urllib.request @@ -23,6 +24,7 @@ CERT_WIKI = "https://wiki.sei.cmu.edu" RULES_LIST_C = "/confluence/display/c/2+Rules" +RECOMMENDED_LIST_C = "/confluence/display/c/3+Recommendations" RULES_LIST_CPP = "/confluence/display/cplusplus/2+Rules" cache_path = script_path.parent / '.cache' @@ -47,16 +49,22 @@ def soupify(url: str) -> BeautifulSoup: return BeautifulSoup(content, 'html.parser') - -def get_rules(): - rules = [] - for soup in [soupify(f"{CERT_WIKI}{RULES_LIST_C}"), soupify(f"{CERT_WIKI}{RULES_LIST_CPP}")]: +def get_rule_listings() -> Generator[Tag, None, None]: + for rule_list_id in [RULES_LIST_C, RULES_LIST_CPP]: + soup = soupify(f"{CERT_WIKI}{rule_list_id}") if soup == None: - return None - - rule_listing_start = soup.find( + continue + + yield soup.find( "h1", string="Rule Listing") + soup = soupify(f"{CERT_WIKI}{RECOMMENDED_LIST_C}") + if soup != None: + yield soup.find("h1", string="Recommendation Listing") + +def get_rules(): + rules = [] + for rule_listing_start in get_rule_listings(): for link in rule_listing_start.next_element.next_element.find_all('a'): if '-C' in link.string: rule, title = map(str.strip, link.string.split('.', 1)) @@ -214,6 +222,8 @@ def helper(node): # Fix a broken url present in many CERT-C pages if node.name == 'a' and 'href' in node.attrs and node['href'] == "http://BB. Definitions#vulnerability": node['href'] = "https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability" + elif node.name == 'a' and 'href' in node.attrs and node['href'] == "http://BB. Definitions#unexpected behavior": + node['href'] = "https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-unexpectedbehavior" # Turn relative URLs into absolute URLS elif node.name == 'a' and 'href' in node.attrs and node['href'].startswith("/confluence"): node['href'] = f"{CERT_WIKI}{node['href']}"