Add limits.h inclusion to ssl_tls.c and udp_proxy.c #1999

redtangent · 2018-08-31T22:54:30Z

Description

The standard C library header limits.h was only being included by check_config.h so was being missing if users provided their own config.h that didn't include check_config.h.

This fixes #1803.

Status

READY

Requires Backporting

Yes
Which branch?
[edited by mpg] mbedtls-2.16 and mbedtls-2.7
[quoting redtangent] I've provided backports with PR #2445 and PR #2446 to mbedtls-2.16 and mbedtls-2.7.

Todos

Tests
Documentation
Changelog updated
Backported

RonEld

Approved.
Though a test for checking such cases is recomended

simonbutcher · 2018-09-03T06:54:47Z

Let's hold this PR until we have a test case for a custom config.h without a check_config.h.

hanno-becker

LGTM

hanno-becker · 2018-09-03T13:27:29Z

I think this PR should be merged and not held back for the purpose of adding a test case of a custom config.h which does not include check_config.h. In fact, I think we should rather ensure that, even for custom configs, check_config.h is included somewhere, to prevent users from accidentally using malformed configurations. But even if we did that and check_config.h was always included somewhere, it's still good style to headers where they are needed, as this PR does.

RonEld · 2018-09-03T13:55:06Z

@hanno-arm I agree that it is good practice to add headers when you need them, however, it will be good to have a way to test it, and this is why a custom config without a call to check_config may be in use

hanno-becker · 2018-09-03T14:30:30Z

@RonEld I disagree - I think check_config.h should never be omitted, and if that's something we can agree upon (point of discussion, of course), then we shouldn't include a test case exercising the opposite.

RonEld · 2018-09-03T14:51:37Z

I agree that check_config.h should never be omitted. I just think that we should have a way to test such cases where there are missing header includes.
Question: Do we really need limits.h included in check_config.h?

RonEld · 2018-09-03T14:56:24Z

Question: Do we really need limits.h included in check_config.h?

Answer: only in one part:

#if CHAR_BIT != 8
#error "mbed TLS requires a platform with 8-bit chars"
#endif

I suggest moving this check to platform.h ( or the c file) , where it is more suited, and remove the limits.h inclusion in check_config.h

redtangent · 2018-09-04T21:36:36Z

Added the test to all.sh as requested. I toyed with the idea of moving limits.h out of check_config.h but then the problem will become wherever it goes, every single include of config.h and therefore check_config.h will also have to include. Better keep it where it is in my view.

RonEld · 2018-09-05T06:08:31Z

@redtangent Thank you for adding the test!

but then the problem will become wherever it goes

I don't understand your point. This is what we want to check. If a file that will need limits.h will fail to compile now, it wll need to add the limits.h file. In my brief check, I removed the limits.h from check_config.h, included it to wherever missing( ssl_tls.c and udp_proxy.c) and didn't encounter compilation errors. Adding to platform.h the check for

#if CHAR_BIT != 8
#error "mbed TLS requires a platform with 8-bit chars"
#endif

should not add a problem, IMHO.
I agree with @hanno-arm that we should be even stricter, byenforcing adding the check_config.h also to custom configs

simonbutcher · 2018-09-05T12:27:46Z

If limits.h is not included, CHAR_BITwill be undefined and evaluate to 0. If we don't include limits.h locally alongside the use of CHAR_BIT, but instead in platform.h, then surely, we need to include platform.h everywhere that check_config.h or config.h is included.

RonEld · 2018-09-05T12:38:08Z

But the check for CHAR_BIT as commented is:

/*
 * We assume CHAR_BIT is 8 in many places. In practice, this is true on our
 * target platforms, so not an issue, but let's just be extra sure.
 */

So, the need for limits.h in check_config.h is only for using CHAR_BIT
If we move the inclusion of limits.h and the CHAR_BIT check to platform.h, we will still have the check, and we don't need to include platform.h everywhere check_config.h or config.h are included.
platform.h is already included in most of the files ( if not all ), so we will encounter the CHAR_BIT check at least once.
If we must keep the check for CHAR_BIT inside check_config.h then I agree we need to keep limits.h as well, but I am not convinced yet that this is a must.

simonbutcher · 2018-09-05T15:24:10Z

Ahhh. I misunderstood - you're proposing to move the check to platform.h as well.

I don't think that's sufficient. Not all client applications will include platform.h, and the library is designed to be highly modular, and allow people to cherry pick the functionality they want. What if they just want X.509? Or Crypto? The platform setup functions are very target specific, and are largely empty. Not everyone will call them.

