Add security foundations section #487

coroiu · 2024-11-25T16:40:48Z

🎟️ Tracking

📔 Objective

This ports over an internal document while making some adjustments to the principles. Requirements will initially be added as-is

⏰ 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 · 2024-11-25T16:43:18Z

Checkmarx One – Scan Summary & Details – 9b8f11ea-7915-483a-ab7a-b1032faaa660

New Issues (2)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CVE-2024-52798	Npm-path-to-regexp-0.1.10	Vulnerable Package
	CVE-2024-55565	Npm-nanoid-3.3.7	Vulnerable Package

cloudflare-workers-and-pages · 2024-11-26T10:34:00Z

Deploying contributing-docs with Cloudflare Pages

Latest commit:	`43675f6`
Status:	✅ Deploy successful!
Preview URL:	https://aba6f503.contributing-docs.pages.dev
Branch Preview URL:	https://ps-add-fundamental-reqs.contributing-docs.pages.dev

View logs

coroiu · 2024-11-26T10:42:07Z

docs/security/requirements.mdx

+The requirements in this section are organized hierarchically, with top-level requirements defining
+the core rules and obligations that must be met, serving as broad objectives for Bitwarden's
+security model. Sub-requirements expand on these by addressing specific scenarios, exceptions, or
+clarifications, and may override their parent requirement when explicitly stated. Sibling
+requirements at the same level must remain independent and free of contradictions.


thought: Might be nice to align this to an industry standard like ISO/IEC/IEEE 29148, but there's only so much we can do in a markdown document

coroiu · 2024-11-26T10:43:11Z

docs/security/requirements.mdx

+2. Vault data **MAY** be unprotected while _in use_.
+
+   - a. The Client **MAY** decrypt all vault data during vault unlock.
+     - i. The Client **SHOULD** minimize the quantity of decrypted vault data.


thought: we could make this more stringent

Suggested change

- i. The Client **SHOULD** minimize the quantity of decrypted vault data.

- i. The Client **SHOULD** limit decrypted data to the records actively in use by the user.

coroiu · 2024-11-26T13:34:27Z

docs/security/principles/03-no-security-on-fully-compromised.mdx

@@ -0,0 +1,21 @@
+# P03 - No security on fully compromised systems


point-of-interest: this was previously called No guarantees for unlocked vaults on fully compromised devices. I changed it to make it more consistent with the other principles, but "no security" might seems a bit harsh

Related to the confluence point; I think we might need to add "no security guarantees on fully compromised bitwarden contexts"

withinfocus · 2024-11-27T14:11:12Z

Thoughts on dropping the acronyms and typed declarations? "P01" and "EK" come off rather strongly to me. Same for the "MUST" and "MUST NOT" all over the place that could probably just be expressed without capitalization and boldface.

coroiu · 2024-11-27T15:00:11Z

@withinfocus for P01 I thought about doing something similar to ADRs. I also wanted to be able to connect requirements to principles somehow later down the line in which I think the numbering could be useful for.

The boldface might be distracting you're probably right, but I'm not sure about removing the capitalization. Here I'm drawing inspiration from W3C specifications such as WebAuthn L3 where those terms are always captialized

withinfocus · 2024-11-27T15:13:45Z

I get where you're coming from with the historical references, but I see those as specifications with a large scope and impact. This is important to the company as principles, but I wouldn't treat it quite so intensely such as to model it vs. writing natural language.

quexten · 2024-11-28T14:35:56Z

Imo:

I prefer the RFC-like MUST NOT, and SHOULD NOT, etc. because it makes it clear that we are unambiguous and very deliberate about whatever meaning we encode with them, because they have a clear definition (from rfc2119).
Natural text, even if it's the same words, does not do that. In natural text, "must not" does not necessarily mean an absolute prohibition, and could just be a strong recommendation.

