Fix/ocall oram storage authentication #1732

wjuan-mob · 2022-03-30T08:12:32Z

This is a clone of 1585
Adds some naming clarification around the various indices used in the tests.
Fixes the tests by using indices that actually share a trusted merkle root.

jcape

Could you merge from master, there are quite a few (presumably) unintentional downgrades of packages in this PR.

wjuan-mob · 2022-03-30T16:17:22Z

Could you merge from master, there are quite a few (presumably) unintentional downgrades of packages in this PR.

Sorry about that. I had actually rebased to master. Not sure what went wrong here. I'll look at it.

cbeck88

LGTM, thank you!

jcape · 2022-03-30T20:38:07Z

@wjuan-mob LMK when you've gotten the Cargo.lock restored, as of right now every dependency is updated...

wjuan-mob · 2022-03-31T01:55:38Z

@wjuan-mob LMK when you've gotten the Cargo.lock restored, as of right now every dependency is updated...

I'm sorry. I looked into this further and I can not figure out this dependency tree. I tried copying in the cargo.lock from master and rebuilding as suggested, but that just returned me to the previous state with the downgraded dependencies. I'm trying to find some reason that it might be intentionally downgrading the dependencies but I can not find that either.

cbeck88 · 2022-03-31T02:03:51Z

I will take a look

This is a test-driven approach to investigating and fixing the issue mobilecoinfoundation#1576 These tests confirm that the trusted side indeed does not always detect when untrusted tampers with the memory, at this revision I have these results: ``` running 5 tests test helpers::exercise_oram_storage_clear_data ... FAILED test helpers::exercise_oram_storage_hammer_data ... FAILED test helpers::exercise_oram_storage_hammer_metadata ... ok test helpers::exercise_oram_storage_clear_metadata ... FAILED test helpers::exercise_oram_storage_shims ... ok ``` So the shims work normally when the memory is not tampered with, and the code detects when the untrusted metadata is hammered. But it does not detect when it is cleared, or when the data is tampered with. This is likely because of the issue point out in mobilecoinfoundation#1576, in the next commits we will fix it and get all these tests to pass. WIP try to fix trusted merkle root check Fix typo in readme Fix tests by using a branch that shares a trusted merkle root. Adding clarifying names to indices used in the test to better see what it is doing

cbeck88 · 2022-03-31T03:52:15Z

i think what was happening was, two things:

because of the merge commit (which merged a more recent master into this PR), github was showing a diff that includes all of the changes that were merged from master. this makes the github diff view useless, and shows all the cargo.lock changes from the recent version bumps.

to resolve this, i decided to squash the whole PR. i did this by git rebase -i master and squashed every commit into the first one.

i got a conflict like this:

