Fix leakage when the cacheline is 32-bytes in CBC_MAC_ROTATE_IN_PLACE #18033
Conversation
openssl-machine
added
the
hold: cla required
github-actions
bot
added
the
severity: fips change
openssl-machine
removed
the
hold: cla required
|
This will require a CLA to be submitted: |
mattcaswell
added
the
approval: review pending
t8m
added
branch: master
t8m
added
approval: done
|
@mattcaswell do you agree this should be cherry picked to all branches? |
rotated_mac is a 64-byte aligned buffer of size 64 and rotate_offset is secret.
Consider a weaker leakage model(CL) where only cacheline base address is leaked,
i.e address/32 for 32-byte cacheline(CL32).
Previous code used to perform two loads
1. rotated_mac[rotate_offset ^ 32] and
2. rotated_mac[rotate_offset++]
which would leak 2q + 1, 2q for 0 <= rotate_offset < 32
and 2q, 2q + 1 for 32 <= rotate_offset < 64
The proposed fix performs load operations which will always leak 2q, 2q + 1 and
selects the appropriate value in constant-time.
Yes - although I strongly suspect it won't cherry-pick to 1.1.1 cleanly so we will probably need another PR there. |
|
@mattcaswell @t8m |
|
Ah, missed that #18050 wasn't the main PR. Reposting my comment here, to keep the main discussion in one place: Rather than making assumptions about cachelines, we switched to a simpler cacheline-independent O(N log N) algorithm in BoringSSL. It removes the need for the I think that would be a better strategy. It is more straightforwardly constant-time. CVE-2016-0702 is an example where assumptions about cacheline behavior don't quite match how CPUs behave. The O(N log N) approach avoids these assumptions. |
|
I prefer @davidben's suggestion. Making assumptions about cache lines is less than ideal. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
t8m
added
approval: ready to merge
|
Merged to master and 3.0 branches. Thank you for your contribution. |
rotated_mac is a 64-byte aligned buffer of size 64 and rotate_offset is secret.
Consider a weaker leakage model(CL) where only cacheline base address is leaked,
i.e address/32 for 32-byte cacheline(CL32).
Previous code used to perform two loads
1. rotated_mac[rotate_offset ^ 32] and
2. rotated_mac[rotate_offset++]
which would leak 2q + 1, 2q for 0 <= rotate_offset < 32
and 2q, 2q + 1 for 32 <= rotate_offset < 64
The proposed fix performs load operations which will always leak 2q, 2q + 1 and
selects the appropriate value in constant-time.
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18033)
(cherry picked from commit 3b83638)
rotated_mac is a 64-byte aligned buffer of size 64 and rotate_offset is secret.
Consider a weaker leakage model(CL) where only cacheline base address is leaked,
i.e address/32 for 32-byte cacheline(CL32).
Previous code used to perform two loads
1. rotated_mac[rotate_offset ^ 32] and
2. rotated_mac[rotate_offset++]
which would leak 2q + 1, 2q for 0 <= rotate_offset < 32
and 2q, 2q + 1 for 32 <= rotate_offset < 64
The proposed fix performs load operations which will always leak 2q, 2q + 1 and
selects the appropriate value in constant-time.
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18033)
rotated_mac is a 64-byte aligned buffer of size 64 and rotate_offset is secret.
Consider a weaker leakage model(CL) where only cacheline base address is leaked,
i.e address/32 for 32-byte cacheline(CL32).
Previous code used to perform two loads
1. rotated_mac[rotate_offset ^ 32] and
2. rotated_mac[rotate_offset++]
which would leak 2q + 1, 2q for 0 <= rotate_offset < 32
and 2q, 2q + 1 for 32 <= rotate_offset < 64
The proposed fix performs load operations which will always leak 2q, 2q + 1 and
selects the appropriate value in constant-time.