Fix warning in PSK parsing

[irwir]

Description

The algorithm in RFC4279 requires 16-bit value, and nothing more than 16 bits could be obtained from combining two 8-bit values.
Therefore it would be appropriate to use uint16_t instead of possibly too wide size_t.
Then the range check (n > 65535) could go away together with the warning.

The same type change applies to ssl_cli.c, even though the mentioned check was in ssl_srv.c only.

Status

READY

Requires Backporting

NO
Which branch?
Development

Migrations

NO

[gilles-peskine-arm]

Thank you for your contribution! I agree with the change in principle, but I think there's a mistake in one of the checks.

CI is passing except for some known flaky tests.

As you know we normally credit all contributions in the changelog file. Can you please add a changelog entry? Or let us know if you prefer us to write it (we are picky about English and style consistency in the changelog).

[gilles-peskine-arm]

Please don't use !n when n isn't a boolean. n < 1 is fine here, even now that n is unsigned.

[irwir]

I suggest to change !n to n == 0.

[gilles-peskine-arm]

The cast of end - *p to uint16_t isn't right, is it? What if e.g. end == *p + 65538?

[irwir]

Type cast to uint16_t is a serious mistake; it should have been cast to size_t.
But let's try to avoid type casts by rearranging comparison (the other comparison was changed to have the same style).

[irwir]

@gilles-peskine-arm
Thanks for reviewing.

Or let us know if you prefer us to write it (we are picky about English and style consistency in the changelog).

It would be great if you take care of that changelog entry.

[gilles-peskine-arm]

No, this change is wrong, and likewise end < *p + n below, for a somewhat subtle reason. If *p points less than offset bytes before the end of the buffer, *p + offset has undefined behavior. In practice, the risk is that if *p is close to the top of the address space, the addition wraps around and end < wraparound(*p + offset) is true even though end < *p + offset is false. That's why the code uses subtraction: it doesn't risk causing an overflow.

[irwir]

If *p points less than offset bytes before the end of the buffer, *p + offset has undefined behavior.

Integer overflow is an undefined behaviour, unsigned wraparound is not.

In practice, the risk is that if *p is close to the top of the address space, the addition wraps around

ptrdiff_t effectively uses 31 bits, therefore it has more troubles when applied to 32-bit pointers (about by a factor of 2).

Let's take VS 2019, 32-bit x86 code

char *end = (char*)0x80000002u;
char *p = (char *)0x2u;
int nRetCode = 0;

if (end - p > 3)
	nRetCode = 1;
//nRetCode is 0 here, though end is well above p

What about casting ptrdiff_t to size_t?

//'end' is below 'p'
char *end = (char*)0x2u;
char *p = (char *)0x80000002u;
int nRetCode = 0;

if ((size_t)(end - p) > 3)
	nRetCode = 1;
//nRetCode is 1 here, though end is below p

Something like this was discussed in other issue here

Would you suggest, to return to the old ways as in (size_t)(end - p) > 3?
In any case, at least one more condition have to be checked to avoid wraparound and overflow issues. For example, if the condition end > p is true.

[gilles-peskine-arm]

Unsigned integer wraparound has well-defined behavior, but pointer wraparound does not. *p + offset is a pointer overflow, which in practice often does wraparound but could also cause a segmentation fault or nasal demons.

If p and q are pointers, p - q is defined only if p and q point to the same object. Quoting C99+TC3 (n1256):

When two pointers are subtracted, both shall point to elements of the same array
or one past the last element of the array object

So your experiments with constant values for end and p are unconclusive.

Recall that all the parsing function in Mbed TLS have the following precondition: *p and end point inside (or to the end of) the same object and *p <= end. The code takes care to maintain the invariant *p <= end. Therefore end - *p is guaranteed to be a non-negative value provided that it fits in ptrdiff_t. We only support 32-bit and larger platforms, so end - *p is always correct if the value is at most 2^31 - 1. TLS has a maximum record length of 16kB, so a TLS record always fits (it would even fit on a 16-bit platform). Other parsers in Mbed TLS may be unsafe if given objects whose size doesn't fit in ptrdiff_t, i.e. on most platforms objects that span more than half the address space.

[irwir]

So your experiments with constant values for end and p are unconclusive.

On the contrary, it shows limited usability and generally poor design of ptrdiff_t type.
But that deviates too much from the subject of this PR.
Constant values (without pointing to the same object) were used to make the example as short and simple as possible.

Therefore end - *p is guaranteed to be a non-negative value provided that it fits in ptrdiff_t.

If so, then type casts could be removed.
I have changed the conditions in question, and added the proposed changelog entry - again into 2.19.2 version.
Thanks!

[irwir]

@gilles-peskine-arm
Interesting follow-up.
Please take a look at ssl_parse_client_dh_public (line 3561), and what pointer checks are used.

This PR might also replace size_t n; with uint16_t n; (line 3565).

[irwir]

@gilles-peskine-arm

This PR might also replace size_t n; with uint16_t n; (line 3565).

What would you say if this change was also included into the current PR?

[gilles-peskine-arm]

This PR has been approved and has acceptable (if old) CI results. I'd prefer to merge it (dealing with the changelog manually) and move on.

[gilles-peskine-arm]

Proposed changelog entry, under Bugfix:

• Remove a spurious check in ssl_parse_client_psk_identity that triggered a warning with some compilers. Fix contributed by irwir in Fix warning in PSK parsing #2856.

[gilles-peskine-arm]

New full CI run (internal link): https://jenkins-internal.mbed.com/job/mbedtls-pr-multibranch/job/PR-2856-merge/11/

[mpg]

This already has ChangeLog entry, so I'm removing the label "needs: ChangeLog".

[mpg]

The CI is passing, except the known-broken mbed-os test, and the API/ABI check which finds a change in PSA Crypto, so unrelated to this PR. With two approvals and a passing-enough CI, this is ready for merge.