CodeQL warnings

[aido]

Hi,

I am using bc-shamir in an application I am writing to generate SSKR shares on Ledger hardware wallet devices: SSKR Check
CodeQL is giving a couple of warnings about bc-shamir which maybe you would like to be aware of.

See here:

https://github.com/aido/app-sskr-check/security/code-scanning/11
or here:
https://codeql.github.com/codeql-query-help/cpp/cpp-comparison-with-wider-type/

bc-shamir/src/shamir.c

Line 142 in cc574ce

for(uint8_t j=0; j<share_length; ++j) {

and here:

https://github.com/aido/app-sskr-check/security/code-scanning/12

bc-shamir/src/shamir.c

Line 79 in cc574ce

for(uint8_t j=0; j<secret_length; ++j) {

[wolfmcnally]

Unfortunately the issue links above are now returning a 404. Can you either provide updated links or at least describe the warning you're receiving?

Also, I notice you're not using our original repos for SSKR or Shamir, but if you do have a simple code fix for any warnings you're receiving, we'd love it if you could file a PR on the original repos!

[aido]

Hi @wolfmcnally,

The links above seem to still work fine for me so I think github may be blocking public access to security warnings generated by Code QL. I have added you as a colaborator on the app-sskr-check repo so hopefully the links work now.

The gist of the warnings are that In a loop condition, comparison of a value of a narrow type with a value of a wide type may result in unexpected behavior if the wider value is sufficiently large (or small). This is because the narrower value may overflow. This can lead to an infinite loop. i.e. a uint8_t being compared to a uint32_t in a loop condition in line 79 and 142 of shamir.c

A simple fix would be to cast uint32_t share_length and uint32_t secret_length to uint8_t or question why share_length and secret_length are uint32_t to begin with.

[ChristopherA]

@wolfmcnally status?

[wolfmcnally]

I'll review his PR shortly.