1 / 133

A scrupulous code review - 15 bugs in C code

A close look at 15 problems one can find when reviewing C code.<br>Offers code examples.<br>Covers indexed loops, tainted data, copy and paste errors, problems with comparisons, exceptions, etc.<br>You can use static code analysis tools to make code review easier. Code analyzers find errors and potential vulnerabilities in code, while saving the developers' time and the companies' money.<br>Manual code review is expensive - a group of programmers get together regularly to review the code.<br>One can run static analysis tools regularly to find mistakes and vulnerabilities early.

pvsstudio
Download Presentation

A scrupulous code review - 15 bugs in C code

An Image/Link below is provided (as is) to download presentation Download Policy: Content on the Website is provided to you AS IS for your information and personal use and may not be sold / licensed / shared on other websites without getting consent from its author. Content is provided to you AS IS for your information and personal use only. Download presentation by click this link. While downloading, if for some reason you are not able to download a presentation, the publisher may have deleted the file from their server. During download, if you can't get a presentation, the file might be deleted by the publisher.

E N D

Presentation Transcript

  1. A scrupulous code review – 15 bugs in C++ code PhillipKhandeliants khandeliants@viva64.com

  2. Over 5 years with the PVS-Studio team Team lead at the C++ analyzer development team Microsoft Certified Professional, C# Talk on modern C++ Live by the C++ ISO standard

  3. Intro: the code review • We all do code reviews • Who doesn't admit this – does it twice as often

  4. 1. 'Auto'matic coding

  5. void foo(const std::vector<....> &vec) • { • .... • for (auto i = 0; i < vec.size(); ++i) • { • // do some magic with vec[i] • .... • } • .... • }

  6. void foo(const std::vector<....> &vec) • { • .... • for (inti = 0; i < vec.size(); ++i) // 64-bit problems :) • { • // do some magic with vec[i] • .... • } • .... • }

  7. void foo(const std::vector<....> &vec) • { • .... • for (size_ti = 0; i < vec.size(); ++i) // ok • { • // do some magic with vec[i] • .... • } • .... • }

  8. void foo(const std::vector<....> &vec) • { • .... • for (std::vector<....>::size_typei = 0; i < vec.size(); ++i) • { • // do some magic with vec[i] • .... • } • .... • }

  9. void foo(const std::vector<....> &vec) • { • .... • for (auto i = 0uLL; i < vec.size(); ++i) // don't do that on • // 128-bit processors • { • // do some magic with vec[i] • .... • } • .... • }

  10. void foo(const std::vector<....> &vec) • { • .... • for (auto i = 0uLLL; i < vec.size(); ++i) // ok since C++7d • { • // do some magic with vec[i] • .... • } • .... • }

  11. void foo(const std::vector<....> &vec) • { • .... • for (auto i = 0uLLL; i < vec.size(); ++i) // ok since C++7d • { • // do some magic with vec[i] • .... • } • .... • }

  12. 2. Reference! I said reference! Perfection!

  13. template <typename T> • int ColumnDecimal<T>::compareAt(size_t n, size_t m, • const IColumn & rhs_, int) const • { • auto other = static_cast<const Self &>(rhs_); • const T &a = data[n]; • const T &b = other.data[m]; • return decimalLess<T>(b, a, other.scale, scale) • ? 1 • : (decimalLess<T>(a, b, scale, other.scale) ? -1 : 0); • }

  14. template <typename T> • int ColumnDecimal<T>::compareAt(size_t n, size_t m, • const IColumn & rhs_, int) const • { • auto other = static_cast<const Self &>(rhs_); • const T &a = data[n]; • const T &b = other.data[m]; • return decimalLess<T>(b, a, other.scale, scale) • ? 1 • : (decimalLess<T>(a, b, scale, other.scale) ? -1 : 0); • }

  15. template <typename T> • int ColumnDecimal<T>::compareAt(size_t n, size_t m, • const IColumn & rhs_, int) const • { • auto &other = static_cast<const Self &>(rhs_); • const T &a = data[n]; • const T &b = other.data[m]; • return decimalLess<T>(b, a, other.scale, scale) • ? 1 • : (decimalLess<T>(a, b, scale, other.scale) ? -1 : 0); • }

  16. template <typename T> • int ColumnDecimal<T>::compareAt(size_t n, size_t m, • const IColumn & rhs_, int) const • { • decltype(auto) other = static_cast<const Self &>(rhs_); • const T &a = data[n]; • const T &b = other.data[m]; • return decimalLess<T>(b, a, other.scale, scale) • ? 1 • : (decimalLess<T>(a, b, scale, other.scale) ? -1 : 0); • }

  17. 3. Vector-vector, *pChar++

  18. void vector32_inc(std::vector<uint32_t> &v) • { • for (size_ti = 0; i < v.size(); i++) • { • v[i]++; • } • }

  19. void vector32_inc(std::vector<uint32_t> &v) • { • for (size_ti = 0; i < v.size(); i++) • { • v[i]++; • } • } • void vector8_inc(std::vector<uint8_t> &v) • { • for (size_ti = 0; i < v.size(); i++) • { • v[i]++; • } • }

  20. Let's benchmark :)

  21. vector8_inc(std::vector<uint8_t> &): • mov rdx, QWORD PTR [rdi] ; it = begin() • cmprdx, QWORD PTR [rdi+8] ; if (it == end()) • je .L1 ; return • xoreax, eax; i = 0 • .L3: ; do { • add BYTE PTR [rdx+rax], 1 ; ++(*(it + i)) • mov rdx, QWORD PTR [rdi] ; it = begin() • add rax, 1 ; ++i • mov rcx, QWORD PTR [rdi+8] ; end = end() • sub rcx, rdx; end = end - it • cmprax, rcx • jb .L3 ; } while (i < end) • .L1: • ret vector32_inc(std::vector<uint32_t> &): mov rax, QWORD PTR [rdi] ; it = begin() mov rdx, QWORD PTR [rdi+8] ; end = end() sub rdx, rax; end = end - it mov rcx, rdx; size in bytes shrrcx, 2 ; size in elements je .L1 ; if (size == 0) ; return add rdx, rax; end = end + it .L3: ; do { add DWORD PTR [rax], 1 ; ++(*it) add rax, 4 ; ++it cmprax, rdx jne .L3 ; } while (it != end) .L1: ret

  22. vector8_inc(std::vector<uint8_t> &): • mov rdx, QWORD PTR [rdi] ; it = begin() • cmprdx, QWORD PTR [rdi+8] ; if (it == end()) • je .L1 ; return • xoreax, eax; i = 0 • .L3: ; do { • add BYTE PTR [rdx+rax], 1 ; ++(*(it + i)) • mov rdx, QWORD PTR [rdi] ; it = begin() • add rax, 1 ; ++i • mov rcx, QWORD PTR [rdi+8] ; end = end() • sub rcx, rdx; end = end - it • cmprax, rcx • jb .L3 ; } while (i < end) • .L1: • ret vector32_inc(std::vector<uint32_t> &): mov rax, QWORD PTR [rdi] ; it = begin() mov rdx, QWORD PTR [rdi+8] ; end = end() sub rdx, rax; end = end - it mov rcx, rdx; size in bytes shrrcx, 2 ; size in elements je .L1 ; if (size == 0) ; return add rdx, rax; end = end + it .L3: ; do { add DWORD PTR [rax], 1 ; ++(*it) add rax, 4 ; ++it cmprax, rdx jne .L3 ; } while (it != end) .L1: ret

  23. void vector8_inc(std::vector<uint8_t> &v) • { • for (size_ti = 0; i < v.size(); i++) • { • v[i]++; • } • }

  24. void vector8_inc(std::vector<uint8_t> &v) • { • for (size_ti = 0; i < v.size(); i++) • { • v[i]++; • } • } • void vector8_inc(std::vector<uint8_t> &v) • { • auto it = v.begin(); • const auto end = v.end(); • for (; it != end; ++it) • { • ++(*it); • } • }

  25. vector8_inc(std::vector<uint8_t> &): • mov rax, QWORD PTR [rdi] ; it = begin() • mov rdx, QWORD PTR [rdi+8] ; end = end() • cmprax, rdx; if (it == end) • je .L1 ; return • .L3: ; do { • add BYTE PTR [rax], 1; ++(*it) • add rax, 1 ; ++it • cmprax, rdx; }while (it != end) • .L1: • ret

  26. vector8_inc(std::vector<uint8_t> &): • mov rax, QWORD PTR [rdi] ; it = begin() • mov rdx, QWORD PTR [rdi+8] ; end = end() • cmprax, rdx; if (it == end) • je .L1 ; return • .L3: ; do { • add BYTE PTR [rax], 1; ++(*it) • add rax, 1 ; ++it • cmprax, rdx; }while (it != end) • .L1: • ret

  27. Let's make benchmarks great again!

  28. void vector8_inc(std::vector<uint8_t> &v) • { • auto it = v.begin(); • const auto end = v.end(); • for (; it != end; ++it) • { • ++(*it); • } • }

  29. void vector8_inc(std::vector<uint8_t> &v) • { • auto it = v.begin(); • const auto end = v.end(); • for (; it != end; ++it) • { • ++(*it); • } • } • void vector8_inc(std::vector<uint8_t> &v) • { • for (auto &elem : v) • { • ++elem; • } • }

  30. 4. Security? Security!

  31. #include <cstring> • #include <memory> • void InputPassword(char *pswd); • void ProcessPassword(const char *pswd); • #define MAX_PASSWORD_LEN .... • void Foo() • { • char password[MAX_PASSWORD_LEN]; • InputPassword(password); • ProcessPassword(password); • memset(password, 0, sizeof(password)); • }

  32. #include <cstring> • #include <memory> • void InputPassword(char *pswd); • void ProcessPassword(const char *pswd); • #define MAX_PASSWORD_LEN .... • void Foo() • { • char password[MAX_PASSWORD_LEN]; • InputPassword(password); • ProcessPassword(password); • memset(password, 0, sizeof(password)); • }

  33. #include <cstring> • #include <memory> • void InputPassword(char *pswd); • void ProcessPassword(const char *pswd); • #define MAX_PASSWORD_LEN .... • void Foo() • { • char *password = new char[MAX_PASSWORD_LEN]; • InputPassword(password); • ProcessPassword(password); • memset(password, 0, MAX_PASSWORD_LEN); • delete[] password; • }

  34. voidtds_answer_challenge(....) • { • #define MAX_PW_SZ 14 • .... • if (ntlm_v == 1) { • .... • /* with security is best be pedantic */ • memset(hash, 0, sizeof(hash)); • memset(passwd_buf, 0, sizeof(passwd_buf)); • memset(ntlm2_challenge, 0, sizeof(ntlm2_challenge)); • } else { • .... • } • }

  35. typedef struct tds_answer • { • unsigned char lm_resp[24]; • unsigned char nt_resp[24]; • } TDSANSWER; • static TDSRETtds7_send_auth(....) • { • size_tcurrent_pos; • TDSANSWER answer; • .... • /* for security reason clear structure */ • memset(&answer, 0, sizeof(TDSANSWER)); • return tds_flush_packet(tds); • }

  36. char* crypt_md5(const char* pw, const char* salt) • { • unsigned char final[MD5_SIZE]; • .... • /* Don't leave anything around in vm they could use. */ • memset(final, 0, sizeof final); • return passwd; • }

  37. void MD4Engine::transform (UInt32 state[4], • const unsigned char block[64]) • { • UInt32 a = state[0], b = state[1], • c = state[2], d = state[3], x[16]; • decode(x, block, 64); • .... • /* Zeroize sensitive information. */ • std::memset(x, 0, sizeof(x)); • }

  38. char* px_crypt_md5(const char *pw, const char *salt, • char *passwd, unsigned dstlen) • { • .... • unsigned char final[MD5_SIZE]; • .... • /* Don't leave anything around in vm they could use. */ • memset(final, 0, sizeof final); • .... • }

  39. Ways to fix • Custom safe_memset + disabled LTO/WPO • Access a non-volatile object through a volatile pointer • Call memset through a volatile function pointer • Volatile assembly code • Memset + memory barrier • Disable compiler optimizations (-fno-builtin-memset) • C11: memset_s • Windows: RtlSecureZeroMemory • FreeBSD & OpenBSD: explicit_bzero • Linux Kernel: memzero_explicit

  40. 5. Dirty thoughts data

  41. static const char* basic_gets(int *cnt) • { • .... • int c = getchar(); • if (c < 0) • { • if ( fgets(command_buf, sizeof(command_buf) - 1, stdin) • != command_buf) • { • break; • } • /* remove endline */ • command_buf[strlen(command_buf)-1] = '\0'; • break; • } • .... • }

  42. static const char* basic_gets(int *cnt) • { • .... • int c = getchar(); • if (c < 0) • { • if ( fgets(command_buf, sizeof(command_buf) - 1, stdin) • != command_buf) • { • break; • } • /* remove endline */ • command_buf[strlen(command_buf)-1] = '\0'; • break; • } • .... • }

  43. int main (int argc, char *argv[]) • { • .... • else if (fgets(readbuf, BUFSIZ, stdin) == NULL) • { • if (feof (stdin)) • break; • error (EXIT_FAILURE, errno, _("input error")); • } • if (readbuf[strlen(readbuf) - 1] == '\n') • readbuf[strlen(readbuf) - 1] = '\0'; • .... • }

  44. int main (int argc, char *argv[]) • { • .... • else if (fgets(readbuf, BUFSIZ, stdin) == NULL) • { • if (feof (stdin)) • break; • error (EXIT_FAILURE, errno, _("input error")); • } • if (readbuf[strlen(readbuf) - 1] == '\n') • readbuf[strlen(readbuf) - 1] = '\0'; • .... • } CVE-2015-8948

  45. int main (int argc, char *argv[]) • { • .... • else if (getline(&line, &linelen, stdin) == -1) • { • if (feof (stdin)) • break; • error (EXIT_FAILURE, errno, _("input error")); • } • if (line[strlen(line) - 1] == '\n') • line[strlen(line) - 1] = '\0'; • .... • }

  46. int main (int argc, char *argv[]) • { • .... • else if (getline(&line, &linelen, stdin) == -1) • { • if (feof (stdin)) • break; • error (EXIT_FAILURE, errno, _("input error")); • } • if (line[strlen(line) - 1] == '\n') • line[strlen(line) - 1] = '\0'; • .... • } CVE-2016-6262

More Related