Add end-to-end encryption as a principle

[MGibson1]

📔 Objective

Adds a definition of what we mean by end-to-end encryption as a security principle.

I moved this to principle 01 simply because I feel its the foundational principle Bitwarden was built on and is necessary to vette Bitwarden when considering it for use.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
  team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
  issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[github-actions]

Checkmarx One – Scan Summary & Details – a30d65b1-0901-488d-a54c-9d3f970384e9

Great job, no security vulnerabilities found in this Pull Request

[coroiu]

This is a great addition! If there is ever any doubt this will plainly explain what we mean, which is one of the main goals of this document!

[coroiu]

suggestion: do you think "in supplying data" might be redundant? Might also be unnecessarily restrictive

Suggested change

	Clients do not need to trust Bitwarden server or web infrastructure to be good actors in supplying
	data to keep an account's encrypted content secure. In other words, the special status of providing
	Clients do not need to trust Bitwarden server or web infrastructure to be good actors to keep an account's encrypted content secure. In other words, the special status of providing

[quexten]

Would this include serving a malicious web vault then? Is that a guarntee we want to / can give?

[MGibson1]

Ideally, it definitely would be able to guarantee web vault code. I don't know if we'd ever achieve that goal, but not knowing how to do it right now doesn't mean it's not inline with a principle we have.

@coroiu I agree that's better both because it removes weird caveats and is clearer in intent

[coroiu]

question: What do you think about the server being able to capture a protected version of the encryption key? Could that technically be seen as "aiding in decryption" as compared to not having that key at all? E.g. it might be easier to attack the UserKey by guessing the MP than it would be to attack the vault data directly by guessing the UserKey. This would not be possible if you were not in a position to access the UserKey.

On the other hand, you still need to have gotten access to the vault data and if you are able to do that then you most likely get the key while you're at it 🤷‍♂️

[quexten]

I don't know what matt intended, but from my perspective, attacks that force a client to use a server-known key, or leak (parts of) the key directly, then that is covered. If all the server gets, is the ability to brute-force the password, using the user's intended KDF settings, then that is not covered.

[MGibson1]

quite difficult to define what I'm going for, but generally I think @quexten and I agree: there are going to be features that allow you to protect a key in some way. Using those feature is signing up for the protection those feature provide. The server should not be able to decrease that level of protection. Having a password to unlock your vault is weaker than something like a passkey, but if the server could manipulate you into weakening your password key, that weakens the security of the feature and would be an acknowledged vulnerability we would fix.

That doesn't make passwords an anti-feature, it just means there's a feature with less security than another one.

Similarly. If the server were able to force the client to use a known key by any means, that weakens the security provided by the feature that was supposed to protect that user key.

[coroiu]

That's a clever way of looking at it! How about:

Clients do not need to trust Bitwarden’s server or infrastructure to act as good actors in order to keep an account’s encrypted content secure. The special status of providing a network sync for client data does not grant the server, or any intermediary between the server and client, the ability to reduce the effective security of the protections that guard a user’s vault. If a user chooses a weaker form of vault protection (e.g., a password instead of a passkey), that is an intentional user decision, but the server must not be able to manipulate or coerce a client into reducing security beyond what the user knowingly configures.

[MGibson1]

I tweaked your suggestion a little to be less password manager centric

[coroiu]

suggestion: infrastructure is a bit vague here and has not been defined earlier in the text, I think we can just remove it

[MGibson1]

If we do, I think we need a term to mean anything not the local client. We discuss this a bit above that the goal isn't to not trust the server repo code, but anything the client communicates to while pulling cloud-stored data.

I'm not yet sure how to say that, but it feels like a sticking point we need to handle. Maybe something as simple as cloud or web infrastructure?

[coroiu]

I suggested adding Clients do not need to trust Bitwarden’s server or infrastructure further up which I think would resolve this comment. I mostly reacted to infrastructure being mentioned for the first time in the middle of the text

[coroiu]

suggestion: consider using the existing phrase "Informed and explicit consent" instead of "declared to the user"

[MGibson1]

Nice. See ec84aafd9

[coroiu]

nit: mini adjustments

Suggested change

	Key connector is a self-host test only feature that allows an Organization to log in and unlock with
	SSO and no password input. This feature is specifically limited to self-hosted instances due to
	Key connector is a self-host test only feature that allows an organization user to log in and unlock with
	SSO and no password input. This feature is specifically limited to self-hosted instances due to

[coroiu]

question/suggestion: should we consider a more structured approach to this list? Something like:

### Key connector
Key connector is a self-host test only feature that allows an Organization to log in and unlock with
SSO and no password input. We encourage strict isolation of key connector servers to private networks and only to be used by advanced self-hosted users.

#### Scope
This feature is limited to self-hosted instances and requires the administrator to set-up additional services and explicitly enable the feature.

#### Risk
Bitwarden servers can create an authentication token, contact the Key Connector server, and retrieve key material that will allow decryption of a user's vault.

[MGibson1]

It was brought up outside of github to think about moving these exceptions somewhere else to allow them room to grow into a more defined format. I think I like that, but hesitated to do it here.

I'm not opposed to trying a bit of structure, but I think the top blurb would always be duplicated by the sections here because I don't want to bury the lede of scope and risk

[coroiu]

suggestion: "enable" might be confused with "activating the feature"

Suggested change

	Bitwarden clients. To enable this functionality, clients do send clear text domain name information
	Bitwarden clients. To implement this functionality, clients need to send clear text domain name information

[MGibson1]

See 80334b5

[coroiu]

suggestion: The "Exceptions" section mentions "tightly limited scope" but the "Icons service" does not explicitly say how the icon service is limited. It might be implicitly understood that this is only used for icons, but I think we should write that out!

[MGibson1]

See 80334b5

[coroiu]

And one more general question/suggestion, what would you think about moving this down one step to P02? In my head this could be seen as an extension to P01 - Locked vault is secure if we define that a locked vault is what is sent to the server:

Clients must ensure that highly sensitive vault data cannot be accessed in plain text once the vault has been locked

Or am I stretching it and it would be better to keep different use-cases separate?

[quexten]

I would highly recommend keeping the locally encrypted state separate from the cloud encrypted shared state TBH.

[coroiu]

Yeah you are probably right, disregard this :)

[MGibson1]

I'm not necessarily against moving, but I put this first because I feel it's the justification for why we exist, we make software that is end-to-end encrypted and we need to be clear about what that means.

[cloudflare-workers-and-pages]

Deploying contributing-docs with    Cloudflare Pages

Latest commit:	e2753a5
Status:	✅  Deploy successful!
Preview URL:	https://870f309b.contributing-docs.pages.dev
Branch Preview URL:	https://ps-include-end-to-end-encryp.contributing-docs.pages.dev

View logs

[quexten]

I would highly recommend keeping the locally encrypted state separate from the cloud encrypted shared state TBH.

[quexten]

Would this include serving a malicious web vault then? Is that a guarntee we want to / can give?

[quexten]

I do not think this goes far enough. I'm not sure how to word it, but I want to include not just the actual server, but anything that from the client's perspective is the server (i.e include malicious MITMs that *do not have database access).

[MGibson1]

That's what I was going for with "or web infrastructure," but perhaps being second implies bitwarden owned web infrastructure? that's not the intent and I'm open to changing to clarify.

[quexten]

I don't know what matt intended, but from my perspective, attacks that force a client to use a server-known key, or leak (parts of) the key directly, then that is covered. If all the server gets, is the ability to brute-force the password, using the user's intended KDF settings, then that is not covered.

[quexten]

I don't think this goes far enough. It should also include that the server cannot inject new data that the application presents as being the user's, decrypting successfully.

[MGibson1]

I'm not sure I agree here, yet. It's less that ideal, but not the same as weakening the security of the content.

For example, would duplication of a single vault item be injecting new data? that feels more like a corruption event than a weakening of security. Of course, there might be scarier attacks like duplicating URIs between items to try and take advantage of autofill behaviors.

Then there's just totally new data that was never in the vault. that seems clearly in scope, but I don't know how to differentiate from the above.

I'd like to add something here, but I don't have the phrasing for it, yet.

[quexten]

I agree that: injecting net new, chosen plaintext data (through e.g. fake orgs) should be covered here. That should never be possible to happen. Duplicating existing data is not an issue.

I don't know what you mean by:

Then there's just totally new data that was never in the vault. that seems clearly in scope, but I don't know how to differentiate from the above.

F.e: "It should not be possible to inject chosen data into the user's decrypted vault.".

[MGibson1]

that quote means

injecting net new, chosen plaintext data (through e.g. fake orgs) should be covered here

😅

I'll update to include chosen cleartext data protection. see e2753a5

[coroiu]

Some minor new comments and answers to previous ones

[coroiu]

suggestion(non-blocking): no need to repeat the name

Suggested change

	To enable this functionality, clients need to send clear text domain name information to the
	Bitwarden icons service. Communicated information is limited to vault item URIs matching some simple
	To enable this functionality, clients need to send clear text domain name information to the
	service. Communicated information is limited to vault item URIs matching some simple

[coroiu]

suggestion: clarify or remove "some simple structural requirement", sounds a bit vague in a place where we need to be precise to make us sound confident and trustworthy

[coroiu]

suggestion: use previously established terminology

Suggested change

	Bitwarden icons service. Communicated information is limited to vault item URIs matching some simple
	structural requirement. These URIs are encrypted in a vault, but we do this to aggregate access to
	Bitwarden icons service. Communicated information is limited to vault item URIs matching some simple
	structural requirement. These URIs are part of an account's encrypted content, but we do this to aggregate access to

[coroiu]

suggestion(non-blocking): the advantage of "aggregate access to the requested domain" is immediately obvious, we might want to clarify, or maybe just replace all the advantages without something more broad like "improve user experience"