Skip to content

Conversation

@dkrupp
Copy link
Owner

@dkrupp dkrupp commented Feb 16, 2023

This patch improves the diagnostics of the alpha.security.taint.TaintPropagation and taint related checkers by showing the "Taint originated here" note at the correct place, where the attacker may inject it. This greatly improves the understandability of the taint reports.

Taint Analysis: The attacker injects the malicious data at the taint source (e.g. getenv() call) which is then propagated and used at taint sink (e.g. exec() call) causing a security vulnerability (e.g. shell injection vulnerability), without data sanitation.

The goal of the checker is to discover and show to the user these potential taint source, sink pairs and the propagation call chain.

In the baseline the taint source was pointing to an invalid location, typically somewhere between the real taint source and sink.

After the fix, the "Taint originated here" tag is correctly shown at the taint source. This is the function call where the attacker can inject a malicious data (e.g. reading from environment variable, reading from file, reding from standard input etc.).

Before the patch the clang static analyzer puts the taint origin note wrongly to the strtol(..) call.

int main(){
  char *pathbuf;
  char *user_data=getenv("USER_INPUT"); 
  char *end;  
  long size=strtol(user_data, &end, 10); // note: Taint originated here. 
  if (size > 0){
    pathbuf=(char*) malloc(size+1);//note: Untrusted data is used to specify the buffer size ...
    // ... 
    free(pathbuf);
  }
  return 0;
}

After the fix, the taint origin point is correctly annotated at getenv() where the attacker really injects the value.

int main(){
  char *pathbuf;
  char *user_data=getenv("USER_INPUT");  // note: Taint originated here. 
  char *end;
  long size=strtol(user_data, &end, 10); 
  if (size > 0){
    pathbuf=(char*) malloc(size+1);//note: Untrusted data is used to specify the buffer size ...
    // ... 
    free(pathbuf);
  }
  return 0;
}

The BugVisitor placing the note was wrongly going back only until introduction of the tainted SVal in the sink.

This patch creates a new uniquely identified taint flow for each taint source (e.g.getenv()) it traverses and places a NoteTag ("Taint originated here") with the new id. Then, when the bug report is generated, the taint flow id is propagated back (in the new TainBugReport) along the bug path and the correct "Taint originated here." annotation is generated (matching the flow id).

You can find the new improved reports

here

And the old reports (look out for "Taint originated here" notes. They are at the wrong place, close to the end of the reports)

here

dkrupp pushed a commit that referenced this pull request Feb 16, 2023
This reverts commit d768b97.

Causes sanitizer failure: https://lab.llvm.org/buildbot/#/builders/238/builds/1114

```
/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Support/xxhash.cpp:107:12: runtime error: applying non-zero offset 8 to null pointer
    #0 0xaaaab28ec6c8 in llvm::xxHash64(llvm::StringRef) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Support/xxhash.cpp:107:12
    #1 0xaaaab28cbd38 in llvm::StringMapImpl::LookupBucketFor(llvm::StringRef) /b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Support/StringMap.cpp:87:28
```

Probably causes test failure in `warn-unsafe-buffer-usage-fixits-local-var-span.cpp`: https://lab.llvm.org/buildbot/#/builders/60/builds/10619

Probably causes reverse-iteration test failure in `test-output-format.ll`: https://lab.llvm.org/buildbot/#/builders/54/builds/3545
@dkrupp dkrupp changed the base branch from main to taint_dataflow February 16, 2023 18:04
@dkrupp dkrupp changed the base branch from taint_dataflow to main February 16, 2023 18:04
The alpha.security.taint.TaintPropagation checker
indicated incorrectly the origin of the source if
the taintedness propagated through multiple variables.

This is fixed now.
dkrupp pushed a commit that referenced this pull request Sep 16, 2023
…ttempting to dereferencing iterators.

Runnign some tests with asan built of LLD would throw errors similar to the following:

AddressSanitizer:DEADLYSIGNAL
    #0 0x55d8e6da5df7 in operator() /mnt/ssd/repo/lld/llvm-project/lld/MachO/Arch/ARM64.cpp:612
    #1 0x55d8e6daa514 in operator() /mnt/ssd/repo/lld/llvm-project/lld/MachO/Arch/ARM64.cpp:650

Differential Revision: https://reviews.llvm.org/D157027
dkrupp pushed a commit that referenced this pull request Aug 28, 2024
…lvm#104148)

