Fix mismatched CFI type signatures with type generalization |
||
Issue descriptionC, and less frequently C++, code uses function pointer casts as a form of abstraction for pointer types. For example, the qsort() comparison function type signature takes a pointer to two const void*s but the implementations are likely to implement the function using two const specific_type*s and cast the comparison function pointer given to qsort. This causes CFI-icall failures because the called function does not match the type signature of the function pointer it was called with. In order to accommodate the use of this pattern (it's too frequent and deeply-embedded in some libraries like boringssl/freetype to hope to get rid of it without re-writing them entirely) the -fsanitize-cfi-icall-generalize-pointers flag 'generalizes' all pointer types used for cfi-icall comparisons to 'void*' with the qualifiers of the original pointer. So for example, calling a function pointer with type signature void(*)(void*) would match functions with types void(void*), void(int*), void(long**) but not void(int) or void(const void*). This issue tracks removing all of the blacklisted CFI-icall failures that can be fixed by simply applying type generalization.
,
Nov 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/521fd17a903cdae78c797770348320d99064b0cc commit 521fd17a903cdae78c797770348320d99064b0cc Author: Vlad Tsyrklevich <vtsyrklevich@chromium.org> Date: Tue Nov 21 23:34:40 2017 [CFI] Enable cfi-icall type generalization Control Flow Integrity [1] indirect call checking verifies that function pointers only call valid functions with a matching type signature. This condition can be too strict, a common form of 'abstraction' relies on function pointers being cast to generalize argument pointer types to void*. For example, qsort() accepts two const void*s but the implementations are likely to implement the comparison function using pointers to the specific type being sorted. This function relaxes cfi-icall type checking for code that uses this pattern by using the new -fsanitize-cfi-icall-generalize-pointers argument. It considers all pointer types equal as long as their qualifiers match. TBR=drott@chromium.org,rsleevi@chromium.org,piman@chromium.org Bug: 785442 Change-Id: Ic9487908b6372898f031502c84284d008a3fdec1 Reviewed-on: https://chromium-review.googlesource.com/777555 Commit-Queue: Peter Collingbourne <pcc@chromium.org> Reviewed-by: Ryan Sleevi <rsleevi@chromium.org> Reviewed-by: Dominik Röttsches <drott@chromium.org> Reviewed-by: David Benjamin <davidben@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Peter Collingbourne <pcc@chromium.org> Reviewed-by: Johann Koenig <johannkoenig@google.com> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#518446} [modify] https://crrev.com/521fd17a903cdae78c797770348320d99064b0cc/build/config/sanitizers/BUILD.gn [modify] https://crrev.com/521fd17a903cdae78c797770348320d99064b0cc/third_party/boringssl/BUILD.gn [modify] https://crrev.com/521fd17a903cdae78c797770348320d99064b0cc/third_party/freetype/BUILD.gn [modify] https://crrev.com/521fd17a903cdae78c797770348320d99064b0cc/third_party/mesa/BUILD.gn [modify] https://crrev.com/521fd17a903cdae78c797770348320d99064b0cc/third_party/yasm/BUILD.gn [modify] https://crrev.com/521fd17a903cdae78c797770348320d99064b0cc/tools/cfi/blacklist.txt
,
Nov 28 2017
FYI your fix for libvpx: https://chromium-review.googlesource.com/c/webm/libvpx/+/780144 has been rolled: https://chromium-review.googlesource.com/792112
,
Mar 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fc6ccf6d00008834d326125c873216621e468578 commit fc6ccf6d00008834d326125c873216621e468578 Author: Vlad Tsyrklevich <vtsyrklevich@chromium.org> Date: Tue Mar 13 22:30:26 2018 [CFI] Enable generalized cfi-icall for sqlite Upstream cfi-icall fixes to sqlite have landed such that it is now type generalization cfi-icall safe. Bug: 785442 Change-Id: I162aa0e13eba9f11b06d8edeb452c6015f1f4e99 Reviewed-on: https://chromium-review.googlesource.com/938494 Reviewed-by: Peter Collingbourne <pcc@chromium.org> Reviewed-by: Victor Costan <pwnall@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#542939} [modify] https://crrev.com/fc6ccf6d00008834d326125c873216621e468578/third_party/sqlite/BUILD.gn [modify] https://crrev.com/fc6ccf6d00008834d326125c873216621e468578/tools/cfi/blacklist.txt
,
Mar 14 2018
3rd party libraries fixable by type generalization have all been fixed.
,
Oct 1
The following revision refers to this bug: https://boringssl.googlesource.com/boringssl/+/217bfd3c96b04c6f1001924d202f4181a617ea6b commit 217bfd3c96b04c6f1001924d202f4181a617ea6b Author: David Benjamin <davidben@google.com> Date: Mon Oct 01 17:34:44 2018 Fix undefined function pointer casts in IMPLEMENT_PEM_*. While it is okay to cast function pointers into different types for generic storage, the pointer must be cast back to the exact same type when calling. In particular, although C libraries do this sort of thing all the time, calling a T* d2i function as a void* d2i function is undefined: If the function is defined with a type that is not compatible with the type (of the expression) pointed to by the expression that denotes the called function, the behavior is undefined Fix some instances in the PEM/ASN1 wrapper functions. Synthesize helper functions instead. This CL just addresses the function pointer issues. The inherited legacy OpenSSL ASN.1 code is still full other questionable data pointer dances that will be much more difficult to excise. Continuing to exise that code altogether (it is already unshipped from Cronet and unshipped from Chrome but for WebRTC) is probably a better tack there. This removes one (of many many) places where we require -fsanitize-cfi-icall-generalize-pointers. Bug: chromium:785442 Change-Id: Id8056ead6ef471f0fdf263bb50dc659da500e8ce Reviewed-on: https://boringssl-review.googlesource.com/32105 Reviewed-by: Adam Langley <agl@google.com> Reviewed-by: Adam Langley <alangley@gmail.com> [modify] https://crrev.com/217bfd3c96b04c6f1001924d202f4181a617ea6b/include/openssl/pem.h
,
Oct 1
The following revision refers to this bug: https://boringssl.googlesource.com/boringssl/+/419144adce049b5341bd94d355c52d099eac56e3 commit 419144adce049b5341bd94d355c52d099eac56e3 Author: David Benjamin <davidben@google.com> Date: Mon Oct 01 17:34:53 2018 Fix undefined function pointer casts in {d2i,i2d}_Foo_{bio,fp} Lacking C++, this instead adds a mess of macros. With this done, all the function-pointer-munging "_of" macros in asn1.h can also be removed. Update-Note: A number of *really* old and unused ASN.1 macros were removed. Bug: chromium:785442 Change-Id: Iab260d114c7d8cdf0429759e714d91ce3f3c04b2 Reviewed-on: https://boringssl-review.googlesource.com/32106 Reviewed-by: Adam Langley <agl@google.com> Reviewed-by: Adam Langley <alangley@gmail.com> [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/crypto/pkcs7/pkcs7_x509.c [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/crypto/x509v3/v3_genn.c [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/crypto/asn1/a_dup.c [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/crypto/bio/bio.c [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/ssl/ssl_x509.cc [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/crypto/asn1/a_d2i_fp.c [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/crypto/x509/x_all.c [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/crypto/pkcs8/pkcs8_x509.c [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/crypto/asn1/a_i2d_fp.c [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/include/openssl/type_check.h [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/include/openssl/bio.h [modify] https://crrev.com/419144adce049b5341bd94d355c52d099eac56e3/include/openssl/asn1.h
,
Oct 1
The following revision refers to this bug: https://boringssl.googlesource.com/boringssl/+/792c1dc43eb888470b2ecb162be2acc5e0e1d61b commit 792c1dc43eb888470b2ecb162be2acc5e0e1d61b Author: David Benjamin <davidben@google.com> Date: Mon Oct 01 17:35:10 2018 Rewrite PEM_X509_INFO_read_bio. This fixes: - Undefined function pointer casts. - Missing X509_INFO_new malloc failure checks. - Pointless (int) cast on strlen. - Missing ERR_GET_LIB in PEM_R_NO_START_LINE check. - Broken error-handling if passing in an existing stack and we hit a syntax error. Bug: chromium:785442 Change-Id: I8be3523b0f13bdb3745938af9740d491486f8bf1 Reviewed-on: https://boringssl-review.googlesource.com/32109 Reviewed-by: Adam Langley <agl@google.com> [modify] https://crrev.com/792c1dc43eb888470b2ecb162be2acc5e0e1d61b/crypto/x509/x509_test.cc [modify] https://crrev.com/792c1dc43eb888470b2ecb162be2acc5e0e1d61b/include/openssl/asn1.h [modify] https://crrev.com/792c1dc43eb888470b2ecb162be2acc5e0e1d61b/crypto/pem/pem_info.c
,
Oct 1
The following revision refers to this bug: https://boringssl.googlesource.com/boringssl/+/fb4e2e0f0c6547d29a53a48d7f6f131cbb936c1e commit fb4e2e0f0c6547d29a53a48d7f6f131cbb936c1e Author: David Benjamin <davidben@google.com> Date: Mon Oct 01 20:04:07 2018 Fix undefined casts in sk_*_pop_free and sk_*_deep_copy. Unfortunately, some projects are calling into sk_pop_free directly, so we must leave a compatibility version around for now. Bug: chromium:785442 Change-Id: I1577fce6f23af02114f7e9f7bf2b14e9d22fa9ae Reviewed-on: https://boringssl-review.googlesource.com/32113 Commit-Queue: David Benjamin <davidben@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Reviewed-by: Adam Langley <agl@google.com> [modify] https://crrev.com/fb4e2e0f0c6547d29a53a48d7f6f131cbb936c1e/crypto/stack/stack.c [modify] https://crrev.com/fb4e2e0f0c6547d29a53a48d7f6f131cbb936c1e/include/openssl/stack.h
,
Oct 1
The following revision refers to this bug: https://boringssl.googlesource.com/boringssl/+/52483994c8f4e2861a071c20d0342970709d60a1 commit 52483994c8f4e2861a071c20d0342970709d60a1 Author: David Benjamin <davidben@google.com> Date: Mon Oct 01 20:25:15 2018 Mostly fix undefined casts around STACK_OF's comparator. The calls to qsort and bsearch are still invalid, but not avoidable without reimplementing them. Fortunately, they cross libraries, so CFI does not object. With that, all that's left is LHASH! Bug: chromium:785442 Change-Id: I6d29f60fac5cde1f7870d7cc515346e55b98315b Reviewed-on: https://boringssl-review.googlesource.com/32114 Commit-Queue: David Benjamin <davidben@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Reviewed-by: Adam Langley <agl@google.com> [modify] https://crrev.com/52483994c8f4e2861a071c20d0342970709d60a1/crypto/stack/stack.c [modify] https://crrev.com/52483994c8f4e2861a071c20d0342970709d60a1/include/openssl/stack.h
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6bd7dab6856e45a31b5b234b79fcb6ddfc56754f commit 6bd7dab6856e45a31b5b234b79fcb6ddfc56754f Author: David Benjamin <davidben@chromium.org> Date: Tue Oct 02 21:47:18 2018 Roll src/third_party/boringssl/src 13fd62744..ce00828c8 https://boringssl.googlesource.com/boringssl/+log/13fd627449cefbae7576d4b145cb24fac303fc7d..ce00828c89df3d4c40de7d715b1a032eb03c525c The following commits have Chromium bugs associated: 9e97c022e Bring Mac and iOS builders back to the CQ. 7c3ce519e Actually disable RandTest.Fork on iOS. 52483994c Mostly fix undefined casts around STACK_OF's comparator. fb4e2e0f0 Fix undefined casts in sk_*_pop_free and sk_*_deep_copy. cbc3e076f Take iOS builders out of the CQ rotation too. 792c1dc43 Rewrite PEM_X509_INFO_read_bio. 419144adc Fix undefined function pointer casts in {d2i,i2d}_Foo_{bio,fp} 217bfd3c9 Fix undefined function pointer casts in IMPLEMENT_PEM_*. I've also updated the roll_boringssl.py script to print the above message and the below bug list, so we don't need to manually attach things to rolls. Bug: 785442 , 888687 , 890115, 890351 Change-Id: If08496fcff132e71052a89c60aefb609d28ad3c7 Reviewed-on: https://chromium-review.googlesource.com/c/1257561 Commit-Queue: Steven Valdez <svaldez@chromium.org> Reviewed-by: Steven Valdez <svaldez@chromium.org> Cr-Commit-Position: refs/heads/master@{#595994} [modify] https://crrev.com/6bd7dab6856e45a31b5b234b79fcb6ddfc56754f/DEPS [modify] https://crrev.com/6bd7dab6856e45a31b5b234b79fcb6ddfc56754f/third_party/boringssl/roll_boringssl.py
,
Oct 15
The following revision refers to this bug: https://boringssl.googlesource.com/boringssl/+/b68b8322384a536d6d4ee9a8d386c208915ab89a commit b68b8322384a536d6d4ee9a8d386c208915ab89a Author: David Benjamin <davidben@google.com> Date: Mon Oct 15 23:53:24 2018 Fix undefined function pointer casts in LHASH. Bug: chromium:785442 Change-Id: I516e42684b913dc0de778dd9134f1ca108c04dfc Reviewed-on: https://boringssl-review.googlesource.com/c/32120 Commit-Queue: David Benjamin <davidben@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Reviewed-by: Adam Langley <agl@google.com> [modify] https://crrev.com/b68b8322384a536d6d4ee9a8d386c208915ab89a/crypto/lhash/lhash.c [modify] https://crrev.com/b68b8322384a536d6d4ee9a8d386c208915ab89a/include/openssl/lhash.h
,
Oct 15
The following revision refers to this bug: https://boringssl.googlesource.com/boringssl/+/6650898e099b1b978954d57899297989c635a81c commit 6650898e099b1b978954d57899297989c635a81c Author: David Benjamin <davidben@google.com> Date: Mon Oct 15 23:54:44 2018 Remove -fsanitize-cfi-icall-generalize-pointers. Bug: chromium:785442 Change-Id: Ia073fcae716541bc9d008e3e2148e9f0ac30e637 Reviewed-on: https://boringssl-review.googlesource.com/c/32121 Commit-Queue: David Benjamin <davidben@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Reviewed-by: Adam Langley <agl@google.com> [modify] https://crrev.com/6650898e099b1b978954d57899297989c635a81c/CMakeLists.txt
,
Oct 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a3d7cb29b20a1b5278e83e1a00f802a8de60cb27 commit a3d7cb29b20a1b5278e83e1a00f802a8de60cb27 Author: David Benjamin <davidben@chromium.org> Date: Thu Oct 18 19:48:01 2018 Roll src/third_party/boringssl/src 2d98d49cf..dd412c428 https://boringssl.googlesource.com/boringssl/+log/2d98d49cf712ca7dc6f4b23b9c5f5542385d8dbe..dd412c428ad7c2a60ae4709dfbad6301e499dcb8 The following commits have Chromium bugs associated: 6650898e0 Remove -fsanitize-cfi-icall-generalize-pointers. b68b83223 Fix undefined function pointer casts in LHASH. Bug: 785442 Change-Id: I8a7cdc34cf9c6d9991452eaed6156361ee8c0bab Reviewed-on: https://chromium-review.googlesource.com/c/1289159 Commit-Queue: Steven Valdez <svaldez@chromium.org> Reviewed-by: Steven Valdez <svaldez@chromium.org> Cr-Commit-Position: refs/heads/master@{#600871} [modify] https://crrev.com/a3d7cb29b20a1b5278e83e1a00f802a8de60cb27/DEPS [modify] https://crrev.com/a3d7cb29b20a1b5278e83e1a00f802a8de60cb27/third_party/boringssl/BUILD.generated.gni
,
Oct 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/68b5626a822ac5101be35f7b017df5f2931d8f5f commit 68b5626a822ac5101be35f7b017df5f2931d8f5f Author: David Benjamin <davidben@chromium.org> Date: Thu Oct 18 21:04:07 2018 Remove cfi_icall_generalize_pointers from BoringSSL. BoringSSL should be clean w.r.t. this flag now. Bug: 785442 Change-Id: I8fa8b2589c82a7c446a01d96c4702a2d1535de1e Reviewed-on: https://chromium-review.googlesource.com/c/1289160 Reviewed-by: Vlad Tsyrklevich <vtsyrklevich@chromium.org> Commit-Queue: David Benjamin <davidben@chromium.org> Cr-Commit-Position: refs/heads/master@{#600902} [modify] https://crrev.com/68b5626a822ac5101be35f7b017df5f2931d8f5f/third_party/boringssl/BUILD.gn |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Nov 17 2017