Skip to content

Commit

Permalink
Add missing OpenSSL includes (#684)
Browse files Browse the repository at this point in the history
* 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).
  • Loading branch information
ilammy committed Jul 28, 2020
1 parent 8ae934c commit 1ca96de
Show file tree
Hide file tree
Showing 8 changed files with 49 additions and 8 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -10,6 +10,7 @@ _Code:_

- Include embedded BoringSSL into Soter for convenience ([#681](https://github.com/cossacklabs/themis/pull/681)).
- `make deb` and `make rpm` with `ENGINE=boringssl` will now produce `libthemis-boringssl` packages with embedded BoringSSL ([#683](https://github.com/cossacklabs/themis/pull/683)).
- Improved compatibility with OpenSSL 1.1.1 ([#684](https://github.com/cossacklabs/themis/pull/684)).

- **Android**

Expand Down
16 changes: 16 additions & 0 deletions ObjCThemis.xcodeproj/project.pbxproj
Expand Up @@ -1481,6 +1481,9 @@
"$(PROJECT_DIR)/Carthage/Build/Mac",
);
FRAMEWORK_VERSION = A;
GCC_TREAT_IMPLICIT_FUNCTION_DECLARATIONS_AS_ERRORS = YES;
GCC_TREAT_INCOMPATIBLE_POINTER_TYPE_WARNINGS_AS_ERRORS = YES;
GCC_TREAT_WARNINGS_AS_ERRORS = YES;
HEADER_SEARCH_PATHS = (
"$(PROJECT_DIR)/src",
"$(PROJECT_DIR)/src/wrappers/themis/Obj-C",
Expand All @@ -1498,6 +1501,7 @@
PRODUCT_NAME = objcthemis;
SDKROOT = macosx;
SKIP_INSTALL = YES;
WARNING_CFLAGS = "-Wno-documentation";
};
name = Debug;
};
Expand All @@ -1518,6 +1522,9 @@
"$(PROJECT_DIR)/Carthage/Build/Mac",
);
FRAMEWORK_VERSION = A;
GCC_TREAT_IMPLICIT_FUNCTION_DECLARATIONS_AS_ERRORS = YES;
GCC_TREAT_INCOMPATIBLE_POINTER_TYPE_WARNINGS_AS_ERRORS = YES;
GCC_TREAT_WARNINGS_AS_ERRORS = YES;
HEADER_SEARCH_PATHS = (
"$(PROJECT_DIR)/src",
"$(PROJECT_DIR)/src/wrappers/themis/Obj-C",
Expand All @@ -1535,6 +1542,7 @@
PRODUCT_NAME = objcthemis;
SDKROOT = macosx;
SKIP_INSTALL = YES;
WARNING_CFLAGS = "-Wno-documentation";
};
name = Release;
};
Expand All @@ -1553,6 +1561,9 @@
"$(inherited)",
"$(PROJECT_DIR)/Carthage/Build/iOS",
);
GCC_TREAT_IMPLICIT_FUNCTION_DECLARATIONS_AS_ERRORS = YES;
GCC_TREAT_INCOMPATIBLE_POINTER_TYPE_WARNINGS_AS_ERRORS = YES;
GCC_TREAT_WARNINGS_AS_ERRORS = YES;
HEADER_SEARCH_PATHS = (
"$(PROJECT_DIR)/src",
"$(PROJECT_DIR)/src/wrappers/themis/Obj-C",
Expand All @@ -1575,6 +1586,7 @@
SWIFT_OPTIMIZATION_LEVEL = "-Onone";
SWIFT_VERSION = 4.2;
TARGETED_DEVICE_FAMILY = "1,2";
WARNING_CFLAGS = "-Wno-documentation";
};
name = Debug;
};
Expand All @@ -1593,6 +1605,9 @@
"$(inherited)",
"$(PROJECT_DIR)/Carthage/Build/iOS",
);
GCC_TREAT_IMPLICIT_FUNCTION_DECLARATIONS_AS_ERRORS = YES;
GCC_TREAT_INCOMPATIBLE_POINTER_TYPE_WARNINGS_AS_ERRORS = YES;
GCC_TREAT_WARNINGS_AS_ERRORS = YES;
HEADER_SEARCH_PATHS = (
"$(PROJECT_DIR)/src",
"$(PROJECT_DIR)/src/wrappers/themis/Obj-C",
Expand All @@ -1615,6 +1630,7 @@
SWIFT_OPTIMIZATION_LEVEL = "-O";
SWIFT_VERSION = 4.2;
TARGETED_DEVICE_FAMILY = "1,2";
WARNING_CFLAGS = "-Wno-documentation";
};
name = Release;
};
Expand Down
1 change: 1 addition & 0 deletions src/soter/openssl/soter_ec_key.c
Expand Up @@ -18,6 +18,7 @@

#include <string.h>

#include <openssl/bn.h>
#include <openssl/ec.h>
#include <openssl/evp.h>

Expand Down
1 change: 1 addition & 0 deletions src/soter/openssl/soter_rsa_common.c
Expand Up @@ -16,6 +16,7 @@

#include "soter/openssl/soter_rsa_common.h"

#include <openssl/bn.h>
#include <openssl/evp.h>
#include <openssl/rsa.h>

Expand Down
1 change: 1 addition & 0 deletions src/soter/openssl/soter_rsa_key.c
Expand Up @@ -18,6 +18,7 @@

#include <string.h>

#include <openssl/bn.h>
#include <openssl/evp.h>
#include <openssl/rsa.h>

Expand Down
1 change: 1 addition & 0 deletions src/soter/openssl/soter_rsa_key_pair_gen.c
Expand Up @@ -18,6 +18,7 @@

#include <string.h>

#include <openssl/bn.h>
#include <openssl/evp.h>
#include <openssl/rsa.h>

Expand Down
26 changes: 21 additions & 5 deletions src/themis/secure_cell_seal_passphrase.c
Expand Up @@ -99,7 +99,13 @@ static themis_status_t themis_auth_sym_derive_encryption_key_pbkdf2(
goto error;
}

res = themis_auth_sym_kdf_context(message_length, soter_kdf_context, &soter_kdf_context_length);
/*
* themis_auth_sym_encrypt_message_with_passphrase_() makes sure that
* message_length fits into uint32_t.
*/
res = themis_auth_sym_kdf_context((uint32_t)message_length,
soter_kdf_context,
&soter_kdf_context_length);
if (res != THEMIS_SUCCESS) {
goto error;
}
Expand Down Expand Up @@ -181,6 +187,7 @@ themis_status_t themis_auth_sym_encrypt_message_with_passphrase_(const uint8_t*
uint8_t auth_tag[THEMIS_AUTH_SYM_AUTH_TAG_LENGTH] = {0};
uint8_t derived_key[THEMIS_AUTH_SYM_KEY_LENGTH / 8] = {0};
size_t derived_key_length = sizeof(derived_key);
size_t auth_token_real_length = 0;
struct themis_scell_auth_token_passphrase hdr;

/* Message length is currently stored as 32-bit integer, sorry */
Expand Down Expand Up @@ -233,16 +240,19 @@ themis_status_t themis_auth_sym_encrypt_message_with_passphrase_(const uint8_t*
goto error;
}

if (*auth_token_length < themis_scell_auth_token_passphrase_size(&hdr)) {
*auth_token_length = themis_scell_auth_token_passphrase_size(&hdr);
/* In valid Secure Cells auth token length always fits into uint32_t. */
auth_token_real_length = (uint32_t)themis_scell_auth_token_passphrase_size(&hdr);

if (*auth_token_length < auth_token_real_length) {
*auth_token_length = auth_token_real_length;
res = THEMIS_BUFFER_TOO_SMALL;
goto error;
}
res = themis_write_scell_auth_token_passphrase(&hdr, auth_token, *auth_token_length);
if (res != THEMIS_SUCCESS) {
goto error;
}
*auth_token_length = themis_scell_auth_token_passphrase_size(&hdr);
*auth_token_length = auth_token_real_length;
*encrypted_message_length = message_length;

error:
Expand Down Expand Up @@ -324,7 +334,13 @@ static themis_status_t themis_auth_sym_derive_decryption_key_pbkdf2(
goto error;
}

res = themis_auth_sym_kdf_context(message_length, soter_kdf_context, &soter_kdf_context_length);
/*
* themis_auth_sym_decrypt_message_with_passphrase_() makes sure that
* message_length fits into uint32_t.
*/
res = themis_auth_sym_kdf_context((uint32_t)message_length,
soter_kdf_context,
&soter_kdf_context_length);
if (res != THEMIS_SUCCESS) {
goto error;
}
Expand Down
10 changes: 7 additions & 3 deletions src/themis/sym_enc_message.c
Expand Up @@ -277,6 +277,7 @@ themis_status_t themis_auth_sym_encrypt_message_(const uint8_t* key,
uint8_t derived_key[THEMIS_AUTH_SYM_KEY_LENGTH / 8] = {0};
size_t kdf_context_length = sizeof(kdf_context);
size_t derived_key_length = sizeof(derived_key);
size_t auth_token_real_length = 0;
struct themis_scell_auth_token_key hdr;

/* Message length is currently stored as 32-bit integer, sorry */
Expand Down Expand Up @@ -331,16 +332,19 @@ themis_status_t themis_auth_sym_encrypt_message_(const uint8_t* key,
goto error;
}

if (*auth_token_length < themis_scell_auth_token_key_size(&hdr)) {
*auth_token_length = themis_scell_auth_token_key_size(&hdr);
/* In valid Secure Cells auth token length always fits into uint32_t. */
auth_token_real_length = (uint32_t)themis_scell_auth_token_key_size(&hdr);

if (*auth_token_length < auth_token_real_length) {
*auth_token_length = auth_token_real_length;
res = THEMIS_BUFFER_TOO_SMALL;
goto error;
}
res = themis_write_scell_auth_token_key(&hdr, auth_token, *auth_token_length);
if (res != THEMIS_SUCCESS) {
goto error;
}
*auth_token_length = themis_scell_auth_token_key_size(&hdr);
*auth_token_length = auth_token_real_length;
*encrypted_message_length = message_length;

error:
Expand Down

0 comments on commit 1ca96de

Please sign in to comment.