Initialisation Fixes

[danielbarry]

Some fixes around possible uninitialized variables that could lead to undesired behaviour. They are unlikely to be seen in the wild as they would mostly require misconfiguration.

I've tried to make sure they are initialized to sane defaults and throw a compiler error where that's not really possible (neither IPv4 or IPv6 set).

[danielbarry]

I'm not entirely sure why there are extra commits in there... I forked from master, then branched, made changes and then am looking to merge? Not sure what I messed up?

[gamelaster]

Hello,
hmm, strange, but merging isn't problematic. I will recheck if it's OK

[Avamander]

I'm not entirely sure why there are extra commits in there... I forked from master, then branched, made changes and then am looking to merge? Not sure what I messed up?

@danielbarry Try fetching latest master and rebase your branch on top of it and then force push. Be attentive.

[danielbarry]

@gamelaster @Avamander I think that's now done?

[Avamander]

Looks correct(er) now.

[Avamander]

Does MbedTLS have these changes upstream?

[danielbarry]

@Avamander I hadn't checked... But:

• ecjpake.c sets ret to MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED which would fix this bug [1]
• ssl_cli.c doesn't zero hashlen, so it's possible to end up with some random hash length if the compiler doesn't zero out the memory (and the right precompiler conditions are met) [2]
• ssl_cli.c doesn't zero n or i, it seems to use a different process at first glance [3]
• ssl_tls.c doesn't NULL (and therefore check) mac_enc or mac_dec [4], meaning it's possible for the memcpy to point to some random location in memory [5]

[1] https://github.com/ARMmbed/mbedtls/blob/development/library/ecjpake.c#L108

[2] https://github.com/ARMmbed/mbedtls/blob/development/library/ssl_cli.c#L3257

[3] https://github.com/ARMmbed/mbedtls/blob/development/library/ssl_cli.c#L3674

[4] https://github.com/ARMmbed/mbedtls/blob/development/library/ssl_tls.c#L858

[5] https://github.com/ARMmbed/mbedtls/blob/development/library/ssl_tls.c#L1149

[Avamander]

It sounds to me like MbedTLS should just be updated, turned into a submodule. What do you think?

[danielbarry]

@Avamander That sounds sensible, otherwise you're always going to miss out on upstream patches. Would probably make sense to point it at their latest release branch rather than the development branch :)

https://github.com/ARMmbed/mbedtls/tree/v2.24.0

[danielbarry]

Perhaps it would make sense to fork your own version - that way you can apply patches over the top until the next release is ready?

[v3l0c1r4pt0r]

It sounds to me like MbedTLS should just be updated, turned into a submodule. What do you think?

@Avamander That's exactly what I thought about, when I've read your previous comment. Maybe as a first step, mbedtls should be turned into submodule at exactly same state of repo that was originally in SDK. This could be merged immediately. This would preserve history, so we will know, where we started.

Then another pull request could try updating the submodule to latest release.

[danielbarry]

Good thinking, seems the commit version is here: https://github.com/bouffalolab/bl_iot_sdk/blob/ee4a10b1a1e3609243bd5e7b3a45f02d768f6c14/components/security/mbedtls/version

Do you want me to make a PR?

[Avamander]

Maybe as a first step, mbedtls should be turned into submodule at exactly same state of repo that was originally in SDK.

Yes, one commit to delete the folder, one to set it to current repo equivalent.

[gamelaster]

Closing in favor of #64

[danielbarry]

@gamelaster I think there are still some points here, but I'll open a new PR for those. Apparently the PineCone will arrive at some time so I could even test it for real :)

[gamelaster]

Okay, thank you @danielbarry 😊