`hasOperands` does not always execute matchers in the order they are
written. This can cause issue in code using bindings when one operand
matcher is relying on a binding set by the other. With this change, the
first matcher present in the code is always executed first and any
binding it sets are available to the second matcher.

Simple example with current version (1 match) and new version (2
matches):
```bash
> cat tmp.cpp
int a = 13;
int b = ((int) a) - a;
int c = a - ((int) a);

> clang-query tmp.cpp
clang-query> set traversal IgnoreUnlessSpelledInSource
clang-query> m binaryOperator(hasOperands(cStyleCastExpr(has(declRefExpr(hasDeclaration(valueDecl().bind("d"))))), declRefExpr(hasDeclaration(valueDecl(equalsBoundNode("d"))))))

Match #1:

tmp.cpp:1:1: note: "d" binds here
int a = 13;
^~~~~~~~~~
tmp.cpp:2:9: note: "root" binds here
int b = ((int)a) - a;
        ^~~~~~~~~~~~
1 match.

> ./build/bin/clang-query tmp.cpp
clang-query> set traversal IgnoreUnlessSpelledInSource
clang-query> m binaryOperator(hasOperands(cStyleCastExpr(has(declRefExpr(hasDeclaration(valueDecl().bind("d"))))), declRefExpr(hasDeclaration(valueDecl(equalsBoundNode("d"))))))

Match #1:

tmp.cpp:1:1: note: "d" binds here
    1 | int a = 13;
      | ^~~~~~~~~~
tmp.cpp:2:9: note: "root" binds here
    2 | int b = ((int)a) - a;
      |         ^~~~~~~~~~~~

Match #2:

tmp.cpp:1:1: note: "d" binds here
    1 | int a = 13;
      | ^~~~~~~~~~
tmp.cpp:3:9: note: "root" binds here
    3 | int c = a - ((int)a);
      |         ^~~~~~~~~~~~
2 matches.
```

If this should be documented or regression tested anywhere please let me
know where.
dkrupp pushed a commit that referenced this pull request Aug 28, 2024
…104523)

Compilers and language runtimes often use helper functions that are
fundamentally uninteresting when debugging anything but the
compiler/runtime itself. This patch introduces a user-extensible
mechanism that allows for these frames to be hidden from backtraces and
automatically skipped over when navigating the stack with `up` and
`down`.

This does not affect the numbering of frames, so `f <N>` will still
provide access to the hidden frames. The `bt` output will also print a
hint that frames have been hidden.

My primary motivation for this feature is to hide thunks in the Swift
programming language, but I'm including an example recognizer for
`std::function::operator()` that I wished for myself many times while
debugging LLDB.

rdar://126629381


Example output. (Yes, my proof-of-concept recognizer could hide even
more frames if we had a method that returned the function name without
the return type or I used something that isn't based off regex, but it's
really only meant as an example).

before:
```
(lldb) thread backtrace --filtered=false
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x0000000100001f04 a.out`foo(x=1, y=1) at main.cpp:4:10
    frame #1: 0x0000000100003a00 a.out`decltype(std::declval<int (*&)(int, int)>()(std::declval<int>(), std::declval<int>())) std::__1::__invoke[abi:se200000]<int (*&)(int, int), int, int>(__f=0x000000016fdff280, __args=0x000000016fdff224, __args=0x000000016fdff220) at invoke.h:149:25
    frame #2: 0x000000010000399c a.out`int std::__1::__invoke_void_return_wrapper<int, false>::__call[abi:se200000]<int (*&)(int, int), int, int>(__args=0x000000016fdff280, __args=0x000000016fdff224, __args=0x000000016fdff220) at invoke.h:216:12
    frame #3: 0x0000000100003968 a.out`std::__1::__function::__alloc_func<int (*)(int, int), std::__1::allocator<int (*)(int, int)>, int (int, int)>::operator()[abi:se200000](this=0x000000016fdff280, __arg=0x000000016fdff224, __arg=0x000000016fdff220) at function.h:171:12
    frame llvm#4: 0x00000001000026bc a.out`std::__1::__function::__func<int (*)(int, int), std::__1::allocator<int (*)(int, int)>, int (int, int)>::operator()(this=0x000000016fdff278, __arg=0x000000016fdff224, __arg=0x000000016fdff220) at function.h:313:10
    frame llvm#5: 0x0000000100003c38 a.out`std::__1::__function::__value_func<int (int, int)>::operator()[abi:se200000](this=0x000000016fdff278, __args=0x000000016fdff224, __args=0x000000016fdff220) const at function.h:430:12
    frame llvm#6: 0x0000000100002038 a.out`std::__1::function<int (int, int)>::operator()(this= Function = foo(int, int) , __arg=1, __arg=1) const at function.h:989:10
    frame llvm#7: 0x0000000100001f64 a.out`main(argc=1, argv=0x000000016fdff4f8) at main.cpp:9:10
    frame llvm#8: 0x0000000183cdf154 dyld`start + 2476
