Avoid overflows on 32-bit systems #677

ilammy · 2020-07-14T09:25:29Z

Avoid overflows in Secure Cell

Themis Core C API works with buffer sizes expressed as size_t while in Go lengths are expressed as int. Themis containers can typically contain up to 4 GB of data with internal length fields using uint32_t.

On typical 64-bit systems this does not cause overflows since uint32_t fits into both Go's int and C's size_t. However, on 32-bit system this can cause overflows. There, size_t is unsigned 32-bit value identical to uint32_t while int is 32-bit signed value, so the size may not fit into Go's size range.

We can't do anything about that. On 32-bit systems the buffer sizes are typically limited to 2 GB anyway due to the way memory is distributed. However, if the overflow happens, Go will panic when trying to allocate (effectively) negatively-sized arrays. We should return an error instead.

Add size checks before casting C.size_t into int and return an error if the size will overflow. Do this for all API, both new and old.

Normally, Themis is not used to encrypt real 2+ GB messages, but this condition can easily happen if the data has been corrupted where the length field is stored. We don't want this to be a source of DOS attacks.

Reenable tests for corrupted data

The panic condition has been originally detected by a couple of tests for Secure Cell's Token Protect mode which has the stars properly aligned for the issue to be visible. Now that the issue is fixed, we can enable these tests for 32-bit machines again.

Avoid overflows in other cryptosystems

Just like Secure Cell, add more checks to other cryptosystems as well. Unfortunately, we have to duplicate the size check utility. GoThemis does not have a common utility module, and even if it did, it would not work due to the way CGo is implemented (C.size_t is a distinct type in different modules).

Checklist

Change is covered by automated tests
The coding guidelines are followed
Public API has proper documentation
Changelog is updated (in case of notable or breaking changes)

Themis Core C API works with buffer sizes expressed as "size_t" while in Go lengths are expressed as "int". Themis containers can typically contain up to 4 GB of data with internal length fields using "uint32_t". On typical 64-bit systems this does not cause overflows since uint32_t fits into both Go's int and C's size_t. However, on 32-bit system this can cause overflows. There, size_t is unsigned 32-bit value identical to uint32_t while int is 32-bit signed value, so the size may not fit into Go's size range. We can't do anything about that. On 32-bit systems the buffer sizes are typically limited to 2 GB anyway due to the way memory is distributed. However, if the overflow happens, Go will panic when trying to allocate (effectively) negatively-sized arrays. We should return an error instead. Add size checks before casting "C.size_t" into "int" and return an error if the size will overflow. Do this for all API, both new and old. Normally, Themis is not used to encrypt real 2+ GB messages, but this condition can easily happen if the data has been corrupted where the length field is stored. We don't want this to be a source of DOS attacks.

The panic condition has been originally detected by a couple of tests for Secure Cell's Token Protect mode which has the stars properly aligned for the issue to be visible. Now that the issue is fixed, we can enable these tests for 32-bit machines again.

Just like Secure Cell, add more checks to other cryptosystems as well. Unfortunately, we have to duplicate the size check utility. GoThemis does not have a common utility module, and even if it did, it would not work due to the way CGo is implemented ("C.size_t" is a distinct type in different modules).

vixentael

Let's wait for @Lagovas input

* Avoid overflows in Secure Cell Themis Core C API works with buffer sizes expressed as "size_t" while in Go lengths are expressed as "int". Themis containers can typically contain up to 4 GB of data with internal length fields using "uint32_t". On typical 64-bit systems this does not cause overflows since uint32_t fits into both Go's int and C's size_t. However, on 32-bit system this can cause overflows. There, size_t is unsigned 32-bit value identical to uint32_t while int is 32-bit signed value, so the size may not fit into Go's size range. We can't do anything about that. On 32-bit systems the buffer sizes are typically limited to 2 GB anyway due to the way memory is distributed. However, if the overflow happens, Go will panic when trying to allocate (effectively) negatively-sized arrays. We should return an error instead. Add size checks before casting "C.size_t" into "int" and return an error if the size will overflow. Do this for all API, both new and old. Normally, Themis is not used to encrypt real 2+ GB messages, but this condition can easily happen if the data has been corrupted where the length field is stored. We don't want this to be a source of DOS attacks. * Reenable tests for corrupted data The panic condition has been originally detected by a couple of tests for Secure Cell's Token Protect mode which has the stars properly aligned for the issue to be visible. Now that the issue is fixed, we can enable these tests for 32-bit machines again. * Avoid overflows in Secure Compartor * Avoid overflows in key generation * Avoid overflows in Secure Message * Avoid overflows in Secure Session Just like Secure Cell, add more checks to other cryptosystems as well. Unfortunately, we have to duplicate the size check utility. GoThemis does not have a common utility module, and even if it did, it would not work due to the way CGo is implemented ("C.size_t" is a distinct type in different modules). (cherry picked from commit 8b83a71)

