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 constant-time behaviour for ct_reduce/ct_div_rem #117

Merged
merged 2 commits into from
Aug 25, 2022

Conversation

andrewwhitehead
Copy link
Contributor

Fixes #116

Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
src/uint/div.rs Outdated
quo = quo.shl_vartime(1);
}

let is_some = if mb == 0 { 0 } else { 1 };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is sufficient since there are no constant-time guarantees around rhs.

Copy link
Member

Choose a reason for hiding this comment

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

Might be good to leave a comment to that effect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I threw in a ct_select since it doesn't make a significant difference to the runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add your test case that triggered this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Want to make sure it’s actually fixed

Copy link
Member

@tarcieri tarcieri Aug 25, 2022

Choose a reason for hiding this comment

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

Just saw this comment.

Perhaps it would make sense to use something like sidefuzz to test constant-time behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My test was a criterion benchmark, I suspected it from reading the code.

#[macro_use]
extern crate criterion;
use criterion::{black_box, Criterion};
fn criterion_benchmark(c: &mut Criterion) {
    use crypto_bigint::U256;
    let u1 = U256::from_be_hex("0123456780123456780123456780123456780123456780123456780123456780");
    let u2 = U256::from_be_hex("0000000000000000000000000000000000000000000000000000000000000012");
    let md = U256::from_be_hex("0000000000000000000000000000000000000000000000000000000012345678");
    c.bench_function("reduce large", move |b| {
        b.iter(|| black_box(u1).reduce(&md))
    });
    c.bench_function("reduce small", move |b| {
        b.iter(|| black_box(u2).reduce(&md))
    });
}
criterion_group!(benches, criterion_benchmark);
criterion_main!(benches);

Old timing:

reduce large            time:   [948.59 ns 949.00 ns 949.46 ns]                           
Found 8 outliers among 100 measurements (8.00%)
  6 (6.00%) high mild
  2 (2.00%) high severe

reduce small            time:   [15.520 ns 15.535 ns 15.549 ns]                           
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) low mild
  2 (2.00%) high severe

And patched:

reduce large            time:   [955.12 ns 955.84 ns 956.81 ns]                          
Found 10 outliers among 100 measurements (10.00%)
  3 (3.00%) high mild
  7 (7.00%) high severe

reduce small            time:   [955.58 ns 956.55 ns 957.99 ns]                          
Found 11 outliers among 100 measurements (11.00%)
  4 (4.00%) high mild
  7 (7.00%) high severe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also if somebody feels like implementing a limb-based method this looks interesting: https://www.bearssl.org/bigint.html#modred

src/uint/div.rs Outdated Show resolved Hide resolved
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
@tarcieri tarcieri merged commit 377c8b3 into RustCrypto:master Aug 25, 2022
@andrewwhitehead andrewwhitehead deleted the fix/ct-reduce branch August 25, 2022 22:10
@tarcieri tarcieri mentioned this pull request Oct 11, 2022
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.

UInt::{ct_div_rem, ct_reduce} are not constant-time
3 participants