(lldb) 
```

after

```
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x0000000100001f04 a.out`foo(x=1, y=1) at main.cpp:4:10
    frame #1: 0x0000000100003a00 a.out`decltype(std::declval<int (*&)(int, int)>()(std::declval<int>(), std::declval<int>())) std::__1::__invoke[abi:se200000]<int (*&)(int, int), int, int>(__f=0x000000016fdff280, __args=0x000000016fdff224, __args=0x000000016fdff220) at invoke.h:149:25
    frame #2: 0x000000010000399c a.out`int std::__1::__invoke_void_return_wrapper<int, false>::__call[abi:se200000]<int (*&)(int, int), int, int>(__args=0x000000016fdff280, __args=0x000000016fdff224, __args=0x000000016fdff220) at invoke.h:216:12
    frame llvm#6: 0x0000000100002038 a.out`std::__1::function<int (int, int)>::operator()(this= Function = foo(int, int) , __arg=1, __arg=1) const at function.h:989:10
    frame llvm#7: 0x0000000100001f64 a.out`main(argc=1, argv=0x000000016fdff4f8) at main.cpp:9:10
    frame llvm#8: 0x0000000183cdf154 dyld`start + 2476
Note: Some frames were hidden by frame recognizers
```
dkrupp pushed a commit that referenced this pull request Nov 7, 2025
…e exception specification of a function (llvm#90760)

[temp.deduct.general] p6 states:
> At certain points in the template argument deduction process it is
necessary to take a function type that makes use of template parameters
and replace those template parameters with the corresponding template
arguments.
This is done at the beginning of template argument deduction when any
explicitly specified template arguments are substituted into the
function type, and again at the end of template argument deduction when
any template arguments that were deduced or obtained from default
arguments are substituted.

[temp.deduct.general] p7 goes on to say:
> The _deduction substitution loci_ are
> - the function type outside of the _noexcept-specifier_,
> - the explicit-specifier,
> - the template parameter declarations, and
> - the template argument list of a partial specialization
>
> The substitution occurs in all types and expressions that are used in
the deduction substitution loci. [...]

Consider the following:
```cpp
struct A
{
    static constexpr bool x = true;
};

template<typename T, typename U>
void f(T, U) noexcept(T::x); // #1

template<typename T, typename U>
void f(T, U*) noexcept(T::y); // #2

template<>
void f<A>(A, int*) noexcept; // clang currently accepts, GCC and EDG reject
```

Currently, `Sema::SubstituteExplicitTemplateArguments` will substitute
into the _noexcept-specifier_ when deducing template arguments from a
function declaration or when deducing template arguments for taking the
address of a function template (and the substitution is treated as a
SFINAE context). In the above example, `#1` is selected as the primary
template because substitution of the explicit template arguments into
the _noexcept-specifier_ of `#2` failed, which resulted in the candidate
being ignored.

This behavior is incorrect ([temp.deduct.general] note 4 says as much), and
this patch corrects it by deferring all substitution into the
_noexcept-specifier_ until it is instantiated.

As part of the necessary changes to make this patch work, the
instantiation of the exception specification of a function template
specialization when taking the address of a function template is changed
to only occur for the function selected by overload resolution per
[except.spec] p13.1 (as opposed to being instantiated for every candidate).
dkrupp pushed a commit that referenced this pull request Nov 7, 2025
…ined member functions & member function templates (llvm#88963)

Consider the following snippet from the discussion of CWG2847 on the core reflector:
```
template<typename T>
concept C = sizeof(T) <= sizeof(long);

template<typename T>
struct A 
{
    template<typename U>
    void f(U) requires C<U>; // #1, declares a function template 

    void g() requires C<T>; // #2, declares a function

    template<>
    void f(char);  // #3, an explicit specialization of a function template that declares a function
};

template<>
template<typename U>
void A<short>::f(U) requires C<U>; // llvm#4, an explicit specialization of a function template that declares a function template

template<>
template<>
void A<int>::f(int); // llvm#5, an explicit specialization of a function template that declares a function

template<>
void A<long>::g(); // llvm#6, an explicit specialization of a function that declares a function
```

A number of problems exist:
- Clang rejects `llvm#4` because the trailing _requires-clause_ has `U`
substituted with the wrong template parameter depth when
`Sema::AreConstraintExpressionsEqual` is called to determine whether it
matches the trailing _requires-clause_ of the implicitly instantiated
function template.
- Clang rejects `llvm#5` because the function template specialization
instantiated from `A<int>::f` has a trailing _requires-clause_, but `llvm#5`
does not (nor can it have one as it isn't a templated function).
- Clang rejects `llvm#6` for the same reasons it rejects `llvm#5`.

This patch resolves these issues by making the following changes:
- To fix `llvm#4`, `Sema::AreConstraintExpressionsEqual` is passed
`FunctionTemplateDecl`s when comparing the trailing _requires-clauses_
of `llvm#4` and the function template instantiated from `#1`.
- To fix `llvm#5` and `llvm#6`, the trailing _requires-clauses_ are not compared
for explicit specializations that declare functions.

In addition to these changes, `CheckMemberSpecialization` now considers
constraint satisfaction/constraint partial ordering when determining
which member function is specialized by an explicit specialization of a
member function for an implicit instantiation of a class template (we
previously would select the first function that has the same type as the
explicit specialization). With constraints taken under consideration, we
match EDG's behavior for these declarations.
dkrupp pushed a commit that referenced this pull request Nov 7, 2025
In `Driver.cpp` `std::atomic<uint64_t>` is used which may need
libatomic.

Build failure (if that is of interest):
```
[127/135] Linking CXX shared library lib/liblldMachO.so.20.1
ninja: job failed: : && /usr/lib/ccache/bin/clang++-20 -fPIC -Os -fstack-clash-protection -Wformat -Werror=format-security -D_GLIBCXX_ASSERTIONS=1 -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS=1 -D_LIBCPP_ENABLE_HARDENED_MODE=1 -g -O2 -DNDEBUG -g1 -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections  -Wl,--as-needed,-O1,--sort-common -Wl,-z,defs -Wl,-z,nodelete   -Wl,-rpath-link,/home/user/aports/main/lld20/src/lld-20.1.5.src/build/./lib  -Wl,--gc-sections -shared -Wl,-soname,liblldMachO.so.20.1 -o lib/liblldMachO.so.20.1 MachO/CMakeFiles/lldMachO.dir/Arch/ARM64.cpp.o MachO/CMakeFiles/lldMachO.dir/Arch/ARM64Common.cpp.o MachO/CMakeFiles/lldMachO.dir/Arch/ARM64_32.cpp.o MachO/CMakeFiles/lldMachO.dir/Arch/X86_64.cpp.o MachO/CMakeFiles/lldMachO.dir/ConcatOutputSection.cpp.o MachO/CMakeFiles/lldMachO.dir/Driver.cpp.o MachO/CMakeFiles/lldMachO.dir/DriverUtils.cpp.o MachO/CMakeFiles/lldMachO.dir/Dwarf.cpp.o MachO/CMakeFiles/lldMachO.dir/EhFrame.cpp.o MachO/CMakeFiles/lldMachO.dir/ExportTrie.cpp.o MachO/CMakeFiles/lldMachO.dir/ICF.cpp.o MachO/CMakeFiles/lldMachO.dir/InputFiles.cpp.o MachO/CMakeFiles/lldMachO.dir/InputSection.cpp.o MachO/CMakeFiles/lldMachO.dir/LTO.cpp.o MachO/CMakeFiles/lldMachO.dir/MapFile.cpp.o MachO/CMakeFiles/lldMachO.dir/MarkLive.cpp.o MachO/CMakeFiles/lldMachO.dir/ObjC.cpp.o MachO/CMakeFiles/lldMachO.dir/OutputSection.cpp.o MachO/CMakeFiles/lldMachO.dir/OutputSegment.cpp.o MachO/CMakeFiles/lldMachO.dir/Relocations.cpp.o MachO/CMakeFiles/lldMachO.dir/BPSectionOrderer.cpp.o MachO/CMakeFiles/lldMachO.dir/SectionPriorities.cpp.o MachO/CMakeFiles/lldMachO.dir/Sections.cpp.o MachO/CMakeFiles/lldMachO.dir/SymbolTable.cpp.o MachO/CMakeFiles/lldMachO.dir/Symbols.cpp.o MachO/CMakeFiles/lldMachO.dir/SyntheticSections.cpp.o MachO/CMakeFiles/lldMachO.dir/Target.cpp.o MachO/CMakeFiles/lldMachO.dir/UnwindInfoSection.cpp.o MachO/CMakeFiles/lldMachO.dir/Writer.cpp.o -L/usr/lib/llvm20/lib -Wl,-rpath,"\$ORIGIN/../lib:/usr/lib/llvm20/lib:/home/user/aports/main/lld20/src/lld-20.1.5.src/build/lib:"  lib/liblldCommon.so.20.1  /usr/lib/llvm20/lib/libLLVM.so.20.1 && :
/usr/lib/gcc/powerpc-alpine-linux-musl/14.3.0/../../../../powerpc-alpine-linux-musl/bin/ld: MachO/CMakeFiles/lldMachO.dir/Driver.cpp.o: in function `handleExplicitExports()':
/usr/lib/gcc/powerpc-alpine-linux-musl/14.3.0/../../../../include/c++/14.3.0/bits/atomic_base.h:501:(.text._ZL21handleExplicitExportsv+0xb8): undefined reference to `__atomic_load_8'
/usr/lib/gcc/powerpc-alpine-linux-musl/14.3.0/../../../../powerpc-alpine-linux-musl/bin/ld: /usr/lib/gcc/powerpc-alpine-linux-musl/14.3.0/../../../../include/c++/14.3.0/bits/atomic_base.h:501:(.text._ZL21handleExplicitExportsv+0x180): undefined reference to `__atomic_load_8'
/usr/lib/gcc/powerpc-alpine-linux-musl/14.3.0/../../../../powerpc-alpine-linux-musl/bin/ld: MachO/CMakeFiles/lldMachO.dir/Driver.cpp.o: in function `void llvm::function_ref<void (unsigned int)>::callback_fn<llvm::parallelForEach<lld::macho::Symbol* const*, handleExplicitExports()::$_0>(lld::macho::Symbol* const*, lld::macho::Symbol* const*, handleExplicitExports()::$_0)::{lambda(unsigned int)#1}>(int, unsigned int)':
/usr/lib/gcc/powerpc-alpine-linux-musl/14.3.0/../../../../include/c++/14.3.0/bits/atomic_base.h:631:(.text._ZN4llvm12function_refIFvjEE11callback_fnIZNS_15parallelForEachIPKPN3lld5macho6SymbolEZL21handleExplicitExportsvE3$_0EEvT_SC_T0_EUljE_EEvij+0xd4): undefined reference to `__atomic_fetch_add_8'
clang++-20: error: linker command failed with exit code 1 (use -v to see invocation)
```

CC @int3 @gkmhub @smeenai

Similar to
llvm@f0b451c
dkrupp pushed a commit that referenced this pull request Nov 7, 2025
llvm#164955 has a use-after-scope
(https://lab.llvm.org/buildbot/#/builders/169/builds/16454):

```
==mlir-opt==3940651==ERROR: AddressSanitizer: stack-use-after-scope on address 0x6e1f6ba5c878 at pc 0x6336b214912a bp 0x7ffe607f1670 sp 0x7ffe607f1668
READ of size 4 at 0x6e1f6ba5c878 thread T0
    #0 0x6336b2149129 in size /home/b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:80:32
    #1 0x6336b2149129 in operator[] /home/b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:299:5
    #2 0x6336b2149129 in populateBoundsForShapedValueDim /home/b/sanitizer-x86_64-linux-fast/build/llvm-project/mlir/lib/Dialect/MemRef/IR/ValueBoundsOpInterfaceImpl.cpp:113:43
...
```

This patch attempts to fix-forward by stack-allocating reassocIndices,
instead of taking a reference to a return value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants