Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

DAWN-47 ⁃ Prevent Access to Tables not Specified in ABI #642

Closed
blockone-syncclient opened this issue Nov 6, 2017 · 10 comments
Closed
Assignees
Milestone

Comments

@blockone-syncclient
Copy link

blockone-syncclient commented Nov 6, 2017

(Replaces github issue #331)

Dan:
verify the table type matches
verify that a table name can only be used for a single index
verify that the ABI definition doesn't define multiple tables with the same name

These checks can be skipped during replay

Brian:

verify that the ABI definition doesn't define multiple tables with the same name
Already done by setcode call to AbiSerializer ctor, which calls setabi, which compares size of AbiSerializer map of tables to Abi vector of tables.

Brian:
@bytemaster
"verify the table type matches"
There are 2 ways I can see this being done. It could be checked in the store/read_*** wasm_interface intrinsic methods, but this would be a lot of wasted checks, since each wasm store/read only needs to be verified once.
So I am thinking that this needs to be done in the load, and the ModuleInstance analyzed.

"verify that a table name can only be used for a single index"
This really should be "verify that every code's table name can only be used for a single index", right? And then doesn't the fact that we verify that the ABI definition doesn't define multiple tables with the same name and that we "verify that the table type matches" already ensure this?

Also, I am presuming these are only performed if an ABI is provided, are there any checks we want to perform if there is no ABI? Like ensure that table reads and writes are consistent with each other?

Would think that this information has to be available in the processed WASM structure, but having a hard time identifying it. If it isn't we could identify it as we process the WASM.

Brian:

@bytemaster
Needs Clarification:

I am presuming the intent of this is safety (ensuring table access is consistent with ABI), not security, is that correct?

The statement about "these checks can be skipped during replay" sounds like these checks were planned to occur every time the code runs, but this seems to be a cost that shouldn't have to be incurred every time the code runs, vs. either using data in the existing loaded WASM structure (if it is possible) or else added to the WASM structure processing. If I don't get clarification on this I will plan to do the checks as the code is run, since that comment seems to assume this.

If the ABI is not provided, should a "lazy" ABI table definition be constructed as the code is run through, to ensure a table only has one type? Until I hear otherwise, I will assume the answer to this question is "yes".

For "verify that a table name can only be used for a single index", I will not do anything on this till I get clarification on exactly what it means.

Brian:
"verify the table type matches"
Assuming this means that the indextype matches, since that is available in the code, but actually identifying the type involved would require parsing the wast.

Working on tests.

ATC (Acceptance Test Criteria)

#1

  • Create ABI with duplicate table names
  • Verify ABI will not load

#2

  • Attempt to use a table not in ABI
  • Verify table access fails

#3

  • Use a valid table that matches ABI
  • Verify table access works

#4

  • With a valid table in ABI
  • Attempt to load with different key type (identify i64 key in ABI) and access with different type (use i64i64i64)
  • Verify access fails

#5

@blockone-syncclient
Copy link
Author

➤ Kevin Heifner commented:

This is in review. If it meets Dan's expectations it can be closed.

@blockone-syncclient
Copy link
Author

➤ Kevin Heifner commented:

I will review these changes for merge. Note the 1 story point is only for the review.

@blockone-syncclient
Copy link
Author

➤ Joshua Lavin commented:

[~kevin.heifner] [~corey.lederer] [~admin]] [~thomas.cox]

Not clear on the QA for this issue (and others). Who will be writing the test cases for these issues? I had thought this would be with the developer responsible for fixing the issue? Just want to make sure we're not doubling our efforts.

@blockone-syncclient
Copy link
Author

➤ Kevin Heifner commented:

Unit tests were created as part of the development. All other additional testing, I understood was to be performed by HK.

@blockone-syncclient
Copy link
Author

➤ Bezwada Satyapravin commented:

If I understand correctly, validating this JIRA requires us to implement a test contract that performs actions mentioned in Acceptance Criteria and execute the contract. Please let me know if that is not the case.

@blockone-syncclient
Copy link
Author

➤ Kevin Heifner commented:

That is correct. Unittests that perform similar operations as the ATC have been created and are part of the build. You can see the tests by looking at the diff for this issue:
224e238

@blockone-syncclient
Copy link
Author

➤ Andrianto Lie commented:

I have written sample contracts to verify these 5 cases and assures that they are all working based on the ATC.
The sample contracts can be found here https://github.com/andriantolie/table-test

@blockone-syncclient
Copy link
Author

➤ Andrianto Lie commented:

The sample contracts behaves differently when we tested it on the testnet by the way.
For every action where we expect to be failed, we got the following error instead from the testnet:

status_code == 200: Error code 500
: 10 assert_exception: Assert Exception
valuelen >= keylen: insufficient data passed
    {}
    thread-0  wasm_interface.cpp:70 validate

{"name":"apply","current_validate_context->msg.type":"foocase4"}
    thread-0  wasm_interface.cpp:648 vm_call

{}
    thread-0  wasm_interface.cpp:710 apply

{"context.msg":{"code":"case5","type":"foocase4","authorization":[],"data":""}}
    thread-0  chain_controller.cpp:949 apply_message

{"trx":{"refBlockNum":621,"refBlockPrefix":3998459910,"expiration":"2017-11-03T10:03:01","scope":["case5"],"readscope":[],"messages":[{"code":"case5","type":"foocase4","authorization":[],"data":""}],"signatures":[]}}
    thread-0  chain_controller.cpp:986 process_transaction

{"trx":{"refBlockNum":621,"refBlockPrefix":3998459910,"expiration":"2017-11-03T10:03:01","scope":["case5"],"readscope":[],"messages":[{"code":"case5","type":"foocase4","authorization":[],"data":""}],"signatures":[]}}
    thread-0  chain_controller.cpp:958 apply_transaction

{"trx":{"refBlockNum":621,"refBlockPrefix":3998459910,"expiration":"2017-11-03T10:03:01","scope":["case5"],"readscope":[],"messages":[{"code":"case5","type":"foocase4","authorization":[],"data":""}],"signatures":[]}}
    thread-0  chain_controller.cpp:244 push_transaction

{"c":500,"msg":"10 assert_exception: Assert Exception\nvaluelen >= keylen: insufficient data passed\n    {}\n    thread-0  wasm_interface.cpp:70 validate\n\n    {\"name\":\"apply\",\"current_validate_context->msg.type\":\"foocase4\"}\n    thread-0  wasm_interface.cpp:648 vm_call\n\n    {}\n    thread-0  wasm_interface.cpp:710 apply\n\n    {\"context.msg\":{\"code\":\"case5\",\"type\":\"foocase4\",\"authorization\":[],\"data\":\"\"}}\n    thread-0  chain_controller.cpp:949 apply_message\n\n    {\"trx\":{\"refBlockNum\":621,\"refBlockPrefix\":3998459910,\"expiration\":\"2017-11-03T10:03:01\",\"scope\":[\"case5\"],\"readscope\":[],\"messages\":[{\"code\":\"case5\",\"type\":\"foocase4\",\"authorization\":[],\"data\":\"\"}],\"signatures\":[]}}\n    thread-0  chain_controller.cpp:986 process_transaction\n\n    {\"trx\":{\"refBlockNum\":621,\"refBlockPrefix\":3998459910,\"expiration\":\"2017-11-03T10:03:01\",\"scope\":[\"case5\"],\"readscope\":[],\"messages\":[{\"code\":\"case5\",\"type\":\"foocase4\",\"authorization\":[],\"data\":\"\"}],\"signatures\":[]}}\n    thread-0  chain_controller.cpp:958 apply_transaction\n\n    {\"trx\":{\"refBlockNum\":621,\"refBlockPrefix\":3998459910,\"expiration\":\"2017-11-03T10:03:01\",\"scope\":[\"case5\"],\"readscope\":[],\"messages\":[{\"code\":\"case5\",\"type\":\"foocase4\",\"authorization\":[],\"data\":\"\"}],\"signatures\":[]}}\n    thread-0  chain_controller.cpp:244 push_transaction"}
    thread-0  httpc.cpp:113 call

{"server":"10.160.12.152","port":8889,"path":"/v1/chain/push_transaction","postdata":{"refBlockNum":621,"refBlockPrefix":3998459910,"expiration":"2017-11-03T10:03:01","scope":["case5"],"readscope":[],"messages":[{"code":"case5","type":"foocase4","authorization":[],"data":""}],"signatures":[]}}
    thread-0  httpc.cpp:117 call

However, I suspect it not to be the problem with the code but because the testnet doesn't have the latest version.

@blockone-syncclient
Copy link
Author

➤ Kevin Heifner commented:

From the line numbers in the previous comment, I can tell this is from the previous week build. This code is NOT the latest from master.

@coreylederer
Copy link
Contributor

Sync'd the issue back to JIRA and closing.

@coreylederer coreylederer added this to the EOS Dawn 2.0 milestone Nov 6, 2017
@blockone-syncclient blockone-syncclient changed the title STAT-47 ⁃ Prevent Access to Tables not Specified in ABI DAWN-47 ⁃ Prevent Access to Tables not Specified in ABI Jan 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants