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

Improve safety patterns in ring::endian #1662

Closed
briansmith opened this issue Sep 29, 2023 · 9 comments
Closed

Improve safety patterns in ring::endian #1662

briansmith opened this issue Sep 29, 2023 · 9 comments

Comments

@briansmith
Copy link
Owner

briansmith commented Sep 29, 2023

Recently @joshlf commented, roughly, that it would be easier to audit the code in the internal ring::endian module if we made use of the zerocopy crate and/or the byteorder crate. During the discussion several issues came up including:

  • zerocopy has a derive feature that uses proc macros to derive implementations of the traits that help make unsafe code type-safe, but that requires (a) using proc macros, and (b) adding syn, quote, and proc-macro2 as dependencies. So far we have gone out of our way to avoid using proc macros to avoid this.

  • ring uses ring::endian directly in the implementation of cryptography primitives. So, as far as the FIPS project is concerned, whatever dependencies we use, if any, would be affected by our FIPS policy. For example, we might not be able to upgrade these dependencies as freely as we would like. ring would likely feel the need to lock itself to specific versions of these dependencies and/or fork/vendor them. Until we are further along in the FIPS project it is hard to say what the effect on dependencies would be.

  • Because ring uses ring::endian directly in the implementation of cryptographic primitives, any dependencies used in its implementation need to have a plan for preventing compiler-introduced side channels. Currently we all kind of just hope the compiler does the right thing. But in the long term we need to find a solution that goes beyond hope, and any dependencies need to commit to that solution.

  • ring::endian has a potentially huge impact on performance, so changes to ring::endian also need to be benchmarked.

@joshlf
Copy link
Contributor

joshlf commented Sep 29, 2023

  • zerocopy has a derive feature that uses proc macros to derive implementations of the traits that help make unsafe code type-safe, but that requires (a) using proc macros, and (b) adding syn, quote, and proc-macro2 as dependencies. So far we have gone out of our way to avoid using proc macros to avoid this.

IIUC, the ideal situation would be to provide the ability to depend on zerocopy in such a way that it, in turn, has no dependencies? Right now, the byteorder feature implies a dependency on the byteorder crate, but it's likely feasible to separate those two so that you can depend on the byteorder feature and still not have any dependencies beyond zerocopy itself.

  • ring uses ring::endian directly in the implementation of cryptography primitives. So, as far as the FIPS project is concerned, whatever dependencies we use, if any, would be affected by our FIPS policy. For example, we might not be able to upgrade these dependencies as freely as we would like. ring would likely feel the need to lock itself to specific versions of these dependencies and/or fork/vendor them. Until we are further along in the FIPS project it is hard to say what the effect on dependencies would be.

Would it help if we published a release specific to ring (e.g., 0.0.0-alpha.ring) so that ring can depend on it without causing semver incompatibilities with other crates? (e.g., it could be an issue if ring pinned to 0.7.5, and another crate in the graph depended upon 0.7.6; IIUC, Cargo would fail compilation in that case)

  • Because ring uses ring::endian directly in the implementation of cryptographic primitives, any dependencies used in its implementation need to have a plan for preventing compiler-introduced side channels. Currently we all kind of just hope the compiler does the right thing. But in the long term we need to find a solution that goes beyond hope, and any dependencies need to commit to that solution.

Just to make sure I understand correctly: The endianness utilities are used to store secret data (keys, plaintexts, etc), and so any operation on them (e.g. performing a byteorder swap) could in principle be performed in a data-dependent way and thus introduce a timing side-channel?

@briansmith
Copy link
Owner Author

google/zerocopy#450 tracks changes to zerocopy to support this.

MSRV was bumped to 1.61.0 in ring in part to make it possible to use zerocopy.

@briansmith
Copy link
Owner Author

OK, after looking at everything (I think), I think the long-term solution is to "just" eliminate ring::endian completely.

  • Refactor ring::aead to remove the Block type. I have some (old) WIP for this.
  • Remove Endian from Counter
  • Refactor ring::digest to remove Endian from Output.
  • Add the ability to construct [u8; A * B] from [[u8; A]; B] (i.e. concatenate arrays) for specific values of A and B, which can be done efficiently (I believe) using safe code.

etc., etc.

@briansmith
Copy link
Owner Author

Note to help explain why ring::endian exists in the first place: We try to keep the data structure definitions used to define the interface to the BoringSSL C/assembly functions "equivalent," especially regarding which primitive types they use. In the past (and maybe still in the present, to some extent) some of the low-level BoringSSL code has used u64 and u32 and similar for parameter types. But recently many of these functions were either rewritten in Rust in ring or BoringSSL has changed the prototypes of the functions that previously took fixed-endianness u32 or u64 so that they now take (the equivalent of) &[u8; N]. And I believe in general BoringSSL is open to changing more of the prototypes in that way.

And, perhaps I was just trying to be too clever, such as with digest::Output. Especially, some of this code predates some standard library functions like u32::to_be_bytes, which we might have chosen to use from the beginning if they had existed.

@briansmith
Copy link
Owner Author

I did a sketch of this tonight and it's 159 insertions(+), 176 deletions(-) including license boilerplate so I think it is very practical to eliminate almost all of ring::endian, and in particular eliminate ALL the unsafe in ring::endian, without a performance penalty or new dependency, and with simpler and better code in the end.

@joshlf
Copy link
Contributor

joshlf commented Oct 11, 2023

I did a sketch of this tonight and it's 159 insertions(+), 176 deletions(-) including license boilerplate so I think it is very practical to eliminate almost all of ring::endian, and in particular eliminate ALL the unsafe in ring::endian, without a performance penalty or new dependency, and with simpler and better code in the end.

Oh awesome!

@briansmith
Copy link
Owner Author

briansmith commented Oct 11, 2023

So, what did we learn from this?:

  • It seems easier easier for the zerocopy developers to change ring to use zerocopy than it is to "just" eliminate the need for any transmutes in the first place. Not only that, but it's seemingly easier to change zerocopy. Understandable, as it's hard to understand the (performance, side channel) impact on ring.
  • Later versions of BoringSSL removed many uses of unions. There is now much less need for ring::endian which was mostly existing to deal with union types that BoringSSL used to use.
  • Later versions of Rust libcore/libstd have more facilities available to us to further reduce need for endian and probably a bunch of stuff in polyfill.

I think there are still probably some good uses of zerocopy elsewhere in ring, especially if/where we actually need zero-copy type conversions or where we can make better use of the zerocopy traits to model safety. In the case of ring::endian, the unsafe provided a zero-copy conversion API but the users of that API then just immediately copied the data, so nothing was zero-copy.

@joshlf
Copy link
Contributor

joshlf commented Oct 11, 2023

Yeah that sounds like a really good summary.

@briansmith
Copy link
Owner Author

Well, ring::endian is long gone.

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

No branches or pull requests

2 participants