Skip to content

Commit

Permalink
Avoid usage of <arpa/inet.h> (#470)
Browse files Browse the repository at this point in the history 
* Move "portable_endian.h" to Soter

We're going to need these functions in both Soter and Themis, so move
the header to Soter. Keep a stub in Themis to avoid breaking existing
code.

Also note that <sys/param.h> is a POSIX header and it is not available
on Windows. Remove it. All conversions are provided by <WinSock2.h>.

* Avoid usage of <arpa/inet.h>

Unfortunately, Windows systems do not provide this header normally.
We use to get access to htonl() and ntohl() functions. On Windows
these functions are provided by <WinSock2.h> header (and ws2_32.dll
library).

We could litter the code with ifdefs, but that's not nice. Instead,
let's use the "portable_endian.h" header. It contains all necessary
ifdefs, but provides slightly different (and explicit) API:

  - htonl == htobe32
  - ntohl == be32toh

Update all code to include common header and use new portable names.

* Turn Soter container accessors into functions

soter_container.h uses htonl() and ntohl() functions directly in macros.
This is not nice because it leaks implementation details and requires
POSIX API which is not available on Windows. Furthermore, this header
does not even include relevant headers for these functions!

Replace this macros with proper functions and move the implementation
to soter_container.c which includes "portable_endian.h". This allows
our API to not include private headers and does not unnecessarily expose
implementation details.

Theoretically, this will impact performance, but in practice the effect
should be negligible. These macros are not used in any hot paths, and
the compiler may even optimize these functions when using LTO.

* Safety checks in Soter container accessors

Now that we have replaced macros with functions, we can make them more
resilient to invalid input. Check pointer parameters for null, return
errors where appropriate. Also avoid integer overflow when computing
data sizes.
  • Loading branch information
@ilammy
ilammy committed May 22, 2019
1 parent 7cb03be commit 57485fb
Show file tree
Hide file tree
Showing 16 changed files with 264 additions and 179 deletions.
6 changes: 6 additions & 0 deletions Themis.xcodeproj/project.pbxproj
View file
Expand Up @@ -196,6 +196,8 @@
9F4A2620223ABEF2005CB63A /* secure_session_utils.c in Sources */ = {isa = PBXBuildFile; fileRef = 9F4A242E223A74AF005CB63A /* secure_session_utils.c */; };
9F4A2621223ABEF2005CB63A /* secure_session.c in Sources */ = {isa = PBXBuildFile; fileRef = 9F4A2438223A74AF005CB63A /* secure_session.c */; };
9F4A2622223ABEF2005CB63A /* sym_enc_message.c in Sources */ = {isa = PBXBuildFile; fileRef = 9F4A2431223A74AF005CB63A /* sym_enc_message.c */; };
9F5391F12293F2D90051DA66 /* portable_endian.h in Headers */ = {isa = PBXBuildFile; fileRef = 9F5391F02293F2D90051DA66 /* portable_endian.h */; };
9F5391F22293F2D90051DA66 /* portable_endian.h in Headers */ = {isa = PBXBuildFile; fileRef = 9F5391F02293F2D90051DA66 /* portable_endian.h */; };
9FBD853D223BFB5E009EAEB3 /* openssl.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 9FBD853C223BFB5E009EAEB3 /* openssl.framework */; };
/* End PBXBuildFile section */

