fix:Correct ECDSA signature malleability handling

[jlguochn]

Description

The function check_s_lt_order_half is intended to ensure that the value of S in an ECDSA signature is less than the order of the Secp256r1 curve divided by 2 to eliminate signature malleability. However, there's a boundary condition issue in the function's return logic.

At the end of the function, the return value is set to false when S == ORDER_HALF. This decision seems incorrect. To eliminate ECDSA signature malleability, S is required to be less than n/2, where n is the curve order. Since the curve's order is a prime number and cannot be divided by 2 evenly, ORDER_HALF effectively represents the floor(n/2). Therefore, ORDER_HALF should be considered a valid S value, and its corresponding invalid S would be ORDER_HALF + 1. This boundary value consideration aligns with the approach taken in BIP62(despite the curve and its order being different, both are prime numbers).

If ORDER_HALF is treated as an invalid S value, then computing n - S tries to convert it to a valid S, resulting in ORDER_HALF + 1, which remains an invalid S.

This issue could potentially render signatures invalid when they should be considered valid, impacting the reliability of the system. Although the probability of S exactly equaling ORDER_HALF in a signature is very low, ORDER_HALF should not be considered a invalid S value. If the repository is referenced by other developers in specific business scenarios, it could pose a potential security risk.

Type of Change

• Bug fix

Which Components or Systems Does This Change Impact?

• Validator Node
• Full Node (API, Indexer, etc.)
• Move/Aptos Virtual Machine
• Aptos Framework
• Aptos CLI/SDK
• Developer Infrastructure
• Other (specify)

How Has This Been Tested?

We can verify using the following Python code that half_order and half_order + 1 form a pair of associated malleable S values. Among them, half_order is considered a valid S value, while half_order + 1 is an invalid S value.

order = 0xFFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551
print("order:",hex(order))
print("order is odd:",order%2 == 1)

helf_order = hex(order>>1)
print("helf_order:",helf_order)

s = 0x7fffffff800000007fffffffffffffffde737d56d38bcf4279dce5617e3192a8
print("s:",hex(s))
print("is equal:",helf_order == hex(s))

new_s = hex(order - s)

print("new_s:",new_s)

print("is low s values:",new_s<=helf_order)

Key Areas to Review

Checklist

• I have read and followed the CONTRIBUTING doc
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I identified and added all stakeholders and component owners affected by this change as reviewers
• I tested both happy and unhappy path of the functionality
• I have made corresponding changes to the documentation

[trunk-io]

⏱️ 9h 38m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
windows-build	2h 10m	🟩 🟩 🟩
execution-performance / single-node-performance	1h 20m	🟩 🟥 🟩 🟩
rust-smoke-tests	1h 8m	🟩 🟩
rust-targeted-unit-tests	51m	⬜ 🟥 🟩
forge-framework-upgrade-test / forge	33m	🟩 🟩
rust-move-tests	32m	⬜ ⬜ 🟩 ⬜ 🟩
rust-images / rust-all	31m	🟩 🟩
forge-e2e-test / forge	28m	🟩 🟩
forge-compat-test / forge	23m	🟩 🟩
rust-lints	16m	⬜ 🟩 🟩
run-tests-main-branch	16m	⬜ 🟩 🟩 🟩
cli-e2e-tests / run-cli-tests	14m	🟩 🟩
execution-performance / test-target-determinator	11m	🟩 🟩 🟩
rust-build-cached-packages	10m	🟩 🟩
check-dynamic-deps	8m	🟩 🟩 🟩 🟩 🟩
check	8m	🟩 ⬜ 🟩
test-target-determinator	7m	🟩 ⬜ 🟩
general-lints	5m	🟩 🟩 🟩
node-api-compatibility-tests / node-api-compatibility-tests	2m	🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩
file_change_determinator	54s	⬜ 🟩 🟩 ⬜ 🟩
file_change_determinator	47s	🟩 🟩 🟩 🟩
file_change_determinator	28s	🟩 ⬜ 🟩
permission-check	27s	🟥 🟥 🟥 🟩 🟩 (+1 more)
permission-check	19s	🟥 🟥 🟥 🟩 🟩 (+1 more)
permission-check	19s	🟥 🟥 🟥 🟩 🟩 (+1 more)
permission-check	17s	🟥 🟥 🟥 🟩 🟩 (+1 more)
determine-docker-build-metadata	8s	🟩 🟩 🟩
permission-check	5s	🟩 🟩

🚨 2 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
rust-move-tests	16m	9m
rust-targeted-unit-tests	26m	19m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

[alinush]

Can you rename this to check_s_le_order_half?

[jlguochn]

I have renamed the function to check_s_le_order_half as requested. The changes have been committed under hash f46e76c21551f17ca7b0a04bd473b50e7d6c11e8.

[mstraka100]

Looks correct to me

[alinush]

Makes sense. If the order $q$ were 3, then right now, we haveORDER_HALF = (3-1)/2 = 1.

When we get a signature s = ORDER_HALF = 1 it would fail the malleability check and we would change it to $q-s = 3-1=2$ instead of leaving it at 1.

On the other hand, if we got a signature s = 2, it would also fail the malleability check and we would change it to $q-s = 3-2 = 1$ 😆

[jlguochn]

Makes sense. If the order q were 3, then right now, we haveORDER_HALF = (3-1)/2 = 1.

When we get a signature s = ORDER_HALF = 1 it would fail the malleability check and we would change it to q−s=3−1=2 instead of leaving it at 1.

On the other hand, if we got a signature s = 2, it would also fail the malleability check and we would change it to q−s=3−2=1 😆

It is indeed the case. Fortunately, the function make_canonical does not perform check_s_malleability recursively. Otherwise, there would be a risk of a DoS attack.

[github-actions]

✅ Forge suite compat success on a68e71c05caebf01504d4499110f3fba213fb53d ==> 927cb18932e9bcc3943d6182ae6a8c8175023e37

Compatibility test results for a68e71c05caebf01504d4499110f3fba213fb53d ==> 927cb18932e9bcc3943d6182ae6a8c8175023e37 (PR)
1. Check liveness of validators at old version: a68e71c05caebf01504d4499110f3fba213fb53d
compatibility::simple-validator-upgrade::liveness-check : committed: 9870.321237229045 txn/s, latency: 3346.453936245157 ms, (p50: 2400 ms, p90: 6000 ms, p99: 23200 ms), latency samples: 340680
2. Upgrading first Validator to new version: 927cb18932e9bcc3943d6182ae6a8c8175023e37
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3341.0051011307364 txn/s, latency: 9236.856529529816 ms, (p50: 9100 ms, p90: 14100 ms, p99: 14400 ms), latency samples: 139520
3. Upgrading rest of first batch to new version: 927cb18932e9bcc3943d6182ae6a8c8175023e37
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3350.418317080119 txn/s, latency: 9275.521761732853 ms, (p50: 9400 ms, p90: 14100 ms, p99: 14400 ms), latency samples: 138500
4. upgrading second batch to new version: 927cb18932e9bcc3943d6182ae6a8c8175023e37
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 4507.286268211745 txn/s, latency: 6272.473041787265 ms, (p50: 4800 ms, p90: 10900 ms, p99: 23800 ms), latency samples: 221120
5. check swarm health
Compatibility test for a68e71c05caebf01504d4499110f3fba213fb53d ==> 927cb18932e9bcc3943d6182ae6a8c8175023e37 passed
Test Ok

• Grafana dashboard
• Humio Logs
• Axiom Logs
• Validator CPU Profile
• Fullnode CPU Profile
• Test runner output
• Test run is land-blocking

[github-actions]

✅ Forge suite realistic_env_max_load success on 927cb18932e9bcc3943d6182ae6a8c8175023e37

two traffics test: inner traffic : committed: 8599.74438361594 txn/s, latency: 4558.777588592906 ms, (p50: 4500 ms, p90: 5400 ms, p99: 10200 ms), latency samples: 3714180
two traffics test : committed: 100.02066004557165 txn/s, latency: 2011.3181818181818 ms, (p50: 2000 ms, p90: 2200 ms, p99: 2700 ms), latency samples: 1760
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.215, avg: 0.206", "QsPosToProposal: max: 0.209, avg: 0.192", "ConsensusProposalToOrdered: max: 0.432, avg: 0.372", "ConsensusOrderedToCommit: max: 0.363, avg: 0.349", "ConsensusProposalToCommit: max: 0.732, avg: 0.721"]
Max round gap was 1 [limit 4] at version 1758776. Max no progress secs was 4.9458227 [limit 15] at version 1758776.
Test Ok

• Grafana dashboard
• Humio Logs
• Axiom Logs
• Validator CPU Profile
• Fullnode CPU Profile
• Test runner output
• Test run is land-blocking

[github-actions]

✅ Forge suite framework_upgrade success on a68e71c05caebf01504d4499110f3fba213fb53d ==> 927cb18932e9bcc3943d6182ae6a8c8175023e37

Compatibility test results for a68e71c05caebf01504d4499110f3fba213fb53d ==> 927cb18932e9bcc3943d6182ae6a8c8175023e37 (PR)
Upgrade the nodes to version: 927cb18932e9bcc3943d6182ae6a8c8175023e37
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1134.1653751551635 txn/s, submitted: 1138.1148773321402 txn/s, failed submission: 3.9495021769767735 txn/s, expired: 3.9495021769767735 txn/s, latency: 2741.9545173147612 ms, (p50: 2100 ms, p90: 4800 ms, p99: 9000 ms), latency samples: 103380
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1055.2898411359754 txn/s, submitted: 1057.6235205723663 txn/s, failed submission: 2.333679436390923 txn/s, expired: 2.333679436390923 txn/s, latency: 2953.316828836798 ms, (p50: 2100 ms, p90: 5700 ms, p99: 8400 ms), latency samples: 90440
5. check swarm health
Compatibility test for a68e71c05caebf01504d4499110f3fba213fb53d ==> 927cb18932e9bcc3943d6182ae6a8c8175023e37 passed
Upgrade the remaining nodes to version: 927cb18932e9bcc3943d6182ae6a8c8175023e37
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1132.016745344084 txn/s, submitted: 1134.4530952166697 txn/s, failed submission: 2.436349872585585 txn/s, expired: 2.436349872585585 txn/s, latency: 2664.9781843083547 ms, (p50: 2100 ms, p90: 4500 ms, p99: 9000 ms), latency samples: 102220
Test Ok

• Grafana dashboard
• Humio Logs
• Axiom Logs
• Validator CPU Profile
• Fullnode CPU Profile
• Test runner output
• Test run is land-blocking