Implement SLIP-0023 for Cardano #27152

supermassive · 2025-01-08T11:05:51Z

Resolves brave/brave-browser#43201
Sec review: https://github.com/brave/reviews/issues/1840

The PR includes:

Master key generation from bip-39 entropy.
Private child key generation for normal and hardened indexes.
Adjusted BoringSSL/OpenSSL methods for pubkey derivation and signing:
- ED25519_pubkey_from_scalar. Generates pubkey from scalar Ai = [kL]B. Same as part of ED25519_keypair_from_seed but scalar comes from Ed25519_BIP32 algorithm instead of hashed seed
- ED25519_sign_with_scalar_and_prefix. Same as a part of ED25519_sign. For ED25519_sign private_key[0..31] -> SHA512 -> [scalar, prefix] and private_key[32..63] -> public_key. For ED25519_sign_with_scalar_and_prefix scalar, prefix, public_key come from Ed25519_BIP32 algorithm
- Also added pseudo random test vectors generated with @cardano-sdk/crypto library

Reference implementations:

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:

chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c

components/brave_wallet/browser/internal/hd_key_common.cc

components/brave_wallet/browser/internal/hd_key_ed25519_slip23.cc

test/data/brave-wallet/cardano/cardano_sdk_crypto_slip23/index.mts

kdenhartog

Would be good to clarify what's going on here since SLIP0023 is referencing papers that aren't considered secure anymore.

kdenhartog · 2025-01-20T04:24:47Z

chromium_src/third_party/boringssl/src/include/openssl/curve25519.h

