Immediate Action to Protect Internet Identity w/ Seed Phrases

@justmythoughts I want to address some of your concerns. Like @dostro has said we want to keep this conversation scoped, as many people are suggesting ideas that could be great but would be much more complicated or take much more time than the (hopefully) simple fix we are suggesting.

As for your specific concerns about the seed phrase, I have a few points:

  1. I personally believe the long-term solution to the central point of failure of the seed phrase is to use some thing like social recovery which splits the seed phrase into shares. These shares can then be stored in different locations or with different users that you trust, and a threshold of them (say 3 out of 5) would be required to restore an account
  2. I think it’s reasonable that if you can opt-in by typing in the seed phrase, then you could opt-out with the same mechanism, but we’ll need to think about it more
  3. As a simple fix, I like focusing on the seed phrase as it’s already common practice in the world of cryptocurrency to assign the seed phrase as the ultimate backstop, and there is a culture and general know-how on what it takes to secure a seed phrase

Maybe to sum up, we’re suggesting that this simple change optimizes for the quickest solution that might drastically increase security, given the current capabilities and vulnerabilities of Internet Identity.

If anyone sees major issues with this specific proposal, please bring them up. And if there are specific suggestions for alternatives that optimize for what we’re trying to optimize for, that sounds in scope to me.

What doesn’t seem useful to me here is coming up with all sorts of complicated ideas that we obviously want to look at down the line but are out of scope for a quick fix that greatly improves security now.

6 Likes

hi team, we started a monthly working group meeting on Internet Identity and Authentication: Working Group: Identity & Authentication Come and join us. FYI @frederikrothenberger

This turned out much longer than expected, sorry about that :man_facepalming: these are just my 2cts so feel free to just skip it

Wow, this is a great thread!

I love seeing everyone coming together and discussing features like this. I’m a little late to the party but I’ll share some thoughts (the first one being: this thread probably belongs in the Internet Identity category, which dfinitly deserves some love (pun intended)).

There are some points that were brought up in the original “II Lack of Security” post that I think we should keep in mind. The first one is that II was made to be as simple as possible to be flexible, but now it’s clear that it is too simple, because it lacks some important security features raised originally by @Roman . The second point was that “seed phrase” really is a misnommer in II land. A “seed” phrase really is some random (unique, immutable) material generated when initializing e.g. a wallet. In II land I think it’s more correct to call this a “recovery phrase”.

I really like the idea put forward by @lastmjs, and I LOVE that @dostro actually took the time to make mock-ups! What I understand from the original idea is this: If you have a recovery phrase, then you cannot remove said recovery phrase without proving you have the recovery phrase. dostro in your (very welcome) “sheparding” post you suggest we focus on agreeing (or not) on whether this should be opt-in or not (also brought up by lastmjs and others), and bring up bjoern 's worry about whether implementing a “simple” solution now and implementing a full solution later will be costly; I’ll share some thoughts on both (again, just my 2cts).

First, should this be opt-in? Whatever solution is agreed on here will probably need to be opt-in; at the very least we cannot just make all existing recovery phrases “secured” (i.e. you need the phrase to delete the phrase) because lots of people (probably myself included :see_no_evil:) never actually wrote down the phrase (and we can’t just have those people have their single recovery phrase slot be taken by a phrase they don’t remember). Basically we need to support (some) backwards compatibility (as also brought up by @skilesare and the Origyn (Orygin?) use case). Either way, looks like everybody agrees: solution should be opt-in.

Then, is it (too) expensive to implement a stopgap solution? Well, that largely depends on the stopgap solution, the “final” perfect solution, and the engineers who implement it. And while the Identity Labs are stellar, I can’t vouch for those @#* Internet Identity guys!!

More seriously, it’s very likely that whatever solution we come up with will effectively involve “changing the database schema”, i.e. changing the data we write to the canister stable memory. This means: an upgrade that can go wrong, potentially no rollback, and potentially shooting ourselves in the foot and regretting it for the next 5 years. So I think we can agree on a stopgap solution, but before we implement it we will need to have some idea of what the “final” solution will be, and some idea of how it will be implemented (basically do some thought experiments on what the future stable memory schema will look like and how we can migrate to it as we implement the “final” solution).

Now I’d like to share some thoughts on other aspects of the proposed solution. What I gathered from your posts lastmjs and dostro , and I may very well be wrong, is that you are only talking about recovery phrases, and not recovery devices. There are some very good points (I think by @coteclaude and @vavram, and empirical evidence shared by bjoern) that plaintext phrases can be lost and corrupted. But more importantly in my opinion (which appears to be shared by @Desinternauta, @GLdev and bjoern) is that the phrase keeps transiting through the browser and the monitor (on generation and then on input), which makes it subject to theft.

(quick side note: wpd suggested we force the user to input their recovery phrase when deleting any device, not just recovery devices; that’s something I thought about as well when reading the thread, but it would mean getting the “secured” recovery phrase in and out of the safe repeatedly, and typing it in the browser, which will be a real hassle and a risk; so I suggest we don’t do that)

With all that in mind, I think it’s worth thinking about making the recovery devices “secured”. bjoern also gives a nice example of how e.g. a recovery phrase can be used on a Ledger device; in this case the user still has a recovery device from II’s point of view, but can use an actual seed phrase which is only used on the Ledger device, and not in the browser. My recommendation would then be to only make the recovery devices “secured”, and not the recovery phrases. I’m sure some people will disagree though, in particular dostro and lastmjs since they explicitly suggest making the recovery phrase secured (and not the device); I’d love to hear examples of workflows and situations where the FIDO device/hardware wallet is not suitable.

Now there are also some great ideas (by @kusiyo and @justmythoughts for instance) that are more complicated (multiple devices, social recovery, etc) and that we should keep in mind for the future (or as I believe dostro puts it: “backlog out-of-scope questions, ideas, and discussions”).

Ok I didn’t expect this post to be so long and my wrist is starting to hurt so I’ll wrap up, but there’s one more thing I’d like to bring up, and that’s the question of “who will implement this”. This is really exciting to me because this is the first feature that’s really driven by the community. I think as many people as possible should share their (structured) ideas here and in the community chat tomorrow (can someone share the link?). For implementation however, I think DFINITY’s policy (at least for NNS and II) has been that external contributions are not accepted. I think this makes a lot of sense, because so far virtually no one has submitted patches as far as I know, and because a malicious contribution in either II or NNS (dapp) would be catastrophic for the whole ecosystem.

But then, I don’t believe this to be a “hard and fast” rule/policy. I would be uncomfortable knowing that a feature as sensitive as the one suggested here is the first to be contributed externally to a DFINITY team that hasn’t had a chance to sharpen their anti-byzantine review skills, but it would be a great opportunity to loop the community in. After we’ve decided on what exactly the solution will be here, the team could for instance deploy a test version of II that everyone can play with and then feedback can be shared here. Basically, while DFINITY will probably be writing the actual code, it would be awesome if everyone could still be working on this.

(note, these are just my own 2cts)

(P.S.: Actually a great first external contribution would be a patch that replaces all the occurrences of “seed phrase” with “recovery phrase”)

7 Likes

Thank you for this elaborate input! Really like the recovery phrase terminology instead of seed phrase

I’m operating under two assumptions:

  • The ability for users to remove their recovery phrase without some kind of confirmation is too dangerous a threat to leave open any longer than it needs to be
  • An opt-in feature where a recovery phrase must be resubmitted can be done without introducing breaking or irreversible changes

Are you in disagreement on the first, given your suggestion to secure recovery devices, and what does that mean exactly? Anyone else disagree with this first assumption?

Is anyone in disagreement on the second assumption - that the optional enhancement of security without introducing breaking or irreversible changes (and approved by more eyes, inclusive of dfinity’s II team) - makes this feature an attractive trade-off between time to implement and positive security effect? Some of our team started researching the exact change requirements and as a first step will summarize here the effects on the stable memory schema when they’re done. We’ve yet to receive any other technical input, but expect to hear from dfinity’s II team soon. And please do join tomorrow’s working group session @emmaperetti suggested - this will be a topic of conversation!

Btw we’ve introduced one small feature to II as external contributors - developers have the option to open II in a new tab or new window as of dfinity/agent 0.11.0. Here’s how it looks in a new window:

3 Likes

Great post. To retake your wise distinction, a lot of us want to get a secured recovery phrase, because we have been told that the secured device : Ledger Hardwallet using FIDO, was risky because we were not sure that an authentication through FIDO could always work (bug, deprecation or anything else). What is your opinion about this. An impossibility of failure of FIDO used with an hard wallet would make the securing of the seedphrase useless, since we would have an unsecured seedphrase BUT ON a secured device, so eventually an indirectly secured seedphrase.

So the question is : do we see any risk of failure of FIDO on our hard wallets ? If not, I think the question is solved : keep the elegant simplicity of the II, and complete it with an hard wallet and FIDO.