Expand Down Expand Up @@ -397,6 +399,7 @@
9F4A24C1223A8FA8005CB63A /* skeygen.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = skeygen.h; path = "src/wrappers/themis/Obj-C/objcthemis/skeygen.h"; sourceTree = "<group>"; };
9F4A24C2223A8FA8005CB63A /* smessage.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = smessage.h; path = "src/wrappers/themis/Obj-C/objcthemis/smessage.h"; sourceTree = "<group>"; };
9F4A24C3223A8FA8005CB63A /* scell_token.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = scell_token.h; path = "src/wrappers/themis/Obj-C/objcthemis/scell_token.h"; sourceTree = "<group>"; };
9F5391F02293F2D90051DA66 /* portable_endian.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = portable_endian.h; path = src/soter/portable_endian.h; sourceTree = "<group>"; };
9FBD853C223BFB5E009EAEB3 /* openssl.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = openssl.framework; path = Carthage/Build/iOS/openssl.framework; sourceTree = "<group>"; };
9FD4C3522260D41700132A88 /* soter_api.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = soter_api.h; path = src/soter/soter_api.h; sourceTree = "<group>"; };
9FD4C3532260D43B00132A88 /* themis_api.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = themis_api.h; path = src/themis/themis_api.h; sourceTree = "<group>"; };
Expand Down Expand Up @@ -445,6 +448,7 @@
children = (
9F4A23A6223A742F005CB63A /* ed25519 */,
9F4A2383223A740E005CB63A /* openssl */,
9F5391F02293F2D90051DA66 /* portable_endian.h */,
9FD4C3522260D41700132A88 /* soter_api.h */,
9F4A2343223A73B0005CB63A /* soter_asym_cipher.h */,
9F4A234B223A73B0005CB63A /* soter_asym_ka.h */,
Expand Down Expand Up @@ -654,6 +658,7 @@
isa = PBXHeadersBuildPhase;
buildActionMask = 2147483647;
files = (
9F5391F22293F2D90051DA66 /* portable_endian.h in Headers */,
9F00E8E0223C19E000EC1EF3 /* themis.h in Headers */,
);
runOnlyForDeploymentPostprocessing = 0;
Expand All @@ -662,6 +667,7 @@
isa = PBXHeadersBuildPhase;
buildActionMask = 2147483647;
files = (
9F5391F12293F2D90051DA66 /* portable_endian.h in Headers */,
9F00E8DF223C19D900EC1EF3 /* themis.h in Headers */,
);
runOnlyForDeploymentPostprocessing = 0;
Expand Down
10 changes: 6 additions & 4 deletions src/soter/boringssl/soter_ec_key.c
View file
Expand Up @@ -22,6 +22,8 @@
#include <openssl/ec.h>
#include <openssl/evp.h>

#include "soter/portable_endian.h"

