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

New BSIP: fix locked accounts / recursive account permissions #94

Open
abitmore opened this issue Aug 9, 2018 · 39 comments

Comments

9 participants
@abitmore
Copy link
Member

commented Aug 9, 2018

Earlier discussion is here: bitshares/bitshares-core#269

We do need to identify the locked accounts first.

After identified, an approach to fix them is reverting the last account_update_operation done in the accounts.

@dayman32

This comment has been minimized.

Copy link

commented Aug 9, 2018

What does it mean to identify? My account is "dayman32" - and I have all the keys to it.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2018

  • We should not modify accounts without the explicit permission of the account owners (which is tricky obviously, because they can't prove ownership in the first place). Proving ownership of previous keys may be sufficient.
  • Does this BSIP include funding of required development, or only the permission to modify the code?
  • This is a recurring subject. A BSIP should not only fix accounts, but try to mitigate the issue for the future.
@abitmore

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2018

@pmconrad good points.

This is an issue but not a formal BSIP document. What should we put in the BSIP is to be discussed.

@xeroc

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

Would it make sense to start integrating the account recovery feature that steem has?
That way, he could challenge the new owner role and have the previous installed again ..

@dayman32

This comment has been minimized.

Copy link

commented Aug 29, 2018

When will this issue be solved to unblock accounts with our funds?

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

I feel this Issue needs a BSIP drafted. It will be a consensus change, so December at the earliest for release (unlikely). Next opportunity would be June. Let's get an abstract defined here, then begin the specification within a PR. This BSIP drafting process is still TBD.

@bycz6

This comment has been minimized.

Copy link

commented Sep 12, 2018

My account is bycz2. I have the brain key and wallet backup.

@iceworlder

This comment has been minimized.

Copy link

commented Sep 15, 2018

waiting for solutions.
account: bcbwbrldbr

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2018

Possible approach for preventing future accidents:

  • add a flag to account_update, if set the account_update operation must be authorized by both the old and the new authorities
@abitmore

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2018

add a flag to account_update, if set the account_update operation must be authorized by both the old and the new authorities

An account_update operation with the account itself in authorities or with an impossible loop after updated will still pass current authorization check in bitshares-core, because authorization is only verified before applying the changes (bitshares/bitshares-core#1373 is related).

Actually, I think it's possible to check whether an account is locked after applied the update. I think YOYOW has implemented it or almost there, related code is here.

@xeroc

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

Do you guys recall the account recovery stuff in steem?
Basically, one can challange the change of the authory (by paying a fee), then (one of) the previous permissions can call a claim for the account and replace the authority. That could help in this case too.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

IIRC there's a limit to how long you can claim back an account, so that wouldn't really help the locked accounts.
And I'm not sure if it's a good idea to retroactively introduce this feature. Suppose you have bought an account from someone, assuming that it's yours now, and on return you find that it has been claimed back by him...

@xeroc

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

And I'm not sure if it's a good idea to retroactively introduce this feature. Suppose you have bought an account from someone, assuming that it's yours now, and on return you find that it has been claimed back by him...

Good point.

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

Perhaps a way for current auth to sign intention to revoke legacy auths. This would make it incumbent on a new owner to perform this step as part of account transfer. In case the account is locked, a prior auth could request restoration (because current auth was unable to sign the revocation).

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

IMHO how to recover stolen account is out of this issue's scope.

@ryanRfox your proposal about "current auth to sign intention to revoke legacy auths" is exactly current behavior. If you changed authorities, new authorities would gain control, old authorities would lose control.

For locked accounts, my proposal would be simply undo the last account_update operation with a hard fork, no exception, no request required. As mentioned above, likely we'll be able to detect and avoid further account locking up with a hard fork. So all will be settled.

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

Adding to (next) Protocol Release for further research and discussion. Not clear where the best place to discuss this will be. It seems identifying the impacted accounts is required step. Shall we begin with an effort to do this?

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

We agree with @abitmore:

For locked accounts, my proposal would be simply undo the last account_update operation with a hard fork, no exception, no request required. As mentioned above, likely we'll be able to detect and avoid further account locking up with a hard fork. So all will be settled.

Also, we offer to add functionality preventing create or update account authority with circular dependencies.

The toolkit/script of identification of the impacted users should be elaborated before hard-fork.

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Yes, @OpenLedgerApp please do some research to identify the locked accounts. Suggesting from @oxarbitrage is to use elasticsearch plugin. Next, would be some effort to identify the accounts in a deterministic way. Please add an effort estimate in this Issue. Thanks

@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

actually in regards to elasticsearch what needed is the es_objects plugin.

here are all the accounts created in bitshares:

www.bitshares-kibana.info:5601/app/kibana#/discover?_g=(refreshInterval:(display:Off,pause:!f,value:0),time:(from:now-5y,mode:quick,to:now))&_a=(columns:!(_source),index:debd10a0-20b6-11e9-82af-8dfbd16c55a9,interval:auto,query:(language:lucene,query:'_index:"objects-account"'),sort:!(block_time,desc))

However, to get the locked accounts we need to among other things, do a query on top of that to compare if owner.account_auths == name.
Will get us the locked accounts that added their own account into the owner auth. Other checks must also be done like if active.account_auths == name, etc.

Unfortunately, it seems that this kind of checks cant be done by kibana(lucene syntax) but can be done by elasticsearch script queries, something like: https://stackoverflow.com/questions/28845052/query-for-one-field-doesnt-equal-another-field-in-elasticsearch

I dont want to go further into that as maybe @OpenLedgerApp already have its own way to identify them, can be also by done by the bitshares api with python or whatever by pulling all the accounts and make the checks there; 1,2 million accounts to check should not be a huge performance issue.

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Regarding the scope of this BSIP: Should we limit to only owner.account_auths == name or expand to include active.account_auths == name? I tend toward least intervention. If active is currently "locked" it can be reset with owner.

Are we interested to learn if any additional "locked accounts" exist because their authority references a locked account? This gets into recursion and I'm not suggesting we dig too deep. If the remedy is to unlock the parent account, the children will become unlocked without further action required by this BSIP.

@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Inside owner or active we have account_auths and also key_auths and address_auths.

Unsure if you can also block your own account with thoses but worth discussion.

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

We have made research to identify the locked accounts:
At the first step, we collected potential accounts authorities with circular dependencies using Elastic.
Then we found authorities with circular dependencies using a python script.

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

@OpenLedgerApp please create a PR with your findings thus far. It will be most beneficial if the ES queries and Python code can be posted for review and evaluation by other reviewers. We can use the results to continue refining a BSIP draft.

@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

By the way, the following url can be used to make direct queries to ES with es_objects data:

alfredo@alfredo-Inspiron-5559 ~ $ curl -X GET 'https://elasticsearch.bitshares-kibana.info/objects-account/data/_count?pretty=true' -H 'Content-Type: application/json' -d ' 
{              
    "query" : {                                  
        "bool" : { "must" : [{"match_all": {}}] }
    }
}
'
{
  "count" : 1303561,
  "_shards" : {
    "total" : 5,
    "successful" : 5,
    "skipped" : 0,
    "failed" : 0
  }
}
alfredo@alfredo-Inspiron-5559 ~ $ 

Also, the kibana url posted above changed to https://bitshares-kibana.info/ instead of www.bitshares-kibana.info:5601

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

We have created a branch with our results - https://github.com/openledger/bitshares-core/tree/94-fix-locked-accounts/tests/locked_accounts
The branch contains ES queries, Python code, and instructions.
You can use instructions in order to check our investigation results.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

IMHO, we need consensus code (C++) in bitshares-core to avoid future circuits, so python scripts or ES queries which are only able to detect existing circuits (after damage has been done) are not enough, although they are indeed helpful for testing.

We don't need to put the list of already locked accounts into the BSIP, although we can take some of them as examples.

I may be able to write the C++ code, but I don't have time in the near future, if OpenLedgerDev can get it done, that's great.

@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Of course identify them is not enough, i think it is just to do this part of the issue:
"We do need to identify the locked accounts first."

My understanding is @OpenLedgerApp will do the c++ code as well.

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

IMHO, we need consensus code (C++) in bitshares-core to avoid future circuits, so python scripts or ES queries which are only able to detect existing circuits (after damage has been done) are not enough, although they are indeed helpful for testing.

it's a good point and we understand it. We wanted to share our investigation results with the community and found out how many locked accounts there are.
Also, our python code can be adopted into C++ code which fix locked accounts.

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

After internal discussions, we are seeing the solution of this issue into 2 steps:
First, we offer to add additional functionality "avoid circular dependencies" (@abitmore -c) to the nearest hard fork. This functionality will prevent to create or update account authority with circular dependencies in the future.

The second and final step, we can develop C++ code which will be executed in the maintenance interval. This C++ code will find all locked accounts with circular dependencies and undo the last account_update operation. However, this step in comparison with the first one should be discussed with community. Maybe we should publish such accounts and ask the approval for this step for every owner.

What do you think?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

The same code that forbids circular dependencies after a hardfork can detect these before the hardfork and collect them for fixing them at maintenance after the hardfork.

Both steps are a change of consensus and require specification in BSIP that is particularly specific on the detection logic.

IMO owner approval from individual accounts is not needed. If someone did this on purpose to make their account unusable there are different ways to achieve this again.

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

As discussed with Ryan we are developing additional functionality "avoid circular dependencies". Tomorrow we will provide the first result, so you can review our code.

For step 2 we have been starting to create BSIP. We are going to finalize the spec for BSIP based on our existing code logic. We will provide our solution and describe potential risks for unlocking the accounts at the middle of next week.

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

We have created a branch with our results - openledger/bitshares-core@d343200

The branch contains С++ code for additional functionality "Prevent to create cycled accounts". This functionality will prevent to create or update account authority with circular dependencies in the future.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

Your code is based on a wrong understanding of owner authorities: If Alice is listed as Bob's owner authority, then Alice's active authority is sufficient to approve key changes on Bob's account, for example.

Please let's agree on a specification first, then you can implement what we have agreed upon.

Also note @abitmore 's remark in bitshares/bitshares-core#269 (comment) :

Setting the owner permission to an account itself is legit, with this approach a group with active permission can sign transactions/operations that require owner permission. And vise versa.

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

@pmconrad, thank you for your comment.

Could you explain several details to avoid our misunderstanding:

At this moment Bitshares has the implementation: active authority can change the owner authority.
Preconditions:
Alice is listed as Alice's owner authority and Bob is listed as Alice's active authority.
Steps:
Bob updates Alice's account. He changes Alice's owner authority: Alice.owner= authority{1, bob_id, 1}
Results:
Alice's account was updated. Now Bob is listed as Alice's owner.
Expected result:
According to https://bitshares.org/technology/dynamic-account-permissions, this transaction should fail.

Our solution based on documentation - https://bitshares.org/technology/dynamic-account-permissions
1.1. The active authority is meant to be a hot key and can perform any action except changing the owner authority.
1.2. The owner authority is designed for cold-storage, and its primary role is to update the active authority or to change the owner authority.

Could you please explain how authority has to work?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

1.1. The active authority is meant to be a hot key and can perform any action except changing the owner authority.
1.2. The owner authority is designed for cold-storage, and its primary role is to update the active authority or to change the owner authority.

This is correct.

At this moment Bitshares has the implementation: active authority can change the owner authority.

By including another account in the owner authority you delegate owner authority to that other account. Confirming actions on behalf of another account requires only active authority, even if that action requires owner authority itself (but in that case the delegation must come from owner authority, not from active!). In your specific example, Alice delegates her owner authority to active, which results in her active authority being sufficient to change owner keys.

This is the way it is implemented and I believe this is intentionally so.

@OpenLedgerApp

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

@pmconrad, thank you for your comments. They're quite useful!

So comes another use case.
We have made a unit test - openledger/bitshares-core@04af1e4 for the following use case:
Preconditions:
There are two accounts Alice and Jill.
They have authorities created by default ( owner = authority(123, key, 123) active = authority(321, key, 321) where key is alice_public and jill_public)
Steps:
Alice add Jill into Alice's owner authority: owner = authority(2, public_key_type(alice_owner_key.get_public_key()), 2, jill_id, 2);
Then Jill set her active authority: active = authority(4, jill1_id, 2, jill2_id, 2);
Results:
Jill2 and Jill1 can't change Jill's owner auth
But
Jill2 and Jill1 are able to change Alice's owner auth.

So, we see potential risk for Alice account.

Could you please describe the expected results?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

The behaviour is as expected. Authority delegation is transitive, on purpose, for a limited number of levels. The limit is configurable by the committee.

Delegating authority to someone else comes with a risk of course, especially in the case of owner authority. Alice should do that only if she trusts Jill.

@dayman32

This comment has been minimized.

Copy link

commented Feb 20, 2019

Why is this question not tagged with a hard fork? I have been waiting for a decision for more than 2 years. When will fix this bug? * When will hard fork?

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

@dayman32 all discussions in this repository (BSIP) require hard fork so we don't tag individuals. We're now discussing the solution, when done and approved by stake holders via on-chain voting, there will be a hard fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.