Check for zero length and NULL buffer pointer.

[jainvikas8]

In reference to issue #49

Backport to 2.16: Mbed-TLS/mbedtls#2793

After analysis this turned out not to be applicable to 2.7.

[Patater]

Should this have a ChangeLog entry, like Mbed-TLS/mbedtls@e5c9e79 ?

[Patater]

Should this have a ChangeLog entry, like ARMmbed/mbedtls@e5c9e79 ?

Nevermind, this is to Mbed Crypto which doesn't have a Changelog. Misread it as going to Mbed TLS.

[Patater]

Will merge at same time as Mbed-TLS/mbedtls#2793 and Mbed-TLS/mbedtls#2794

[athoelke]

Surely if buf == NULL and len > 0 there is a serious programming error somewhere? This change silently ignores the problem and carries on. Is that an appropriate option for a security service?

[Patater]

Given the discussion in #239 (comment), we should probably change this check to just if ( len > 0 ) and avoid the NULL check. This will allow static analyzers to see that buf is not allowed to be NULL for this function (and, as a very small bonus, we can save a tiny amount of code space).

[gilles-peskine-arm]

PSA and mbedtls APIs work differently here. In PSA Crypto, a null pointer dereference is supposed to invoke the platform's equivalent of a segmentation fault. In mbedtls, in low-level modules, a null pointer dereference is supposed to invoke MBEDTLS_PARAM_FAILED if MBEDTLS_CHECK_PARAMS is enabled (and segfaults otherwise), and many high-level modules (e.g. md, cipher and pk) return an error code on null pointers.

Here, I think the correct behavior is

MBEDTLS_INTERNAL_VALIDATE( len == 0 || buf != NULL );
if( len > 0 )
    memset_func( buf, 0, len );

[jainvikas8]

Forced push to accommodate reviewer change request.

[Patater]

Please remove trailing whitespace from the commit that added it, so that no trailing whitespace is ever added in the git log.

[gilles-peskine-arm]

Style:

Suggested change

	if ( len > 0 )
	if( len > 0 )

[jainvikas8]

Forced push to correct style and trailing whitespaces.