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

Refactor of fuel-crypto for clarity around safety #346

Merged
merged 6 commits into from
Feb 22, 2023

Conversation

mitchmindtree
Copy link
Contributor

This started with the aim of carrying over FuelLabs/fuel-crypto#36 which aimed to address FuelLabs/fuel-crypto#35, though after noticing a few other instances of the same issue decided to take a closer look throughout the crate.

This PR removes many of the unsafe constructors exposed in the fuel-crypto API, some due to the implementations not actually being unsafe, and some due to a revised implementation that safely encapsulates the unsafe behaviour.

The Message and Signature from_bytes and from_bytes_ref constructors have been documented to clarify expectations that they expect signature or cryptographically hashed data respectively.

The PublicKey and SecretKey from_bytes_unchecked constructors have been made private in order to ensure holding an instance of one of these types guarantees their inner byte arrays at least satisfies the curve.

The original, custom curve-checking functions have been removed in favour of using the secp256k1 library constructors directly which share the same implementation but with an added range check that is likely to get optimised out due to the use of const-sized arrays anyway.

Adds some missing #[repr(transparent)] decorators to fuel-types.

The FuelMnemonic type provided unnecessary indirection between the user and the free function, and has been removed in favour of exposing the function directly from the crate root.

@mitchmindtree mitchmindtree added breaking A breaking api change tech-debt fuel-vm Related to the `fuel-vm` crate. fuel-crypto Related to the `fuel-crypto` crate. fuel-types Related to the `fuel-types` crate. labels Feb 10, 2023
@mitchmindtree mitchmindtree self-assigned this Feb 10, 2023
public_with_flag[1..].copy_from_slice(bytes);
secp256k1::PublicKey::from_slice(&public_with_flag)
.map(|_| ())
.map_err(|_| Error::InvalidPublicKey)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: The original is_slice_in_curve_unchecked function would also leave the first byte zeroed before invoking the secp256k1 pubkey parser like this, so I've carried over the behaviour in this function. However, it looks like we should possibly be setting the first byte to SECP_UNCOMPRESSED_FLAG? Should investigate or open an issue before landing.

This removes many of the unsafe constructors exposed in the `fuel-
crypto` API, some due to the implementations not actually being unsafe,
and some due to a revised implementation that safely encapsulates the
unsafe behaviour.

The `Message` and `Signature` `from_bytes` and `from_bytes_ref`
constructors have been documented to clarify expectations that they
expect signature or cryptographically hashed data respectively.

The `PublicKey` and `SecretKey` `from_bytes_unchecked` constructors have
been made private in order to ensure holding an instance of one of these
types guarantees their inner byte arrays at least satisfies the curve.

The original, custom curve-checking functions have been removed in
favour of using the secp256k1 library constructors directly which share
the same implementation but with an added range check that is likely to
get optimised out due to the use of const-sized arrays anyway.

Adds some missing `#[repr(transparent)]` decorators to `fuel-types`.

The `FuelMnemonic` type provided unnecessary indirection between the
user and the free function, and has been removed in favour of exposing
the function directly from the crate root.
@mitchmindtree mitchmindtree force-pushed the mitchmindtree/fuel-crypto-safety branch from 67ca6be to c387305 Compare February 10, 2023 16:25
@mitchmindtree mitchmindtree requested a review from a team February 10, 2023 16:32
@mitchmindtree mitchmindtree marked this pull request as ready for review February 10, 2023 16:33
/// are not canonically encoded to a valid `secp256k1` signature.
pub unsafe fn from_bytes_unchecked(bytes: [u8; Self::LEN]) -> Self {
/// This constructor expects the given bytes to be a valid signature. No signing is performed.
pub fn from_bytes(bytes: [u8; Self::LEN]) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this is not a valid signature? I know we use this function in fuel core with bytes from the network so maybe we need to add from_bytes_checked -> Option<Self>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Afaik there's no way to know if the signature is valid until we actually try to validate the transaction, as we don't know what data has been signed at this point.

It should be fine to use this with bytes read from the network as if the signature isn't valid, the transaction validation will fail because the signature won't be valid for some spend or whatever.

Self(Bytes32::from_slice_unchecked(bytes))
/// This constructor expects the given bytes to be a valid,
/// cryptographically hashed message. No hashing is performed.
pub fn from_bytes_ref(bytes: &[u8; Self::LEN]) -> &Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually uphold the lifetime. If the bytes array is moved after this call and &Self is still being used, do we get a compile error? If not this should still be an unsafe function right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this actually uphold the lifetime.

Yes, in this case the lifetime of the reference in &Self is bound by the only input lifetime, i.e. the lifetime of bytes: &[u8; Self::LEN]. In this case, Rust elides the lifetime because there is only one possible valid lifetime for the output. Here is what the un-ellided signature would look like:

    pub fn from_bytes_ref<'a>(bytes: &'a [u8; Self::LEN]) -> &'a Self {

Comment on lines +28 to +30
let sig_bytes = <&_>::try_from(&self.memory[b..bx]).expect("memory bounds checked");
let msg_bytes = <&_>::try_from(&self.memory[c..cx]).expect("memory bounds checked");
let signature = Signature::from_bytes_ref(sig_bytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be results instead of panics?

Copy link
Contributor Author

@mitchmindtree mitchmindtree Feb 13, 2023

Choose a reason for hiding this comment

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

If these fail, I believe it's a bug in our implementation logic, i.e. these should always be correct because bx and cx are exactly Bytes*::LEN greater than b or c respectively.

@Voxelot
Copy link
Member

Voxelot commented Feb 13, 2023

This would also be a good opportunity to upgrade from secp256k1:0.24 to secp256k1:0.26

@mitchmindtree
Copy link
Contributor Author

Feel free to merge this when ready (keeping in mind there is a little breakage of the pub API).

@freesig freesig enabled auto-merge (squash) February 22, 2023 22:45
@freesig freesig merged commit 513749c into master Feb 22, 2023
@freesig freesig deleted the mitchmindtree/fuel-crypto-safety branch February 22, 2023 22:50
xgreenx added a commit to FuelLabs/fuel-core that referenced this pull request Apr 27, 2023
The change bumps the version to `0.18.0` and exposes
`sync_max_get_header` and `sync_max_get_txns` in the helm chart.

# Release 0.18.0

## Overview

A new release brings:
- Optimization for the execution based on the performance from beta 3
and on internal benchmarks. Improved metrics gathering.
- Stabilization and better test coverage of the `fuel-vm`. We removed
almost all unsafe code and added test cases for each opcode. Fixed some
edge cases with memory in the `fuel-vm`.
- Fully integrated Merkle trees and filled all malleable fields in the
transactions.
- Added retryable messages, removed redundant fields from it, and
updated the API to support a new commitment schema.

## What's Changed

### Breaking

- All unsafe functions were replaced with safe analog in the
`fuel-crypto` - FuelLabs/fuel-vm#346
- `$hp` holds the address of the last available byte in a heap, while
previously it was `$hp - 1` -
FuelLabs/fuel-vm#377
- Each variant in the `fuel_tx::Input` enum now has its own type -
FuelLabs/fuel-vm#364
- Message nonce is unified and now `Bytes32` everywhere -
FuelLabs/fuel-vm#394
- Removed the `message_id` field from all places -
FuelLabs/fuel-vm#397,
FuelLabs/fuel-vm#373,
- Unified the block height in all places with the introduction of a new
`BlockHeigh` - FuelLabs/fuel-vm#410
- Make SMO instruction take data ptr as an argument -
FuelLabs/fuel-vm#404
- Now the chain id affects the signature and predicate's owner and
should be passed into the `sign` function -
FuelLabs/fuel-vm#406
- Updated the `produce_blocks` endpoint to accept the start time and the
number of blocks. All new blocks will use the previous timestamp as a
base - #1059
- The `fuel-core` stores only unspent coins and messages, so all API
that previously returned spent coins is affected - Prune owned coin idx
when inputs are spent by @Voxelot in
#1055
- The message proof API has been changed to be compatible with a new
version - #1071
- The `fuel-core` now has retryable messages and coin messages.
Retryable messages can only be consumed during successful transaction
execution. The coin message acts as common coins. `resouces_to_spend`
API was replaced with `coins_to_spend` that returns a new `CoinType`
type. - #1067

## All changes
* adding fuel-core service monitor to helm chart by @rfuelsh in
#1037
* Adding specific app selector by @rfuelsh in
#1039
* use sticky sessions for GQL requests to sentries by @Voxelot in
#1042
* Add ingress service name to helm chart by @rfuelsh in
#1043
* Added test to verify the iteration over owned transactions by @xgreenx
in #1041
* Change sentry lb to use clusterIP by @Voxelot in
#1045
* Fixed Tempfile dependency by @ControlCplusControlV in
#1048
* Write contract code in raw by @freesig in
#994
* attempt to use buildjet runners by @Voxelot in
#1017
* Set tx pointer during block production by @Voxelot in
#1054
* Used `BASE_AMOUNT` for test with bob to pay for fee by @xgreenx in
#1057
* Prepare GraphQL Crate for extraction by @ControlCplusControlV in
#1012
* Support large contract deployments over p2p by @Voxelot in
#1062
* fix yaml and add linting job by @Voxelot in
#1063
* Actualized the ABI for message to be compatible with last version of
the Solidity contracts. by @xgreenx in
#1060
* feat: Contract state and assets merkle data storage by @bvrooman in
#1008
* Take into account the previous block timestamp during block production
by @xgreenx in #1059
* Prune owned coin idx when inputs are spent by @Voxelot in
#1055
* Retrayable messages fuel-core part by @xgreenx in
#1067
* chore: Additional Tests for Contract States and Balances by @bvrooman
in #1073
* Rust 1.68.1 & Sparse Registry by @Voxelot in
#1077
* Nginx tuneup by @Voxelot in
#1080
* chore: Update balance and state Merkleization by @bvrooman in
#1084
* Support sticky session in the client by @xgreenx in
#1079
* Added e2e test to run 1000 `dry_run` by @xgreenx in
#1091
* Use docker.io auth credentials to overcome rate limiting by @Voxelot
in #1092
* honeycomb tracing subscription by @Voxelot in
#1083
* Update withdrawal proof API to support proving from a block header
committed to L1 by @xgreenx in
#1071
* Upstream v0.17.8 by @Voxelot in
#1102
* use chainid for transactions and predicates by @Voxelot in
#1107
* fix the dry-run e2e test to actually perform dry runs by @Voxelot in
#1111
* peer reputation with separate app & gossipsub score by @leviathanbeak
in #1028
* Adding sentry data synchronization by @rfuelsh in
#1115
* Capture metrics for graphql api by @Voxelot in
#1113
* Update cargo audit and add to commit by @freesig in
#1152
* add task params by @leviathanbeak in
#1159
* Took into account that block creation could take some time by @xgreenx
in #1162
* Applying optimizations from `0.17.11` release by @xgreenx in
#1158


**Full Changelog**:
v0.17.3...v0.18.0
crypto523 pushed a commit to crypto523/fuel-core that referenced this pull request Oct 7, 2024
The change bumps the version to `0.18.0` and exposes
`sync_max_get_header` and `sync_max_get_txns` in the helm chart.

# Release 0.18.0

## Overview

A new release brings:
- Optimization for the execution based on the performance from beta 3
and on internal benchmarks. Improved metrics gathering.
- Stabilization and better test coverage of the `fuel-vm`. We removed
almost all unsafe code and added test cases for each opcode. Fixed some
edge cases with memory in the `fuel-vm`.
- Fully integrated Merkle trees and filled all malleable fields in the
transactions.
- Added retryable messages, removed redundant fields from it, and
updated the API to support a new commitment schema.

## What's Changed

### Breaking

- All unsafe functions were replaced with safe analog in the
`fuel-crypto` - FuelLabs/fuel-vm#346
- `$hp` holds the address of the last available byte in a heap, while
previously it was `$hp - 1` -
FuelLabs/fuel-vm#377
- Each variant in the `fuel_tx::Input` enum now has its own type -
FuelLabs/fuel-vm#364
- Message nonce is unified and now `Bytes32` everywhere -
FuelLabs/fuel-vm#394
- Removed the `message_id` field from all places -
FuelLabs/fuel-vm#397,
FuelLabs/fuel-vm#373,
- Unified the block height in all places with the introduction of a new
`BlockHeigh` - FuelLabs/fuel-vm#410
- Make SMO instruction take data ptr as an argument -
FuelLabs/fuel-vm#404
- Now the chain id affects the signature and predicate's owner and
should be passed into the `sign` function -
FuelLabs/fuel-vm#406
- Updated the `produce_blocks` endpoint to accept the start time and the
number of blocks. All new blocks will use the previous timestamp as a
base - FuelLabs/fuel-core#1059
- The `fuel-core` stores only unspent coins and messages, so all API
that previously returned spent coins is affected - Prune owned coin idx
when inputs are spent by @Voxelot in
FuelLabs/fuel-core#1055
- The message proof API has been changed to be compatible with a new
version - FuelLabs/fuel-core#1071
- The `fuel-core` now has retryable messages and coin messages.
Retryable messages can only be consumed during successful transaction
execution. The coin message acts as common coins. `resouces_to_spend`
API was replaced with `coins_to_spend` that returns a new `CoinType`
type. - FuelLabs/fuel-core#1067

## All changes
* adding fuel-core service monitor to helm chart by @rfuelsh in
FuelLabs/fuel-core#1037
* Adding specific app selector by @rfuelsh in
FuelLabs/fuel-core#1039
* use sticky sessions for GQL requests to sentries by @Voxelot in
FuelLabs/fuel-core#1042
* Add ingress service name to helm chart by @rfuelsh in
FuelLabs/fuel-core#1043
* Added test to verify the iteration over owned transactions by @xgreenx
in FuelLabs/fuel-core#1041
* Change sentry lb to use clusterIP by @Voxelot in
FuelLabs/fuel-core#1045
* Fixed Tempfile dependency by @ControlCplusControlV in
FuelLabs/fuel-core#1048
* Write contract code in raw by @freesig in
FuelLabs/fuel-core#994
* attempt to use buildjet runners by @Voxelot in
FuelLabs/fuel-core#1017
* Set tx pointer during block production by @Voxelot in
FuelLabs/fuel-core#1054
* Used `BASE_AMOUNT` for test with bob to pay for fee by @xgreenx in
FuelLabs/fuel-core#1057
* Prepare GraphQL Crate for extraction by @ControlCplusControlV in
FuelLabs/fuel-core#1012
* Support large contract deployments over p2p by @Voxelot in
FuelLabs/fuel-core#1062
* fix yaml and add linting job by @Voxelot in
FuelLabs/fuel-core#1063
* Actualized the ABI for message to be compatible with last version of
the Solidity contracts. by @xgreenx in
FuelLabs/fuel-core#1060
* feat: Contract state and assets merkle data storage by @bvrooman in
FuelLabs/fuel-core#1008
* Take into account the previous block timestamp during block production
by @xgreenx in FuelLabs/fuel-core#1059
* Prune owned coin idx when inputs are spent by @Voxelot in
FuelLabs/fuel-core#1055
* Retrayable messages fuel-core part by @xgreenx in
FuelLabs/fuel-core#1067
* chore: Additional Tests for Contract States and Balances by @bvrooman
in FuelLabs/fuel-core#1073
* Rust 1.68.1 & Sparse Registry by @Voxelot in
FuelLabs/fuel-core#1077
* Nginx tuneup by @Voxelot in
FuelLabs/fuel-core#1080
* chore: Update balance and state Merkleization by @bvrooman in
FuelLabs/fuel-core#1084
* Support sticky session in the client by @xgreenx in
FuelLabs/fuel-core#1079
* Added e2e test to run 1000 `dry_run` by @xgreenx in
FuelLabs/fuel-core#1091
* Use docker.io auth credentials to overcome rate limiting by @Voxelot
in FuelLabs/fuel-core#1092
* honeycomb tracing subscription by @Voxelot in
FuelLabs/fuel-core#1083
* Update withdrawal proof API to support proving from a block header
committed to L1 by @xgreenx in
FuelLabs/fuel-core#1071
* Upstream v0.17.8 by @Voxelot in
FuelLabs/fuel-core#1102
* use chainid for transactions and predicates by @Voxelot in
FuelLabs/fuel-core#1107
* fix the dry-run e2e test to actually perform dry runs by @Voxelot in
FuelLabs/fuel-core#1111
* peer reputation with separate app & gossipsub score by @leviathanbeak
in FuelLabs/fuel-core#1028
* Adding sentry data synchronization by @rfuelsh in
FuelLabs/fuel-core#1115
* Capture metrics for graphql api by @Voxelot in
FuelLabs/fuel-core#1113
* Update cargo audit and add to commit by @freesig in
FuelLabs/fuel-core#1152
* add task params by @leviathanbeak in
FuelLabs/fuel-core#1159
* Took into account that block creation could take some time by @xgreenx
in FuelLabs/fuel-core#1162
* Applying optimizations from `0.17.11` release by @xgreenx in
FuelLabs/fuel-core#1158


**Full Changelog**:
FuelLabs/fuel-core@v0.17.3...v0.18.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking api change fuel-crypto Related to the `fuel-crypto` crate. fuel-types Related to the `fuel-types` crate. fuel-vm Related to the `fuel-vm` crate. tech-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants