Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add security foundations section #487
base: main
Are you sure you want to change the base?
Add security foundations section #487
Changes from 18 commits
648221f
585c402
bf8369d
e65f223
cebbeb3
40a8bb7
6e650d7
bcd9467
8da13ec
4c755bb
e958797
5d8a121
3247efe
f9e585d
425f808
e4f44ea
b4a8c8d
0222f3f
663b464
aaca302
0373e12
528330f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a User include an organization?
Who is the owner of vault data? For example - organizations often consider themselves the "owner" of data stored in an individual user's vault if it's a business account. But we generally mean "the User or Organization linked to the cipher in the database". This will increase as we give organizations more control over individual accounts (e.g. password reset, delete).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked this a little bit to avoid the question of ownership
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
And then say something like "low sensitivity data" may be stored unprotected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 harshThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to the confluence point; I think we might need to add "no security guarantees on fully compromised bitwarden contexts"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: we could make this more stringent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we mean by linger? This is all very precise but "linger" is quite vague.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I think
is not present in memory
is what I'm looking for here, but that makes this requirement hard to implement. Maybe we should change it to aSHOULD
at the same timeI still think this requirement will have a pretty large impact if adopted since it tends to discourage our huge caches, but having no caches is the current approach in the SDK so hopefully we won't have to prioritize any additional work just for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this; being more specific with the requirement while loosening the language a little (MUST -> SHOULD) seems like a good balance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about account recovery and emergency access?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
Not yet. I think we should redefine this to be "Sharing" and then start writing some principles and requirements on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to maintain consistency of font weight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.