In my view - better to keep the check in check_config.h.

RonEld · 2018-09-06T07:28:43Z

A quick grep for platfortm.h shows the following:

library/ssl_tls.c:#include "mbedtls/platform.h"
library/x509_crt.c:#include "mbedtls/platform.h"
library/bignum.c:#include "mbedtls/platform.h"
library/net_sockets.c:#include "mbedtls/platform.h"
library/pkwrite.c:#include "mbedtls/platform.h"
library/cipher.c:#include "mbedtls/platform.h"
library/x509_crl.c:#include "mbedtls/platform.h"
library/asn1write.c:#include "mbedtls/platform.h"
library/ctr_drbg.c:#include "mbedtls/platform.h"
library/aria.c:#include "mbedtls/platform.h"
library/pk_wrap.c:#include "mbedtls/platform.h"
library/x509.c:#include "mbedtls/platform.h"
library/ccm.c:#include "mbedtls/platform.h"
library/des.c:#include "mbedtls/platform.h"
library/md.c:#include "mbedtls/platform.h"
library/poly1305.c:#include "mbedtls/platform.h"
library/timing.c:#include "mbedtls/platform.h"
library/sha1.c:#include "mbedtls/platform.h"
library/pkcs11.c:#include "mbedtls/platform.h"
library/arc4.c:#include "mbedtls/platform.h"
library/ecjpake.c:#include "mbedtls/platform.h"
library/entropy.c:#include "mbedtls/platform.h"
library/entropy.c:#include "mbedtls/platform.h"
library/ssl_srv.c.orig:#include "mbedtls/platform.h"
library/sha256.c:#include "mbedtls/platform.h"
library/platform.c:#include "mbedtls/platform.h"
library/md_wrap.c:#include "mbedtls/platform.h"
library/aes.c:#include "mbedtls/platform.h"
library/hmac_drbg.c:#include "mbedtls/platform.h"
library/dhm.c:#include "mbedtls/platform.h"
library/chacha20.c:#include "mbedtls/platform.h"
library/gcm.c:#include "mbedtls/platform.h"
library/ssl_cookie.c:#include "mbedtls/platform.h"
library/memory_buffer_alloc.c:#include "mbedtls/platform.h"
library/ecp.c:#include "mbedtls/platform.h"
library/ssl_ticket.c:#include "mbedtls/platform.h"
library/rsa.c:#include "mbedtls/platform.h"
library/md2.c:#include "mbedtls/platform.h"
library/asn1parse.c:#include "mbedtls/platform.h"
library/cmac.c:#include "mbedtls/platform.h"
library/pem.c:#include "mbedtls/platform.h"
library/ssl_ciphersuites.c:#include "mbedtls/platform.h"
library/nist_kw.c:#include "mbedtls/platform.h"
library/x509_csr.c:#include "mbedtls/platform.h"
library/entropy_poll.c:#include "mbedtls/platform.h"
library/pkparse.c:#include "mbedtls/platform.h"
library/ripemd160.c:#include "mbedtls/platform.h"
library/ssl_cache.c:#include "mbedtls/platform.h"
library/ssl_cli.c:#include "mbedtls/platform.h"
library/ssl_srv.c:#include "mbedtls/platform.h"
library/base64.c:#include "mbedtls/platform.h"
library/oid.c:#include "mbedtls/platform.h"
library/debug.c:#include "mbedtls/platform.h"
library/pkcs5.c:#include "mbedtls/platform.h"
library/md5.c:#include "mbedtls/platform.h"
library/chachapoly.c:#include "mbedtls/platform.h"
library/md4.c:#include "mbedtls/platform.h"
library/camellia.c:#include "mbedtls/platform.h"
library/sha512.c:#include "mbedtls/platform.h"
library/error.c:#include "mbedtls/platform.h"
library/error.c:#include "mbedtls/platform.h"
library/xtea.c:#include "mbedtls/platform.h"
library/cipher_wrap.c:#include "mbedtls/platform.h"

So, I believe this file will be included at least once by any application.
However, I understand that it might clash with the modular design of the library, so I won't insist on this suggestion.

redtangent · 2018-09-09T12:43:55Z

I think in practice, @RonEld, you're right, everything includes platform.h, but that's not intentional nor part of the philosophy of the library, which aims to be very modular.

