Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduced a PRIVATE(member) macro #4409

Closed
wants to merge 3 commits into from
Closed

Introduced a PRIVATE(member) macro #4409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be9a842
Introduced a PRIVATE(member) macro, to make accessing
mstarzyk-mobica Apr 23, 2021
888c16c
Move enabling structs private members access to a header file.
mstarzyk-mobica Apr 27, 2021
38b3ff4
Move MBEDTLS_ALLOW_PRIVATE macro to the common tests header.
mstarzyk-mobica Apr 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion include/mbedtls/md.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#else
#include MBEDTLS_CONFIG_FILE
#endif
#include "mbedtls/private_access.h"

#define MBEDTLS_ERR_MD_FEATURE_UNAVAILABLE -0x5080 /**< The selected feature is not available. */
#define MBEDTLS_ERR_MD_BAD_INPUT_DATA -0x5100 /**< Bad input parameters to function. */
Expand Down Expand Up @@ -99,7 +100,7 @@ typedef struct mbedtls_md_context_t
const mbedtls_md_info_t *md_info;

/** The digest-specific context. */
void *md_ctx;
void *MBEDTLS_PRIVATE(md_ctx);
gilles-peskine-arm marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely to confuse source code navigation. I don't know if this really matters: do we often look up field names (as opposed to function or type names) when working on the library?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't speak for others, but I never look up field names. So I don't think the disruption to source code navigation is a big issue here.


/** The HMAC part of the context. */
void *hmac_ctx;
Expand Down
32 changes: 32 additions & 0 deletions include/mbedtls/private_access.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* \file private_access.h
*
* \brief Macro wrapper for struct's memebrs.
*/
/*
* Copyright The Mbed TLS Contributors
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef MBEDTLS_PRIVATE_ACCESS_H
#define MBEDTLS_PRIVATE_ACCESS_H

#ifndef MBEDTLS_ALLOW_PRIVATE_ACCESS
#define MBEDTLS_PRIVATE(member) private_##member
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the name of struct members is not guaranteed to result in compatible object files. I expect this works with most compilers when invoked in an ordinary way, but does it work with link-time optimization? And even if it does, are we comfortable taking the risk that it will break in some environments, perhaps in subtle ways resulting in access to the wrong field (although most likely breaking with a link error)?

Copy link

@hanno-becker hanno-becker May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate how changing the name of a struct member could, possibly and/or realistically, result in incompatible object files?

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, as far as I can tell from reading the C standard (i.e. I haven't found a provision to the contrary), a compiler could base padding on the field names and not just on the field types. (Not the order: a field always has to be at a higher address than the previous field.) In other words, given

struct public { a_t a; b_t b; c_t c; };
struct private { a_t private_a; b_t private_b; c_t private_c; };

it's possible, but “exotic”, that offsetof(struct private, private_b) != offsetof(struct public, private_b). The C standard only guarantees that offsetof(…a) == 0 and offsetof(…b) >= sizeof(a_t) and offsetof(…c) >= offsetof(…b) + sizeof(b_t).

I can't think of a plausible reason why a compiler would do this, but it could. Maybe some build instrumented for runtime debugging that includes the field name after each field?

For bit-fields, N1256 states “The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined”. So something like ordering field names by hash value would not be permitted, but basing the choice between low-to-high or high-to-low on parity of the total number of letters in field names would be as long as it's documented (but of course it would be really bizarre).

With link-time optimization, the link-time optimizer may try to access struct members and error out because a field doesn't exist. I have not checked any compiler's LTO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now checked LTO with GCC 10 and Clang 10. This PR passes the unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had a similar concern, but then thought (1) I think it's unlikely to matter in practice for common compilers with common options, and (2) even if it happens with some compiler/option, I think people using those exotic settings can always define MBEDTLS_ALLOW_PRIVATE_ACCESS on the command line to get rid of the renaming.

Again, the point of this is not to fully prevent users from accessing private fields, but to help them notice if they do it inadvertently. I think it's OK for people using hypothetical exotic build settings to not have access to that layer of protection. (And actually, they could still in their CI do an extra build with more standard settings to catch any unintentional uses of private fields, since this would be compile-time error.)

Thanks for checking what the standard says and how it works with LTO.

#else
#define MBEDTLS_PRIVATE(member) member
#endif

#endif /* MBEDTLS_PRIVATE_ACCESS_H */
9 changes: 5 additions & 4 deletions include/psa/crypto_extra.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#ifndef PSA_CRYPTO_EXTRA_H
#define PSA_CRYPTO_EXTRA_H

#include "mbedtls/private_access.h"
#include "mbedtls/platform_util.h"

#include "crypto_compat.h"
Expand Down Expand Up @@ -71,7 +72,7 @@ static inline void psa_set_key_enrollment_algorithm(
psa_key_attributes_t *attributes,
psa_algorithm_t alg2)
{
attributes->core.policy.alg2 = alg2;
attributes->MBEDTLS_PRIVATE(core).policy.alg2 = alg2;
}

/** Retrieve the enrollment algorithm policy from key attributes.
Expand All @@ -83,7 +84,7 @@ static inline void psa_set_key_enrollment_algorithm(
static inline psa_algorithm_t psa_get_key_enrollment_algorithm(
const psa_key_attributes_t *attributes)
{
return( attributes->core.policy.alg2 );
return( attributes->MBEDTLS_PRIVATE(core).policy.alg2 );
}

#if defined(MBEDTLS_PSA_CRYPTO_SE_C)
Expand Down Expand Up @@ -141,7 +142,7 @@ static inline void psa_set_key_slot_number(
psa_key_attributes_t *attributes,
psa_key_slot_number_t slot_number )
{
attributes->core.flags |= MBEDTLS_PSA_KA_FLAG_HAS_SLOT_NUMBER;
attributes->MBEDTLS_PRIVATE(core).flags |= MBEDTLS_PSA_KA_FLAG_HAS_SLOT_NUMBER;
attributes->slot_number = slot_number;
}

Expand All @@ -154,7 +155,7 @@ static inline void psa_set_key_slot_number(
static inline void psa_clear_key_slot_number(
psa_key_attributes_t *attributes )
{
attributes->core.flags &= ~MBEDTLS_PSA_KA_FLAG_HAS_SLOT_NUMBER;
attributes->MBEDTLS_PRIVATE(core).flags &= ~MBEDTLS_PSA_KA_FLAG_HAS_SLOT_NUMBER;
}

/** Register a key that is already present in a secure element.
Expand Down
39 changes: 20 additions & 19 deletions include/psa/crypto_struct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ extern "C" {

#include "mbedtls/cmac.h"
#include "mbedtls/gcm.h"
#include "mbedtls/private_access.h"

/* Include the context definition for the compiled-in drivers */
#include "psa/crypto_driver_contexts.h"
Expand Down Expand Up @@ -328,7 +329,7 @@ typedef struct

struct psa_key_attributes_s
{
psa_core_key_attributes_t core;
psa_core_key_attributes_t MBEDTLS_PRIVATE(core);
#if defined(MBEDTLS_PSA_CRYPTO_SE_C)
psa_key_slot_number_t slot_number;
#endif /* MBEDTLS_PSA_CRYPTO_SE_C */
Expand All @@ -351,13 +352,13 @@ static inline struct psa_key_attributes_s psa_key_attributes_init( void )
static inline void psa_set_key_id( psa_key_attributes_t *attributes,
mbedtls_svc_key_id_t key )
{
psa_key_lifetime_t lifetime = attributes->core.lifetime;
psa_key_lifetime_t lifetime = attributes->MBEDTLS_PRIVATE(core).lifetime;

attributes->core.id = key;
attributes->MBEDTLS_PRIVATE(core).id = key;

if( PSA_KEY_LIFETIME_IS_VOLATILE( lifetime ) )
{
attributes->core.lifetime =
attributes->MBEDTLS_PRIVATE(core).lifetime =
PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION(
PSA_KEY_LIFETIME_PERSISTENT,
PSA_KEY_LIFETIME_GET_LOCATION( lifetime ) );
Expand All @@ -367,59 +368,59 @@ static inline void psa_set_key_id( psa_key_attributes_t *attributes,
static inline mbedtls_svc_key_id_t psa_get_key_id(
const psa_key_attributes_t *attributes)
{
return( attributes->core.id );
return( attributes->MBEDTLS_PRIVATE(core).id );
}

#ifdef MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER
static inline void mbedtls_set_key_owner_id( psa_key_attributes_t *attributes,
mbedtls_key_owner_id_t owner )
{
attributes->core.id.owner = owner;
attributes->MBEDTLS_PRIVATE(core).id.owner = owner;
}
#endif

static inline void psa_set_key_lifetime(psa_key_attributes_t *attributes,
psa_key_lifetime_t lifetime)
{
attributes->core.lifetime = lifetime;
attributes->MBEDTLS_PRIVATE(core).lifetime = lifetime;
if( PSA_KEY_LIFETIME_IS_VOLATILE( lifetime ) )
{
#ifdef MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER
attributes->core.id.key_id = 0;
attributes->MBEDTLS_PRIVATE(core).id.key_id = 0;
#else
attributes->core.id = 0;
attributes->MBEDTLS_PRIVATE(core).id = 0;
#endif
}
}

static inline psa_key_lifetime_t psa_get_key_lifetime(
const psa_key_attributes_t *attributes)
{
return( attributes->core.lifetime );
return( attributes->MBEDTLS_PRIVATE(core).lifetime );
}

static inline void psa_set_key_usage_flags(psa_key_attributes_t *attributes,
psa_key_usage_t usage_flags)
{
attributes->core.policy.usage = usage_flags;
attributes->MBEDTLS_PRIVATE(core).policy.usage = usage_flags;
}

static inline psa_key_usage_t psa_get_key_usage_flags(
const psa_key_attributes_t *attributes)
{
return( attributes->core.policy.usage );
return( attributes->MBEDTLS_PRIVATE(core).policy.usage );
}

static inline void psa_set_key_algorithm(psa_key_attributes_t *attributes,
psa_algorithm_t alg)
{
attributes->core.policy.alg = alg;
attributes->MBEDTLS_PRIVATE(core).policy.alg = alg;
}

static inline psa_algorithm_t psa_get_key_algorithm(
const psa_key_attributes_t *attributes)
{
return( attributes->core.policy.alg );
return( attributes->MBEDTLS_PRIVATE(core).policy.alg );
}

/* This function is declared in crypto_extra.h, which comes after this
Expand All @@ -435,7 +436,7 @@ static inline void psa_set_key_type(psa_key_attributes_t *attributes,
if( attributes->domain_parameters == NULL )
{
/* Common case: quick path */
attributes->core.type = type;
attributes->MBEDTLS_PRIVATE(core).type = type;
}
else
{
Expand All @@ -450,22 +451,22 @@ static inline void psa_set_key_type(psa_key_attributes_t *attributes,
static inline psa_key_type_t psa_get_key_type(
const psa_key_attributes_t *attributes)
{
return( attributes->core.type );
return( attributes->MBEDTLS_PRIVATE(core).type );
}

static inline void psa_set_key_bits(psa_key_attributes_t *attributes,
size_t bits)
{
if( bits > PSA_MAX_KEY_BITS )
attributes->core.bits = PSA_KEY_BITS_TOO_LARGE;
attributes->MBEDTLS_PRIVATE(core).bits = PSA_KEY_BITS_TOO_LARGE;
else
attributes->core.bits = (psa_key_bits_t) bits;
attributes->MBEDTLS_PRIVATE(core).bits = (psa_key_bits_t) bits;
}

static inline size_t psa_get_key_bits(
const psa_key_attributes_t *attributes)
{
return( attributes->core.bits );
return( attributes->MBEDTLS_PRIVATE(core).bits );
}

#ifdef __cplusplus
Expand Down
7 changes: 7 additions & 0 deletions library/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,11 @@
#define MBEDTLS_STATIC_TESTABLE static
#endif

/** Allow library to access it's structs' private members.
*
* Although structs defined in header files are publicly available,
* their members are private and should not be accessed by the user.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a short note for now, later on when this task is fully implemented, this will need more in-depth explanation

*/
#define MBEDTLS_ALLOW_PRIVATE_ACCESS

#endif /* MBEDTLS_LIBRARY_COMMON_H */
1 change: 1 addition & 0 deletions library/psa_crypto_driver_wrappers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
* limitations under the License.
*/

#include "common.h"
#include "psa_crypto_aead.h"
#include "psa_crypto_cipher.h"
#include "psa_crypto_core.h"
Expand Down
5 changes: 5 additions & 0 deletions programs/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ APPS = \
ssl/ssl_server2$(EXEXT) \
test/benchmark$(EXEXT) \
test/query_compile_time_config$(EXEXT) \
test/private_access_test$(EXEXT) \
test/selftest$(EXEXT) \
test/udp_proxy$(EXEXT) \
test/zeroize$(EXEXT) \
Expand Down Expand Up @@ -320,6 +321,10 @@ test/udp_proxy$(EXEXT): test/udp_proxy.c $(DEP)
echo " CC test/udp_proxy.c"
$(CC) $(LOCAL_CFLAGS) $(CFLAGS) test/udp_proxy.c $(LOCAL_LDFLAGS) $(LDFLAGS) -o $@

test/private_access_test$(EXEXT): test/private_access_test.c $(DEP)
echo " CC test/private_access_test.c"
$(CC) $(LOCAL_CFLAGS) $(CFLAGS) test/private_access_test.c $(LOCAL_LDFLAGS) $(LDFLAGS) -o $@

test/zeroize$(EXEXT): test/zeroize.c $(DEP)
echo " CC test/zeroize.c"
$(CC) $(LOCAL_CFLAGS) $(CFLAGS) test/zeroize.c $(LOCAL_LDFLAGS) $(LDFLAGS) -o $@
Expand Down
1 change: 1 addition & 0 deletions programs/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ endif(USE_PKCS11_HELPER_LIBRARY)
set(executables_libs
selftest
udp_proxy
private_access_test
)

set(executables_mbedcrypto
Expand Down
46 changes: 46 additions & 0 deletions programs/test/private_access_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* Test applicaiton access to private library components (struct members).
*
* Copyright The Mbed TLS Contributors
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include <stdio.h>
#include <stdlib.h>

#include "mbedtls/md.h"
#include "psa/crypto.h"
#include "psa/crypto_types.h"
#include "psa/crypto_struct.h"

int main( void )
{
/* using static inline function */
psa_key_attributes_t local_crypto_struct = psa_key_attributes_init();
mbedtls_svc_key_id_t id =
mbedtls_svc_key_id_make( 0, 0 );
psa_set_key_id(&local_crypto_struct, id);

/* accessing private member using MBEDTLS_PRIVATE() macro */
mbedtls_md_context_t md_ctx;
mbedtls_md_init( &md_ctx );
const char* t = "A";
md_ctx.MBEDTLS_PRIVATE(md_ctx) = (void*)t;

/* accessing private member without MBEDTLS_PRIVATE() macro - compilation wil fail */
// md_ctx.md_ctx = t;

exit( 0 );
}
2 changes: 2 additions & 0 deletions tests/include/test/helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#ifndef TEST_HELPERS_H
#define TEST_HELPERS_H

#define MBEDTLS_ALLOW_PRIVATE_ACCESS

#if !defined(MBEDTLS_CONFIG_FILE)
#include "mbedtls/config.h"
#else
Expand Down
2 changes: 2 additions & 0 deletions tests/src/drivers/aead.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
#include MBEDTLS_CONFIG_FILE
#endif

#include <test/helpers.h>

#if defined(MBEDTLS_PSA_CRYPTO_DRIVERS) && defined(PSA_CRYPTO_DRIVER_TEST)
#include "psa_crypto_aead.h"

Expand Down
2 changes: 2 additions & 0 deletions tests/src/drivers/cipher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#include MBEDTLS_CONFIG_FILE
#endif

#include <test/helpers.h>

#if defined(MBEDTLS_PSA_CRYPTO_DRIVERS) && defined(PSA_CRYPTO_DRIVER_TEST)
#include "psa/crypto.h"
#include "psa_crypto_cipher.h"
Expand Down
2 changes: 2 additions & 0 deletions tests/src/drivers/key_management.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#include MBEDTLS_CONFIG_FILE
#endif

#include <test/helpers.h>

#if defined(MBEDTLS_PSA_CRYPTO_DRIVERS) && defined(PSA_CRYPTO_DRIVER_TEST)
#include "psa/crypto.h"
#include "psa_crypto_core.h"
Expand Down
2 changes: 2 additions & 0 deletions tests/src/drivers/signature.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#include MBEDTLS_CONFIG_FILE
#endif

#include <test/helpers.h>

#if defined(MBEDTLS_PSA_CRYPTO_DRIVERS) && defined(PSA_CRYPTO_DRIVER_TEST)
#include "psa/crypto.h"
#include "psa_crypto_core.h"
Expand Down
2 changes: 2 additions & 0 deletions tests/suites/helpers.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
/*----------------------------------------------------------------------------*/
/* Headers */

#define MBEDTLS_ALLOW_PRIVATE_ACCESS

#include <test/macros.h>
#include <test/helpers.h>
#include <test/random.h>
Expand Down
4 changes: 0 additions & 4 deletions tests/suites/main_test.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@
#include MBEDTLS_CONFIG_FILE
#endif

#if defined(MBEDTLS_USE_PSA_CRYPTO)
#include "psa/crypto.h"
#endif /* MBEDTLS_USE_PSA_CRYPTO */

/* Test code may use deprecated identifiers only if the preprocessor symbol
* MBEDTLS_TEST_DEPRECATED is defined. When building tests, set
* MBEDTLS_TEST_DEPRECATED explicitly if MBEDTLS_DEPRECATED_WARNING is
Expand Down
Loading