w3f / schnorrkel

Schnorr VRFs and signatures on the Ristretto group
BSD 3-Clause "New" or "Revised" License
314 stars 93 forks source link

Aligned `SecretKey` for Intel-SA-00219 mitigation #52

Closed tomtau closed 4 years ago

tomtau commented 4 years ago

Reference: https://github.com/apache/incubator-teaclave-sgx-sdk/wiki/Mitigation-of-Intel-SA-00219-in-Rust-SGX

The aligned representation could perhaps be switched on with:

#![cfg_attr(target_env = "sgx")]
burdges commented 4 years ago

Is the stack itself safe? I shall assume so because otherwise we're fucked so long as curve25519_dalek::Scalar stays Copy.

Yes, one could add u64s before and after the data in MiniSecretKey and SecretKey. Those are pub(crate) fields which makes doing #![cfg_attr(target_env = "sgx")] everywhere mildly annoying, but some macros could save us there. Reasonable?

I'll caution that rustc can sequence fields inside a repr(Rust) type however it likes, so maybe we'd need #[repr(C)] too, and I'm unsure what else that impacts. Any idea?

Would #[repr(align(8))] alone suffice? If so, that's way simpler. Should we ask curve25519-dalek for #[repr(align(8))] on Scalar?

I suppose vrf::vrfs_merge_vartime risks attackers tweaking the merging scalars, which if combined with HDKD creates vulnerabilities, albeit extremely niche. We cannot run musig entirely from the stack per se, but we should only need to protect r_me in CommitStage/RevealStage. We cannot make those #[repr(C)] so we'd need some annoying #[repr(C)] wrapper for Scalar, but then this helps with vrf::vrfs_merge_vartime too.

burdges commented 4 years ago

I'm confused:

Asked for advice: https://github.com/dalek-cryptography/curve25519-dalek/issues/310

tomtau commented 4 years ago

left a comment (+ answers from Intel PSIRT): https://github.com/dalek-cryptography/curve25519-dalek/issues/310#issuecomment-569865231

if people planning to use this library in SGX environments ( https://github.com/w3f/schnorrkel/issues/31 ) think this mitigation may be helpful, feel free to re-open this issue