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

[WIP] Use zerocopy byteorder module to replace endianness stuff #1693

Closed
wants to merge 1 commit into from

Conversation

joshlf
Copy link
Contributor

@joshlf joshlf commented Oct 4, 2023

@briansmith Here's a prototype of replacing endianness support with zerocopy. It's mergeable as-is, but there are two gaps to mention which are worth calling out:

  • There's still a dependency on the byteorder crate
  • The byteorder types have no alignment, which may pessimize codegen

Both of these are on our roadmap to fix.

@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #1693 (0b237cb) into main (3a77fe1) will increase coverage by 3.89%.
Report is 280 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1693      +/-   ##
==========================================
+ Coverage   92.07%   95.97%   +3.89%     
==========================================
  Files         132      133       +1     
  Lines       18921    20012    +1091     
  Branches      199      199              
==========================================
+ Hits        17422    19206    +1784     
+ Misses       1463      767     -696     
- Partials       36       39       +3     
Files Coverage Δ
src/aead/aes.rs 98.65% <100.00%> (+17.88%) ⬆️
src/aead/block.rs 97.22% <100.00%> (ø)
src/aead/chacha20_poly1305.rs 93.16% <100.00%> (ø)
src/aead/chacha20_poly1305_openssh.rs 87.50% <100.00%> (-0.60%) ⬇️
src/digest.rs 97.32% <100.00%> (+0.29%) ⬆️
src/lib.rs 70.83% <ø> (+36.21%) ⬆️

... and 23 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

In general this is looking really good. It would be good for us to talk through the timing side channel stuff to see what we can do about that.

src/aead/aes.rs Show resolved Hide resolved
@joshlf
Copy link
Contributor Author

joshlf commented Oct 5, 2023

In general this is looking really good. It would be good for us to talk through the timing side channel stuff to see what we can do about that.

Sounds good. Can you say more about which operations you're thinking about wrt timing side channels? I see that there's an impl of BitXorAssign in endian.rs and there are byte order swapping operations, but there are also methods to expose those types as e.g. byte arrays; I presume this means that some operations happen on the byte array representation, which the code in endian.rs (and similarly in zerocopy) doesn't have any control over.

@briansmith
Copy link
Owner

Sounds good. Can you say more about which operations you're thinking about wrt timing side channels? I see that there's an impl of BitXorAssign in endian.rs and there are byte order swapping operations,

Byte order swapping, XOR, and eventually we'll need to write a constant-time comparison of arrays. I think right the constant-time comparison is done using the byte array representation but we're hoping we'll be able to make use of SIMD for the comparison in the future, probably by using inline assembly.

@briansmith
Copy link
Owner

By "constant-time comparison," we mean "a == b" in constant time for an array of u8/BEU32/etc./values.

@joshlf
Copy link
Contributor Author

joshlf commented Oct 9, 2023

Makes sense. My gut is that it wouldn't make sense for zerocopy make any promises about constant time-ness because the compiler can always undermine those, and it'd make more sense to do any operations that need to be guaranteed constant time on the byte representation. (Obviously all of the standard caveats about constant time execution technically being impossible without formal language support, the compiler can do whatever it wants, etc etc).

@joshlf
Copy link
Contributor Author

joshlf commented Oct 9, 2023

What would it take to gain confidence in performance and/or constant time-ness of this PR? I still need to write the changes to get rid of the byteorder crate dependency, but it'd be good if I can just do that after we've confirmed that this PR even makes sense from a performance and constant time-ness perspective.

@@ -356,7 +355,7 @@ impl From<Counter> for Iv {

impl Iv {
pub(super) fn as_bytes_less_safe(&self) -> &[u8; 16] {
self.0.as_byte_array()
zerocopy::transmute_ref!(&self.0)
Copy link
Owner

Choose a reason for hiding this comment

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

Once we import in the benchmarks for ring::{digest,hmac,pbkdf2} then we can evaluate the performance of PR #1731. I expect that even if PR #1731 as written has performance issues, we will be able to tweak it so it doesn't. This will almost definitely eliminate this code completely.

@@ -114,7 +114,8 @@ impl BlockContext {

Digest {
algorithm: self.algorithm,
value: (self.algorithm.format_output)(self.state),
// SAFETY: TODO
Copy link
Owner

Choose a reason for hiding this comment

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

PR #1731 will (I hope) address this as well, by making the code 100% safe as long as we are happy with the safety guarantees for the code that writes to the self.state union.

Which functions within digest need to be marked unsafe and which can be safe with unsafe blocks is something that could use discussion as it seems we may disagree, which means I might be misunderstanding something.

@joshlf joshlf closed this Nov 2, 2023
@joshlf joshlf deleted the zerocopy-byteorder branch November 4, 2023 03:27
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