+
+OPENSSL_EXPORT int ED25519_pubkey_from_scalar(uint8_t out_public_key[32],
+                                              const uint8_t scalar[32]);
+OPENSSL_EXPORT int ED25519_sign_with_scalar_and_prefix(


It may be useful to clarify when one or the other should be used here

added comment

kdenhartog · 2025-01-20T04:26:19Z

components/brave_wallet/browser/internal/hd_key_common.cc

@@ -23,6 +23,15 @@ DerivationIndex DerivationIndex::Hardened(uint32_t index) {
  return DerivationIndex(index, true);
 }

+// static
+DerivationIndex DerivationIndex::FromRawValueForTesting(uint32_t index) {


IIRC, we don't want to allow ED25519 to use non-hardened options as it will allow for that attack I investigated previously. Can you clarify what's this for then since I assume it's not for that given we just recently looked at it?

It is not about ED25519 itself, but the way private keys / scalars are hierarchically generated. For SLIP10 it is not possible to do normal derivations, but for SLIP23 it is.

components/brave_wallet/browser/internal/hd_key_ed25519_slip23.cc

kdenhartog · 2025-01-20T04:42:52Z

components/brave_wallet/browser/internal/hd_key_ed25519_slip23.cc

+    z_hmac = crypto::hmac::SignSha512(chain_code_, data);
+    data[0] = 0x01;
+    cc_hmac = crypto::hmac::SignSha512(chain_code_, data);
+  } else {


Isn't this branch something we're not supposed to support according to SLIP10? That seems it was the way that we avoided any concerns for the paper that's broken and referenced in SLIP0023 (see here)

SLIP10 does not support normal derivations but SLIP23 does

components/brave_wallet/browser/internal/hd_key_ed25519_slip23.cc

goodov

chromium_src lgtm. I don't see any other good way of implementing custom ed25519 sign without exposing static internals of boringssl.

goodov · 2025-01-20T07:06:00Z

chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c

+#include "src/third_party/boringssl/src/crypto/curve25519/curve25519.c"
+
+#ifdef UNSAFE_BUFFERS_BUILD
+#pragma allow_unsafe_buffers


is this necessary? I assume src/third_party/boringssl/src/crypto/curve25519/curve25519.c either has this already or is built without unsafe buffers feature.

These lines trigger error otherwise:
(scalar[31] & 0b11000000) == 0b01000000;
sc_muladd(out_sig + 32, hram, scalar, nonce);
Chromium turns off checks for almost whole third_party
https://source.chromium.org/chromium/chromium/src/+/main:build/config/unsafe_buffers_paths.txt;l=67-68 but it is enabled for brave/chromium_src/

if this is a new file, please do not introduce unsafe buffers.

I wanted this file to be consistent with chromium one so didn't split out_sig into two parts and didn't add tempting refactoring.

Alos it seems to me
brave/chromium_src/third_party/boringssl/
src/third_party/boringssl/
and in general
brave/chromium_src/something
src/something
the first ones should follow upstream for unsafe buffer checks policy.

I don't think consistency is an issue here, as most things are being migrated gradually, however I'm now wondering //third_party/boringssl can depend on //base. Better leave it as you did it, as this is shadowing a .c file for now anyway.

cdesouza-chromium · 2025-01-21T00:01:03Z

chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c

+// https://datatracker.ietf.org/doc/html/rfc8032#section-5.1.5
+// requires scalar to follow this requirements 'The lowest 3 bits of the first
+// octet are cleared, the highest bit of the last octet is cleared, and the
+// second highest bit of the last octet is set'.


These comments should be on the header file IMO.

Moved to header

cdesouza-chromium · 2025-01-21T00:04:45Z

chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c

+/* Copyright (c) 2025 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at https://mozilla.org/MPL/2.0/. */


The upstream file seems to be:

third_party/boringssl/src/crypto/curve25519/curve25519.cc

However this one is:

chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c

Is there any particular reason to mark this as a C file, rather than do like upstream and use a C++ file?

upstream is
third_party/boringssl/src/crypto/curve25519/curve25519.c

Thanks to @goodov for the link
https://source.chromium.org/chromium/_/boringssl/boringssl/+/5813c2c10c73d800f1b0d890a7d74ff973abbffc
This is not yet in master

Ah, fair enough.

cdesouza-chromium · 2025-01-21T00:05:18Z

chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c

+int ED25519_is_scalar_pruned(const uint8_t scalar[32]) {
+  return (scalar[0] & 0b00000111) == 0b00000000 &&
+         (scalar[31] & 0b11000000) == 0b01000000;
+}


If possible to change this file from .c to .cc please spanify.

would that work having that base already depends on boringssl?
https://source.chromium.org/chromium/chromium/src/+/main:base/BUILD.gn;l=1510;bpv=1

Explained above, but yeah, it seems to be an obstacle. We could use std::span, but not with the file still being a .c in master.

cdesouza-chromium · 2025-01-21T00:05:28Z

chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c

+int ED25519_pubkey_from_scalar(uint8_t out_public_key[32],
+                               const uint8_t scalar[32]) {


cdesouza-chromium · 2025-01-21T00:06:14Z

chromium_src/third_party/boringssl/src/crypto/curve25519/curve25519.c

+int ED25519_sign_with_scalar_and_prefix(uint8_t out_sig[64],
+                                        const uint8_t* message,
+                                        size_t message_len,
+                                        const uint8_t scalar[32],
+                                        const uint8_t prefix[32],
+                                        const uint8_t public_key[32]) {


components/brave_wallet/browser/internal/hd_key_ed25519_slip23.cc

cdesouza-chromium · 2025-01-21T00:11:41Z

components/brave_wallet/browser/internal/hd_key_ed25519_slip23.cc

+  return base::WrapUnique(new HDKeyEd25519Slip23(
+      scalar, xprv_span.subspan<kSlip23ScalarSize, kSlip23PrefixSize>(),
+      xprv_span.last<kSlip23ChainCodeSize>(), *pubkey));


Please use std::make_unique here, and use the passkey idiom to have constructor only callable from inside HDKeyEd25519Slip23.

cdesouza-chromium · 2025-01-21T00:12:50Z

Pretty good work... I left a few comments.

cdesouza-chromium · 2025-01-22T13:17:51Z

components/brave_wallet/browser/internal/hd_key_ed25519_slip23.h

+ public:
+  using PassKey = base::PassKey<HDKeyEd25519Slip23>;
+
+  HDKeyEd25519Slip23();


I may be wrong but it looks like this default constructor is not used, and it feels like it shouldn't be here?

cdesouza-chromium

LGTM

darkdh

nice work!

github-actions bot assigned supermassive Jan 8, 2025

github-actions bot added feature/web3/wallet feature/web3/wallet/core labels Jan 8, 2025

supermassive force-pushed the slip23 branch 2 times, most recently from 542f8b5 to b039567 Compare January 9, 2025 09:17

supermassive changed the title ~~Slip23~~ Implement SLIP-0023 for Cardano Jan 9, 2025

supermassive force-pushed the slip23 branch from b039567 to e85a847 Compare January 9, 2025 11:35

supermassive marked this pull request as ready for review January 9, 2025 11:35

supermassive requested review from a team as code owners January 9, 2025 11:35

supermassive force-pushed the slip23 branch 2 times, most recently from 8d42cbd to a639e0a Compare January 13, 2025 07:47

darkdh reviewed Jan 14, 2025

View reviewed changes

supermassive force-pushed the slip23 branch from a639e0a to 74e5c72 Compare January 14, 2025 11:24

darkdh reviewed Jan 14, 2025

View reviewed changes

test/data/brave-wallet/cardano/cardano_sdk_crypto_slip23/index.mts Show resolved Hide resolved

supermassive force-pushed the slip23 branch from 74e5c72 to 8249e0c Compare January 15, 2025 04:12

supermassive requested a review from kdenhartog January 16, 2025 01:38

kdenhartog requested changes Jan 20, 2025

View reviewed changes

supermassive force-pushed the slip23 branch from 8249e0c to 48c5a33 Compare January 20, 2025 05:43

supermassive requested a review from kdenhartog January 20, 2025 05:43

goodov approved these changes Jan 20, 2025

View reviewed changes

cdesouza-chromium reviewed Jan 21, 2025

View reviewed changes

components/brave_wallet/browser/internal/hd_key_ed25519_slip23.cc Show resolved Hide resolved

cdesouza-chromium reviewed Jan 21, 2025

View reviewed changes

supermassive force-pushed the slip23 branch from 48c5a33 to bc49e74 Compare January 21, 2025 07:09

supermassive force-pushed the slip23 branch 2 times, most recently from b5ddf06 to 3679f95 Compare January 21, 2025 07:11

cdesouza-chromium reviewed Jan 22, 2025

View reviewed changes

cdesouza-chromium approved these changes Jan 22, 2025

View reviewed changes

darkdh approved these changes Jan 22, 2025

View reviewed changes

supermassive added 5 commits January 24, 2025 10:26

Slip-0023 implementation

9d8cfaf

fix review

aeefecf

fix ci

7ad9b4a

fix review

a6a4524

fix review

55dfb9c

supermassive force-pushed the slip23 branch from 3679f95 to 55dfb9c Compare January 24, 2025 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SLIP-0023 for Cardano #27152

Implement SLIP-0023 for Cardano #27152

supermassive commented Jan 8, 2025 •

edited

Loading

kdenhartog left a comment

kdenhartog Jan 20, 2025

supermassive Jan 20, 2025

kdenhartog Jan 20, 2025

supermassive Jan 20, 2025

kdenhartog Jan 20, 2025

supermassive Jan 20, 2025

goodov left a comment

goodov Jan 20, 2025

supermassive Jan 20, 2025

cdesouza-chromium Jan 20, 2025

supermassive Jan 21, 2025

cdesouza-chromium Jan 22, 2025

cdesouza-chromium Jan 21, 2025

supermassive Jan 21, 2025

cdesouza-chromium Jan 21, 2025

supermassive Jan 21, 2025

supermassive Jan 21, 2025

cdesouza-chromium Jan 22, 2025

cdesouza-chromium Jan 21, 2025

supermassive Jan 21, 2025

cdesouza-chromium Jan 22, 2025

cdesouza-chromium Jan 21, 2025

cdesouza-chromium Jan 21, 2025

cdesouza-chromium Jan 21, 2025

supermassive Jan 21, 2025

cdesouza-chromium commented Jan 21, 2025

cdesouza-chromium Jan 22, 2025

cdesouza-chromium left a comment

darkdh left a comment

		int ED25519_pubkey_from_scalar(uint8_t out_public_key[32],
		const uint8_t scalar[32]) {

Implement SLIP-0023 for Cardano #27152

Are you sure you want to change the base?

Implement SLIP-0023 for Cardano #27152

Conversation

supermassive commented Jan 8, 2025 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

kdenhartog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdesouza-chromium commented Jan 21, 2025

Choose a reason for hiding this comment

cdesouza-chromium left a comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

supermassive commented Jan 8, 2025 •

edited

Loading