Use signed variable for length calculation in SendTls13Certificate #7880
Conversation
|
Prefer to keep length as unsigned if possible as all assignments I've seen are calculated with unsigned. |
|
So I mean we can keep it unsigned but I feel it is still far riskier. The hang two customers saw would have been prevented with this change, and in general any mistakes in length handling will result in a hang. Any mistakes in further modifications of that code could also result in a hang. Would strongly prefer we not do unsigned len > 0 loop, especially when allocating inside that loop. |
|
Retest this please. |
| @@ -8510,7 +8511,7 @@ static int SendTls13Certificate(WOLFSSL* ssl) | |||
| listSz = 0; | |||
| } | |||
| else { | |||
| if (!ssl->buffers.certificate) { | |||
| if (!ssl->buffers.certificate || !ssl->buffers.certificate->buffer) { | |||
There was a problem hiding this comment.
ssl->buffers.certificate->buffer is only dereferenced if certSz > 0. I can imagine there's code somewhere relying on null buffer being tolerated when ssl->buffers.certificate->length is zero, so this nullness check should only be done when ssl->buffers.certificate->length > 0.
There was a problem hiding this comment.
I agree its a bit redundant, but is consistent with the rest of the code. Search for !ssl->buffers.certificate and you will see every single check in internal.c, ssl.c and tls13.c use if (!ssl->buffers.certificate || !ssl->buffers.certificate->buffer)
If we want to address that redundancy check separately I am on board, but I think at least for this PR its best to have the checks be consistent.
Description
Use signed variable for length calculation in SendTls13Certificate. Current code uses unsigned len which only works when decremented exactly to zero. Any mistakes in the length handling will underflow and keep looping, allocating memory until the system runs out.
Additional fix brought up by ZD 18267