[PM-32439] Consolidate safe module cryptographic namespacing strategy - #770
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #770 +/- ##
==========================================
+ Coverage 81.61% 81.68% +0.07%
==========================================
Files 340 340
Lines 39785 40073 +288
==========================================
+ Hits 32469 32734 +265
- Misses 7316 7339 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
96c814a to
9f988f2
Compare
quexten
changed the title
quexten
changed the title
quexten
changed the title
a267c1d to
db20456
Compare
dani-garcia
left a comment
dani-garcia
left a comment
There was a problem hiding this comment.
LGTM, just two small improvements that we may be able to make
Thomas-Avery
left a comment
Thomas-Avery
left a comment
There was a problem hiding this comment.
Looks good, a few minor things to look at from me.
| expected_content_namespace: T, | ||
| ) -> Result<(), ExtractionError> { | ||
| let obj = extract_safe_object_namespace(header); | ||
| println!("Object namespace extraction result: {obj:?}"); |
There was a problem hiding this comment.
Should this be trace debug or just removed?
There was a problem hiding this comment.
Oops, yeah we do not want this. It's left over from debugging
There was a problem hiding this comment.
This reminded me that there's a lint to disable println!, so I've made a separate PR to enable it: #795
There was a problem hiding this comment.
Nice job on these helpers, it helped 😉 me wrap my head around this.
There was a problem hiding this comment.
Nice. It usually needs a few revisions for new API design to find a good abstraction, I think this is validation that this is the right direction.
| // Note: This block ensures ctx is dropped. Otherwise it would cause a deadlock when | ||
| // initializing the user crypto |
There was a problem hiding this comment.
Should this comment be moved to be over the ctx again?
There was a problem hiding this comment.
Moved to outside of the block actually, but yeah, it definitely was not in the right place.
| pub(crate) const SAFE_OBJECT_NAMESPACE: i64 = -80002; | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum SafeObjectNamespace { |
There was a problem hiding this comment.
💭 I would think both SafeObjectNamespace and ContentNamespace would be pub(crate)? Maybe I'm missing something.
There was a problem hiding this comment.
You are right. Changed!
Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
…pe.rs Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
|
Had to resolve some conflicts after the improved logging PR merged. We added the namespaces to the logging of the dataenvelope and passwordprotectedkeyenvelope structs now, so you can easily see them while debugging. |
|
…idate safe module cryptographic namespacing strategy (bitwarden/sdk-internal#770)
## 🎟️ Tracking https://bitwarden.atlassian.net/browse/PM-32729 ## 📔 Objective This comment #770 (comment) reminded me that there's a lint we can enable against println! usage. Our libraries should use `tracing` instead. We allow them to be used in tests, same as unwrap, and the rest of the use cases (CLI, examples) should just allow it as needed. Note, I've set this new lint to warn (and moved unused_async to warn too) as it's something that a dev might quickly try in some code while developing, and setting it to deny is just annoying during development. CI treats all warnings as errors anyway, so it does't have any negative effects. ## 🚨 Breaking Changes <!-- Does this PR introduce any breaking changes? If so, please describe the impact and migration path for clients. If you're unsure, the automated TypeScript compatibility check will run when you open/update this PR and provide feedback. For breaking changes: 1. Describe what changed in the client interface 2. Explain why the change was necessary 3. Provide migration steps for client developers 4. Link to any paired client PRs if needed Otherwise, you can remove this section. -->
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-32439
📔 Objective
To solve a class of cryptographic issues plaguing our e2ee layer, we need cryptographic namespaces. We already had an initial stab at this for signatures and for data envelopes. To make this approach scalable, we need a more systematic approach. Specifically, the problem to solve here is that encrypted/signed objects, encrypted/signed under the same key can be swapped by the server, causing the client to mis-interpret these, leading to security bugs. Namespaces solve this by creating a cryptographic partitioning (by adding a namespace number to the protected headers, in the signed data / authenticated additional data).
This PR introduces this systematic approach for these. Signatures have their own namespace pratitioning, since they are a lower-level primitive.
Safe objects have two layers: the object layer and the content layer.
On the object layer we partition the cryptographic primitives (Encrypt0, Sign1) into safe primitives (DataEnvelope, PasswordProtectedKeyEnvelope, etc).
On the content layer, we partition each of the above subsets further, by use-case. For example, a valid partitioning for PasswordProtectedKeyEnvelope would be {MasterPassword, Pin}.
This prevents a class of attacks where the server switches cryptographic objects, signed/encrypted under the same key. This eliminates any kind of cryptographic analysis that would be needed to prevent the above attacks / limits security analysis to each sub-namespace spanned by the combination of the object and content namespace layer.
🚨 Breaking Changes