diff --cc Cargo.lock
index f6c426bb,b683a64f..00000000
--- a/Cargo.lock
+++ b/Cargo.lock
diff --cc fog/ocall_oram_storage/trusted/Cargo.toml
index 6e9d673a,ac5857fb..00000000
--- a/fog/ocall_oram_storage/trusted/Cargo.toml
+++ b/fog/ocall_oram_storage/trusted/Cargo.toml
@@@ -22,7 -22,11 +22,11 @@@ rand_core = { version = "0.6", default-
  subtle = { version = "2", default-features = false }
  
  [target.'cfg(any(target_feature = "avx2", target_feature = "avx"))'.dependencies]
- blake2 = { version = "0.10.4", default-features = false, features = ["simd"] }
 -blake2 = { version = "0.9.2", default-features = false, features = ["simd"] }
++blake2 = { version = "0.10.2", default-features = false, features = ["simd"] }
  
  [target.'cfg(not(any(target_feature = "avx2", target_feature = "avx")))'.dependencies]
 -blake2 = { version = "0.9.2", default-features = false }
 +blake2 = { version = "0.10.2", default-features = false }
+ 
+ [dev-dependencies]
+ lazy_static = "1.4"
+ mc-util-test-helper = { path = "../../../util/test-helper" }
~
chris@chris-ThinkPad-X1-Carbon-7th:~/mobilecoinofficial/mobilecoin$ git diff
diff --cc Cargo.lock
index f6c426bb,b683a64f..00000000
--- a/Cargo.lock
+++ b/Cargo.lock
diff --cc fog/ocall_oram_storage/trusted/Cargo.toml
index 6e9d673a,ac5857fb..00000000
--- a/fog/ocall_oram_storage/trusted/Cargo.toml
+++ b/fog/ocall_oram_storage/trusted/Cargo.toml
@@@ -22,7 -22,11 +22,11 @@@ rand_core = { version = "0.6", default-
  subtle = { version = "2", default-features = false }
  
  [target.'cfg(any(target_feature = "avx2", target_feature = "avx"))'.dependencies]
- blake2 = { version = "0.10.4", default-features = false, features = ["simd"] }
 -blake2 = { version = "0.9.2", default-features = false, features = ["simd"] }
++blake2 = { version = "0.10.2", default-features = false, features = ["simd"] }
  
  [target.'cfg(not(any(target_feature = "avx2", target_feature = "avx")))'.dependencies]
 -blake2 = { version = "0.9.2", default-features = false }
 +blake2 = { version = "0.10.2", default-features = false }
+ 
+ [dev-dependencies]
+ lazy_static = "1.4"
+ mc-util-test-helper = { path = "../../../util/test-helper" }

the left - shows ours, the right - shows theirs. the ++ shows how I resolved it.
at some point in development we tried to change blake2 to version 0.10.4 but what I saw when i did git grep "blake2=" was that all the other crates were settign this to 0.10.2, so i didn't take ours or theirs, and changed this to use that version.

then i tried a rebuild, to get an up-to-date cargo.lock based on the cargo tomls.

after this i added cargo.lock and continued rebasing. this caused no more cargo lock changes and dropped the last two "update cargo lock" commits.

$ git diff HEAD~
diff --git a/Cargo.lock b/Cargo.lock
index f6c426bb..57d5c99e 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -3861,6 +3861,7 @@ dependencies = [
  "lazy_static",
  "mc-oblivious-traits",
  "mc-sgx-compat",
+ "mc-util-test-helper",
  "rand_core 0.6.3",
  "subtle 2.4.1",
 ]
diff --git a/fog/ocall_oram_storage/trusted/Cargo.toml b/fog/ocall_oram_storage/trusted/Cargo.toml

so i think the moral of the story is:

merging master into the PR can cause confusing diffs in the github view, especially if there were a lot of version bumps and such in the changes that were merged in
if you are getting a diff that shows changes that don't make sense (i.e. don't reflect a difference with current master, but an old version of master), then the right thing to do is probably squash the PR. squashing the merge and linearizing history, and causing the commit to be based off current master, means the reviewers will see a diff with current master rather than an old version of master.

cbeck88 · 2022-03-31T03:59:30Z

@wjuan-mob it may also be that the view was weird because you did the merge in kind of a funky way -- you started by checking out current master, and then made a branch there, and then merged my PR in.

that's backwards from how it's normally done. you should probably have checked out my PR, made a new branch there, and then merged master in if that's what you wanted to do.

merging is not symmetrical, deciding which is "ours" and which is "theirs" affects merge resolution, and may also affect the "view" git has of what the changes are when you look at the diff. so although it may seem counter-intuitive, you will get different results if you merge my PR into master, and then open a PR that merges that into master, from if you merge master into my PR, and then open a PR that merges that branch back into master.

that may be a better moral for the story, because the squash and rebase thing is not good when the PR is big, it breaks the "changes since you last reviewed" feature of github which james likes, and which saves reviewers time

cbeck88 · 2022-03-31T13:54:29Z

also idk why the blake2 0.10.4 was in there, i think that may just have been a mistake and that would definitely cause noise in the cargo locks

except that my diff still shows that so it must have been pre-existing... hmm

jcape · 2022-03-31T15:08:34Z

I'm fine with this as-is, I think the blake2 thing was probably a partial upgrade, we should add a check for diverging dependencies within a single crate to CI.

This is a test-driven approach to investigating and fixing the issue #1576 These tests confirm that the trusted side indeed does not always detect when untrusted tampers with the memory, at this revision I have these results: ``` running 5 tests test helpers::exercise_oram_storage_clear_data ... FAILED test helpers::exercise_oram_storage_hammer_data ... FAILED test helpers::exercise_oram_storage_hammer_metadata ... ok test helpers::exercise_oram_storage_clear_metadata ... FAILED test helpers::exercise_oram_storage_shims ... ok ``` So the shims work normally when the memory is not tampered with, and the code detects when the untrusted metadata is hammered. But it does not detect when it is cleared, or when the data is tampered with. This is likely because of the issue point out in #1576, in the next commits we will fix it and get all these tests to pass. WIP try to fix trusted merkle root check Fix typo in readme Fix tests by using a branch that shares a trusted merkle root. Adding clarifying names to indices used in the test to better see what it is doing Co-authored-by: Chris Beck <beck.ct@gmail.com>

* add tests for untrusted tampering with data and metadata (#1732) This is a test-driven approach to investigating and fixing the issue #1576 These tests confirm that the trusted side indeed does not always detect when untrusted tampers with the memory, at this revision I have these results: ``` running 5 tests test helpers::exercise_oram_storage_clear_data ... FAILED test helpers::exercise_oram_storage_hammer_data ... FAILED test helpers::exercise_oram_storage_hammer_metadata ... ok test helpers::exercise_oram_storage_clear_metadata ... FAILED test helpers::exercise_oram_storage_shims ... ok ``` So the shims work normally when the memory is not tampered with, and the code detects when the untrusted metadata is hammered. But it does not detect when it is cleared, or when the data is tampered with. This is likely because of the issue point out in #1576, in the next commits we will fix it and get all these tests to pass. WIP try to fix trusted merkle root check Fix typo in readme Fix tests by using a branch that shares a trusted merkle root. Adding clarifying names to indices used in the test to better see what it is doing Co-authored-by: Chris Beck <beck.ct@gmail.com> * Reverting blake2 version to 0.9.2 version for release 1.2 Co-authored-by: Chris Beck <beck.ct@gmail.com>

cbeck88 mentioned this pull request Mar 30, 2022

Maybe there is a bug in fog/ocall_oram_storage/trusted/src/lib.rs #1576

Closed

jcape suggested changes Mar 30, 2022

View reviewed changes

cbeck88 approved these changes Mar 30, 2022

View reviewed changes

cbeck88 force-pushed the fix/ocall-oram-storage-authentication branch from 797408d to 99c452d Compare March 31, 2022 03:44

cbeck88 requested a review from jcape March 31, 2022 03:53

remoun approved these changes Mar 31, 2022

View reviewed changes

jcape approved these changes Mar 31, 2022

View reviewed changes

jcape mentioned this pull request Mar 31, 2022

Create a CI test to find diverging dependencies #1742

Open

wjuan-mob merged commit c3f6473 into mobilecoinfoundation:master Mar 31, 2022

wjuan-mob mentioned this pull request Mar 31, 2022

add tests for untrusted tampering with data and metadata (#1732) #1748

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/ocall oram storage authentication #1732

Fix/ocall oram storage authentication #1732

wjuan-mob commented Mar 30, 2022

jcape left a comment

wjuan-mob commented Mar 30, 2022

cbeck88 left a comment

jcape commented Mar 30, 2022 •

edited

Loading

wjuan-mob commented Mar 31, 2022

cbeck88 commented Mar 31, 2022

cbeck88 commented Mar 31, 2022 •

edited

Loading

cbeck88 commented Mar 31, 2022 •

edited

Loading

cbeck88 commented Mar 31, 2022 •

edited

Loading

jcape commented Mar 31, 2022 •

edited

Loading

Fix/ocall oram storage authentication #1732

Fix/ocall oram storage authentication #1732

Conversation

wjuan-mob commented Mar 30, 2022

jcape left a comment

Choose a reason for hiding this comment

wjuan-mob commented Mar 30, 2022

cbeck88 left a comment

Choose a reason for hiding this comment

jcape commented Mar 30, 2022 • edited Loading

wjuan-mob commented Mar 31, 2022

cbeck88 commented Mar 31, 2022

cbeck88 commented Mar 31, 2022 • edited Loading

cbeck88 commented Mar 31, 2022 • edited Loading

cbeck88 commented Mar 31, 2022 • edited Loading

jcape commented Mar 31, 2022 • edited Loading

jcape commented Mar 30, 2022 •

edited

Loading

cbeck88 commented Mar 31, 2022 •

edited

Loading

cbeck88 commented Mar 31, 2022 •

edited

Loading

cbeck88 commented Mar 31, 2022 •

edited

Loading

jcape commented Mar 31, 2022 •

edited

Loading