Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CodeQL warnings #19

Open
aido opened this issue Apr 27, 2023 · 3 comments · May be fixed by #21
Open

CodeQL warnings #19

aido opened this issue Apr 27, 2023 · 3 comments · May be fixed by #21
Assignees

Comments

@aido
Copy link

aido commented Apr 27, 2023

Hi,

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

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

for(uint8_t i = 0; !error && i < next_group; ++i) {

@wolfmcnally
Copy link
Collaborator

Unfortunately the issue link above is now returning a 404. Can you either provide an updated link 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
Copy link
Author

aido commented May 2, 2023

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 size_t in a loop condition in line 385 of encoding.c

A simple fix would be to cast size_t next_group to uint8_t or question why next_group is size_t to begin with.

@aido
Copy link
Author

aido commented May 2, 2023

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!

Yes, I just copied the latest version of bc-shamir and bc-sskr. There has been very minor change to the original.

aido added a commit to aido/bc-sskr that referenced this issue May 4, 2023
@aido aido linked a pull request May 4, 2023 that will close this issue
@wolfmcnally wolfmcnally moved this from 2023 Q2 Priority to 2023 Q2 Done in High Level Roadmap May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 2023 Q2 Done
Development

Successfully merging a pull request may close this issue.

2 participants