So, if we want to treat this as a set of requirements, that we want to design our applications by, then the RFC-style terms helps make these requirements unambiguous and clear.
(If we are just stating principles as recommendations/guidelines, without a strict requirement, then it does not matter.)

coroiu · 2024-11-28T16:21:03Z

My vision was that these requirements would be what we base all of our security design on, and not be "mere guidelines". If that will happen or not is probably beyond the scope of this PR. :) That said, the primary reason that motivated me to create this documentation was to remove ambiguousness surrounding security design and make it very clear for teams that have to design solutions for our products. Given that I am hesitant to make any changes that could potentially decrease clarity and introduce ambiguousness

eliykat

Nice work! A few comments from an AC perspective.

docs/security/definitions.mdx

docs/security/principles/01-locked-vault-is-secure.mdx

docs/security/requirements.mdx

eliykat · 2024-12-03T01:54:05Z

docs/security/requirements.mdx

+2. The UserKey **MUST** be protected _at rest_.
+3. The UserKey **MAY** be unprotected while _in use_.
+4. The UserKey **MUST** be protected while _in transit_.
+5. The UserKey **MUST NOT** be shared.


What about account recovery and emergency access?

Suggested change

5. The UserKey **MUST NOT** be shared.

5. The UserKey **MUST NOT** be shared without _informed and explicit consent_ by the User.

You're actually the second person to give me this exact comment so there is definitely room for improvement here. Account recovery and emergency access is not "sharing" according to the definition:

Data sharing
The controlled process in which data leaves the Bitwarden secure environment unprotected. As a consequence the guarantees made by this document will no longer apply. The receiving party may or may not have its own guarantees.

Interesting. Given that "sharing" has a common use, maybe there is a more specific term that can be used here? (Maybe "export" - or would that be confused with a vault export?)

Alternatively, it's only used a couple of times - maybe you remove the definition and be explicit each time: "The UserKey MUST NOT leave the Bitwarden secure environment in an unprotected state."

As a separate question - does this document have anything to say about when data is exchanged/accessed between users then? EA and Account Recovery are within the Bitwarden secure environment, but it has security implications.

Both very good suggestions. I think we might need the definition just to make the document easier to read, but you are right that it might be mixed up with vault export. Still think its worth it though, I'll update it when I get a chance!

does this document have anything to say about when data is exchanged/accessed between users then? EA and Account Recovery are within the Bitwarden secure environment, but it has security implications.

Not yet. I think we should redefine this to be "Sharing" and then start writing some principles and requirements on it

Renamed sharing -> exporting. Added a new definition for sharing that matches EA

eliykat · 2024-12-03T02:04:49Z

docs/security/principles/01-locked-vault-is-secure.mdx

+Clients must ensure that once the vault has been locked, no vault data can be accessed in plain
+text, even if the device becomes compromised after the lock occurs. Protections are not guaranteed


What about vault data that is intentionally stored in plain text? e.g. modification dates, folder guids, boolean values. Or is this excluded by the definition of "vault data"? (is metadata "sensitive and private information"?)

Yes, there is actually still an open thread in confluence about this because I was hoping to get Vault involved in classifying data a bit better. My idea is to split up the vault data into groups of varying sensitivity. In that case we might get:

High: Password

Moderate: Username

Low: Creation date

And then say something like "low sensitivity data" may be stored unprotected

I think that would work!

I tweak this a little bit by only grouping the data into highly sensitive and less sensitive (see 528330f) to avoid getting into hard discussions. What do you think?

docs/security/principles/01-locked-vault-is-secure.mdx

docs/security/requirements.mdx

trmartin4 · 2024-12-06T22:14:40Z

docs/security/requirements.mdx

+
+1. The authentication tokens MUST be protected at rest if the client provides a mechanism for secure
+   storage.
+


Suggested change

2. The authentication tokens **SHOULD** be protected in transit.

Added! But why SHOULD and not MUST?

🤔 I can't recall what I had in mind when suggesting SHOULD. I believe that it should be MUST, as you said. I've suggested that as another change.

… now called exporting

trmartin4

Just one suggestion.

trmartin4 · 2025-01-22T14:09:21Z

docs/security/requirements.mdx

+
+1. The authentication tokens **MUST** be protected at rest if the client provides a mechanism for
+   secure storage.
+2. The authentication tokens **SHOULD** be protected in transit.


Suggested change

2. The authentication tokens **SHOULD** be protected in transit.

2. The authentication tokens **MUST** be protected in transit.

patrick-bitwarden

Handful of copyediting notes.

patrick-bitwarden · 2025-01-24T14:18:16Z

docs/security/principles/05-minimized-impact-of-security-breaches.mdx

+Even with robust security measures in place, user error or unforeseen vulnerabilities can lead to
+various security breaches, including the compromise of encryption keys or data leaks. Bitwarden
+should take available actions to help users limit the damage caused by such breaches, both in scope
+and duration. This involves:


Is this intended to be comprehensive? If not, we might say "includes" instead

Good point, fixed!

patrick-bitwarden · 2025-01-24T14:18:29Z

docs/security/definitions.mdx

+  <dt>Data at rest</dt>
+  <dd>
+    Any data that is stored on a device or medium that is not actively being used, processed, or
+    transmitted. This includes (but is not limited to) data stored on disk on the users devices, or on


patrick-bitwarden · 2025-01-24T14:18:31Z

docs/security/definitions.mdx

+    party or parties may not have done so, making the channel trusted by one party but untrusted by
+    the other(s).
+  </dd>
+  <dt>Fully trusted Channel</dt>


"Channel" is capitalized here and not in other headings

patrick-bitwarden · 2025-01-24T14:18:35Z

docs/security/definitions.mdx

+
+    - **Confidentiality**: The data is unreadable to unauthorized
+    parties, typically using encryption.
+    - **Integrity**: The data cannot be altered or tampered with


Consider reversing the order of "altered or tampered with" to "tampered with or altered" to avoid the sequence "with without"

patrick-bitwarden · 2025-01-24T14:18:40Z

docs/security/definitions.mdx

+    Data stored in a format that is unreadable without additional information. Usually synonymous to
+    encrypted, but with additional expectations about how the key is stored. Encrypted data which is
+    stored together with its decryption key in plain text is not considered to be protected, even
+    thought it is encrypted.


"thought" should be "though"

Hinton · 2025-01-24T15:20:03Z

@coroiu Could you move this to be under Architecture? This will give it codeowners and possibly a better home.

coroiu · 2025-01-27T09:09:10Z

@Hinton I've moved it under a Architecture, though it doesn't really feel like an architectural thing? Either way, I think it's worth it to at least give owners. For the future I wonder if we could move it out again and place it under a new working group's ownership, I'll float that idea in other channels and see where we end up :)

Hinton · 2025-01-27T14:08:11Z

I know, but this at least gives it a home while we figure out exactly where it should live and whom should own it.

withinfocus

Previously reviewed and I support the other approvals here so far. I think this is a good initial state to land.

coroiu added 16 commits November 22, 2024 16:34

feat: add new security section

648221f

feat: port definitions

585c402

feat: add first principle

bf8369d

feat: add second principle

e65f223

feat: add third principle

cebbeb3

feat: add note about TEEs to P03

40a8bb7

chore: change protection > security

6e650d7

feat: add forth principle

bcd9467

feat: add fifth principle

8da13ec

feat: add fifth principle

4c755bb

feat: add an overview page

e958797

fix: remove repetition

5d8a121

chore: rename file to match content

3247efe

refactor: use hyphen in title

f9e585d

fix: links

425f808

refactor: remove any after comment

e4f44ea

coroiu added 2 commits November 26, 2024 11:32

feat: add current version of requirements

b4a8c8d

feat: add introduction to requirement structure

0222f3f

coroiu requested a review from MGibson1 November 26, 2024 10:35

coroiu marked this pull request as ready for review November 26, 2024 10:35

coroiu requested a review from a team as a code owner November 26, 2024 10:35

coroiu commented Nov 26, 2024

View reviewed changes

eliykat reviewed Dec 3, 2024

View reviewed changes

docs/security/principles/01-locked-vault-is-secure.mdx Outdated Show resolved Hide resolved

trmartin4 reviewed Dec 6, 2024

View reviewed changes

docs/security/requirements.mdx Outdated Show resolved Hide resolved

trmartin4 reviewed Dec 6, 2024

View reviewed changes

coroiu added 9 commits December 10, 2024 14:54

feat: add Client definition

663b464

fix: tweak user definition

aaca302

feat: add technical consideration section to P01

0373e12

fix: clarify which data locking protects

528330f

fix: typo

d5548f7

fix: change linger to not present in memory

563e3a7

fix: re-define sharing to match EA, previous definition of sharing is…

83f0d56

… now called exporting

fix: font-weight

8eceee3

feat: add token protected in transit req

952a43c

coroiu requested review from eliykat and trmartin4 January 22, 2025 08:44

trmartin4 previously approved these changes Jan 22, 2025

View reviewed changes

feat: change AT.2 to must

7de826f

coroiu dismissed trmartin4’s stale review via 7de826f January 22, 2025 14:35

coroiu requested a review from trmartin4 January 22, 2025 14:42

trmartin4 previously approved these changes Jan 23, 2025

View reviewed changes

patrick-bitwarden requested changes Jan 24, 2025

View reviewed changes

coroiu added 2 commits January 27, 2025 10:02

chore: move security under architecture

43c1e0e

fix: various copy feedback

43675f6

coroiu dismissed trmartin4’s stale review via 43675f6 January 27, 2025 09:06

coroiu requested a review from a team as a code owner January 27, 2025 09:06

coroiu requested a review from patrick-bitwarden January 27, 2025 09:09

withinfocus approved these changes Jan 27, 2025

View reviewed changes

	- i. The Client SHOULD minimize the quantity of decrypted vault data.
	- i. The Client SHOULD limit decrypted data to the records actively in use by the user.

		@@ -0,0 +1,21 @@
		# P03 - No security on fully compromised systems

	5. The UserKey MUST NOT be shared.
	5. The UserKey MUST NOT be shared without _informed and explicit consent_ by the User.

		Clients must ensure that once the vault has been locked, no vault data can be accessed in plain
		text, even if the device becomes compromised after the lock occurs. Protections are not guaranteed


		1. The authentication tokens MUST be protected at rest if the client provides a mechanism for secure
		storage.


	2. The authentication tokens SHOULD be protected in transit.

	2. The authentication tokens SHOULD be protected in transit.
	2. The authentication tokens MUST be protected in transit.

Add security foundations section #487

Are you sure you want to change the base?

Add security foundations section #487

Conversation

coroiu commented Nov 25, 2024

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Nov 25, 2024 • edited Loading

cloudflare-workers-and-pages bot commented Nov 26, 2024 • edited Loading

Deploying contributing-docs with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quexten Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

withinfocus commented Nov 27, 2024

coroiu commented Nov 27, 2024

withinfocus commented Nov 27, 2024

quexten commented Nov 28, 2024 • edited Loading

coroiu commented Nov 28, 2024

eliykat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coroiu Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coroiu Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trmartin4 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick-bitwarden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hinton commented Jan 24, 2025

coroiu commented Jan 27, 2025

Hinton commented Jan 27, 2025

withinfocus left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 25, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 26, 2024 •

edited

Loading

quexten Dec 3, 2024 •

edited

Loading

quexten commented Nov 28, 2024 •

edited

Loading

coroiu Dec 4, 2024 •

edited

Loading

coroiu Dec 10, 2024 •

edited

Loading

trmartin4 left a comment •

edited

Loading