Skip to content

Add end-to-end encryption as a principle #535

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

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

MGibson1
Copy link
Member

📔 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

@MGibson1 MGibson1 requested a review from a team as a code owner January 29, 2025 21:37
Copy link

github-actions bot commented Jan 29, 2025

Logo
Checkmarx One – Scan Summary & Details5cb00202-917b-4706-9c97-fb2bc0efaa93

New Issues (7)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
HIGH Cx7c1ed3d7-0e49 Npm-image-size-1.2.0
detailsRecommended version: 1.2.1
Description: Image-size is vulnerable to a Denial of Service (DoS) vulnerability when processing specially crafted images. The issue occurs because of an infine...
Attack Vector: NETWORK
Attack Complexity: LOW

ID: 4esnMk817IhIjayXfM8VWjN75INj5zJ08Wccvd9f%2FYg%3D
Vulnerable Package
MEDIUM CVE-2024-53382 Npm-prismjs-1.29.0
detailsRecommended version: 1.30.0
Description: Prism (aka PrismJS) allows DOM Clobbering (with resultant XSS for untrusted input that contains HTML but does not directly contain JavaScript), bec...
Attack Vector: NETWORK
Attack Complexity: HIGH

ID: q8FwnBmTsWdryAh9%2BWLemy96eXMtGWqjeF0WlJwxBXg%3D
Vulnerable Package
MEDIUM CVE-2025-27789 Npm-@babel/runtime-corejs3-7.26.0
detailsRecommended version: 7.26.10
Description: Babel is a compiler for writing next-generation JavaScript. In affected versions of Babel, to compile regular expressions named capturing groups, B...
Attack Vector: LOCAL
Attack Complexity: LOW

ID: 45%2FsGPJlRlRj2j52pxTiDMlvqwDRMq6DHnV%2FjkTS8UY%3D
Vulnerable Package
MEDIUM CVE-2025-27789 Npm-@babel/helpers-7.26.0
detailsRecommended version: 7.26.10
Description: Babel is a compiler for writing next-generation JavaScript. In affected versions of Babel, to compile regular expressions named capturing groups, B...
Attack Vector: LOCAL
Attack Complexity: LOW

ID: %2BMUee7d1XCI17mQLHDQqY1eeYGFoSAP5uZG9kmRY4Mk%3D
Vulnerable Package
MEDIUM CVE-2025-32014 Npm-estree-util-value-to-estree-3.2.1
detailsRecommended version: 3.3.3
Description: A vulnerability in estree-util-value-to-estree versions prior to 3.3.3 allows an attacker to generate an "ESTree" object that specifies a prototype...
Attack Vector: NETWORK
Attack Complexity: LOW

ID: 3Yvk6L9EhROv%2FMh4hD9NpsAc0qL27wjNlEEoPtVMgxs%3D
Vulnerable Package
MEDIUM CVE-2025-32996 Npm-http-proxy-middleware-2.0.7
detailsRecommended version: 2.0.9
Description: In http-proxy-middleware v1.3.0 through v2.0.7 and v3.x through v3.0.3, "writeBody" function can be called twice because "else if" is not used.
Attack Vector: NETWORK
Attack Complexity: HIGH

ID: B3dLmCftctEoWhkti4Kicr1I5Uo27lEKoiw4CvnPvuw%3D
Vulnerable Package
MEDIUM CVE-2025-32997 Npm-http-proxy-middleware-2.0.7
detailsDescription: In http-proxy-middleware versions 1.3.0 through 2.0.8 and 3.x through 3.0.4, the "fixRequestBody" function proceeds even if "bodyParser" has failed.
Attack Vector: NETWORK
Attack Complexity: HIGH

ID: %2FqC3slRqWGebmg8i6AYYtuTn1ylw09VcZdt4p3U197k%3D
Vulnerable Package

Copy link
Contributor

@coroiu coroiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment on lines 26 to 29
breaking this principle. It is possible for a Bitwarden server to create an authentication token,
contact the Key Connector server, and retrieve key material that will allow decryption of a user's
vault. For these reasons we encourage strict isolation of key connector servers to private networks
and only to be used by advanced self-hosted users.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Base automatically changed from ps/add-fundamental-reqs to main January 31, 2025 16:26
Copy link

cloudflare-workers-and-pages bot commented Feb 4, 2025

Deploying contributing-docs with  Cloudflare Pages  Cloudflare Pages

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

View logs

@MGibson1 MGibson1 force-pushed the ps/include-end-to-end-encryption-in-security-principles branch from 729d737 to 599521c Compare February 6, 2025 19:40
@MGibson1 MGibson1 requested review from coroiu and quexten February 6, 2025 20:04
Copy link
Contributor

@coroiu coroiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor new comments and answers to previous ones

Copy link
Contributor

@coroiu coroiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nits and some smaller suggestions, otherwise I think we can rebase/merge main into this PR and then go ahead and merge

data is fully isolated from the server and infrastructure. It cannot be read nor expanded outside of
the user's client context.

This is what we mean when we sometimes refer to "End-to-end encrypted" or "Zero Knowledge."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
This is what we mean when we sometimes refer to "End-to-end encrypted" or "Zero Knowledge."
This is what we mean when we sometimes refer to "End-to-end encrypted" or "Zero Knowledge".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL. It looks like this is a difference between American and British editing convention (https://style.mla.org/the-placement-of-a-comma-or-period-after-a-quotation/)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants