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.

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

RFC: Fix builds with MBEDTLS_HAVE_TIME disabled and test #3624

Merged
merged 17 commits into from
Mar 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 3 additions & 0 deletions ChangeLog.d/timeless.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Bugfix
* Fix compile errors when MBEDTLS_HAVE_TIME is not defined. Add tests
to catch bad uses of time.h.
2 changes: 2 additions & 0 deletions include/mbedtls/platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ extern "C" {
#if !defined(MBEDTLS_PLATFORM_NO_STD_FUNCTIONS)
#include <stdio.h>
#include <stdlib.h>
#if defined(MBEDTLS_HAVE_TIME)
#include <time.h>
#endif
#if !defined(MBEDTLS_PLATFORM_STD_SNPRINTF)
#if defined(MBEDTLS_PLATFORM_HAS_NON_CONFORMING_SNPRINTF)
#define MBEDTLS_PLATFORM_STD_SNPRINTF mbedtls_platform_win32_snprintf /**< The default \c snprintf function to use. */
Expand Down
2 changes: 2 additions & 0 deletions library/net_sockets.c
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,9 @@ static int wsa_init_done = 0;

#include <stdio.h>

#if defined(MBEDTLS_HAVE_TIME)
#include <time.h>
#endif

#include <stdint.h>

Expand Down
27 changes: 24 additions & 3 deletions library/timing.c
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ struct _hr_time

#include <unistd.h>
#include <sys/types.h>
#include <sys/time.h>
#include <signal.h>
#if defined(MBEDTLS_HAVE_TIME)
#include <time.h>

#include <sys/time.h>
struct _hr_time
{
struct timeval start;
};

#endif
#endif /* _WIN32 && !EFIX64 && !EFI32 */

/**
Expand All @@ -75,6 +75,7 @@ struct _hr_time
* get_timer(0) }` the value time1+time2 is only approximately
* the delay since the first reset.
*/
#if defined(MBEDTLS_HAVE_TIME)
#if defined(_WIN32) && !defined(EFIX64) && !defined(EFI32)

unsigned long mbedtls_timing_get_timer( struct mbedtls_timing_hr_time *val, int reset )
Expand Down Expand Up @@ -157,6 +158,26 @@ int mbedtls_timing_get_delay( void *data )

return( 0 );
}
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

09e803c introduces a dummy implementation of timer functions. Now, when defined(MBEDTLS_TIMING_C) && !defined(MBEDTLS_TIMING_ALT) && !defined(MBEDTLS_HAVE_TIME) (did I get this condition right?), the functions mbedtls_timing_get_timer, mbedtls_timing_set_delay and mbedtls_timing_get_delay exist but they pretend that no time ever elapses.

What is the point of this dummy implementation? Why not make MBEDTLS_TIMING_C require MBEDTLS_HAVE_TIME || MBEDTLS_TIMING_ALT, since this seems to be necessary to implement the timing functions?

The dummy implementation breaks #5582 and I'm trying to figure out how to fix this. Before #3624, in #5582, I made a configuration config-ccm-psk-dtls1_2.h with MBEDTLS_TIMING_C enabled but not MBEDTLS_HAVE_TIME, and I got ssl-opt.sh to pass. Since #3624, the test cases in ssl-opt.sh that use udp_proxy … pack=50 are failing, because the pack option of the proxy does not work with the dummy implementation of mbedtls_timing_get_timer. The pack options causes the proxy to accumulate packets until a certain amount of time has elapsed, but with the dummy implementation, this never happens, so the proxy never forwards any packet.

Currently, in udp_proxy.c, opt.pack is only accepted if defined(MBEDTLS_TIMING_C). Should this change to … && (defined(MBEDTLS_HAVE_TIME) || defined(MBEDTLS_TIMING_ALT))?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall the discussion correctly (and looking at mbedtls_config.h), MBEDTLS_TIMING_C itself provides only the interface that is to be filled by either MBEDTLS_HAVE_TIME, or MBEDTLS_TIMING_ALT.
I would assume that if someone wants to have the interface, but does not wish to provide any working stub - this is the way to go. I don't know why would this be desirable, but that's the way it works now.
It might make sense to go a step further, add dependencies for MBEDTLS_TIMING_C as you mentioned and get rid of the dummy code, but this was out of scope for this PR.
For now, yes, to have a working implementation, (defined(MBEDTLS_HAVE_TIME) || defined(MBEDTLS_TIMING_ALT)) is the way to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm inclined to think those three functions should depend on defined(MBEDTLS_HAVE_TIME) || defined(MBEDTLS_TIMING_ALT) and not be declared otherwise. (Just like for example, mbedtls_ssl_cache_set_timeout() is only declared if HAVE_TIME is defined.) Not being defined seems better than having a dummy implementation that's not suitable for any use: when something doesn't work, it's better for the error to be at compile-time than at runtime.

Now, whether that's a change we can make now without breaking backwards compat as another question. If the difference only happens in builds that were anyway not working in the previous release, then we're good, otherwise we're stuck.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we take the dummy implementation out there will be several compile failures - it's on my list to check, hopefully later today

Copy link
Contributor

@AndrzejKurek AndrzejKurek Apr 6, 2022

Choose a reason for hiding this comment

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

I think if we take the dummy implementation out there will be several compile failures - it's on my list to check, hopefully later today

I have done that already - with the default config minus HAVE_TIME and HAVE_TIME_DATE, but with TIMING_C, test_suite_ssl and test_suite_timing will not compile, also dtls_server, dtls_client and ssl_client2 from programs will fail to compile.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you take mbedtls-3.1 and build with default config minus MBEDTLS_HAVE_TIME and MBEDTLS_HAVE_TIME_DATE on a platform that actually has a working time.h, make lib test works but make programs doesn't. The build of the library breaks in this same configuration if time.h is not available — which was the purpose of this PR.

Is it acceptable for us to break custom configurations that accidentally work because the developer didn't include MBEDTLS_HAVE_TIME in the configuration, but could have? If we do, at the very least, this should be prominently mentioned in the changelog entry.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you take mbedtls-3.1 and build with default config minus MBEDTLS_HAVE_TIME and MBEDTLS_HAVE_TIME_DATE on a platform that actually has a working time.h, make lib test works but make programs doesn't

So you are removing any implementation (by saying "don't have time or date" and not providing _ALT) but leaving MBEDTLS_TIMING_C set, so this seems reasonable if we say that for MBEDTLS_TIMING_C you need either "have time/date" or _ALT

Copy link
Contributor

Choose a reason for hiding this comment

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

The build of the library breaks in this same configuration if time.h is not available

Err, I find that the library doesn't try to include a time.h file if I take development and just disable MBEDTLS_HAVE_TIME and MBEDTLS_HAVE_TIME_DATE - let me investigate further

Copy link
Contributor

Choose a reason for hiding this comment

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

@tom-cosgrove-arm Before this PR (e.g. in 3.1), platform.h included time.h even when MBEDTLS_HAVE_TIME was disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gilles-peskine-arm So I have double-checked: if I take a working Ubuntu build system, rename time.h out of the way, take Mbed TLS development and disable only MBEDTLS_HAVE_TIME and MBEDTLS_HAVE_TIME_DATE, the whole thing builds (as you say, that was the purpose of the PR). i.e. I can't reproduce your failure

int mbedtls_timing_get_delay( void *data )
{
(void) data;
return( 0 );
}

void mbedtls_timing_set_delay( void *data, uint32_t int_ms, uint32_t fin_ms )
{
(void) data;
(void) int_ms;
(void) fin_ms;
}

unsigned long mbedtls_timing_get_timer( struct mbedtls_timing_hr_time *val, int reset )
{
(void) val;
(void) reset;
return( 0 );
}
#endif /* MBEDTLS_HAVE_TIME */
#endif /* !MBEDTLS_TIMING_ALT */
#endif /* MBEDTLS_TIMING_C */
2 changes: 2 additions & 0 deletions library/x509_crl.c
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,13 @@
#define mbedtls_snprintf snprintf
#endif

#if defined(MBEDTLS_HAVE_TIME)
#if defined(_WIN32) && !defined(EFIX64) && !defined(EFI32)
#include <windows.h>
#else
#include <time.h>
#endif
#endif

#if defined(MBEDTLS_FS_IO) || defined(EFIX64) || defined(EFI32)
#include <stdio.h>
Expand Down
2 changes: 2 additions & 0 deletions library/x509_crt.c
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,13 @@
#include "mbedtls/threading.h"
#endif

#if defined(MBEDTLS_HAVE_TIME)
#if defined(_WIN32) && !defined(EFIX64) && !defined(EFI32)
#include <windows.h>
#else
#include <time.h>
#endif
#endif

#if defined(MBEDTLS_FS_IO)
#include <stdio.h>
Expand Down
2 changes: 2 additions & 0 deletions programs/fuzz/common.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@
#include <stdlib.h>
#include "mbedtls/ctr_drbg.h"

#if defined(MBEDTLS_PLATFORM_TIME_ALT)
mbedtls_time_t dummy_constant_time( mbedtls_time_t* time )
{
(void) time;
return 0x5af2a056;
}
#endif

void dummy_init()
{
Expand Down
7 changes: 7 additions & 0 deletions programs/fuzz/common.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
#include "mbedtls/build_info.h"

#if defined(MBEDTLS_HAVE_TIME)
#include "mbedtls/platform_time.h"
#endif
#include <stddef.h>
#include <stdint.h>

typedef struct fuzzBufferOffset
Expand All @@ -8,7 +13,9 @@ typedef struct fuzzBufferOffset
size_t Offset;
} fuzzBufferOffset_t;

#if defined(MBEDTLS_HAVE_TIME)
mbedtls_time_t dummy_constant_time( mbedtls_time_t* time );
#endif
void dummy_init();

int dummy_send( void *ctx, const unsigned char *buf, size_t len );
Expand Down
13 changes: 10 additions & 3 deletions programs/ssl/ssl_context_info.c
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ int main( void )
#include <stdint.h>
#include <stdarg.h>
#include <string.h>
#if defined(MBEDTLS_HAVE_TIME)
#include <time.h>
#endif
#include "mbedtls/ssl.h"
#include "mbedtls/error.h"
#include "mbedtls/base64.h"
Expand Down Expand Up @@ -307,10 +309,11 @@ void print_hex( const uint8_t *b, size_t len,
/*
* Print the value of time_t in format e.g. 2020-01-23 13:05:59
*/
void print_time( const time_t *time )
void print_time( const uint64_t *time )
{
#if defined(MBEDTLS_HAVE_TIME)
char buf[20];
struct tm *t = gmtime( time );
struct tm *t = gmtime( (time_t*) time );
static const char format[] = "%Y-%m-%d %H:%M:%S";
if( NULL != t )
{
Expand All @@ -321,6 +324,10 @@ void print_time( const time_t *time )
{
printf( "unknown\n" );
}
#else
(void) time;
printf( "not supported\n" );
#endif
}

/*
Expand Down Expand Up @@ -608,7 +615,7 @@ void print_deserialized_ssl_session( const uint8_t *ssl, uint32_t len,
( (uint64_t) ssl[7] );
ssl += 8;
printf( "\tstart time : " );
print_time( (time_t*) &start );
print_time( &start );
}

CHECK_SSL_END( 2 );
Expand Down
19 changes: 17 additions & 2 deletions programs/ssl/ssl_server2.c
Original file line number Diff line number Diff line change
Expand Up @@ -317,10 +317,16 @@ int main( void )

#if defined(MBEDTLS_SSL_CACHE_C)
#define USAGE_CACHE \
" cache_max=%%d default: cache default (50)\n" \
" cache_max=%%d default: cache default (50)\n"
#if defined(MBEDTLS_HAVE_TIME)
#define USAGE_CACHE_TIME \
" cache_timeout=%%d default: cache default (1d)\n"
#else
#define USAGE_CACHE_TIME ""
#endif
#else
#define USAGE_CACHE ""
#define USAGE_CACHE_TIME ""
#endif /* MBEDTLS_SSL_CACHE_C */

#if defined(SNI_OPTION)
Expand Down Expand Up @@ -509,6 +515,7 @@ int main( void )
USAGE_NSS_KEYLOG \
USAGE_NSS_KEYLOG_FILE \
USAGE_CACHE \
USAGE_CACHE_TIME \
USAGE_MAX_FRAG_LEN \
USAGE_ALPN \
USAGE_EMS \
Expand Down Expand Up @@ -619,7 +626,9 @@ struct options
int ticket_timeout; /* session ticket lifetime */
int ticket_aead; /* session ticket protection */
int cache_max; /* max number of session cache entries */
int cache_timeout; /* expiration delay of session cache entries */
#if defined(MBEDTLS_HAVE_TIME)
int cache_timeout; /* expiration delay of session cache entries*/
#endif
char *sni; /* string describing sni information */
const char *curves; /* list of supported elliptic curves */
const char *sig_algs; /* supported TLS 1.3 signature algorithms */
Expand Down Expand Up @@ -1549,7 +1558,9 @@ int main( int argc, char *argv[] )
opt.ticket_timeout = DFL_TICKET_TIMEOUT;
opt.ticket_aead = DFL_TICKET_AEAD;
opt.cache_max = DFL_CACHE_MAX;
#if defined(MBEDTLS_HAVE_TIME)
opt.cache_timeout = DFL_CACHE_TIMEOUT;
#endif
opt.sni = DFL_SNI;
opt.alpn_string = DFL_ALPN_STRING;
opt.curves = DFL_CURVES;
Expand Down Expand Up @@ -1945,12 +1956,14 @@ int main( int argc, char *argv[] )
if( opt.cache_max < 0 )
goto usage;
}
#if defined(MBEDTLS_HAVE_TIME)
else if( strcmp( p, "cache_timeout" ) == 0 )
{
opt.cache_timeout = atoi( q );
if( opt.cache_timeout < 0 )
goto usage;
}
#endif
else if( strcmp( p, "cookies" ) == 0 )
{
opt.cookies = atoi( q );
Expand Down Expand Up @@ -2723,8 +2736,10 @@ int main( int argc, char *argv[] )
if( opt.cache_max != -1 )
mbedtls_ssl_cache_set_max_entries( &cache, opt.cache_max );

#if defined(MBEDTLS_HAVE_TIME)
if( opt.cache_timeout != -1 )
mbedtls_ssl_cache_set_timeout( &cache, opt.cache_timeout );
#endif

mbedtls_ssl_conf_session_cache( &conf, &cache,
mbedtls_ssl_cache_get,
Expand Down
2 changes: 2 additions & 0 deletions programs/ssl/ssl_test_lib.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,13 @@ void my_debug( void *ctx, int level,
fflush( (FILE *) ctx );
}

#if defined(MBEDTLS_HAVE_TIME)
mbedtls_time_t dummy_constant_time( mbedtls_time_t* time )
{
(void) time;
return 0x5af2a056;
}
#endif

#if !defined(MBEDTLS_TEST_USE_PSA_CRYPTO_RNG)
static int dummy_entropy( void *data, unsigned char *output, size_t len )
Expand Down
2 changes: 2 additions & 0 deletions programs/ssl/ssl_test_lib.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,9 @@ void my_debug( void *ctx, int level,
const char *file, int line,
const char *str );

#if defined(MBEDTLS_HAVE_TIME)
mbedtls_time_t dummy_constant_time( mbedtls_time_t* time );
#endif

#if defined(MBEDTLS_USE_PSA_CRYPTO)
/* If MBEDTLS_TEST_USE_PSA_CRYPTO_RNG is defined, the SSL test programs will use
Expand Down
8 changes: 3 additions & 5 deletions programs/test/benchmark.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,17 @@
#define mbedtls_free free
#endif

#if !defined(MBEDTLS_TIMING_C)
#if !defined(MBEDTLS_HAVE_TIME)
int main( void )
{
mbedtls_printf("MBEDTLS_TIMING_C not defined.\n");
mbedtls_printf("MBEDTLS_HAVE_TIME not defined.\n");
mbedtls_exit( 0 );
}
#else

#include <string.h>
#include <stdlib.h>

#include "mbedtls/timing.h"

#include "mbedtls/md5.h"
#include "mbedtls/ripemd160.h"
#include "mbedtls/sha1.h"
Expand Down Expand Up @@ -1304,4 +1302,4 @@ int main( int argc, char *argv[] )
mbedtls_exit( 0 );
}

#endif /* MBEDTLS_TIMING_C */
#endif /* MBEDTLS_HAVE_TIME */
8 changes: 7 additions & 1 deletion programs/test/udp_proxy.c
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,11 @@
#else
#include <stdio.h>
#include <stdlib.h>
#if defined(MBEDTLS_HAVE_TIME)
#include <time.h>
#define mbedtls_time time
#define mbedtls_time_t time_t
#endif
#define mbedtls_printf printf
#define mbedtls_calloc calloc
#define mbedtls_free free
Expand Down Expand Up @@ -71,7 +73,9 @@ int main( void )
#endif
#endif /* _MSC_VER */
#else /* ( _WIN32 || _WIN32_WCE ) && !EFIX64 && !EFI32 */
#if defined(MBEDTLS_HAVE_TIME)
#include <sys/time.h>
#endif
#include <sys/types.h>
#include <unistd.h>
#endif /* ( _WIN32 || _WIN32_WCE ) && !EFIX64 && !EFI32 */
Expand Down Expand Up @@ -821,6 +825,7 @@ int main( int argc, char *argv[] )

get_options( argc, argv );

#if defined(MBEDTLS_HAVE_TIME)
/*
* Decisions to drop/delay/duplicate packets are pseudo-random: dropping
* exactly 1 in N packets would lead to problems when a flight has exactly
Expand All @@ -831,11 +836,12 @@ int main( int argc, char *argv[] )
*/
if( opt.seed == 0 )
{
opt.seed = (unsigned int) time( NULL );
opt.seed = (unsigned int) mbedtls_time( NULL );
mbedtls_printf( " . Pseudo-random seed: %u\n", opt.seed );
}

srand( opt.seed );
#endif /* MBEDTLS_HAVE_TIME */

/*
* 0. "Connect" to the server
Expand Down
2 changes: 2 additions & 0 deletions scripts/data_files/query_config.fmt
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@
#include "mbedtls/pk.h"
#include "mbedtls/pkcs12.h"
#include "mbedtls/pkcs5.h"
#if defined(MBEDTLS_HAVE_TIME)
#include "mbedtls/platform_time.h"
#endif
#include "mbedtls/platform_util.h"
#include "mbedtls/poly1305.h"
#include "mbedtls/ripemd160.h"
Expand Down
3 changes: 3 additions & 0 deletions tests/configs/config-wrapper-malloc-0-null.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
#include "mbedtls/mbedtls_config.h"

#include <stdlib.h>

#ifndef MBEDTLS_PLATFORM_STD_CALLOC
static inline void *custom_calloc( size_t nmemb, size_t size )
{
if( nmemb == 0 || size == 0 )
Expand All @@ -30,3 +32,4 @@ static inline void *custom_calloc( size_t nmemb, size_t size )

#define MBEDTLS_PLATFORM_MEMORY
#define MBEDTLS_PLATFORM_STD_CALLOC custom_calloc
#endif
18 changes: 18 additions & 0 deletions tests/include/baremetal-override/time.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* 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.
*/

#error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
Loading