[PM-27115] Force icon uri checksum verification on user crypto v2 #519
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
quexten
changed the title
quexten
changed the title
|
Great job! No new security vulnerabilities introduced in this pull request |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #519 +/- ##
==========================================
+ Coverage 78.36% 78.37% +0.01%
==========================================
Files 291 291
Lines 29343 29366 +23
==========================================
+ Hits 22994 23017 +23
Misses 6349 6349 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // We use an Arc<> to make it easier to pass this store around, as we can | ||
| // clone it instead of passing references | ||
| inner: Arc<RwLock<KeyStoreInner<Ids>>>, | ||
| security_state_version: Arc<RwLock<u64>>, |
There was a problem hiding this comment.
Should this instead be a part of KeyStoreInner to reuse the existing lock?
Having two locks seems wasteful and I feel like could lead to accidental deadlocks. In fact the current code in context_mut() smells like it could cause a deadlock to me, as we get a write lock for the global_keys and then we try to read the security state while holding the lock. If any other part of the codebase read those two in the opposite order, we'd have issues.
There was a problem hiding this comment.
Should be fixed now
…/sdk-internal into km/icon-url-checksum-crypto-v2
| pub fn context(&'_ self) -> KeyStoreContext<'_, Ids> { | ||
| let data = self.inner.read().expect("RwLock is poisoned"); | ||
| KeyStoreContext { | ||
| global_keys: GlobalKeys::ReadOnly(self.inner.read().expect("RwLock is poisoned")), |
There was a problem hiding this comment.
Nitpick: You can probably restructure this in the same way of context_mut to avoid calling inner.read() twice. it won't ever deadlock so this isn't a blocking problem, but it's mildly wasteful to do it twice if not needed.
| /// Sets the security state version for this store. | ||
| pub fn set_security_state_version(&self, version: u64) { | ||
| let mut data = self.inner.write().expect("RwLock is poisoned"); | ||
| data.security_state_version = version; |
There was a problem hiding this comment.
Non-blocking question: Would we want to add some rollback protection here in the future? Something like:
if version < data.security_state_version {
return Err(...);
}There was a problem hiding this comment.
Yes! However, I do think this should be considered when sync and at least the crypto state is managed in SDK.
quexten
requested review from
mzieniukbw
and removed request for
addisonbeck and
shane-melton
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27115
https://bitwarden.atlassian.net/browse/VULN-185
https://bitwarden.atlassian.net/browse/PM-4185
📔 Objective
Forces the icon URI check for crypto v2 users. This adds some plumbing through the key store, which now also includes other non-key cryptographic state. The account security version is expected to be used in many other places that have access to the key store and we do not want to pass through an additional struct to all places.
Please see the tickets for context about what specifically this change achieves.
Further, this fixes the icon uri check to be constant time.
⏰ Reminders before review
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 confirmedissue 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