ORAM Nonce Handling Belt+Suspenders

[jcape]

We should improve the way our ORAM implementation handles nonces. As of right now, it increments a counter each time a query is performed, but does not do anything when this rolls over. In practice, an attacker able to execute 1M queries per second will require 584Kyears to make the nonce roll over, but it's good to just add a panic! when the nonce would roll over in case our assumptions are wrong.

[cbeck88]

For future reference -- this is referring to the code in fog/ocall_oram_storage.

ORAM itself does not specifically call for encryption, but when you use it the way we do in enclave, and you want to have ORAMs that are bigger than EPC, you have to have a way of encrypting pages that leave the enclave. In our code that encryption and decryption is happening in the trusted side of the ocall_oram_storage interface

[cbeck88]

Actually i wonder if this panicking on overflow behavior is already going to happen?

https://github.com/mobilecoinfoundation/mobilecoin/blob/e3d71b55fcdf63be14973b3336a0625661a25264/fog/ocall_oram_storage/trusted/src/lib.rs#L415

At this line we increment "block_ctr" by one when we check something back in, and this is a u64. In C++ that would wrap around on overflow and the behavior is well defined, but in rust I think if you don't use wrapping_add it's a panic even if the value is unsigned?

[jcape]

Actually i wonder if this panicking on overflow behavior is already going to happen?

https://github.com/mobilecoinfoundation/mobilecoin/blob/e3d71b55fcdf63be14973b3336a0625661a25264/fog/ocall_oram_storage/trusted/src/lib.rs#L415

At this line we increment "block_ctr" by one when we check something back in, and this is a u64. In C++ that would wrap around on overflow and the behavior is well defined, but in rust I think if you don't use wrapping_add it's a panic even if the value is unsigned?

It's actually worse than that: debug will panic, release does... something else: https://play.rust-lang.org/?version=stable&mode=release&edition=2015&gist=3ab0ae94901b68fdc355ef8dd5157871

[cbeck88]

It looks to me that we explicitly turned these checks off I guess:

https://github.com/mobilecoinfoundation/mobilecoin/blob/e36f2db667770d204e0fd2c19cda8446c123d54e/Cargo.toml#L172

[profile.release]
opt-level = 3
rpath = false
lto = false
debug-assertions = false
overflow-checks = false

so I guess we should probably just use checked_add(1).expect(...) at that line instead

[cbeck88]

although maybe the config that's most relevant is the enclave

[cbeck88]

I guess its the same in the enclaves:

https://github.com/mobilecoinfoundation/mobilecoin/blob/e36f2db667770d204e0fd2c19cda8446c123d54e/fog/ingest/enclave/trusted/Cargo.toml#L61