So, best, in my view, to not place the check there, but to keep it in check_config.h which should be included everywhere.

Maybe we should move including check_config.h outside of every config.h, which we now know some people omit, and instead include it in every .c file? That would remove the need for the all.sh check in this PR.

I personally think that's a better fix. Let me know what you think.

simonbutcher · 2018-09-10T14:23:30Z

@hanno-arm / @RonEld - Can we bring this discussion to a conclusion? And accept as-is, or reject and do rework?

RonEld · 2018-09-12T08:07:31Z

Maybe we should move including check_config.h outside of every config.h, which we now know some people omit, and instead include it in every .c file?

Yes, I agree with you it may be a better solution. I think that in the long run, we should have one common header file, which within will include the config.h, check_config.h and perhaps some additional common information, such as error codes, but this is out of the scope of this PR.
Let's go with your suggestion, by removing from config,h and including it in every .c file.

check_config.h needs to be included directly after the configuration file, because some headers are naughty and adapt the defined configurations, which can cause check_comfig.h to break. (Specically bignum.h sets the configured platform to MBEDTLS_HAVE_INT32/64 if it hasn't been defined). That arguably is a bug, although it's the existing behaviour of the library, so we respect it and move check_config.h to where it wants to be. Signed-off-by: Simon Butcher <simon.butcher@arm.com>

simonbutcher · 2020-05-19T22:41:15Z

This PR is now passing all CI except for Mbed OS. Mbed OS is failing because the absence of #include "check_config.h" breaks one of the Mbed OS import scripts. I've fixed that with PR #12998.

To avoid this PR becoming yet again quickly outdated and requiring another rebase, my recommendation would be to review and merge without waiting for that PR to be merged - if possible.

Otherwise this PR is now ready for review again.

simonbutcher · 2020-05-19T22:44:44Z

@dgreen-arm / @hanno-arm - Would you be kind enough to review this old PR and it's sister backports, as your names are already on them? If you do - I promise a review in return, for the PR of your choice, as big or as small as you want.

dgreen-arm

Looks good to me. Mbed OS failures will be addressed by the other PR, so that shouldn't block this.

gilles-peskine-arm · 2020-06-02T22:43:13Z

library/ssl_tls.c

@@ -50,6 +50,7 @@
 #include "mbedtls/platform_util.h"
 #include "mbedtls/version.h"

+#include <limits.h>


ssl_tls.c no longer uses anything from limits.h: now only ssl_msg.h does. This is not a blocker: including a standard header which doesn't get used is harmless.

However, 4 other files need limits.h on this branch: bignum.c, ctr_drbg.c, psa_crypto.c and x509_crt.c. I checked and development doesn't need any other as of today. Please add those, otherwise it defeats the purpose of the PR. And please re-check test suites and sample programs.

gilles-peskine-arm · 2020-06-02T22:45:40Z

ChangeLog

+ * Add missing limits.h standard C library header to ssl_tls.c and udp_proxy.c
+ which was only including it via check_config.h previously, which may not be
+ included in custom configuration files. Fixes #1803


This changelog entry would get added to the section for the already-released version 2.22.0. We no longer merge changelog entries manually. Please create a file in ChangeLog.d instead. This doesn't require a rebase.

gilles-peskine-arm · 2020-06-02T22:53:39Z

3rdparty/everest/library/Hacl_Curve25519_joined.c

@@ -24,6 +24,7 @@
 #else
 #include MBEDTLS_CONFIG_FILE
 #endif
+#include "mbedtls/check_config.h"


As I've stated before, I don't like this approach. Every library source file is changed to include something that doesn't really concern it.

Furthermore this is unrelated to the original objective of the pull request. It neither fixes the problem nor contributes to making the problem less likely to happen again.

I wouldn't have vetoed the PR on this basis, but since the PR needs rework anyway, please remove the inclusion of check_config.h everywhere.

To include check_config.h everywhere, on development, add it to library/common.h, which was added to development today (and we'll also need to include common.h everywhere, which isn't done yet).

simonbutcher · 2020-06-03T09:23:58Z

Thanks for the review @gilles-peskine-arm. I guess you weren't aware, but the points you are making I was already aware of and are exactly why I told you the PR needed rework - and those changes have only arisen out of changes to development while this PR was waiting for reviews - which is why I was expressing frustration.

Ideally if it had been possible to review this PR more quickly, then it would not have been effected by common.h nor the ChangeLog changes, nor the changes to which file needed the change in the first place.

gilles-peskine-arm · 2020-06-08T21:28:25Z

@sbutcher-arm Do you intend to rework this PR soon? I'm working on another change (moving stuff to the newly created common.h) which runs into the limits.h inclusion problem and therefore conflicts with this PR. If you intend to rework soon, I'll wait until this is merged. Otherwise I'll make my PR which will fix this bug in development, and I can either take over the backports or leave them for later.

simonbutcher · 2020-06-08T22:16:55Z

@gilles-peskine-arm - After last week I talked to @danh-arm and passed all my open personal PR's including this one over to the core team. I am no longer doing any work on Mbed TLS in my free time and am pursuing my interests in crypto elsewhere.

Any future PR's will be strictly business related.

gilles-peskine-arm · 2020-06-23T18:07:27Z

include/mbedtls/config.h

@@ -3688,6 +3688,4 @@
 #include MBEDTLS_USER_CONFIG_FILE
 #endif

-#include "mbedtls/check_config.h"


There's one place where check_config.h modifies the configuration.

#if defined(_WIN32) #if !defined(MBEDTLS_PLATFORM_C) #error "MBEDTLS_PLATFORM_C is required on Windows" #endif /* Fix the config here. Not convenient to put an #ifdef _WIN32 in config.h as * it would confuse config.py. */ #if !defined(MBEDTLS_PLATFORM_SNPRINTF_ALT) && \ !defined(MBEDTLS_PLATFORM_SNPRINTF_MACRO) #define MBEDTLS_PLATFORM_SNPRINTF_ALT #endif #if !defined(MBEDTLS_PLATFORM_VSNPRINTF_ALT) && \ !defined(MBEDTLS_PLATFORM_VSNPRINTF_MACRO) #define MBEDTLS_PLATFORM_VSNPRINTF_ALT #endif #endif /* _WIN32 */

If check_config.h is no longer included from config.h, and thus only included from library code and not from application code, this won't do. Application code and library code must have the same configuration.

The only code in the library that is functionally affected by MBEDTLS_PLATFORM_SNPRINTF_ALT or MBEDTLS_PLATFORM_VSNPRINTF_ALT is in platform.h and platform.c. Application code that is affected by these macros can be reasonably expected to include mbedtls/platform.h. Therefore I think this hack can be adapted to fit in platform.h instead.

Feature detection (version_features.c, query_config) would no longer report these alt symbols correctly. I think that an acceptable change would be to not set the symbols, but cause their effect in platform.h regardless.

Fortunately, it turns out that changing the configuration is not necessary: platform.h will do its thing (activate an snprintf wrapper) regardless. #3453

gilles-peskine-arm · 2020-06-23T22:16:39Z

I'm working on a PR that currently includes the modifications made here: fixing #1803 and not including check_config.h from config.h. But I'm still working out the implications of the second part. If I can't make it work, I'll make a separate PR that fixes #1803 and nothing else.

daverodgman · 2021-03-30T13:25:08Z

This issue is tracked in #1803 - closing this PR as probably this needs a different approach.

redtangent mentioned this pull request Aug 31, 2018

Missing include to limits.h in ssl_tls.c #1803

Closed

simonbutcher requested review from RonEld and hanno-becker September 1, 2018 18:12

simonbutcher added bug CLA not applicable component-tls needs-review Every commit must be reviewed by at least two team members, labels Sep 1, 2018

RonEld previously approved these changes Sep 2, 2018

View reviewed changes

simonbutcher added the needs-work label Sep 3, 2018

hanno-becker previously approved these changes Sep 3, 2018

View reviewed changes

hanno-becker removed the needs-review Every commit must be reviewed by at least two team members, label Sep 3, 2018

redtangent dismissed stale reviews from hanno-becker and RonEld via f6fad95 September 4, 2018 21:28

simonbutcher added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Sep 10, 2018

simonbutcher force-pushed the missing-limits-header branch from c01507e to 89b1a9e Compare May 19, 2020 17:53

simonbutcher mentioned this pull request May 19, 2020

Fix Mbed TLS adjust-config.sh script ARMmbed/mbed-os#12998

Merged

simonbutcher added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests needs-work labels May 19, 2020

simonbutcher requested a review from dgreen-arm May 19, 2020 22:43

dgreen-arm approved these changes May 20, 2020

View reviewed changes

gilles-peskine-arm requested changes Jun 2, 2020

View reviewed changes

simonbutcher added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 3, 2020

gilles-peskine-arm self-assigned this Jun 23, 2020

gilles-peskine-arm reviewed Jun 23, 2020

View reviewed changes

gilles-peskine-arm mentioned this pull request Jun 24, 2020

Don't force MBEDTLS_PLATFORM_SNPRINTF_ALT on Windows in check_config.h #3453

Closed

daverodgman added the Community label Oct 2, 2020

daverodgman closed this Mar 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add limits.h inclusion to ssl_tls.c and udp_proxy.c #1999

Add limits.h inclusion to ssl_tls.c and udp_proxy.c #1999

redtangent commented Aug 31, 2018 •

edited by mpg

Loading

RonEld left a comment

simonbutcher commented Sep 3, 2018

hanno-becker left a comment

hanno-becker commented Sep 3, 2018

RonEld commented Sep 3, 2018 •

edited

Loading

hanno-becker commented Sep 3, 2018

RonEld commented Sep 3, 2018 •

edited

Loading

RonEld commented Sep 3, 2018

redtangent commented Sep 4, 2018

RonEld commented Sep 5, 2018

simonbutcher commented Sep 5, 2018

RonEld commented Sep 5, 2018

simonbutcher commented Sep 5, 2018

RonEld commented Sep 6, 2018

redtangent commented Sep 9, 2018

simonbutcher commented Sep 10, 2018 •

edited

Loading

RonEld commented Sep 12, 2018

simonbutcher commented May 19, 2020

simonbutcher commented May 19, 2020 •

edited

Loading

dgreen-arm left a comment

gilles-peskine-arm Jun 2, 2020

gilles-peskine-arm Jun 2, 2020

gilles-peskine-arm Jun 2, 2020

simonbutcher commented Jun 3, 2020 •

edited

Loading

gilles-peskine-arm commented Jun 8, 2020

simonbutcher commented Jun 8, 2020

gilles-peskine-arm Jun 23, 2020

gilles-peskine-arm Jun 23, 2020

gilles-peskine-arm Jun 24, 2020

gilles-peskine-arm commented Jun 23, 2020

daverodgman commented Mar 30, 2021

Add limits.h inclusion to ssl_tls.c and udp_proxy.c #1999

Add limits.h inclusion to ssl_tls.c and udp_proxy.c #1999

Conversation

redtangent commented Aug 31, 2018 • edited by mpg Loading

Description

Status

Requires Backporting

Todos

RonEld left a comment

Choose a reason for hiding this comment

simonbutcher commented Sep 3, 2018

hanno-becker left a comment

Choose a reason for hiding this comment

hanno-becker commented Sep 3, 2018

RonEld commented Sep 3, 2018 • edited Loading

hanno-becker commented Sep 3, 2018

RonEld commented Sep 3, 2018 • edited Loading

RonEld commented Sep 3, 2018

redtangent commented Sep 4, 2018

RonEld commented Sep 5, 2018

simonbutcher commented Sep 5, 2018

RonEld commented Sep 5, 2018

simonbutcher commented Sep 5, 2018

RonEld commented Sep 6, 2018

redtangent commented Sep 9, 2018

simonbutcher commented Sep 10, 2018 • edited Loading

RonEld commented Sep 12, 2018

simonbutcher commented May 19, 2020

simonbutcher commented May 19, 2020 • edited Loading

dgreen-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Jun 2, 2020

Choose a reason for hiding this comment

gilles-peskine-arm Jun 2, 2020

Choose a reason for hiding this comment

gilles-peskine-arm Jun 2, 2020

Choose a reason for hiding this comment

simonbutcher commented Jun 3, 2020 • edited Loading

gilles-peskine-arm commented Jun 8, 2020

simonbutcher commented Jun 8, 2020

gilles-peskine-arm Jun 23, 2020

Choose a reason for hiding this comment

gilles-peskine-arm Jun 23, 2020

Choose a reason for hiding this comment

gilles-peskine-arm Jun 24, 2020

Choose a reason for hiding this comment

gilles-peskine-arm commented Jun 23, 2020

daverodgman commented Mar 30, 2021

redtangent commented Aug 31, 2018 •

edited by mpg

Loading

RonEld commented Sep 3, 2018 •

edited

Loading

RonEld commented Sep 3, 2018 •

edited

Loading

simonbutcher commented Sep 10, 2018 •

edited

Loading

simonbutcher commented May 19, 2020 •

edited

Loading

simonbutcher commented Jun 3, 2020 •

edited

Loading