Potential oqs_oid_alg_list overflow?

[beldmit]

I've got the following Coverity report

oqs-provider-0.7.0/oqsprov/oqsprov.c:1004:5: cond_at_most: Checking "2 * idx > 220" implies that "idx" may be up to 110 on the false branch.
oqs-provider-0.7.0/oqsprov/oqsprov.c:1006:5: overrun-local: Overrunning array "oqs_oid_alg_list" of 220 8-byte elements at element index 220 (byte offset 1767) using index "idx * 2" (which evaluates to 220).
#  1004|       if (2 * idx > OQS_OID_CNT)
#  1005|           return 0;
#  1006|->     s = (char *)oqs_oid_alg_list[idx * 2];
#  1007|       len = strlen(s);
#  1008|   

I'm not sure whether everything is OK here with oqs_oid_alg_list buffer size especially having

#ifdef OQS_KEM_ENCODERS
#define OQS_OID_CNT 220
#else
#define OQS_OID_CNT 114
#endif

[baentsch]

Tagging @feventura for insight -- I'm afraid I don't grasp the logic there at first glance (maybe worth while checking/adding commentary as part of #549?)

[SWilson4]

It looks to me like there's risk of an overflow here, but not because of the issue raised by Coverity.

This check should surely use >= instead of >. That said, I don't believe the condition (2 * idx > OQS_OID_CNT) will ever be triggered, based on reading the code:

1. get_composite_idx is only called in the form get_composite_idx(get_oqs_alg_idx(nid)).
2. the return value of get_oqs_alg_idx is strictly less than NID_TABLE_LEN, which is #define'd to be either 110 or 57 (i.e., half of OQS_OID_CNT).

The more pressing issue I see is that get_oqs_alg_idx returns -1 on failure, and get_composite_idx does not handle that case, meaning that the oqs_oid_alg_list array might be accessed at index -2. Maybe the composite code is written so that this error case never arises; if so, it's not apparent to me.

[feventura]

Thanks for the comments, I fully agree with @SWilson4 , I'll fix this for the #549 @baentsch .

[baentsch]

@beldmit #549 just landed, so this issue should be gone. If you can confirm, please close this issue.