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

Fix a logic error in deterministic PRNG with buffer overrun issue #52

Closed
wants to merge 1 commit into from

Conversation

concise
Copy link

@concise concise commented Aug 8, 2015

In uECC_sign_deterministic, when uECC_WORD_SIZE is 8, the array declaration for T becomes uint64_t T[3]; and the following code

    #if (uECC_CURVE == uECC_secp160r1)
        T[uECC_WORDS] &= 0x01;
    #endif

becomes

        T[3] &= 0x01; // FIXME: access an array element outside its boundary?

I think that is not what we want (to keep only 161 pseudo-random bits so that 0 <= k <= 2^161-1) and the code in that case should be

        T[2] &= 0x00000001ffffffff; // keep the lower 33 bits

The entire code block may be changed into something like this:

#if (uECC_CURVE == uECC_secp160r1)
    #if (uECC_WORD_SIZE == 1)
        T[uECC_N_WORDS - 1] &= 0x01;
    #elif (uECC_WORD_SIZE == 4)
        T[uECC_N_WORDS - 1] &= 0x00000001;
    #elif (uECC_WORD_SIZE == 8)
        T[uECC_N_WORDS - 1] &= 0x00000001ffffffff;
    #endif
#endif

@concise
Copy link
Author

concise commented Aug 8, 2015

Note that if #51 is accepted, this pull request can be discarded and closed.

@concise
Copy link
Author

concise commented Aug 8, 2015

Oh, maybe I was doing nonsense. I didn't realized the usage for uECC_WORD_SIZE being 8 when compiling secp160r1 was forbidden for a long time:

#if (uECC_CURVE == uECC_secp160r1 ||
     uECC_CURVE == uECC_secp224r1) && (uECC_WORD_SIZE == 8)
    #undef uECC_WORD_SIZE
    #define uECC_WORD_SIZE 4
    #if (uECC_PLATFORM == uECC_x86_64)
        #undef uECC_PLATFORM
        #define uECC_PLATFORM uECC_x86
    #endif
#endif

Yet, since the macros for domain parameters of secp160r1 when uECC_WORD_SIZE is 8 are still there, I guess the fix might be useful in the future in general.

@kmackay
Copy link
Owner

kmackay commented Aug 8, 2015

I am planning to change micro-ecc to support runtime curve selection (see the 'runtime' branch). I think the implementation in that branch is correct for 8-byte words already.

You may want to base your RFC 6979 work on the runtime branch instead of master, since I will be replacing master with the runtime version as soon as it is ready (I need to add some more optimizations and AVR support).

@kmackay kmackay closed this Aug 8, 2015
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.

2 participants