Add checking in SHE response handlers

[padelsbach]

Check for group, action and len in SHE response handlers. Found with experimental fuzzing.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #391

Scan targets checked: wolfhsm-core-bugs, wolfhsm-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[bigbrett]

all these group and action checks are redundant, as they are handled in the comm layer. Otherwise every single client response check would need these.

Also, do you think the size check could be pushed down to the comm layer somehow in a uniform way, perhaps via same mechanism as #389 and #388? Currently these would need to be done inline everywhere.

Note we dont care THAT much about this fuzzing style stuff - currently the threat model is such that transports are trusted. If someone can modify data in your transport, all bets are off. So temped to just not do the infinite bikeshedding around input sanitation that fenrir keeps pointing out....

[padelsbach]

Fair. Updated the PR to add only the dataSz < sizeof(*resp) which seems legit useful

[padelsbach]

Passing over to @AlexLanzano in Brett's absence

[AlexLanzano]

Hmm, Actually on a second look these checks still wouldn't prevent an overflow since the data is copied prior to these check. This needs to be handled at the comm layer probably by adding a dataLen arg to the wh_Client_RecvResponse and propagating that to the comm layer.

[padelsbach]

Hmm, Actually on a second look these checks still wouldn't prevent an overflow since the data is copied prior to these check. This needs to be handled at the comm layer probably by adding a dataLen arg to the wh_Client_RecvResponse and propagating that to the comm layer.

I think you're referring to this copy down in wh_CommClient_RecvResponse:

            if (    (data != NULL) &&
                    (data_size != 0) &&
                    (data != context->data)) {
                memcpy(data, context->data, data_size);
            }

The vast majority of the handlers provide the data buffer matching context->data, so the data != context->data check prevents this memcpy.

In any case, the checks added here are recv < expected so they wouldn't catch an overflow anyway. They're mainly to prevent a too small buffer from being over indexed.

But...

I see your point that this could be done in a more uniform way, perhaps by passing sizeof(*resp) into wh_Client_RecvResponse. I'm into it if we don't care about breaking the public API.

[AlexLanzano]

I see your point that this could be done in a more uniform way, perhaps by passing sizeof(*resp) into wh_Client_RecvResponse. I'm into it if we don't care about breaking the public API.

Yeah, I think in this case it's fine to break the API. I don't believe that many (if any) users are calling the comm client send/recv functions directly in their applications