* Avoid overflows on 32-bit systems (#677) * Avoid overflows in Secure Cell Themis Core C API works with buffer sizes expressed as "size_t" while in Go lengths are expressed as "int". Themis containers can typically contain up to 4 GB of data with internal length fields using "uint32_t". On typical 64-bit systems this does not cause overflows since uint32_t fits into both Go's int and C's size_t. However, on 32-bit system this can cause overflows. There, size_t is unsigned 32-bit value identical to uint32_t while int is 32-bit signed value, so the size may not fit into Go's size range. We can't do anything about that. On 32-bit systems the buffer sizes are typically limited to 2 GB anyway due to the way memory is distributed. However, if the overflow happens, Go will panic when trying to allocate (effectively) negatively-sized arrays. We should return an error instead. Add size checks before casting "C.size_t" into "int" and return an error if the size will overflow. Do this for all API, both new and old. Normally, Themis is not used to encrypt real 2+ GB messages, but this condition can easily happen if the data has been corrupted where the length field is stored. We don't want this to be a source of DOS attacks. * Reenable tests for corrupted data The panic condition has been originally detected by a couple of tests for Secure Cell's Token Protect mode which has the stars properly aligned for the issue to be visible. Now that the issue is fixed, we can enable these tests for 32-bit machines again. * Avoid overflows in Secure Compartor * Avoid overflows in key generation * Avoid overflows in Secure Message * Avoid overflows in Secure Session Just like Secure Cell, add more checks to other cryptosystems as well. Unfortunately, we have to duplicate the size check utility. GoThemis does not have a common utility module, and even if it did, it would not work due to the way CGo is implemented ("C.size_t" is a distinct type in different modules). (cherry picked from commit 8b83a71) * Publish source and Javadoc JARs (#679) Add tasks and artifacts for publishing JARs with source code and generated Javadocs. These are required for admission to Bintray's JCenter -- well-known public repository of open-source libraries. With this, the users will be able to import Themis without adding Cossack Labs repository. It's highly likely that they already have JCenter added for other dependencies: repositories { jcenter() } Since it's the same package, the dependency is specified as before: dependencies { implementation 'com.cossacklabs.com:themis:0.13.0' } Many Bothans died to bring us this information. No, really, Gradle documentation on this is less than stellar, and Groovy does not make it make it easier. *sigh* Android ecosystem vOv (cherry picked from commit 2344484) * Bump lodash from 4.17.15 to 4.17.19 (#680) Bumps [lodash](https://github.com/lodash/lodash) from 4.17.15 to 4.17.19. - [Release notes](https://github.com/lodash/lodash/releases) - [Commits](lodash/lodash@4.17.15...4.17.19) Signed-off-by: dependabot[bot] <[email protected]> (cherry picked from commit 959e6d4) * Add missing OpenSSL includes (#684) * Add missing OpenSSL includes Add those files use BIGNUM API of OpenSSL but do not include relevant headers. Due to miraculous coincidence, this seems to somehow work for the OpenSSL versions we use, but only because either existing headers include this "bn.h" transitively, or because the compiler generates code that kinda works without function prototype being available. However, curiously enough, this breaks when building Themis for macOS with recent OpenSSL 1.1.1g but not with OpenSSL 1.0.2, or OpenSSL 1.1.1g on Linux. The issue manifests itself as missing "_BN_num_bytes" symbol. Indeed, there is no such symbol because this function is implemented as a macro via BN_num_bits(). However, because of the missing header, the compiler -- being C compiler -- decides that this must be a function "int BN_num_bytes()" and compiles it like a function call. Add the missing includes to define the necessary macros and prototype, resolving the issue with OpenSSL 1.1.1g. It must have stopped including <openssl/bn.h> transitively, revealing this issue. This is why you should always include and import stuff you use directly, not rely on transitive imports. P.S. A mystery for dessert: BoringSSL backend *includes* <openssl/bn.h>. * Treat warnings as errors in Xcode In order to prevent more silly issues in the future, tell Xcode to tell the compiler to treat all warnings as errors. That way the build should fail earlier, and the developers will be less likely to ignore warnings. * Fix implicit cast warnings Now that we treat warnings as errors, let's fix them. themis_auth_sym_kdf_context() accepts message length as "uint32_t" while it's callers use "size_t" to avoid early casts and temporary values. However, the message length has been checked earlier and will fit into "uint32_t", we can safely perform explicit casts here. * Suppress documentation warnings (temporarily) Some OpenSSL headers packaged with Marcin's OpenSSL that we use have borked documentation comments. This has been pointed out several times [1][2], but Marcin concluded this needs to be fixed upstream. [1]: krzyzanowskim/OpenSSL#79 [2]: krzyzanowskim/OpenSSL#41 Meanwhile, having those broken headers breaks the build if the warnings are treated as errors. Since we can't upgrade Marcin's OpenSSL due to other reasons (bitcode support), we have no hope to resolve this issue. For the time being, suppress the warnings about documentation comments. * Fix more implicit cast warnings There are more warnings actual only for 32-bit platforms. Some iOS targets are 32-bit, we should avoid warnings there as well. The themis_scell_auth_token_key_size() and themis_scell_auth_token_passphrase_size() functions compute the size of the autentication token from the header. They return uint64_t values to avoid overflows when working with corrupted input data on the decryption code path. However, they are also used on the encryption path where corruption is not possible. Normally, authentication tokens are small, they most definitely fit into uint32_t, and this is the type used in Secure Cell data format internally. It is not safe to assign arbitrary uint64_t to size_t on 32-bit platforms. However, in this case we are sure that auth tokenn length fits into uint32_t, which can be safely assigned to size_t. Note that we cast into uint32_t, not size_t. This is to still cause a warning on platforms with 16-bit size_t (not likely, but cleaner). (cherry picked from commit 1ca96de) * Update to OpenSSL 1.1.1g (#692) * Use cossacklabs/openssl-apple for Carthage Switch from using Marcin's OpenSSL [1] to our own build of OpenSSL [2] which provides newer OpenSSL 1.1.1g, bitcode, and other goodies. [1]: https://github.com/krzyzanowskim/OpenSSL [2]: https://github.com/cossacklabs/openssl-apple The new OpenSSL is distributed as a binary-only framework. It will be downloaded from GitHub instead of building it from source. This is not much different from what the previous vendor did, but is more stable. Carthage builds use the static flavor of the framework. We have run into issues with dynamic frameworks of OpenSSL when using Carthage, but static frameworks seems to do very good job: the resulting binaries are smaller, apps start a bit faster, and users are freed from the hassle of dealing with OpenSSL linkage to their app. Note that due to the way static linkage works, we will be exporting all OpenSSL symbols from ObjCThemis by default. In order to avoid conflicts, export only limited subset of symbols: Objective-C classes of ObjCThemis. * Use cossacklabs/openssl-apple for CocoaPods Switch from using Levi Groker's OpenSSL [1] to our own build [2] which provides newer OpenSSL 1.1.1g, bitcode, and other goodies. [1]: https://github.com/levigroker/GRKOpenSSLFramework [2]: https://github.com/cossacklabs/openssl-apple The new OpenSSL is distributed as a tricky pod, but for consumers like Themis it's just a pod. Introduce a separate subspec for the build with newer OpenSSL, and make it the default choice. We keep the old specs around in case someone needs them to share GRKOpenSSL or BoringSSL with other dependencies, as it is not possible to use CLOpenSSL simultaneously with them due to OpenSSL symbol conflicts. The new subspec has its oddities, but it's all (un)known magic that seems to be absolutely necessary to build Themis properly for iOS. * Note the update in CHANGELOG * Export global functions as well In Carthage builds, we need to export not only Objective-C classes from TS namespace but global functions as well. (Well, there is only one such function right now: TSGenerateSymmetricKey().) * Note Xcode 11 requirement in CHANGELOG It seems that Xcode 10 cannot handle bitcode data embedded into prebuilt OpenSSL frameworks we distribute which were built with Xcode 11.2. Make Xcode new minimum requirement for ObjCThemis and SwiftThemis. Xcode 11.0 is known to work. * Improve CocoaPods version hack for linting Turns out that the spell we used before is not effective. sed does not understand "\s" so replace it with "[ ]" to match the leading spaces. Now the podspec should be correctly edited for linting and validate as if the current commit is going to be published. (The hack will be disabled for production branches where we are linting the spec as is.) * Change comments to trigger CI (so high-tech) * Add arm64 to default architectures list Make sure that ObjCThemis is compiled for arm64e architecture as noted in Apple guidance [1]. This is important for the case when our users would like to test their apps with this architecture. Currently, Xcode does not build slices for this architecture by default, so Carthage builds will not include it. [1]: https://developer.apple.com/documentation/security/preparing_your_app_to_work_with_pointer_authentication * Explicitly enable bitcode for Carthage It seems that in some cases bitcode gets disabled in Xcode. Make sure that our project files keep it enabled at all times. * Do not claim arm64e support in changelog It seems that ObjCThemis does not support arm64e very well, after all. Let's not mention it in the changelog. Maybe the situation will improve before 0.14 is out though. (cherry picked from commit 6dacf9c) * Update CocoaPods repository (#673) After restoring CocoaPods from cache we also need to fetch the latest updates which were not cached yet. This is not noticeable most of the time, but otherwise it breaks builds right after a new version of Themis is released on CocoaPods and we can't fetch it because the repo is not updated yet. (cherry picked from commit 89304e5) * GoThemis README (#699) pkg.go.dev does not look at the repository README, it needs a file (not a symlink) near the "go.mod" file. Use a short README for Go, similar to the one used by RustThemis, to minimize maintenance effort. Note that pkg.go.dev cannot resolve relative links to files outside of the Go module directory ("gothemis") so we can't link to examples and license like that. Instead, direct links to the master branch of the repository are provided. It's not optimal, but a good compromise. (cherry picked from commit 5d9d5ae) * Themis 0.13.1 (#697) * Bump selected versions to 0.13.1 * Cut Themis 0.13.1 in changelog * Remove ObjCThemis.xcodeproj (#704) * Remove ObjCThemis.xcodeproj The idea behind building "objcthemis.framework" has been to unify import syntax between Carthage and CocoaPods. Unfortunately, it turned out to be a mistake. "objcthemis.framework" does not work without "themis.framework" being present alongside it because of how module resolution works. Despite "objcthemis.framework" providing the same "themis" module as "themis.framework", the compiler will look for a framework named "themis.framework" when resolving "import themis". Moreover, the original issue that "objcthemis.framework" has been called to rectify can be resolved more elegantly by importing the module: @import themis; which work well with "themis.framework" in both Carthage and CocoaPods. Since "objcthemis.framework" does not bring any value, remove it. Move all new things added to ObjCThemis.xcodeproj into Themis.xcodeproj (such as testing Swift 4 vs 5). Remove the import warning. Now Carthage will build only one framework: "themis.framework" from Themis.xcodeproj. I am sorry for the trouble and confusion of this fizzled migration. * Change "product name" to "themis" Make sure that Xcode targets produce "themis.framework", not "objcthemis.framework". * Recreate Xcode schemes It seems that some components stick the schemes after renaming. Recreate them to make sure that we're building "themis.framework" and there are no traces of the old Xcode project. * Bring back proxy umbrella header "themis.h" Since the framework is named "themis.framework", its umbrella header is expected to be called "themis.h". The actual umbrella header for ObjCThemis is "objcthemis.h" which we simply include. * Use alternative imports in unit tests One of the reasons for "objcthemis.framework" existence was to run ObjCThemis unit tests from Xcode. Initially, "themis.framework" has prevented that due to import issues, and "objcthemis.framework" has allowed #import <objcthemis/objcthemis.h> to work. Now that latter is gone, the unit-tests are broken again. However! It seems that using modular imports works for Xcode and Carthage (which uses Xcode project). The bad news here is that it *does not* work for CocoaPods, which still works only with the old form because CocoaPods does some special wicked magic with headers, putting them into the "objcthemis" directory. I do not have much time and willingness to deal with this stupidity anymore right now, so here's a compromise: Carthage uses its form, CocoaPods use their form, and you get this TODO to maybe get rid of this wart some time later. (cherry picked from commit 5522ace) * Themis 0.13.2 (#705) Hotfix for Carthage, removing dysfunctional ObjCThemis.xcodeproj. * Hotfix for Themis CocoaPods podspec: support Xcode12 (#719) * update podspec to fix builds on xcode12 * ok, use 0.13.2 as podspec version * set ios deployment target to 10.0 for cocoapods projects * update themis podspec for 0.13.3 tag Co-authored-by: Alexei Lozovsky <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

ilammy added 7 commits July 14, 2020 12:05

Avoid overflows in key generation

2c43785

Avoid overflows in Secure Message

c319888

Avoid overflows in Secure Session

0ad4fc5

Changelog mention

80a1475

ilammy added bug W-GoThemis 🐹 Wrapper: GoThemis, Go API backport Patches to backport to currently supported releases labels Jul 14, 2020

ilammy requested review from vixentael and Lagovas July 14, 2020 09:25

vixentael approved these changes Jul 14, 2020

View reviewed changes

Lagovas approved these changes Jul 14, 2020

View reviewed changes

ilammy merged commit 8b83a71 into cossacklabs:master Jul 14, 2020

ilammy deleted the gothemis-overflow branch July 14, 2020 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid overflows on 32-bit systems #677

Avoid overflows on 32-bit systems #677

ilammy commented Jul 14, 2020

vixentael left a comment

Avoid overflows on 32-bit systems #677

Avoid overflows on 32-bit systems #677

Conversation

ilammy commented Jul 14, 2020

Avoid overflows in Secure Cell

Reenable tests for corrupted data

Avoid overflows in other cryptosystems

Checklist

vixentael left a comment

Choose a reason for hiding this comment