[cpp] CWE-787: query to detect unsigned integer to signed integer conversions used in pointer arithmetics

JordyZomer commented 3 years ago

Query

Relevant PR: https://github.com/github/codeql/pull/6409

CVE ID(s)

Report

what we do here is obtain a FunctionCall to a Function with any parameter that requires a signed integer. Following that, we look for any function calls that provide an unsigned number to this function despite the fact that it expects a signed integer. After that, we will use the DataFlow library to “taint track” any use of this argument in pointer arithmetic. Running this query on the Linux kernel database successfully identifies the Sequoia vulnerability as well as hundreds of additional instances that may be vulnerable.

[x] Are you planning to discuss this vulnerability submission publicly? (Blog Post, social networks, etc). We would love to have you spread the word about the good work you are doing

This is already done! :)

https://pwning.systems/posts/sequoia-variant-analysis/ https://twitter.com/pwningsystems/status/1422599329324351491

Result(s)

So far, these are the results; there will be more to come as I continue to write patches. If you'd want to keep up with the development, go to https://pwning.systems/posts/sequoia-variant-analysis/ .

ghsecuritylab commented 3 years ago

Your submission is now in status Generate Query Results.

For information, the evaluation workflow is the following: SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

JordyZomer commented 3 years ago

Hello there! Is there any new information? By now, I'm assuming the query results would have been finalized :D

ghsecuritylab commented 3 years ago

Your submission is now in status FP Check.

For information, the evaluation workflow is the following: SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

JarLob commented 3 years ago

Hi, thanks for pinging. It was stuck in the Generate Query Results stage.

JordyZomer commented 3 years ago

Hey there @JarLob ! I'm pleased to say that the problem has been merged, albeit with a slight delay on my part (due to the holidays) :D!

JarLob commented 3 years ago

Hi @JordyZomer, the query returned a significant number of results: like 25% of all c++ projects has one or another related issue. I'm looking if potential false positives can be reduced. One of the patterns is:

std::vector<std::string> a;
size_t idx;
a.erase(a.begin() + idx);

I'm not sure if this is a FP or not. What is your take on it?

JordyZomer commented 3 years ago

@JarLob A test indicates that it could be a positive. With reference to the following code:

#include <iostream>
#include <limits.h>
#include <string>
#include <vector>

int main() {
  std::vector<std::string> a;
  size_t idx = SIZE_MAX;
  a.erase(a.begin() + idx);
  return 0;
}

Once executed, it will segfault due to reading from an unknown address, thus I'm assuming it does some pointer math:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==2181785==ERROR: AddressSanitizer: SEGV on unknown address 0xffffffffffffffe0 (pc 0x7f4e7a21f5c4 bp 0x7ffc23e55190 sp 0x7ffc23e55178 T0)
==2181785==The signal is caused by a READ memory access.
    #0 0x7f4e7a21f5c4 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() (/lib/x86_64-linux-gnu/libstdc++.so.6+0x1435c4)
    #1 0x4cb388 in void __gnu_cxx::new_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::destroy<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) (/root/test_vector+0x4cb388)
    #2 0x4ca89f in void std::allocator_traits<std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::destroy<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) (/root/test_vector+0x4ca89f)
    #3 0x4c9f1e in std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_M_erase(__gnu_cxx::__normal_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >) (/root/test_vector+0x4c9f1e)
    #4 0x4c9038 in std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::erase(__gnu_cxx::__normal_iterator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >) (/root/test_vector+0x4c9038)
    #5 0x4c8bb6 in main (/root/test_vector+0x4c8bb6)
    #6 0x7f4e79d7bcb1 in __libc_start_main csu/../csu/libc-start.c:314:16
    #7 0x41c39d in _start (/root/test_vector+0x41c39d)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-gnu/libstdc++.so.6+0x1435c4) in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string()
==2181785==ABORTING

ghsecuritylab commented 3 years ago

Your submission is now in status CodeQL review.

For information, the evaluation workflow is the following: SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

JarLob commented 3 years ago

Thanks. I also looked into the MS implementation at least and see it is implemented as ptr_diff. There are some FPs when the argument is checked before like if(value <= LONG_MAX). But overall looks good.

JordyZomer commented 3 years ago

Thank you @JarLob ! Glad to hear that there's only a few FP's :)

ghsecuritylab commented 3 years ago

Your submission is now in status SecLab finalize.

For information, the evaluation workflow is the following: SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

ghsecuritylab commented 3 years ago

Your submission is now in status Pay.

For information, the evaluation workflow is the following: SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

xcorail commented 3 years ago

Created Hackerone report 1378946 for bounty 344348 : [418] [cpp] CWE-787: query to detect unsigned integer to signed integer conversions used in pointer arithmetics

ghsecuritylab commented 3 years ago

Your submission is now in status Closed.

For information, the evaluation workflow is the following: SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

github / securitylab