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

More thorough GF256 tests and one resulting fix. #23

Closed
wants to merge 1 commit into from

Conversation

jcflack
Copy link
Contributor

@jcflack jcflack commented Nov 2, 2014

I was reading over the hand-cranked GF256 code and I could not trivially convince myself of the LOG_TABLE/ALOG_TABLE indexing, so I decided to add a more thorough test to work straight from algebraic definitions and verify the properties of a finite field (and then test pow, inverse, and div against the tested mult operation).

That did in fact turn up one problem with pow, only for the edge case a=0. mult handles zero factors cleverly by having LOG_TABLE[0] (which isn't truly a log) be greater than any sum of two true logs, and therefore point into a second half of ALOG_TABLE that is all zeros. But that trick doesn't survive in pow, where a log gets multiplied by p and then reduced mod 255 ... it just produces very strange values for 0^p.

I didn't think of any better way to fix pow than by adding a conditional.

Perhaps it doesn't really matter much if pow is broken for a=0, as (at least in ShamirPSS) it would not be expected to be called with a=0 as that would correspond to the secret itself. But it seemed safer to correct it. If that makes it too slow, then maybe it could be left unfixed but with the javadoc changed to define it only for a != 0.

As an aside, when I first noticed it failed the test, I added TestBCGF256Algebra to run the same tests on the BouncyCastle implementation, and that also has a problem at a=0 (but only for p=0; they forgot that anything^0 is unity, even though 0^anythingelse is of course 0).

pow(0,p) for p > 0 was returning zany results - the idea of encoding a
second half of ALOG_TABLE with zeros cleverly saved a test for zero in
mult, but relies on the log value staying >= 512 which it can't in pow
after p*...%255. Simplest fix is to add a test.
@andreashappe
Copy link
Member

heh. nice find (: I was running into the same problem a couple of days ago.

I've merged your patch, added a small white space fix and running the test cases right now. Will push as soon as this will pass.

In the mean time I've pushed some other (math related) fixes -- mostly making RabinIDS work with other galois fields (my use case is a GF(257)). During this conversion some other bugs have been fixed too -- maybe this also helps you.

cheers and thanks, Andreas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants