[analyzer] Improve the documentation of the alpha.security.taint.TaintPropagation checker

[dkrupp]

Summary

We extend the checker documentation with the following information

-How the user can mark inline that a data sanitation was performed performed to make a superflous report disappear
-Add references to CWEs and coding guidelines
-Describe the limitations of the checker
-Provide life-like examples

[donat.nagy]

clang/docs/analyzer/checkers.rst

78–80

Why is this paragraph (and the one above it) wrapped inconsistently? If we are touching these docs, perhaps we could re-wrap them to e.g 80 characters / line.

2386–2387

This old word choice is awkward, consider replacing it with e.g. "data from the network".

2389

Why not just "databases"?

2403

If the filename is too long (more than 1014 characters), this is a buffer overflow. I admit that having a secondary unrelated vulnerability makes the example more realistic :), but I think we should still avoid it. (This also appears in other variants of the example code, including the "No vulnerability anymore" one.)

2425

Nitpick: the comment formatting is inconsistent: for example, this is lowercase while most others start with an uppercase letter, or half of the comments have a space after the // while the others don't.

2440–2442

Perhaps explicitly mention that there are no built-in filters.

2456–2460

Separating the actual sanitization and the function that's magically recognized by the taint checker doesn't seem to be a good design pattern. Here csa_sanitize() is just a synonym for the "silence this checker here" marker, which is very confusing, because if someone is not familiar with this locally introduced no-op function, they will think that it's performing actual sanitization! At the very least we should rename this magical no-op to csa_mark_sanitized() or something similar.

The root issue is that in this example we would like to use a verifier function (that determines whether the tainted data is safe) instead of a sanitizer function (that can convert any tainted data into safe data) and our taint handling engine is not prepared to handle conditional Filter effects like "this function removes taint from its first argument if its return value is true".

I think it would be much better to switch to a different example where the "natural" solution is more aligned with the limited toolbox provided by our taint framework (i.e. it's possible define a filter function that actually removes problematic parts of the untrusted input).

2614

Spellcheck: "unknown"

[dkrupp]

• changed the main example to a data sanitation example (sanitizeFileName()) instead of a data verification example
• fixed typos
• fixed sphinx warnings

[dkrupp]

Thanks @donat.nagy for your review. I addressed your remarks. After patch https://reviews.llvm.org/D155848 these sanitizing examples work properly.

clang/docs/analyzer/checkers.rst

78–80

The formatting of this paragraph should not be impacted by this unrleated change. I will revert all unrelated formatting changes.

2403

True. cmd buffer increased to 2048

2456–2460

I changed this fist example to be a data sanitation example, where the sanitizeFileName(..) function changes the user input to an empty string if the filneme is invalid.

Then in the next example we show the generic csa_mark_sanitized() function and how it can be used to mark the valid code paths of verifier functions.

[donat.nagy]

Mostly LGTM, but I'd like to ask you to ensure consistent line wrapping (to e.g. 72 or 80 characters) in the free-flowing text that was added or rewritten by this commit. (I know that this is a minor question because the linebreaks won't be visible in the final output, but the raw markdown is itself a human-readable format and we should keep it reasonably clean.)

[dkrupp]

-lines wrapped to 80 characters

[steakhal]

clang/docs/analyzer/checkers.rst

2471

Have you considered unconditionally having this function with an empty body?
That way it would have no "noise" at callsite.

[dkrupp]

clang/docs/analyzer/checkers.rst

2471

But that way the program would not compile, because the definition would not be found. Or maybe I misunderstand you.

Maybe in the future we could add an another type of filter function which would support validation type of functions: would sanitize the passed parameter only, if the function returns with non-null, non-zero.

[steakhal]

clang/docs/analyzer/checkers.rst

2469–2472

I was thinking of this when I mentioned "function with empty body".
Notice the inline, so that one could put this into a header file without violating ODR.

---

This way at the callsites, one wouldn't need to ifdef-clutter the code.
I think this would lead to more maintainable code in the long run, with happier users.

[dkrupp]

clang/docs/analyzer/checkers.rst

2469–2472

Good Point! I will add this to the next patch to this checker. thanks.

clang/docs/analyzer/checkers.rst

 
 This checker specifically does
 not report null pointer dereferences for x86 and x86-64 targets when the
 address space is 256 (x86 GS Segment), 257 (x86 FS Segment), or 258 (x86 SS
 segment). See `X86/X86-64 Language Extensions
 <https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments>`__
 for reference.
 
 The ``SuppressAddressSpaces`` option suppresses
 warnings for null dereferences of all pointers with address spaces. You can
 disable this behavior with the option
 ``-analyzer-config core.NullDereference:SuppressAddressSpaces=false``.
 *Defaults to true*.
 
 .. code-block:: objc
 
  // C
  void test(int *p) {
    if (p)
void previous_call_invalidation() {
 
     *p;
     // dereferencing invalid pointer
   }
 
 alpha.security.taint
 ^^^^^^^^^^^^^^^^^^^^
 
-Checkers implementing `taint analysis <https://en.wikipedia.org/wiki/Taint_checking>`_.
+Checkers implementing
+`taint analysis <https://en.wikipedia.org/wiki/Taint_checking>`_.
 
 .. _alpha-security-taint-TaintPropagation:
 
 alpha.security.taint.TaintPropagation (C, C++)
 """"""""""""""""""""""""""""""""""""""""""""""
 
-Taint analysis identifies untrusted sources of information (taint sources), rules as to how the untrusted data flows along the execution path (propagation rules), and points of execution where the use of tainted data is risky (taints sinks).
+Taint analysis identifies potential security vulnerabilities where the
+attacker can inject malicious data to the program to execute an attack
+(privilege escalation, command injection, SQL injection etc.).
+
+The malicious data is injected at the taint source (e.g. ``getenv()`` call)
+which is then propagated through function calls and being used as arguments of
+sensitive operations, also called as taint sinks (e.g. ``system()`` call).
+
+One can defend agains this type of vulnerability by always checking and
+santizing the potentially malicious, untrusted user input.
+
+The goal of the checker is to discover and show to the user these potential
+taint source-sink pairs and the propagation call chain.
+
 The most notable examples of taint sources are:
 
-  - network originating data
+  - data from network
+  - files or standard input
   - environment variables
-  - database originating data
+  - data from databases
 
-``GenericTaintChecker`` is the main implementation checker for this rule, and it generates taint information used by other checkers.
+Let us examine a practical example of a Command Injection attack.
+
+.. code-block:: c
+
+  // Command Injection Vulnerability Example
+  int main(int argc, char** argv) {
+    char cmd[2048] = "/bin/cat ";
+    char filename[1024];
+    printf("Filename:");
+    scanf (" %1023[^\n]", filename); // The attacker can inject a shell escape here
+    strcat(cmd, filename);
+    system(cmd); // Warning: Untrusted data is passed to a system call
+  }
+
+The program prints the content of any user specified file.
+Unfortunately the attacker can execute arbitrary commands
+with shell escapes. For example with the following input the `ls` command is also
+executed after the contents of `/etc/shadow` is printed.
+`Input: /etc/shadow ; ls /`
+
+The analysis implemented in this checker points out this problem.
+
+One can protect against such attack by for example checking if the provided
+input refers to a valid file and removing any invalid user input.
+
+.. code-block:: c
+
+  // No vulnerability anymore, but we still get the warning
+  void sanitizeFileName(char* filename){
+    if (access(filename,F_OK)){// Verifying user input
+      printf("File does not exist\n");
+      filename[0]='\0';
+      }
+  }
+  int main(int argc, char** argv) {
+    char cmd[2048] = "/bin/cat ";
+    char filename[1024];
+    printf("Filename:");
+    scanf (" %1023[^\n]", filename); // The attacker can inject a shell escape here
+    sanitizeFileName(filename);// filename is safe after this point
+    if (!filename[0])
+      return -1;
+    strcat(cmd, filename);
+    system(cmd); // Superflous Warning: Untrusted data is passed to a system call
+  }
+
+Unfortunately, the checker cannot discover automatically that the programmer
+have performed data sanitation, so it still emits the warning.
+
+One can get rid of this superflous warning by telling by specifying the
+sanitation functions in the taint configuation file (see
+:doc:`user-docs/TaintAnalysisConfiguration`).
+
+.. code-block:: YAML
+
+  Filters:
+  - Name: sanitizeFileName
+    Args: [0]
+
+The clang invocation to pass the configuration file location:
+
+.. code-block:: bash
+
+  clang  --analyze -Xclang -analyzer-config  -Xclang alpha.security.taint.TaintPropagation:Config=`pwd`/taint_config.yml ...
+
+If you are validating your inputs instead of sanitizing them, or don't want to
+mention each sanitizing function in our configuration,
+you can use a more generic approach.
+
+Introduce a generic no-op `csa_mark_sanitized(..)` function to
+tell the Clang Static Analyzer
+that the variable is safe to be used on that analysis path.
+
+.. code-block:: c
+
+  // Marking sanitized variables safe.
+  // No vulnerability anymore, no warning.
+
+  // User csa_mark_sanitize function is for the analyzer only
+  #ifdef __clang_analyzer__
+    void csa_mark_sanitized(const void *);
+  #endif
+
+  int main(int argc, char** argv) {
+    char cmd[2048] = "/bin/cat ";
+    char filename[1024];
+    printf("Filename:");
+    scanf (" %1023[^\n]", filename);
+    if (access(filename,F_OK)){// Verifying user input
+      printf("File does not exist\n");
+      return -1;
+    }
+    #ifdef __clang_analyzer__
+      csa_mark_sanitized(filename); // Indicating to CSA that filename variable is safe to be used after this point
+    #endif
+    strcat(cmd, filename);
+    system(cmd); // No warning
+  }
+
+Similarly to the previous example, you need to
+define a `Filter` function in a `YAML` configuration file
+and add the `csa_mark_sanitized` function.
+
+.. code-block:: YAML
+
+  Filters:
+  - Name: csa_mark_sanitized
+    Args: [0]
+
+Then calling `csa_mark_sanitized(X)` will tell the analyzer that `X` is safe to
+be used after this point, because its contents are verified. It is the
+responisibility of the programmer to ensure that this verification was indeed
+correct. Please note that `csa_mark_sanitized` function is only declared and
+used during Clang Static Analysis and skipped in (production) builds.
+
+Further examples of injection vulnerabilities this checker can find.
 
 .. code-block:: c
 
   void test() {
     char x = getchar(); // 'x' marked as tainted
     system(&x); // warn: untrusted data is passed to a system call
   }
 
   // note: compiler internally checks if the second param to
   // sprintf is a string literal or not.
   // Use -Wno-format-security to suppress compiler warning.
   void test() {
     char s[10], buf[10];
     fscanf(stdin, "%s", s); // 's' marked as tainted
 
-   sprintf(buf, s); // warn: untrusted data as a format string
+    sprintf(buf, s); // warn: untrusted data used as a format string
   }
 
   void test() {
     size_t ts;
     scanf("%zd", &ts); // 'ts' marked as tainted
     int *p = (int *)malloc(ts * sizeof(int));
-     // warn: untrusted data as buffer size
+      // warn: untrusted data used as buffer size
   }
 
-There are built-in sources, propagations and sinks defined in code inside ``GenericTaintChecker``.
-These operations are handled even if no external taint configuration is provided.
-
-Default sources defined by ``GenericTaintChecker``:
- ``_IO_getc``, ``fdopen``, ``fopen``, ``freopen``, ``get_current_dir_name``, ``getch``, ``getchar``, ``getchar_unlocked``, ``getwd``, ``getcwd``, ``getgroups``, ``gethostname``, ``getlogin``, ``getlogin_r``, ``getnameinfo``, ``gets``, ``gets_s``, ``getseuserbyname``, ``readlink``, ``readlinkat``, ``scanf``, ``scanf_s``, ``socket``, ``wgetch``
-
-Default propagations defined by ``GenericTaintChecker``:
-``atoi``, ``atol``, ``atoll``, ``basename``, ``dirname``, ``fgetc``, ``fgetln``, ``fgets``, ``fnmatch``, ``fread``, ``fscanf``, ``fscanf_s``, ``index``, ``inflate``, ``isalnum``, ``isalpha``, ``isascii``, ``isblank``, ``iscntrl``, ``isdigit``, ``isgraph``, ``islower``, ``isprint``, ``ispunct``, ``isspace``, ``isupper``, ``isxdigit``, ``memchr``, ``memrchr``, ``sscanf``, ``getc``, ``getc_unlocked``, ``getdelim``, ``getline``, ``getw``, ``memcmp``, ``memcpy``, ``memmem``, ``memmove``, ``mbtowc``, ``pread``, ``qsort``, ``qsort_r``, ``rawmemchr``, ``read``, ``recv``, ``recvfrom``, ``rindex``, ``strcasestr``, ``strchr``, ``strchrnul``, ``strcasecmp``, ``strcmp``, ``strcspn``, ``strlen``, ``strncasecmp``, ``strncmp``, ``strndup``, ``strndupa``, ``strnlen``, ``strpbrk``, ``strrchr``, ``strsep``, ``strspn``, ``strstr``, ``strtol``, ``strtoll``, ``strtoul``, ``strtoull``, ``tolower``, ``toupper``, ``ttyname``, ``ttyname_r``, ``wctomb``, ``wcwidth``
+There are built-in sources, propagations and sinks even if no external taint
+configuration is provided.
 
-Default sinks defined in ``GenericTaintChecker``:
-``printf``, ``setproctitle``, ``system``, ``popen``, ``execl``, ``execle``, ``execlp``, ``execv``, ``execvp``, ``execvP``, ``execve``, ``dlopen``, ``memcpy``, ``memmove``, ``strncpy``, ``strndup``, ``malloc``, ``calloc``, ``alloca``, ``memccpy``, ``realloc``, ``bcopy``
+Default sources:
+ ``_IO_getc``, ``fdopen``, ``fopen``, ``freopen``, ``get_current_dir_name``,
+ ``getch``, ``getchar``, ``getchar_unlocked``, ``getwd``, ``getcwd``,
+ ``getgroups``, ``gethostname``, ``getlogin``, ``getlogin_r``, ``getnameinfo``,
+ ``gets``, ``gets_s``, ``getseuserbyname``, ``readlink``, ``readlinkat``,
+ ``scanf``, ``scanf_s``, ``socket``, ``wgetch``
+
+Default propagations rules:
+ ``atoi``, ``atol``, ``atoll``, ``basename``, ``dirname``, ``fgetc``,
+ ``fgetln``, ``fgets``, ``fnmatch``, ``fread``, ``fscanf``, ``fscanf_s``,
+ ``index``, ``inflate``, ``isalnum``, ``isalpha``, ``isascii``, ``isblank``,
+ ``iscntrl``, ``isdigit``, ``isgraph``, ``islower``, ``isprint``, ``ispunct``,
+ ``isspace``, ``isupper``, ``isxdigit``, ``memchr``, ``memrchr``, ``sscanf``,
+ ``getc``, ``getc_unlocked``, ``getdelim``, ``getline``, ``getw``, ``memcmp``,
+ ``memcpy``, ``memmem``, ``memmove``, ``mbtowc``, ``pread``, ``qsort``,
+ ``qsort_r``, ``rawmemchr``, ``read``, ``recv``, ``recvfrom``, ``rindex``,
+ ``strcasestr``, ``strchr``, ``strchrnul``, ``strcasecmp``, ``strcmp``,
+ ``strcspn``, ``strlen``, ``strncasecmp``, ``strncmp``, ``strndup``,
+ ``strndupa``, ``strnlen``, ``strpbrk``, ``strrchr``, ``strsep``, ``strspn``,
+ ``strstr``, ``strtol``, ``strtoll``, ``strtoul``, ``strtoull``, ``tolower``,
+ ``toupper``, ``ttyname``, ``ttyname_r``, ``wctomb``, ``wcwidth``
+
+Default sinks:
+ ``printf``, ``setproctitle``, ``system``, ``popen``, ``execl``, ``execle``,
+ ``execlp``, ``execv``, ``execvp``, ``execvP``, ``execve``, ``dlopen``,
+ ``memcpy``, ``memmove``, ``strncpy``, ``strndup``, ``malloc``, ``calloc``,
+ ``alloca``, ``memccpy``, ``realloc``, ``bcopy``
+
+Please note that there are no built-in filter functions.
+
+One can configure their own taint sources, sinks, and propagation rules by
+providing a configuration file via checker option
+``alpha.security.taint.TaintPropagation:Config``. The configuration file is in
+`YAML <http://llvm.org/docs/YamlIO.html#introduction-to-yaml>`_ format. The
+taint-related options defined in the config file extend but do not override the
+built-in sources, rules, sinks. The format of the external taint configuration
+file is not stable, and could change without any notice even in a non-backward
+compatible way.
+
+For a more detailed description of configuration options, please see the
+:doc:`user-docs/TaintAnalysisConfiguration`. For an example see
+:ref:`clangsa-taint-configuration-example`.
+
+**Configuration**
+
+* `Config`  Specifies the name of the YAML configuration file. The user can
+  define their own taint sources and sinks.
+
+**Related Guidelines**
+
+* `CWE Data Neutralization Issues
+  <https://cwe.mitre.org/data/definitions/137.html>`_
+* `SEI Cert STR02-C. Sanitize data passed to complex subsystems
+  <https://wiki.sei.cmu.edu/confluence/display/c/STR02-C.+Sanitize+data+passed+to+complex+subsystems>`_
+* `SEI Cert ENV33-C. Do not call system()
+  <https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152177>`_
+* `ENV03-C. Sanitize the environment when invoking external programs
+  <https://wiki.sei.cmu.edu/confluence/display/c/ENV03-C.+Sanitize+the+environment+when+invoking+external+programs>`_
 
-The user can configure taint sources, sinks, and propagation rules by providing a configuration file via checker option ``alpha.security.taint.TaintPropagation:Config``.
-
-External taint configuration is in `YAML <http://llvm.org/docs/YamlIO.html#introduction-to-yaml>`_ format. The taint-related options defined in the config file extend but do not override the built-in sources, rules, sinks.
-The format of the external taint configuration file is not stable, and could change without any notice even in a non-backward compatible way.
+**Limitations**
 
-For a more detailed description of configuration options, please see the :doc:`user-docs/TaintAnalysisConfiguration`. For an example see :ref:`clangsa-taint-configuration-example`.
+* The taintedness property is not propagated through function calls which are
+  unknown (or too complex) to the analyzer, unless there is a specific
+  propagation rule built-in to the checker or given in the YAML configuration
+  file. This causes potential true positive findings to be lost.
 
 alpha.unix
 ^^^^^^^^^^^
 
 .. _alpha-unix-StdCLibraryFunctions:
 
 alpha.unix.StdCLibraryFunctions (C)
 """""""""""""""""""""""""""""""""""
 Check for calls of standard library functions that violate predefined argument
 constraints. For example, it is stated in the C standard that for the ``int
 isalnum(int ch)`` function the behavior is undefined if the value of ``ch`` is
 not representable as unsigned char and is not equal to ``EOF``.
 
 .. code-block:: c
 
   #define EOF -1
   void test_alnum_concrete(int v) {
     int ret = isalnum(256); // \
     // warning: Function argument outside of allowed range
     (void)ret;
   }
 
   void buffer_size_violation(FILE *file) {