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

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.

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

docs/security/definitions.mdx

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

docs/security/definitions.mdx

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

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

patrick-bitwarden approved these changes Jan 31, 2025

View reviewed changes

coroiu merged commit e559efe into main Jan 31, 2025
10 checks passed

coroiu deleted the ps/add-fundamental-reqs branch January 31, 2025 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add security foundations section #487

Add security foundations section #487

coroiu commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 •

edited

Loading

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

edited

Loading

coroiu Nov 26, 2024

coroiu Nov 26, 2024

coroiu Nov 26, 2024

quexten Dec 3, 2024 •

edited

Loading

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

trmartin4 Dec 6, 2024

coroiu Jan 22, 2025

trmartin4 Jan 22, 2025

trmartin4 left a comment •

edited

Loading

trmartin4 Jan 22, 2025

coroiu Jan 22, 2025

patrick-bitwarden left a comment

patrick-bitwarden Jan 24, 2025

Hinton commented Jan 24, 2025

coroiu commented Jan 27, 2025

Hinton commented Jan 27, 2025

withinfocus left a comment

	- 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


		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

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

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

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

trmartin4 left a comment •

edited

Loading