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

Alternative cmov implementation #457

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

peterdettman
Copy link
Contributor

@peterdettman peterdettman commented May 17, 2017

See the paper "Attacking embedded ECC implementations through cmov side channels" for a description of the problem.

@sipa and I discussed this briefly in NYC, and we thought maybe this alternative construction might improve the situation. Peter Schwabe also independently mentioned the same construction, and that they hoped to get around to a second paper looking at suggested countermeasures.

Edit: Link to paper: https://eprint.iacr.org/2016/923

@gmaxwell
Copy link
Contributor

Interesting. I suppose better is better... though I think we're kinda shooting in the dark. We really need to have a good power analysis test setup to really know if we're successful against those kinds of sidechannels. @tdaede was working on one before.

@peterdettman
Copy link
Contributor Author

My thoughts exactly.

@sipa
Copy link
Contributor

sipa commented Jul 31, 2017

I-guess-it-won't-hurt-ACK 42b6b42

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look perfectly fine to me and pass tests, ACK on these.

gcc introduces some branches here with -O3. I had a glimpse at it and it could be that all of these are not dependent on secrets (wild guesses: checking whether r and a overlap? checking whether len is 0?). We need to have a closer look. I'm not really good in reading assembly, maybe someone wants to give it a try: https://godbolt.org/z/XqeEK5

@real-or-random
Copy link
Contributor

Okay, I played around with this a little bit.

First, the branches are not an issue. These really only implement the loop, even though they looked different to me at first glance.

But GCC manages to factor out the computation of the entire mask half ^ rest and move it before the loop. This happens already with -O1 and even on AVR GCC (which is interesting because the attack in the paper targeted AVR as an example). My understanding is that the 0x55...55 constants (= 0101...01 as bits) here are a crucial part of the countermeasure because they ensure all XOR operations are performed with masks of the same Hamming weight (0101...01 or 1010...10 depending on the secret input bit).

So while this PR does not hurt, it does not really help if you use GCC, which is simply too clever for us. So I'm not sure if there's a simple way that works across platform. What we could do for x86_64 specifically will be an implementation based on the CMOV instruction but even here we don't know what that means for power analysis. At least it's "guaranteed" to be constant-time and fast.

@real-or-random
Copy link
Contributor

So while this PR does not hurt, it does not really help if you use GCC, which is simply too clever for us. So I'm not sure if there's a simple way that works across platform.

#728 reminded me of this PR, and this variant should work:

static void secp256k1_fe_cmov_limbs(uint32_t *r, const uint32_t *a, int len, int flag) {
    int i;
    uint32_t diff, r_i;
    volatile const uint32_t half = 0x55555555UL;
    volatile const uint32_t rest = half << flag;
    for (i=0; i<len; i++) {
        diff = r[i] ^ a[i];
        r[i] ^= (diff & half);
        r[i] ^= (diff & rest);
    }
}

Not sure why I didn't think of volatile when I looked at this previously.

real-or-random added a commit to real-or-random/secp256k1 that referenced this pull request Apr 1, 2023
Apparently clang 15 is able to compile our cmov code into a branch,
at least for fe_cmov and fe_storage_cmov. This commit makes the
condition volatile in all cmov implementations (except ge but that
one only calls into the fe impls).

This is just a quick fix. We should still look into other methods,
e.g., asm and bitcoin-core#457. We should also consider not caring about
constant-time in scalar_low_impl.h

We should also consider testing on very new compilers in nightly CI,
see bitcoin-core#864 (comment)
real-or-random added a commit that referenced this pull request Apr 6, 2023
…ations

4a496a3 ct: Use volatile "trick" in all fe/scalar cmov implementations (Tim Ruffing)

Pull request description:

  Apparently clang 15 is able to compile our cmov code into a branch, at least for fe_cmov and fe_storage_cmov. This commit makes the condition volatile in all cmov implementations (except ge but that one only calls into the fe impls).

  This is just a quick fix. We should still look into other methods, e.g., asm and #457. We should also consider not caring about constant-time in scalar_low_impl.h

  We should also consider testing on very new compilers in nightly CI, see #864 (comment)

ACKs for top commit:
  jonasnick:
    ACK 4a496a3

Tree-SHA512: a6010f9d752e45f01f88b804a9b27e77caf5ddf133ddcbc4235b94698bda41c9276bf588c93710e538250d1a96844bcec198ec5459e675f166ceaaa42da921d5
@real-or-random real-or-random self-assigned this May 8, 2023
@real-or-random
Copy link
Contributor

After discussing with @sipa and @jonasnick, we believe it's worth to do this even if we're not entirely sure how much it helps. (And we anyway have volatile everywhere in CMOV after #1257).

static void secp256k1_fe_cmov_limbs(uint32_t *r, const uint32_t *a, int len, int flag) {
    int i;
    uint32_t diff, r_i;
    volatile const uint32_t half = 0x55555555UL;
    volatile const uint32_t rest = half << flag;
    for (i=0; i<len; i++) {
        diff = r[i] ^ a[i];
        r[i] ^= (diff & half);
        r[i] ^= (diff & rest);
    }
}

I'll open a follow-up PR with that volatile variant above.

dderjoel pushed a commit to dderjoel/secp256k1 that referenced this pull request May 23, 2023
Apparently clang 15 is able to compile our cmov code into a branch,
at least for fe_cmov and fe_storage_cmov. This commit makes the
condition volatile in all cmov implementations (except ge but that
one only calls into the fe impls).

This is just a quick fix. We should still look into other methods,
e.g., asm and bitcoin-core#457. We should also consider not caring about
constant-time in scalar_low_impl.h

We should also consider testing on very new compilers in nightly CI,
see bitcoin-core#864 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants