Update curve25519-dalek to v4.1.3 #24287

rillian · 2024-06-19T22:43:16Z

Switch to the latest version for uses already on v4. Removes the no-longer necessary platforms dependency.

Addresses a timing attack vulnerability
https://rustsec.org/advisories/RUSTSEC-2024-0344

We still have code using different versions of curve25519-dalek

Update current uses of v4 to v4.1.3 (ppoprf)
Update brave_wallet from v3.2.0 to v4.1.3
Update brave_wallet to ed25519-dalek-bip32 v0.3
Update challenge-bypass-ristretto from v3 to v4
Update cxx wrapper to challenge-bypass v2
Update skus to challenge-bypass v2

Resolves brave/brave-browser#39142

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Verification of normal wallet and premium features should cover this change; no behaviour changes are intended.

github-actions · 2024-06-19T22:44:02Z

[puLL-Merge] - brave/brave-core@24287

Description

This PR updates the curve25519-dalek crate from version 4.1.1 to 4.1.3. The curve25519-dalek crate provides an implementation of Curve25519 operations for Rust.

Changes

Changes

third_party/rust/chromium_crates_io/vendor/curve25519-dalek-4.1.1 directory renamed to third_party/rust/chromium_crates_io/vendor/curve25519-dalek-4.1.3
Updated version number in various files under the curve25519-dalek directory
Added new file .cargo_vcs_info.json in curve25519-dalek-4.1.3 directory
Minor changes to Rust source files:
- src/backend/mod.rs
- src/backend/serial/mod.rs
- src/backend/serial/scalar_mul/mod.rs
- src/backend/serial/scalar_mul/straus.rs
- src/backend/serial/scalar_mul/vartime_double_base.rs
- src/backend/vector/mod.rs
- src/backend/vector/scalar_mul/mod.rs
- src/backend/vector/scalar_mul/pippenger.rs
- src/backend/vector/scalar_mul/precomputed_straus.rs
- src/backend/vector/scalar_mul/straus.rs
- src/backend/vector/scalar_mul/variable_base.rs
- src/backend/vector/scalar_mul/vartime_double_base.rs
- src/backend/vector/ifma/edwards.rs
- src/backend/vector/ifma/field.rs
- src/constants.rs
- src/diagnostics.rs
- src/montgomery.rs

The changes appear to be an incremental version update with some code changes in the backend implementation. No major API changes are evident.

Security Hotspots

None identified. The changes are focused on backend optimizations and upgrades. Proper review of the specific code changes, especially in security-sensitive cryptographic operations, is still recommended to ensure no vulnerabilities were introduced.

kdenhartog

LGTM

Dismissing since I see there's more to come in terms of changes

rillian · 2024-06-27T23:01:22Z

~~NB a hitch here is that gnrt seems to have a parsing bug~~ with the name of the new const-oid dependency and can't properly generate build files when it's in Cargo.lock. I've been fixing things up manually, but this needs to be fixed to avoid breaking future updates.

This turned out to be a hyphen vs. underscore confusion; using the native version of the crate name works ok.

rillian · 2024-07-10T18:14:05Z

third_party/rust/byteorder/v1/BUILD.gn

-    "i128",
-    "std",
-  ]
+  features = [ "std" ]


This change was made by gnrt. I looked through the various references and couldn't find a matching specification for the i128 feature, so I suspect this is correct. Normally we need 128-bit support for crypto token serialization, so maybe we were using this at one point and then switched to a different approach without regenerating the rust build files?

rillian · 2024-07-10T18:36:02Z

third_party/rust/rand_chacha/v0_2/BUILD.gn

-    "simd",
-    "std",
-  ]
+  features = [ "std" ]


On t he other hand, this seems incorrect; simd is implied by std in rand_chacha v0.2.2 although it's deprecated in favour of the ppv_lite86 feature used in v0.3.

Any ideas here, @bridiver? Seems like this could be a performance regression. I thought BUILD.gn had to exhaustively list all features, but e.g. upstream doesn't enable the ppv feature

Switch to the latest version for uses already on v4. Removes the no-longer necessary `platforms` dependency. Addresses a timing attack vulnerability https://rustsec.org/advisories/RUSTSEC-2024-0344

Port the wallet code to the latest release to address audit warnings about the new timing attack. There is still a dependency through ed25519-dalek-bip32.

Port to the new ed25519-dalek v2 API which renames Keypair -> SigningKey and PublicKey -> VerifyingKey. This completes migrating the wallet code to curve25519-dalek v4.1.3 removing the v3 variant required by the v0.2 version of this dependency.

Run `gnrt vendor` and `gnrt gen` to update vendored dependencies for the port the current release of this rust code used by brave_wallet.

gnrt seems to have a bug with the const-oid crate name which prevents propagating the extra inputs, so I did this manually, and apparently incorrectly. Result of running `npm run presubmit -- --fix`.

Apply standard formatting to recent changes.

Migrate to newer versions of the underlying library and related dependencies.

Port to the latest stable release of the rust `rand` crate, which is available from upstream chromium.

This removes the last dependency on curve25519-dalek < v4.0.3, consolidating dependencies and addressing RUSTSEC-2024-0344. Updates dependency declarations to match new requirements and consolidate versions. Runs gnrt to update vendored crates and build descriptions. Adds a temporary binding to work around partial borrow issues with the new api.

These are no longer needed.

Add a license header to a few files with did not already have one.

This is no longer necessary now that v0.3.1 is available in upstream chromium. NB the `gnrt` tool seems confused here, trying to restore this file and pointing some build references toward it instead of the upstream copy. Possibly the `gnrt.config` reference needs a version specification?

brave-builds · 2024-07-10T19:02:14Z

A Storybook has been deployed to preview UI for the latest push

rillian · 2024-07-10T21:20:41Z

Thanks for taking a look at the first half of this, @kdenhartog. The complete change set is ready for review now.

brave-builds · 2024-07-10T22:10:02Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-07-10T22:10:09Z

A Storybook has been deployed to preview UI for the latest push

supermassive

wallet core lgtm

Uplift of #24287 (squashed) to beta

rillian self-assigned this Jun 19, 2024

rillian requested review from a team and bridiver as code owners June 19, 2024 22:43

github-actions bot added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label Jun 19, 2024

rillian marked this pull request as draft June 19, 2024 22:43

github-actions bot added puLL-Merge feature/web3/wallet feature/web3/wallet/core labels Jun 19, 2024

This was referenced Jun 20, 2024

[Storybook]: Upgrade to version 8 #24289

Merged

Ignore RUSTSEC-2024-0344 brave/audit-config#50

Merged

kdenhartog previously approved these changes Jun 24, 2024

View reviewed changes

rillian force-pushed the curve25519-dalek-4.1.3 branch from 37721ee to afec5e5 Compare June 25, 2024 23:06

rillian commented Jul 10, 2024

View reviewed changes

rillian added 12 commits July 10, 2024 11:38

Update curve25519-dalek to v4.1.3

c365b5a

Switch to the latest version for uses already on v4. Removes the no-longer necessary `platforms` dependency. Addresses a timing attack vulnerability https://rustsec.org/advisories/RUSTSEC-2024-0344

Update brave_wallet to curve25519-dalek v4.1.3

13bdb85

Port the wallet code to the latest release to address audit warnings about the new timing attack. There is still a dependency through ed25519-dalek-bip32.

Vendor ed25519-dalek-bip32 v0.3 and deps

750dc32

Run `gnrt vendor` and `gnrt gen` to update vendored dependencies for the port the current release of this rust code used by brave_wallet.

Fix gn formatting

9568233

gnrt seems to have a bug with the const-oid crate name which prevents propagating the extra inputs, so I did this manually, and apparently incorrectly. Result of running `npm run presubmit -- --fix`.

brave_wallet: cargo fmt

b2a5099

Apply standard formatting to recent changes.

Update challenge-bypass-ristretto cxx wrapper

fa38e4c

Migrate to newer versions of the underlying library and related dependencies.

Update skus to rand v0.8

852700d

Port to the latest stable release of the rust `rand` crate, which is available from upstream chromium.

Remove unused hmac v0.10 and crypto-mac v0.10 build files

c78421a

These are no longer needed.

skus: add copyright headers

e8046d0

Add a license header to a few files with did not already have one.

rillian force-pushed the curve25519-dalek-4.1.3 branch from 73d45e1 to 0688a9c Compare July 10, 2024 21:19

rillian marked this pull request as ready for review July 10, 2024 21:19

rillian requested review from a team as code owners July 10, 2024 21:19

supermassive approved these changes Jul 17, 2024

View reviewed changes

bridiver approved these changes Jul 17, 2024

View reviewed changes

rillian merged commit e9a42f9 into master Jul 17, 2024
16 checks passed

rillian deleted the curve25519-dalek-4.1.3 branch July 17, 2024 19:22

github-actions bot added this to the 1.70.x - Nightly milestone Jul 17, 2024

brave-builds added a commit that referenced this pull request Jul 17, 2024

Uplift of #24287 (squashed) to beta

d7a3588

brave-builds mentioned this pull request Jul 17, 2024

Update curve25519-dalek to v4.1.3 (uplift to 1.69.x) #24716

Merged

7 tasks

kjozwiak pushed a commit that referenced this pull request Jul 18, 2024

Update curve25519-dalek to v4.1.3 (uplift to 1.69.x) (#24716)

09758fb

Uplift of #24287 (squashed) to beta

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update curve25519-dalek to v4.1.3 #24287

Update curve25519-dalek to v4.1.3 #24287

rillian commented Jun 19, 2024 •

edited

Loading

github-actions bot commented Jun 19, 2024

Changes

kdenhartog left a comment

rillian commented Jun 27, 2024 •

edited

Loading

rillian Jul 10, 2024 •

edited

Loading

rillian Jul 10, 2024

brave-builds commented Jul 10, 2024

rillian commented Jul 10, 2024 •

edited

Loading

brave-builds commented Jul 10, 2024

brave-builds commented Jul 10, 2024

supermassive left a comment

Update curve25519-dalek to v4.1.3 #24287

Update curve25519-dalek to v4.1.3 #24287

Conversation

rillian commented Jun 19, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

github-actions bot commented Jun 19, 2024

Description

Changes

Security Hotspots

kdenhartog left a comment

Choose a reason for hiding this comment

rillian commented Jun 27, 2024 • edited Loading

rillian Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

rillian Jul 10, 2024

Choose a reason for hiding this comment

brave-builds commented Jul 10, 2024

rillian commented Jul 10, 2024 • edited Loading

brave-builds commented Jul 10, 2024

brave-builds commented Jul 10, 2024

supermassive left a comment

Choose a reason for hiding this comment

rillian commented Jun 19, 2024 •

edited

Loading

rillian commented Jun 27, 2024 •

edited

Loading

rillian Jul 10, 2024 •

edited

Loading

rillian commented Jul 10, 2024 •

edited

Loading