Skip to content
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

audit: ensure unsafe blocks are really safe [WPB-10887] #895

Merged
merged 7 commits into from
Jan 30, 2025

Conversation

coriolinus
Copy link
Contributor

What's new in this PR

  • audit all unsafe blocks in core-crypto
  • add deny rules for undocumented unsafe
  • adjust code as required to ensure safety of unsafe blocks
  • add documentation to each unsafe block about how our use of it is safe

PR Submission Checklist for internal contributors
  • The PR Title
    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@coriolinus coriolinus requested a review from a team as a code owner January 29, 2025 11:06
Copy link

github-actions bot commented Jan 29, 2025

🐰 Bencher Report

Branchprgn/doc/unsafe-blocks
Testbedubuntu-latest
Click to view all benchmark results
BenchmarkLatencymilliseconds (ms)
Commit add f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
18.69
Commit add f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
6.94
Commit add f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
9.60
Commit add f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
11.98
Commit add f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
14.89
Commit add f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
16.85
Commit add f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold
985.74
Commit add f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold
6.80
Commit add f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold
85.56
Commit add f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold
220.57
Commit add f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold
426.73
Commit add f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold
675.35
Commit pending proposals f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
117.91
Commit pending proposals f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
28.45
Commit pending proposals f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
46.12
Commit pending proposals f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
61.55
Commit pending proposals f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
80.08
Commit pending proposals f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
94.71
Commit pending proposals f(pending size)/cs1/mem/1📈 view plot
🚷 view threshold
19.11
Commit pending proposals f(pending size)/cs1/mem/101📈 view plot
🚷 view threshold
116.23
Commit pending proposals f(pending size)/cs1/mem/21📈 view plot
🚷 view threshold
35.86
Commit pending proposals f(pending size)/cs1/mem/41📈 view plot
🚷 view threshold
57.22
Commit pending proposals f(pending size)/cs1/mem/61📈 view plot
🚷 view threshold
75.94
Commit pending proposals f(pending size)/cs1/mem/81📈 view plot
🚷 view threshold
95.78
Commit remove f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
27.67
Commit remove f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
6.74
Commit remove f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
9.08
Commit remove f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
12.19
Commit remove f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
17.36
Commit remove f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
21.73
Commit remove f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold
31.39
Commit remove f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold
137.30
Commit remove f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold
115.21
Commit remove f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold
93.87
Commit remove f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold
72.47
Commit remove f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold
51.41
Commit update f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
137.59
Commit update f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
7.02
Commit update f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
33.71
Commit update f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
60.09
Commit update f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
86.80
Commit update f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
111.71
🐰 View full continuous benchmarking report in Bencher

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 63.96396% with 40 lines in your changes missing coverage. Please review.

Project coverage is 78.40%. Comparing base (26dde1d) to head (5a64fe4).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
keystore/src/entities/mls.rs 16.66% 5 Missing ⚠️
...eystore/src/entities/platform/generic/mls/group.rs 33.33% 4 Missing ⚠️
...re/src/entities/platform/generic/mls/credential.rs 40.00% 3 Missing ⚠️
...ntities/platform/generic/mls/encryption_keypair.rs 40.00% 3 Missing ⚠️
...s/platform/generic/mls/epoch_encryption_keypair.rs 40.00% 3 Missing ⚠️
.../entities/platform/generic/mls/hpke_private_key.rs 40.00% 3 Missing ⚠️
...re/src/entities/platform/generic/mls/keypackage.rs 40.00% 3 Missing ⚠️
...src/entities/platform/generic/mls/pending_group.rs 50.00% 3 Missing ⚠️
...c/entities/platform/generic/mls/pending_message.rs 40.00% 3 Missing ⚠️
...re/src/entities/platform/generic/mls/psk_bundle.rs 40.00% 3 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #895      +/-   ##
==========================================
- Coverage   78.44%   78.40%   -0.05%     
==========================================
  Files         113      113              
  Lines       20097    20166      +69     
==========================================
+ Hits        15766    15811      +45     
- Misses       4331     4355      +24     
Files with missing lines Coverage Δ
crypto-macros/src/entity_derive/derive_impl.rs 100.00% <100.00%> (ø)
keystore/src/connection/mod.rs 72.28% <ø> (ø)
keystore/src/entities/mod.rs 83.72% <100.00%> (-0.19%) ⬇️
keystore/src/transaction/mod.rs 91.39% <100.00%> (+0.17%) ⬆️
.../src/entities/platform/generic/proteus/identity.rs 77.52% <50.00%> (-0.64%) ⬇️
...re/src/entities/platform/generic/proteus/prekey.rs 94.59% <66.66%> (-1.19%) ⬇️
keystore/src/connection/platform/generic/mod.rs 91.66% <93.93%> (+1.34%) ⬆️
...re/src/entities/platform/generic/mls/credential.rs 96.15% <40.00%> (-1.47%) ⬇️
...ntities/platform/generic/mls/encryption_keypair.rs 96.09% <40.00%> (-1.49%) ⬇️
...s/platform/generic/mls/epoch_encryption_keypair.rs 95.28% <40.00%> (-1.78%) ⬇️
... and 8 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26dde1d...5a64fe4. Read the comment docs.

@istankovic
Copy link
Member

istankovic commented Jan 29, 2025

I see only some of the crates are covered by deny(clippy...). I think we also want to include at least crypto-macros too.

Regarding the rest, such as decode, stuff under extras, interop... I guess those are not as critical, but would like to clarify their status explicitly.

Ideally, it would be nice if we could configure some lints globally, so that we don't have to repeat this everywhere. I think this is one of those that could make sense to apply globally.

@coriolinus coriolinus force-pushed the prgn/doc/unsafe-blocks branch 5 times, most recently from 66b4c1a to 5b39ab7 Compare January 29, 2025 14:48
…ctions

Note that every individual crate in the workspace has to explicitly
opt-in to this policy.
Transmutation is intrinsically unsafe, and the safety checks here
were insufficient. We'll accept a small loss of flexibility here
(the introduction of the `'static` bound) in exchange for some
_much_ better guarantees, and eliminating our responsibility for
this unsafe block entirely.
@coriolinus coriolinus force-pushed the prgn/doc/unsafe-blocks branch 2 times, most recently from 55340fd to 3d87ffa Compare January 29, 2025 14:53
Previously it had simply inherited the safety comment from a
completely different `unsafe impl` family, for a different type
Turns out that we are doing some pretty naive/unsafe things with
regard to `TransactionWrapper`. Ultimately this comes down to the
fact that we're using the wrong tool for the job: the advice
from the maintainers of rusqlite regarding async is "don't use this
tool in that context."

In the meantime, we can't easily change any of these problems...
but we can at least document them!
There used to exist a safety note for the unsafe impls of auto-traits
for `SqlCipherConnection`. Unfortunately, it straight-up lied about
the properties of the struct.

Fixing this involved adding a mutex, which required removing the
`Deref` and `DerefMut` implementations, which then propagated into
some moderately widespread code changes. Nothing too interesting.
…mpat`

In this case, the fix was just to add safety notes, which was nice.
@coriolinus coriolinus force-pushed the prgn/doc/unsafe-blocks branch from 3d87ffa to 5a64fe4 Compare January 29, 2025 15:06
Copy link
Contributor

@SimonThormeyer SimonThormeyer left a comment

Choose a reason for hiding this comment

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

To the best of my knowledge, this is good to go.
Nice work!

@coriolinus coriolinus merged commit 5a64fe4 into main Jan 30, 2025
38 checks passed
@coriolinus coriolinus deleted the prgn/doc/unsafe-blocks branch January 30, 2025 08:32
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.

4 participants