1 Like

I must be missing your point… if someone gains access to your anchor by some means (i.e. stole your Yubikey), wouldn’t you think their ability to remove your recovery phrase without knowing what it is a rather significant threat? Building a concept of recovery device hierarchy would certainly solve this problem and what we’re proposing here is the first iteration of that where “only proving possession of a recovery phrase unlocks the ability to remove itself”, and which @paulyoung helpfully generalized to “you can only remove a device/seed phrase by proving it’s in your possession”

Am I understanding correctly your proposal is to rely on the security of recovery devices and if so, should we just remove recovery phrases from II entirely as a solution to the aforementioned threat?

2 Likes

Yes, you are right. My mistake. I take it back.
I was talking about the situation where you use only fido with Ledger Hardwallet. But ICP is meant for an universal and fluid adoption on phones etc. So I take back.

1 Like

In hindsight this would make it impossible to remove devices added by an attacker.

The difference with the recovery phrase is that there’s only one.

3 Likes

As mentioned, I already suspected the “let’s only make the FIDO devices secure” bit to be controversial.

Here I guess that making the whole feature opt-in already means leaving the threat open for most people, but I see your point. My fear is that by allowing recovery phrases to be “secured” in the way we discussed we make it easy for users to think their account is secure enough, whereas really it carries all the issues mentioned here:

Maybe a UX expert can chime in here?

About the implementation, you say

And while it’s true that in theory everything can be reversed, making changes to the stable memory does come at a cost (that of twiddling with the schema and opening the door to really bad bugs, as well as the cost of preparation to make sure upgrades and rollbacks do still somewhat work). There’s also the problem that once we roll this out, it’s not really “reversible” in the sense that we can’t tell people “your recovery phrase is secure!” and then roll back the feature a week later, that would defeat the purpose.

Some pointers on implementation now; the code suggest that a Storage<T> object is the only thing read from stable memory:

If we follow the code and the types, we see this is actually a Storage of a vector of DeviceDataInternal:

and here’s the definition:

So the stable memory is cut in chunks that are indexed by anchors, and where each chunk contains a vector of devices, where some values seem hardcoded/computed by hand:

One thing to note is that the canister doesn’t really make a distinction between “recovery” and “authentication” devices (it looks like the purpose field is only read by the frontend code). This means that the simplest way to implement the feature is to add some data to DeviceDataInternal (meaning to all devices, including recovery phrases, recovery devices, etc) which specifies whether the user should prove they have the private key before removing the device; this is quite a big change and we’d need to think hard. Also, whatever data we add there, we need to make sure the new storage object fits. Depending on the implementation we’ll also need a migration step (say if we go from storage Vec<DeviceDataInternal> to Vec<DeviceDataInternalWithExtraData>).

All of this to say that it’s not quite backwards compatible, and while in theory it is reversible, in practice it’ll be non-trivial and there’s some risk. Voilà!

4 Likes

I think we should do something simple and flexible, which does not impose on the user how to exactly organize their security. This is why I like the first option here summarized by @timo :

creating two levels of authentication methods, higher level and lower level, and you need to be logged in with a higher level method in order to remove (i.e. delete) a lower level method. But
a) using the higher level must be optional,
b) the user must be able to choose which method to put on the higher level (for some it is the phrase, for others a Yubikey, etc.)
c) it must be possible to put more than one method to the higher level if the user wants that.
This is just like an admin vs user account in an OS. You can create multiple accounts of either type.

People that want to have a recovery phrase that can only be changed by entering it, could achieve that by setting the recovery phrase as the only device on the higher level.

Additionally, this simple model maps well to the current II backend, which does not differentiate between device types.

2 Likes

For implementation however, I think DFINITY’s policy (at least for NNS and II) has been that external contributions are not accepted . I think this makes a lot of sense, because so far virtually no one has submitted patches as far as I know, and because a malicious contribution in either II or NNS (dapp) would be catastrophic for the whole ecosystem.

As for external contributions, I think we would all hope/expect DFINITY to do a thorough review of the submitted code. But if DFINITY wants to take this on I would prefer that. It just seemed like no progress was being made, so we thought we’d put some pressure on and do it ourselves if possible, subject to the rigorous requirements of DFINITY.

With all that in mind, I think it’s worth thinking about making the recovery devices “secured”. bjoern also gives a nice example of how e.g. a recovery phrase can be used on a Ledger device; in this case the user still has a recovery device from II’s point of view, but can use an actual seed phrase which is only used on the Ledger device, and not in the browser. My recommendation would then be to only make the recovery devices “secured”, and not the recovery phrases . I’m sure some people will disagree though, in particular dostro and lastmjs since they explicitly suggest making the recovery phrase secured (and not the device); I’d love to hear examples of workflows and situations where the FIDO device/hardware wallet is not suitable.

I really like the idea of making one or more recovery devices promoted in security, especially since the recovery phrase mechanisms used in II are definitely sub-par compared with generating a seed phrase on something like a ledger.

If the implementation time/complexity/generality makes sense, I think we should consider shifting the focus to promoting recovering devices instead of just the seed/recovery phrase. There is at least one issue that @paulyoung has brought up:

In hindsight this would make it impossible to remove devices added by an attacker.

The difference with the recovery phrase is that there’s only one.

So perhaps you can only promote one recovery device at a time to this extra level of security.

Hopefully today during the working group meeting we can discuss all of this.

1 Like

One of our team members accidentally submitted a pr (git automatically creates one when you fork, I guess) with the following change to the remove function, which first checks the device’s purpose is recovery and key type is seedphrase, no change to stable memory… is there a reason this couldn’t be implemented even though it might currently only be read by the frontend code?

#[update]
async fn remove(user_number: UserNumber, device_key: DeviceKey) {
    ensure_salt_set().await;
    STATE.with(|s| {
        prune_expired_signatures(&s.asset_hashes.borrow(), &mut s.sigs.borrow_mut());

        let mut entries = s.storage.borrow().read(user_number).unwrap_or_else(|err| {
            trap(&format!(
                "failed to read device data of user {}: {}",
                user_number, err
            ))
        });

        trap_if_not_authenticated(entries.iter().map(|e| &e.pubkey));

        if let Some(i) = entries.iter().position(|e| e.pubkey == device_key) {
            //find required public key
            let entry_to_remove = entries.get(i as usize).unwrap();

            //verify that required pk is a recovery device
            if entry_to_remove.purpose.as_ref().unwrap() == &Purpose::Recovery
                // and specifically a seed phrase
                && entry_to_remove.key_type.as_ref().unwrap() == &KeyType::SeedPhrase {

                //verify that caller signature from recovery phrase
                if ic_cdk::api::caller() != Principal::self_authenticating(entry_to_remove.pubkey.clone()) {
                    trap("Please use public key from recovery phrase to remove recovery");
                }

            }
            entries.swap_remove(i as usize);
        }

        s.storage
            .borrow()
            .write(user_number, entries)
            .unwrap_or_else(|err| {
                trap(&format!(
                    "failed to persist device data of user {}: {}",
                    user_number, err
                ))
            });
    })
}
3 Likes

I didn’t check the code thoroughly, but doesn’t this mean this will be enabled for every (existing) recovery phrases? i.e. it’s not optional at all

1 Like

Wouldn’t it be pretty simple to add some state to DeviceData to keep track of opt-ins? To keep it simple we could just add a boolean flag to DeviceData like remove_recovery_phrase_confirmation or remove_recovery_device_confirmation, and add the methods to update that state.

2 Likes

Yes this isn’t complete, just an example of how it could be done. Of course we’ll need to add the opt-in logic somewhere.

1 Like

Writing the code to add some state would definitely be simple; most likely just adding a field; but see what I wrote here about the work and checks that need to be done as well:

Guys what do you think about such option?


3 Likes

That’s a smart workaround! I’m not entirely sure what the original intent of KeyType was TBH.

2 Likes

Ok IC team, it seems we have a contender with @Oleksii 's proposed fix!

  • if user has no seed phrase, they can create a seed_phrase or seed_phrase_protected (the UI checkbox will dictate which)
  • if user creates a seed_phrase, any device with anchor access can remove it (this is existing behavior and will be backwards-and-forwards compatible with Origyn FYI @skilesare)
  • if user creates a seed_phrase_protected, any device with anchor access can attempt to remove it, but will be forced to re-enter the phrase to confirm the removal
  • if user has a seed_phrase now and wants to change it to seed_phrase_protected, they will have to remove the seed_phrase and create a new seed_phrase_protected
  • if user has a seed_phrase_protected and wants to change it to seed_phrase, they will have to click the X to remove it, re-submit the phrase to confirm removal, and create a new seed_phrase
  • pending @nmattia 's and community review, we can submit this PR and start working on the frontend (though @lastmjs said he might get to it before us)
9 Likes

This is wonderful, very elegant.

3 Likes