Merge "stable" #729
Merged
Merge "stable" #729
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* 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)
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)
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] <support@github.com> (cherry picked from commit 959e6d4)
* 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)
* 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)
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)
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)
* Bump selected versions to 0.13.1 * Cut Themis 0.13.1 in changelog
* 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)
Hotfix for Carthage, removing dysfunctional ObjCThemis.xcodeproj.
* 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
Rewrite `match` block using `matches` macro as it is now a recommended way to perform such checks (cherry picked from commit 2522a5d) Signed-off-by: Alexei Lozovsky <alexei@cossacklabs.com>
ilammy
requested review from
iamnotacake,
ignatk,
Lagovas,
storojs72 and
vixentael
as code owners
|
Although GitHub now allows resolving conflicts via web interface, it's not that powerful to remember the resolution and apply it once the PR is merged. I've committed a merge with resolution to Once approved, I'll redo the merge manually and roll |
vixentael
reviewed
Oct 18, 2020
vixentael
reviewed
Oct 18, 2020
Oh well... This is why I don't really like cherry-picking things all over the place. Since sometimes it ends up like this once you want to merge. Okay, change of plans. Instead of letting GitHub commit the merge commit, I'll manually merge This produces a merge commit: I'll push that merge commit into Once the review is complete, I'll push that commit as is into and roll back |
|
Now, this one should be okay. I've resolved all conflicts it favor of |
|
Thank you @vixentael for keeping me from doing silly things! ❤️ |
|
thank you @ilammy! This PR changes look correct (as no changes from |
vixentael
approved these changes
Oct 19, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following #728, merge
stableintomasterto pull in latest updates.The branches have diverged wildly, and there was cherry-picking left, right & center, and there was a merge of
release/0.13not via a merge commit, etc. etc. etc. All this results in merge conflicts which need to be resolved manually. Inmostall cases the master version should be preferred.Checklist
Change is covered by automated tests(it is, but it will probably fail, wait for it to land)(need see to it manually during the merge)via CLI(GitHub now has an interface for that)