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

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

Closed
AmbitionXiang opened this issue Mar 6, 2022 · 3 comments
Closed
Assignees
Labels
bug Something isn't working old-1.2.0 Targetted for v1.2 release security Public issues which impact security impact

Comments

@AmbitionXiang
Copy link

Hi, there.

At line 331, if idx == first_treetop_index seems to be if idx == first_treetop_index - 1? Otherwise, the check against trusted roots is never encountered.

@cbeck88 cbeck88 self-assigned this Mar 7, 2022
cbeck88 added a commit to cbeck88/mobilecoin that referenced this issue Mar 8, 2022
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.
@cbeck88 cbeck88 added bug Something isn't working old-1.2.0 Targetted for v1.2 release security Public issues which impact security impact labels Mar 12, 2022
@cbeck88
Copy link
Contributor

cbeck88 commented Mar 12, 2022

I am still investigating and started working on this -- I think you are right that there is an off by one mistake that makes the merkle root check not happen. I will finish writing a patch and tests soon.

Thanks for reporting this issue!

@cbeck88
Copy link
Contributor

cbeck88 commented Mar 30, 2022

@AmbitionXiang we think we have a fix for this here: #1732
We plan to fix this in the 1.2.0 release.

Thank you again for reporting!

@AmbitionXiang
Copy link
Author

You're welcome!

cbeck88 added a commit to wjuan-mob/mobilecoin that referenced this issue Mar 31, 2022
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
wjuan-mob added a commit that referenced this issue Mar 31, 2022
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>
wjuan-mob added a commit that referenced this issue Mar 31, 2022
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>
wjuan-mob added a commit that referenced this issue Mar 31, 2022
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working old-1.2.0 Targetted for v1.2 release security Public issues which impact security impact
Projects
None yet
Development

No branches or pull requests

2 participants