static bool is_curve_supported(int curve)
{
switch (curve) {
Expand Down Expand Up @@ -161,7 +163,7 @@ soter_status_t soter_engine_specific_to_ec_pub_key(const soter_engine_specific_e
}

memcpy(key->tag, ec_pub_key_tag(curve), SOTER_CONTAINER_TAG_LENGTH);
key->size = htonl(output_length);
key->size = htobe32(output_length);
soter_update_container_checksum(key);
*key_length = output_length;
res = SOTER_SUCCESS;
Expand Down Expand Up @@ -228,7 +230,7 @@ soter_status_t soter_engine_specific_to_ec_priv_key(const soter_engine_specific_
}

memcpy(key->tag, ec_priv_key_tag(curve), SOTER_CONTAINER_TAG_LENGTH);
key->size = htonl(output_length);
key->size = htobe32(output_length);
soter_update_container_checksum(key);
*key_length = output_length;
res = SOTER_SUCCESS;
Expand All @@ -255,7 +257,7 @@ soter_status_t soter_ec_pub_key_to_engine_specific(const soter_container_hdr_t*
return SOTER_INVALID_PARAMETER;
}

if (key_length != ntohl(key->size)) {
if (key_length != be32toh(key->size)) {
return SOTER_INVALID_PARAMETER;
}

Expand Down Expand Up @@ -352,7 +354,7 @@ soter_status_t soter_ec_priv_key_to_engine_specific(const soter_container_hdr_t*
EVP_PKEY* pkey = (EVP_PKEY*)(*engine_key);
soter_status_t res;

if (key_length != ntohl(key->size)) {
if (key_length != be32toh(key->size)) {
return SOTER_INVALID_PARAMETER;
}

Expand Down
26 changes: 14 additions & 12 deletions src/soter/boringssl/soter_rsa_key.c
View file
Expand Up @@ -22,6 +22,8 @@
#include <openssl/evp.h>
#include <openssl/rsa.h>

#include "soter/portable_endian.h"

static size_t rsa_pub_key_size(int mod_size)
{
switch (mod_size) {
Expand Down Expand Up @@ -158,9 +160,9 @@ soter_status_t soter_engine_specific_to_rsa_pub_key(const soter_engine_specific_

pub_exp = (uint32_t*)((unsigned char*)(key + 1) + rsa_mod_size);
if (BN_is_word(rsa->e, RSA_F4)) {
*pub_exp = htonl(RSA_F4);
*pub_exp = htobe32(RSA_F4);
} else if (BN_is_word(rsa->e, RSA_3)) {
*pub_exp = htonl(RSA_3);
*pub_exp = htobe32(RSA_3);
} else {
res = SOTER_INVALID_PARAMETER;
goto err;
Expand All @@ -172,7 +174,7 @@ soter_status_t soter_engine_specific_to_rsa_pub_key(const soter_engine_specific_
}

memcpy(key->tag, rsa_pub_key_tag(rsa_mod_size), SOTER_CONTAINER_TAG_LENGTH);
key->size = htonl(output_length);
key->size = htobe32(output_length);
soter_update_container_checksum(key);
*key_length = output_length;
res = SOTER_SUCCESS;
Expand Down Expand Up @@ -224,9 +226,9 @@ soter_status_t soter_engine_specific_to_rsa_priv_key(const soter_engine_specific

pub_exp = (uint32_t*)(curr_bn + ((rsa_mod_size * 4) + (rsa_mod_size / 2)));
if (BN_is_word(rsa->e, RSA_F4)) {
*pub_exp = htonl(RSA_F4);
*pub_exp = htobe32(RSA_F4);
} else if (BN_is_word(rsa->e, RSA_3)) {
*pub_exp = htonl(RSA_3);
*pub_exp = htobe32(RSA_3);
} else {
res = SOTER_INVALID_PARAMETER;
goto err;
Expand Down Expand Up @@ -281,7 +283,7 @@ soter_status_t soter_engine_specific_to_rsa_priv_key(const soter_engine_specific
}

memcpy(key->tag, rsa_priv_key_tag(rsa_mod_size), SOTER_CONTAINER_TAG_LENGTH);
key->size = htonl(output_length);
key->size = htobe32(output_length);
soter_update_container_checksum(key);
*key_length = output_length;
res = SOTER_SUCCESS;
Expand Down Expand Up @@ -310,7 +312,7 @@ soter_status_t soter_rsa_pub_key_to_engine_specific(const soter_container_hdr_t*
EVP_PKEY* pkey = (EVP_PKEY*)(*engine_key);
const uint32_t* pub_exp;

if (key_length != ntohl(key->size)) {
if (key_length != be32toh(key->size)) {
return SOTER_INVALID_PARAMETER;
}

Expand Down Expand Up @@ -345,7 +347,7 @@ soter_status_t soter_rsa_pub_key_to_engine_specific(const soter_container_hdr_t*
}

pub_exp = (const uint32_t*)((unsigned char*)(key + 1) + rsa_mod_size);
switch (ntohl(*pub_exp)) {
switch (be32toh(*pub_exp)) {
case RSA_3:
case RSA_F4:
break;
Expand All @@ -364,7 +366,7 @@ soter_status_t soter_rsa_pub_key_to_engine_specific(const soter_container_hdr_t*
return SOTER_NO_MEMORY;
}

if (!BN_set_word(rsa->e, ntohl(*pub_exp))) {
if (!BN_set_word(rsa->e, be32toh(*pub_exp))) {
RSA_free(rsa);
return SOTER_FAIL;
}
Expand Down Expand Up @@ -399,7 +401,7 @@ soter_status_t soter_rsa_priv_key_to_engine_specific(const soter_container_hdr_t
const uint32_t* pub_exp;
const unsigned char* curr_bn = (const unsigned char*)(key + 1);

if (key_length != ntohl(key->size)) {
if (key_length != be32toh(key->size)) {
return SOTER_INVALID_PARAMETER;
}

Expand Down Expand Up @@ -435,7 +437,7 @@ soter_status_t soter_rsa_priv_key_to_engine_specific(const soter_container_hdr_t

pub_exp = (const uint32_t*)(curr_bn + ((rsa_mod_size * 4) + (rsa_mod_size / 2)));
;
switch (ntohl(*pub_exp)) {
switch (be32toh(*pub_exp)) {
case RSA_3:
case RSA_F4:
break;
Expand All @@ -454,7 +456,7 @@ soter_status_t soter_rsa_priv_key_to_engine_specific(const soter_container_hdr_t
return SOTER_NO_MEMORY;
}

if (!BN_set_word(rsa->e, ntohl(*pub_exp))) {
if (!BN_set_word(rsa->e, be32toh(*pub_exp))) {
RSA_free(rsa);
return SOTER_FAIL;
}
Expand Down
10 changes: 6 additions & 4 deletions src/soter/openssl/soter_ec_key.c
View file
Expand Up @@ -21,6 +21,8 @@
#include <openssl/ec.h>
#include <openssl/evp.h>

#include "soter/portable_endian.h"

static bool is_curve_supported(int curve)
{
switch (curve) {
Expand Down Expand Up @@ -160,7 +162,7 @@ soter_status_t soter_engine_specific_to_ec_pub_key(const soter_engine_specific_e
}

memcpy(key->tag, ec_pub_key_tag(curve), SOTER_CONTAINER_TAG_LENGTH);
key->size = htonl(output_length);
key->size = htobe32(output_length);
soter_update_container_checksum(key);
*key_length = output_length;
res = SOTER_SUCCESS;
Expand Down Expand Up @@ -227,7 +229,7 @@ soter_status_t soter_engine_specific_to_ec_priv_key(const soter_engine_specific_
}

memcpy(key->tag, ec_priv_key_tag(curve), SOTER_CONTAINER_TAG_LENGTH);
key->size = htonl(output_length);
key->size = htobe32(output_length);
soter_update_container_checksum(key);
*key_length = output_length;
res = SOTER_SUCCESS;
Expand All @@ -254,7 +256,7 @@ soter_status_t soter_ec_pub_key_to_engine_specific(const soter_container_hdr_t*
return SOTER_INVALID_PARAMETER;
}

if (key_length != ntohl(key->size)) {
if (key_length != be32toh(key->size)) {
return SOTER_INVALID_PARAMETER;
}

Expand Down Expand Up @@ -351,7 +353,7 @@ soter_status_t soter_ec_priv_key_to_engine_specific(const soter_container_hdr_t*
EVP_PKEY* pkey = (EVP_PKEY*)(*engine_key);
soter_status_t res;

if (key_length != ntohl(key->size)) {
if (key_length != be32toh(key->size)) {
return SOTER_INVALID_PARAMETER;
}

Expand Down
26 changes: 14 additions & 12 deletions src/soter/openssl/soter_rsa_key.c
View file
Expand Up @@ -21,6 +21,8 @@
#include <openssl/evp.h>
#include <openssl/rsa.h>

#include "soter/portable_endian.h"

static size_t rsa_pub_key_size(int mod_size)
{
switch (mod_size) {
Expand Down Expand Up @@ -165,9 +167,9 @@ soter_status_t soter_engine_specific_to_rsa_pub_key(const soter_engine_specific_
RSA_get0_key(rsa, (const BIGNUM**)&rsa_n, &rsa_e, NULL);
#endif
if (BN_is_word(rsa_e, RSA_F4)) {
*pub_exp = htonl(RSA_F4);
*pub_exp = htobe32(RSA_F4);
} else if (BN_is_word(rsa_e, RSA_3)) {
*pub_exp = htonl(RSA_3);
*pub_exp = htobe32(RSA_3);
} else {
res = SOTER_INVALID_PARAMETER;
goto err;
Expand All @@ -179,7 +181,7 @@ soter_status_t soter_engine_specific_to_rsa_pub_key(const soter_engine_specific_
}

memcpy(key->tag, rsa_pub_key_tag(rsa_mod_size), SOTER_CONTAINER_TAG_LENGTH);
key->size = htonl(output_length);
key->size = htobe32(output_length);
soter_update_container_checksum(key);
*key_length = output_length;
res = SOTER_SUCCESS;
Expand Down Expand Up @@ -241,9 +243,9 @@ soter_status_t soter_engine_specific_to_rsa_priv_key(const soter_engine_specific
RSA_get0_key(rsa, &rsa_n, &rsa_e, &rsa_d);
#endif
if (BN_is_word(rsa_e, RSA_F4)) {
*pub_exp = htonl(RSA_F4);
*pub_exp = htobe32(RSA_F4);
} else if (BN_is_word(rsa_e, RSA_3)) {
*pub_exp = htonl(RSA_3);
*pub_exp = htobe32(RSA_3);
} else {
res = SOTER_INVALID_PARAMETER;
goto err;
Expand Down Expand Up @@ -318,7 +320,7 @@ soter_status_t soter_engine_specific_to_rsa_priv_key(const soter_engine_specific
}

memcpy(key->tag, rsa_priv_key_tag(rsa_mod_size), SOTER_CONTAINER_TAG_LENGTH);
key->size = htonl(output_length);
key->size = htobe32(output_length);
soter_update_container_checksum(key);
*key_length = output_length;
res = SOTER_SUCCESS;
Expand Down Expand Up @@ -347,7 +349,7 @@ soter_status_t soter_rsa_pub_key_to_engine_specific(const soter_container_hdr_t*
EVP_PKEY* pkey = (EVP_PKEY*)(*engine_key);
const uint32_t* pub_exp;

if (key_length != ntohl(key->size)) {
if (key_length != be32toh(key->size)) {
return SOTER_INVALID_PARAMETER;
}

Expand Down Expand Up @@ -382,7 +384,7 @@ soter_status_t soter_rsa_pub_key_to_engine_specific(const soter_container_hdr_t*
}

pub_exp = (const uint32_t*)((unsigned char*)(key + 1) + rsa_mod_size);
switch (ntohl(*pub_exp)) {
switch (be32toh(*pub_exp)) {
case RSA_3:
case RSA_F4:
break;
Expand All @@ -406,7 +408,7 @@ soter_status_t soter_rsa_pub_key_to_engine_specific(const soter_container_hdr_t*
return SOTER_NO_MEMORY;
}

if (!BN_set_word(rsa_e, ntohl(*pub_exp))) {
if (!BN_set_word(rsa_e, be32toh(*pub_exp))) {
BN_free(rsa_n);
BN_free(rsa_e);
RSA_free(rsa);
Expand Down Expand Up @@ -447,7 +449,7 @@ soter_status_t soter_rsa_priv_key_to_engine_specific(const soter_container_hdr_t
const uint32_t* pub_exp;
const unsigned char* curr_bn = (const unsigned char*)(key + 1);

if (key_length != ntohl(key->size)) {
if (key_length != be32toh(key->size)) {
return SOTER_INVALID_PARAMETER;
}

Expand Down Expand Up @@ -483,7 +485,7 @@ soter_status_t soter_rsa_priv_key_to_engine_specific(const soter_container_hdr_t

pub_exp = (const uint32_t*)(curr_bn + ((rsa_mod_size * 4) + (rsa_mod_size / 2)));
;
switch (ntohl(*pub_exp)) {
switch (be32toh(*pub_exp)) {
case RSA_3:
case RSA_F4:
break;
Expand All @@ -501,7 +503,7 @@ soter_status_t soter_rsa_priv_key_to_engine_specific(const soter_container_hdr_t
return SOTER_NO_MEMORY;
}

if (!BN_set_word(rsa_e, ntohl(*pub_exp))) {
if (!BN_set_word(rsa_e, be32toh(*pub_exp))) {
RSA_free(rsa);
return SOTER_FAIL;
}
Expand Down

0 comments on commit 57485fb

Please sign in to comment.