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 short-circuit in debug mode #42

Closed
wants to merge 1 commit into from

Conversation

brycx
Copy link
Contributor

@brycx brycx commented Apr 10, 2019

Edit: Moved information to #43 as it seemed more appropriate.

This applies a strict OR evaluation to debug_assert to fix short-ciruiting.

Closes #43.

@brycx brycx changed the title Short-circuit in debug mode resulting in non-constant-time execution Fix short-circuit in debug mode Apr 10, 2019
@isislovecruft isislovecruft self-requested a review April 16, 2019 19:01
@@ -76,7 +76,7 @@ impl From<Choice> for bool {
/// verification.
#[inline]
fn from(source: Choice) -> bool {
debug_assert!(source.0 == 0u8 || source.0 == 1u8);
debug_assert!((source.0 == 0u8) | (source.0 == 1u8));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the modified version still does two equality checks, and the debug_assert! branches on the result, it seems like this still involves flow from secret variables (source.0) into a branch.

Copy link
Contributor Author

@brycx brycx Apr 18, 2019

Choose a reason for hiding this comment

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

I'm not quite sure what you mean. The debug_assert! should not branch on the result of (source.0 == 0u8), because after applying the strict OR evaluation, (source.0 == 1u8) should always be evaluated regardless of the first. So, IIUC, this means that debug_assert! can no longer branch. If you mean the actual equality checks, in the sense that they branch on true/false, then using the equality checks have given the best results when testing. I have also tested:

debug_assert!(((source.0 ^ 0u8) | (source.0 ^ 1u8)) == 1u8);
debug_assert!(((source.0 | 0u8) | (source.0 | 1u8)) == 1u8);

These seemed to give the same results, sometimes worse, compared to using the equality checks.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I don't see branching on secret data in the assembly output of the new (using bitwise ORs with equality checks):

pub struct Choice(u8);

pub fn old(source: Choice) {
    assert!(source.0 == 0u8 || source.0 == 1u8);
}

pub fn new(source: Choice) {
    assert!((source.0 == 0u8) | (source.0 == 1u8));
}

compiles to (stripping out stdlib fluff):

example::old:
        push    rax
        mov     al, dil
        cmp     al, 0
        mov     byte ptr [rsp + 6], al
        jne     .LBB48_2
        mov     byte ptr [rsp + 7], 1
        jmp     .LBB48_3
.LBB48_2:
        mov     al, byte ptr [rsp + 6]
        cmp     al, 1
        sete    cl
        and     cl, 1
        mov     byte ptr [rsp + 7], cl
.LBB48_3:
        mov     al, byte ptr [rsp + 7]
        xor     al, -1
        test    al, 1
        jne     .LBB48_4
        jmp     .LBB48_5
.LBB48_4:
        lea     rdi, [rip + .L__unnamed_5]
        lea     rdx, [rip + .L__unnamed_6]
        mov     rax, qword ptr [rip + std::panicking::begin_panic@GOTPCREL]
        mov     esi, 52
        call    rax
        ud2
.LBB48_5:
        pop     rax
        ret

example::new:
        push    rax
        mov     al, dil
        cmp     al, 0
        sete    cl
        cmp     al, 1
        sete    al
        or      cl, al
        xor     cl, -1
        test    cl, 1
        jne     .LBB49_1
        jmp     .LBB49_2
.LBB49_1:
        lea     rdi, [rip + .L__unnamed_7]
        lea     rdx, [rip + .L__unnamed_8]
        mov     rax, qword ptr [rip + std::panicking::begin_panic@GOTPCREL]
        mov     esi, 55
        call    rax
        ud2
.LBB49_2:
        pop     rax
        ret

(And conversely I see a branching on the output of the first equality check in the old function.)

@hdevalence
Copy link
Contributor

This seems to have been included in the develop branch by hand already, closing this PR.

@hdevalence hdevalence closed this Apr 29, 2019
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.

Short-circuit in debug mode resulting in non-constant-time execution
3 participants