Skip to content

Commit 5755d5e

Browse files
authored
sui daa security audit fixes (#18398)
1 parent 5524f4d commit 5755d5e

File tree

4 files changed

+128
-11
lines changed

4 files changed

+128
-11
lines changed

aptos-move/framework/aptos-framework/doc/auth_data.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@
209209

210210

211211
<pre><code><b>public</b> <b>fun</b> <a href="auth_data.md#0x1_auth_data_derivable_abstract_signature">derivable_abstract_signature</a>(self: &<a href="auth_data.md#0x1_auth_data_AbstractionAuthData">AbstractionAuthData</a>): &<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>&lt;u8&gt; {
212-
<b>assert</b>!(self is DerivableV1, <a href="../../aptos-stdlib/../move-stdlib/doc/error.md#0x1_error_invalid_argument">error::invalid_argument</a>(<a href="auth_data.md#0x1_auth_data_ENOT_REGULAR_AUTH_DATA">ENOT_REGULAR_AUTH_DATA</a>));
212+
<b>assert</b>!(self is DerivableV1, <a href="../../aptos-stdlib/../move-stdlib/doc/error.md#0x1_error_invalid_argument">error::invalid_argument</a>(<a href="auth_data.md#0x1_auth_data_ENOT_DERIVABLE_AUTH_DATA">ENOT_DERIVABLE_AUTH_DATA</a>));
213213
&self.abstract_signature
214214
}
215215
</code></pre>

aptos-move/framework/aptos-framework/doc/sui_derivable_account.md

Lines changed: 71 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,12 @@ Nonce: <digest>
3737
- [Function `split_signature_bytes`](#0x1_sui_derivable_account_split_signature_bytes)
3838
- [Function `derive_account_address_from_public_key`](#0x1_sui_derivable_account_derive_account_address_from_public_key)
3939
- [Function `authenticate_auth_data`](#0x1_sui_derivable_account_authenticate_auth_data)
40+
- [Function `authenticate_auth_data_internal`](#0x1_sui_derivable_account_authenticate_auth_data_internal)
4041
- [Function `authenticate`](#0x1_sui_derivable_account_authenticate)
4142
- [Specification](#@Specification_1)
4243
- [Function `derive_account_address_from_public_key`](#@Specification_1_derive_account_address_from_public_key)
4344
- [Function `authenticate_auth_data`](#@Specification_1_authenticate_auth_data)
45+
- [Function `authenticate_auth_data_internal`](#@Specification_1_authenticate_auth_data_internal)
4446
- [Function `authenticate`](#@Specification_1_authenticate)
4547

4648

@@ -398,6 +400,16 @@ https://github.com/MystenLabs/sui/blob/main/crates/shared-crypto/src/intent.rs#L
398400
## Constants
399401

400402

403+
<a id="0x1_sui_derivable_account_EMALFORMED_DATA"></a>
404+
405+
Malformed data with trailing bytes.
406+
407+
408+
<pre><code><b>const</b> <a href="sui_derivable_account.md#0x1_sui_derivable_account_EMALFORMED_DATA">EMALFORMED_DATA</a>: u64 = 8;
409+
</code></pre>
410+
411+
412+
401413
<a id="0x1_sui_derivable_account_EINVALID_PUBLIC_KEY"></a>
402414

403415
Invalid public key.
@@ -408,6 +420,16 @@ Invalid public key.
408420

409421

410422

423+
<a id="0x1_sui_derivable_account_EDEPRECATED"></a>
424+
425+
Function is deprecated and should not be called.
426+
427+
428+
<pre><code><b>const</b> <a href="sui_derivable_account.md#0x1_sui_derivable_account_EDEPRECATED">EDEPRECATED</a>: u64 = 9;
429+
</code></pre>
430+
431+
432+
411433
<a id="0x1_sui_derivable_account_EMISSING_ENTRY_FUNCTION_PAYLOAD"></a>
412434

413435
Entry function payload is missing.
@@ -515,6 +537,7 @@ serialized <code><a href="sui_derivable_account.md#0x1_sui_derivable_account_Sui
515537
<b>let</b> stream = <a href="../../aptos-stdlib/doc/bcs_stream.md#0x1_bcs_stream_new">bcs_stream::new</a>(*abstract_public_key);
516538
<b>let</b> sui_account_address = <a href="../../aptos-stdlib/doc/bcs_stream.md#0x1_bcs_stream_deserialize_vector">bcs_stream::deserialize_vector</a>&lt;u8&gt;(&<b>mut</b> stream, |x| deserialize_u8(x));
517539
<b>let</b> domain = <a href="../../aptos-stdlib/doc/bcs_stream.md#0x1_bcs_stream_deserialize_vector">bcs_stream::deserialize_vector</a>&lt;u8&gt;(&<b>mut</b> stream, |x| deserialize_u8(x));
540+
<b>assert</b>!(!<a href="../../aptos-stdlib/doc/bcs_stream.md#0x1_bcs_stream_has_remaining">bcs_stream::has_remaining</a>(&<b>mut</b> stream), <a href="sui_derivable_account.md#0x1_sui_derivable_account_EMALFORMED_DATA">EMALFORMED_DATA</a>);
518541
<a href="sui_derivable_account.md#0x1_sui_derivable_account_SuiAbstractPublicKey">SuiAbstractPublicKey</a> { sui_account_address, domain }
519542
}
520543
</code></pre>
@@ -544,6 +567,7 @@ Returns a tuple of the signature.
544567
<b>let</b> signature_type = <a href="../../aptos-stdlib/doc/bcs_stream.md#0x1_bcs_stream_deserialize_u8">bcs_stream::deserialize_u8</a>(&<b>mut</b> stream);
545568
<b>if</b> (signature_type == 0x00) {
546569
<b>let</b> signature = <a href="../../aptos-stdlib/doc/bcs_stream.md#0x1_bcs_stream_deserialize_vector">bcs_stream::deserialize_vector</a>&lt;u8&gt;(&<b>mut</b> stream, |x| deserialize_u8(x));
570+
<b>assert</b>!(!<a href="../../aptos-stdlib/doc/bcs_stream.md#0x1_bcs_stream_has_remaining">bcs_stream::has_remaining</a>(&<b>mut</b> stream), <a href="sui_derivable_account.md#0x1_sui_derivable_account_EMALFORMED_DATA">EMALFORMED_DATA</a>);
547571
SuiAbstractSignature::MessageV1 { signature }
548572
} <b>else</b> {
549573
<b>abort</b>(<a href="sui_derivable_account.md#0x1_sui_derivable_account_EINVALID_SIGNATURE_TYPE">EINVALID_SIGNATURE_TYPE</a>)
@@ -652,9 +676,10 @@ Derives the account address from the public key and returns it is a hex string w
652676

653677
## Function `authenticate_auth_data`
654678

679+
@deprecated This function is deprecated and will always abort.
655680

656681

657-
<pre><code><b>public</b> <b>fun</b> <a href="sui_derivable_account.md#0x1_sui_derivable_account_authenticate_auth_data">authenticate_auth_data</a>(aa_auth_data: <a href="auth_data.md#0x1_auth_data_AbstractionAuthData">auth_data::AbstractionAuthData</a>, entry_function_name: &<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>&lt;u8&gt;)
682+
<pre><code><b>public</b> <b>fun</b> <a href="sui_derivable_account.md#0x1_sui_derivable_account_authenticate_auth_data">authenticate_auth_data</a>(_aa_auth_data: <a href="auth_data.md#0x1_auth_data_AbstractionAuthData">auth_data::AbstractionAuthData</a>, _entry_function_name: &<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>&lt;u8&gt;)
658683
</code></pre>
659684

660685

@@ -664,6 +689,33 @@ Derives the account address from the public key and returns it is a hex string w
664689

665690

666691
<pre><code><b>public</b> <b>fun</b> <a href="sui_derivable_account.md#0x1_sui_derivable_account_authenticate_auth_data">authenticate_auth_data</a>(
692+
_aa_auth_data: AbstractionAuthData,
693+
_entry_function_name: &<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>&lt;u8&gt;
694+
) {
695+
<b>abort</b>(<a href="sui_derivable_account.md#0x1_sui_derivable_account_EDEPRECATED">EDEPRECATED</a>)
696+
}
697+
</code></pre>
698+
699+
700+
701+
</details>
702+
703+
<a id="0x1_sui_derivable_account_authenticate_auth_data_internal"></a>
704+
705+
## Function `authenticate_auth_data_internal`
706+
707+
708+
709+
<pre><code><b>fun</b> <a href="sui_derivable_account.md#0x1_sui_derivable_account_authenticate_auth_data_internal">authenticate_auth_data_internal</a>(aa_auth_data: <a href="auth_data.md#0x1_auth_data_AbstractionAuthData">auth_data::AbstractionAuthData</a>, entry_function_name: &<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>&lt;u8&gt;)
710+
</code></pre>
711+
712+
713+
714+
<details>
715+
<summary>Implementation</summary>
716+
717+
718+
<pre><code><b>fun</b> <a href="sui_derivable_account.md#0x1_sui_derivable_account_authenticate_auth_data_internal">authenticate_auth_data_internal</a>(
667719
aa_auth_data: AbstractionAuthData,
668720
entry_function_name: &<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>&lt;u8&gt;
669721
) {
@@ -739,7 +791,7 @@ Authorization function for domain account abstraction.
739791

740792

741793
<pre><code><b>public</b> <b>fun</b> <a href="sui_derivable_account.md#0x1_sui_derivable_account_authenticate">authenticate</a>(<a href="account.md#0x1_account">account</a>: <a href="../../aptos-stdlib/../move-stdlib/doc/signer.md#0x1_signer">signer</a>, aa_auth_data: AbstractionAuthData): <a href="../../aptos-stdlib/../move-stdlib/doc/signer.md#0x1_signer">signer</a> {
742-
daa_authenticate(<a href="account.md#0x1_account">account</a>, aa_auth_data, |<a href="auth_data.md#0x1_auth_data">auth_data</a>, entry_name| <a href="sui_derivable_account.md#0x1_sui_derivable_account_authenticate_auth_data">authenticate_auth_data</a>(<a href="auth_data.md#0x1_auth_data">auth_data</a>, entry_name))
794+
daa_authenticate(<a href="account.md#0x1_account">account</a>, aa_auth_data, |<a href="auth_data.md#0x1_auth_data">auth_data</a>, entry_name| <a href="sui_derivable_account.md#0x1_sui_derivable_account_authenticate_auth_data_internal">authenticate_auth_data_internal</a>(<a href="auth_data.md#0x1_auth_data">auth_data</a>, entry_name))
743795
}
744796
</code></pre>
745797

@@ -773,7 +825,23 @@ Authorization function for domain account abstraction.
773825
### Function `authenticate_auth_data`
774826

775827

776-
<pre><code><b>public</b> <b>fun</b> <a href="sui_derivable_account.md#0x1_sui_derivable_account_authenticate_auth_data">authenticate_auth_data</a>(aa_auth_data: <a href="auth_data.md#0x1_auth_data_AbstractionAuthData">auth_data::AbstractionAuthData</a>, entry_function_name: &<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>&lt;u8&gt;)
828+
<pre><code><b>public</b> <b>fun</b> <a href="sui_derivable_account.md#0x1_sui_derivable_account_authenticate_auth_data">authenticate_auth_data</a>(_aa_auth_data: <a href="auth_data.md#0x1_auth_data_AbstractionAuthData">auth_data::AbstractionAuthData</a>, _entry_function_name: &<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>&lt;u8&gt;)
829+
</code></pre>
830+
831+
832+
833+
834+
<pre><code><b>pragma</b> verify = <b>false</b>;
835+
</code></pre>
836+
837+
838+
839+
<a id="@Specification_1_authenticate_auth_data_internal"></a>
840+
841+
### Function `authenticate_auth_data_internal`
842+
843+
844+
<pre><code><b>fun</b> <a href="sui_derivable_account.md#0x1_sui_derivable_account_authenticate_auth_data_internal">authenticate_auth_data_internal</a>(aa_auth_data: <a href="auth_data.md#0x1_auth_data_AbstractionAuthData">auth_data::AbstractionAuthData</a>, entry_function_name: &<a href="../../aptos-stdlib/../move-stdlib/doc/vector.md#0x1_vector">vector</a>&lt;u8&gt;)
777845
</code></pre>
778846

779847

aptos-move/framework/aptos-framework/sources/account/auth_data.move

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ module aptos_framework::auth_data {
4747
}
4848

4949
public fun derivable_abstract_signature(self: &AbstractionAuthData): &vector<u8> {
50-
assert!(self is DerivableV1, error::invalid_argument(ENOT_REGULAR_AUTH_DATA));
50+
assert!(self is DerivableV1, error::invalid_argument(ENOT_DERIVABLE_AUTH_DATA));
5151
&self.abstract_signature
5252
}
5353

aptos-move/framework/aptos-framework/sources/account/common_account_abstractions/sui_derivable_account.move

Lines changed: 55 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ module aptos_framework::sui_derivable_account {
4141
const EINVALID_PUBLIC_KEY: u64 = 6;
4242
/// Account address mismatch.
4343
const EACCOUNT_ADDRESS_MISMATCH: u64 = 7;
44+
/// Malformed data with trailing bytes.
45+
const EMALFORMED_DATA: u64 = 8;
46+
/// Function is deprecated and should not be called.
47+
const EDEPRECATED: u64 = 9;
4448

4549
enum SuiAbstractSignature has drop {
4650
MessageV1 {
@@ -110,6 +114,7 @@ module aptos_framework::sui_derivable_account {
110114
let stream = bcs_stream::new(*abstract_public_key);
111115
let sui_account_address = bcs_stream::deserialize_vector<u8>(&mut stream, |x| deserialize_u8(x));
112116
let domain = bcs_stream::deserialize_vector<u8>(&mut stream, |x| deserialize_u8(x));
117+
assert!(!bcs_stream::has_remaining(&mut stream), EMALFORMED_DATA);
113118
SuiAbstractPublicKey { sui_account_address, domain }
114119
}
115120

@@ -119,6 +124,7 @@ module aptos_framework::sui_derivable_account {
119124
let signature_type = bcs_stream::deserialize_u8(&mut stream);
120125
if (signature_type == 0x00) {
121126
let signature = bcs_stream::deserialize_vector<u8>(&mut stream, |x| deserialize_u8(x));
127+
assert!(!bcs_stream::has_remaining(&mut stream), EMALFORMED_DATA);
122128
SuiAbstractSignature::MessageV1 { signature }
123129
} else {
124130
abort(EINVALID_SIGNATURE_TYPE)
@@ -189,7 +195,21 @@ module aptos_framework::sui_derivable_account {
189195
pragma verify = false;
190196
}
191197

198+
/// @deprecated This function is deprecated and will always abort.
192199
public fun authenticate_auth_data(
200+
_aa_auth_data: AbstractionAuthData,
201+
_entry_function_name: &vector<u8>
202+
) {
203+
abort(EDEPRECATED)
204+
}
205+
206+
spec authenticate_auth_data_internal {
207+
// TODO: Issue with `cannot appear in both arithmetic and bitwise
208+
// operation`
209+
pragma verify = false;
210+
}
211+
212+
fun authenticate_auth_data_internal(
193213
aa_auth_data: AbstractionAuthData,
194214
entry_function_name: &vector<u8>
195215
) {
@@ -244,13 +264,13 @@ module aptos_framework::sui_derivable_account {
244264
}
245265

246266
spec authenticate {
247-
// TODO: Issue with spec for authenticate_auth_data
267+
// TODO: Issue with spec for authenticate_auth_data_internal
248268
pragma verify = false;
249269
}
250270

251271
/// Authorization function for domain account abstraction.
252272
public fun authenticate(account: signer, aa_auth_data: AbstractionAuthData): signer {
253-
daa_authenticate(account, aa_auth_data, |auth_data, entry_name| authenticate_auth_data(auth_data, entry_name))
273+
daa_authenticate(account, aa_auth_data, |auth_data, entry_name| authenticate_auth_data_internal(auth_data, entry_name))
254274
}
255275

256276
#[test_only]
@@ -322,7 +342,7 @@ module aptos_framework::sui_derivable_account {
322342

323343
let auth_data = create_derivable_auth_data(digest, abstract_signature, abstract_public_key);
324344

325-
authenticate_auth_data(auth_data, &entry_function_name);
345+
authenticate_auth_data_internal(auth_data, &entry_function_name);
326346
}
327347

328348
#[test(framework = @0x1)]
@@ -342,7 +362,7 @@ module aptos_framework::sui_derivable_account {
342362

343363
let auth_data = create_derivable_auth_data(digest, abstract_signature, abstract_public_key);
344364

345-
authenticate_auth_data(auth_data, &entry_function_name);
365+
authenticate_auth_data_internal(auth_data, &entry_function_name);
346366
}
347367

348368
#[test(framework = @0x1)]
@@ -362,7 +382,7 @@ module aptos_framework::sui_derivable_account {
362382

363383
let auth_data = create_derivable_auth_data(digest, abstract_signature, abstract_public_key);
364384

365-
authenticate_auth_data(auth_data, &entry_function_name);
385+
authenticate_auth_data_internal(auth_data, &entry_function_name);
366386
}
367387

368388

@@ -383,7 +403,36 @@ module aptos_framework::sui_derivable_account {
383403

384404
let auth_data = create_derivable_auth_data(digest, abstract_signature, abstract_public_key);
385405

386-
authenticate_auth_data(auth_data, &entry_function_name);
406+
authenticate_auth_data_internal(auth_data, &entry_function_name);
407+
}
408+
409+
#[test]
410+
#[expected_failure(abort_code = EMALFORMED_DATA)]
411+
fun test_deserialize_abstract_signature_with_trailing_bytes() {
412+
let signature_bytes = vector[0, 151, 47, 171, 144, 115, 16, 129, 17, 202, 212, 180, 155, 213, 223, 249, 203, 195, 0, 84, 142, 121, 167, 29, 113, 159, 33, 177, 108, 137, 113, 160, 118, 41, 246, 199, 202, 79, 151, 27, 86, 235, 219, 123, 168, 152, 38, 124, 147, 146, 118, 101, 37, 187, 223, 206, 120, 101, 148, 33, 141, 80, 60, 155, 13, 25, 200, 235, 92, 139, 72, 175, 189, 40, 0, 65, 76, 215, 148, 94, 194, 78, 134, 60, 189, 212, 116, 40, 134, 179, 104, 31, 249, 222, 84, 104, 202];
413+
let abstract_signature = create_raw_signature(signature_bytes);
414+
// Append trailing bytes to simulate griefing attack
415+
abstract_signature.push_back(0xDE);
416+
abstract_signature.push_back(0xAD);
417+
abstract_signature.push_back(0xBE);
418+
abstract_signature.push_back(0xEF);
419+
// This should fail with EMALFORMED_DATA due to trailing bytes
420+
deserialize_abstract_signature(&abstract_signature);
421+
}
422+
423+
#[test]
424+
#[expected_failure(abort_code = EMALFORMED_DATA)]
425+
fun test_deserialize_abstract_public_key_with_trailing_bytes() {
426+
let sui_account_address = b"0x8d6ce7a3c13617b29aaf7ec58bee5a611606a89c62c5efbea32e06d8d167bd49";
427+
let domain = b"localhost:3001";
428+
let abstract_public_key = create_abstract_public_key(sui_account_address, domain);
429+
// Append trailing bytes to simulate griefing attack
430+
abstract_public_key.push_back(0xDE);
431+
abstract_public_key.push_back(0xAD);
432+
abstract_public_key.push_back(0xBE);
433+
abstract_public_key.push_back(0xEF);
434+
// This should fail with EMALFORMED_DATA due to trailing bytes
435+
deserialize_abstract_public_key(&abstract_public_key);
387436
}
388437

389438
}

0 commit comments

Comments
 (0)