kokke / kokke/tiny-ECDH-c

DSA Broken: Troubleshooting

Open
#22 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
296
Forks
70
PR merge metrics
No merged PRs in 30d

Description

Hello, I tried troubleshooting this library. I was attracted by how readable and well commented it is, but it seems there might be deeper problems with this implementation that I do not have time to inspect. Here are my findings to get the ball rolling:

Casting a `uint8_t` array to a `uint32_t` is dangerous, as it is affected by the underlying architecture's endianness. When you bit shift, the shift is abstracting the hardware's endianness and treating everything as big endian. So the safest procedure is to either copy to a `uint32_t` array via bitshifts, or to define it as such in the first place. The other option is to only use byte arrays of `uint8_t`. I didn't see a good reason to go for the `uint32_t`.

There may be unexpected behavior caused by `bitvec_clr_bits()`. If the range you are clearing over is not less than 32 or a multiple of 32, you will have some uncleared bits floating in the middle of the cleared range. It would be safer to clear strictly in one direction as opposed to starting at the LSB of each byte. (Clever use of `& 31U` as a mod 32 (consider commenting).)

In step 2 of the signing, you are not taking the leftmost bits. You are actually blanking the MS (most significant) bits with use of `bitvec_clr_bits()` incrementing over a range. It can be said that you're modding the natural number represented by the entire hash. This may cause variation when testing results against other libraries. Maybe you could use `bitvec_clr_bits()` as is, but decrement over a range starting at the LSB; then you'd be blanking right to left.

I would check the conditional operations in the Galois field arithmetic. There are places where you say, if this is zero then it's the equivalent of the addition of that, etc.. I would check all to be safe actually.

I would also consider not defining the bitvec with a size (see [this post](https://stackoverflow.com/questions/4523497/typedef-fixed-length-array)). More importantly though, it makes the copy calls unclear to the reader. How do we know we are copying the right ranges? I do see the value in the sizes updating automatically, but it wouldn't hurt to just pass these `#define`d values as parameters.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by auditing the uint8_t/uint32_t conversions, bitvec_clr_bits(), signing step 2, and the conditional operations in the Galois field arithmetic. Reproduce or add focused checks for endianness, bit clearing ranges, and signing results against another library; done requires deciding which reported behaviors are actual defects and addressing them consistently.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
cryptography
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.