Skip to content

Commit

Permalink
update (#264)
Browse files Browse the repository at this point in the history 
<!-- enter the gh issue after hash -->
- [ ] More progress on #247 
- [ ] follows contribution
[guide](https://github.com/keep-starknet-strange/shinigami/blob/main/CONTRIBUTING.md)
- [ ] code change includes tests

<!-- PR description below -->
Fix more issues.
```
Script tests complete!
Passed: 1182    Failed: 25    Total: 1207
```

---------

Co-authored-by: Brandon Roberts <[email protected]>
  • Loading branch information
@bloomingpeach @b-j-roberts
bloomingpeach and b-j-roberts authored Oct 31, 2024
1 parent 3415ed6 commit d509dfe
Show file tree
Hide file tree
Showing 8 changed files with 156 additions and 87 deletions.
4 changes: 4 additions & 0 deletions packages/engine/src/errors.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ pub mod Error {
pub const INVALID_P2MS: felt252 = 'Invalid P2MS transaction';
pub const SCRIPT_UNFINISHED: felt252 = 'Script unfinished';
pub const SCRIPT_ERR_SIG_DER: felt252 = 'Signature DER error';
pub const PUBKEYTYPE: felt252 = 'Unsupported public key type';
pub const SIG_HIGH_S: felt252 = 'Sig not canonical high S value';
pub const SIG_HASHTYPE: felt252 = 'invalid hash type';
pub const INVALID_PUBKEY_LEN: felt252 = 'Invalid public key length';
}

pub fn byte_array_err(err: felt252) -> ByteArray {
Expand Down
38 changes: 24 additions & 14 deletions packages/engine/src/opcodes/crypto.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,10 @@ pub fn opcode_checksig<
let res = BaseSigVerifierTrait::new(ref engine, @full_sig_bytes, @pk_bytes);
if res.is_err() {
let err = res.unwrap_err();
if err == Error::SCRIPT_ERR_SIG_DER || err == Error::WITNESS_PUBKEYTYPE {
if err == Error::SCRIPT_ERR_SIG_DER
|| err == Error::PUBKEYTYPE
|| err == Error::SIG_HASHTYPE
|| err == Error::SIG_HIGH_S {
return Result::Err(err);
};
engine.dstack.push_bool(false);
Expand Down Expand Up @@ -152,6 +155,7 @@ pub fn opcode_checkmultisig<
}

let verify_der = engine.has_flag(ScriptFlags::ScriptVerifyDERSignatures);
let strict_encoding = engine.has_flag(ScriptFlags::ScriptVerifyStrictEncoding);
// Get number of public keys and construct array
let num_keys = engine.dstack.pop_int()?;
let mut num_pub_keys: i64 = ScriptNum::to_int32(num_keys).into();
Expand Down Expand Up @@ -201,7 +205,6 @@ pub fn opcode_checkmultisig<
if err != 0 {
return Result::Err(err);
}

// Historical bug
let dummy = engine.dstack.pop_byte_array()?;

Expand All @@ -211,12 +214,14 @@ pub fn opcode_checkmultisig<

let mut script = engine.sub_script();

let mut s: u32 = 0;
let end = sigs.len();
while s != end {
script = signature::remove_signature(@script, sigs.at(s)).clone();
s += 1;
};
if (engine.is_witness_active(0)) {
let mut s: u32 = 0;
let end = sigs.len();
while s != end {
script = signature::remove_signature(@script, sigs.at(s)).clone();
s += 1;
};
}

let mut success = true;
num_pub_keys += 1; // Offset due to decrementing it in the loop
Expand All @@ -236,6 +241,7 @@ pub fn opcode_checkmultisig<
if sig.len() == 0 {
continue;
}

let res = signature::parse_base_sig_and_pk(ref engine, pub_key, sig);
if res.is_err() {
success = false;
Expand All @@ -254,10 +260,6 @@ pub fn opcode_checkmultisig<
}
};

if err != 0 {
return Result::Err(err);
}

if !success {
if engine.has_flag(ScriptFlags::ScriptVerifyNullFail) {
let mut err = '';
Expand All @@ -270,8 +272,16 @@ pub fn opcode_checkmultisig<
if err != '' {
return Result::Err(err);
}
} else if verify_der {
return Result::Err(Error::SCRIPT_ERR_SIG_DER);
} else if verify_der || strict_encoding {
if err == 'invalid sig fmt: S padding' {
return Result::Err(Error::SCRIPT_ERR_SIG_DER);
} else if err != '' {
if err != Error::INVALID_PUBKEY_LEN {
return Result::Err(err);
}
}
} else if err == Error::SIG_HIGH_S {
return Result::Err(err);
}
}

Expand Down
125 changes: 80 additions & 45 deletions packages/engine/src/signature/signature.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::hash_cache::SigHashMidstateTrait;
use shinigami_utils::byte_array::u256_from_byte_array_with_offset;
use crate::signature::{sighash, constants};
use crate::errors::Error;
use shinigami_utils::byte_array::{sub_byte_array};
use crate::parser;

//`BaseSigVerifier` is used to verify ECDSA signatures encoded in DER or BER format (pre-SegWit sig)
Expand Down Expand Up @@ -154,9 +155,8 @@ pub fn check_hash_type_encoding<
if hash_type > constants::SIG_HASH_ANYONECANPAY {
hash_type -= constants::SIG_HASH_ANYONECANPAY;
}

if hash_type < constants::SIG_HASH_ALL || hash_type > constants::SIG_HASH_SINGLE {
return Result::Err('invalid hash type');
return Result::Err(Error::SIG_HASHTYPE);
}

return Result::Ok(());
Expand Down Expand Up @@ -186,9 +186,15 @@ pub fn check_signature_encoding<
T, I, O, IEngineTransactionInputTrait, IEngineTransactionOutputTrait
>
>(
ref vm: Engine<T>, sig_bytes: @ByteArray, strict_encoding: bool
ref vm: Engine<T>, sig_bytes: @ByteArray
) -> Result<(), felt252> {
// https://github.com/btcsuite/btcd/blob/master/txscript/engine.go#L1221
let low_s = vm.has_flag(ScriptFlags::ScriptVerifyLowS);
let strict_encoding = vm.has_flag(ScriptFlags::ScriptVerifyStrictEncoding);
let verify_der = vm.has_flag(ScriptFlags::ScriptVerifyDERSignatures);
if !low_s && !strict_encoding && !verify_der {
return Result::Ok(());
}

// ASN.1 identifiers for sequence and integer types.*
let asn1_sequence_id: u8 = 0x30;
Expand All @@ -206,24 +212,29 @@ pub fn check_signature_encoding<
if sig_bytes_len == 0 {
return Result::Err('invalid sig fmt: empty sig');
}

// Calculate the actual length of the signature, excluding the hash type.
let sig_len = sig_bytes_len - constants::HASH_TYPE_LEN;
// Check if the signature is too short.
if sig_len < constants::MIN_SIG_LEN {
return Result::Err('invalid sig fmt: too short');
}

// Check if the signature is too long.
if sig_len > constants::MAX_SIG_LEN {
return Result::Err('invalid sig fmt: too long');
}

// Ensure the signature starts with the correct ASN.1 sequence identifier.
if sig_bytes[sequence_offset] != asn1_sequence_id {
return Result::Err('invalid sig fmt: wrong type');
}

// Verify that the length field matches the expected length.
if sig_bytes[data_len_offset] != (sig_len - data_offset).try_into().unwrap() {
return Result::Err('invalid sig fmt: bad length');
}

// Determine the length of the `R` value in the signature.
let r_len: usize = sig_bytes[r_len_offset].into();
let s_type_offset = r_offset + r_len;
Expand All @@ -232,6 +243,7 @@ pub fn check_signature_encoding<
if s_type_offset > sig_len {
return Result::Err('invalid sig fmt: S type missing');
}

// Check if the `S` length offset exceeds the length of the signature.
if s_len_offset > sig_len {
return Result::Err('invalid sig fmt: miss S length');
Expand All @@ -247,16 +259,14 @@ pub fn check_signature_encoding<
if r_len <= 0 || r_len > sig_len - r_offset - 3 {
return Result::Err('invalid sig fmt:R length');
}
// If strict encoding is enforced, check for negative or excessively padded `R` values.
if strict_encoding {
if sig_bytes[r_offset] & 0x80 != 0 {
return Result::Err('invalid sig fmt: negative R');
}
if sig_bytes[r_offset] & 0x80 != 0 {
return Result::Err('invalid sig fmt: negative R');
}

if r_len > 1 && sig_bytes[r_offset] == 0 && sig_bytes[r_offset + 1] & 0x80 == 0 {
return Result::Err('invalid sig fmt: R padding');
}
if r_len > 1 && sig_bytes[r_offset] == 0 && sig_bytes[r_offset + 1] & 0x80 == 0 {
return Result::Err('invalid sig fmt: R padding');
}

// Ensure the `S` value is correctly identified as an ASN.1 integer.
if sig_bytes[s_type_offset] != asn1_integer_id {
return Result::Err('invalid sig fmt:S ASN.1');
Expand All @@ -265,28 +275,25 @@ pub fn check_signature_encoding<
if s_len <= 0 || s_len > sig_len - s_offset {
return Result::Err('invalid sig fmt:S length');
}
// If strict encoding is enforced, check for negative or excessively padded `S` values.
if strict_encoding {
if sig_bytes[s_offset] & 0x80 != 0 {
return Result::Err('invalid sig fmt: negative S');
}

if s_len > 1 && sig_bytes[s_offset] == 0 && sig_bytes[s_offset + 1] & 0x80 == 0 {
return Result::Err('invalid sig fmt: S padding');
}
if sig_bytes[s_offset] & 0x80 != 0 {
return Result::Err('invalid sig fmt: negative S');
}
if s_len > 1 && sig_bytes[s_offset] == 0 && sig_bytes[s_offset + 1] & 0x80 == 0 {
return Result::Err('invalid sig fmt: S padding');
}

// If the "low S" rule is enforced, check that the `S` value is below the threshold.
if low_s {
let s_value = u256_from_byte_array_with_offset(sig_bytes, s_offset, 32);
let s_value = u256_from_byte_array_with_offset(sig_bytes, s_offset, s_len);
let mut half_order = Secp256Trait::<Secp256k1Point>::get_curve_size();

let (half_order_high_upper, half_order_high_lower) = DivRem::div_rem(half_order.high, 2);
let carry = half_order_high_lower;
half_order.low = (half_order.low / 2) + (carry * (constants::MAX_U128 / 2 + 1));
half_order.high = half_order_high_upper;

if s_value > half_order {
return Result::Err('sig not canonical high S value');
return Result::Err(Error::SIG_HIGH_S);
}
}

Expand Down Expand Up @@ -342,7 +349,7 @@ pub fn check_pub_key_encoding<
}

if !is_supported_pub_key_type(pk_bytes) {
return Result::Err('unsupported public key type');
return Result::Err(Error::PUBKEYTYPE);
}

return Result::Ok(());
Expand Down Expand Up @@ -375,7 +382,7 @@ pub fn parse_pub_key(pk_bytes: @ByteArray) -> Result<Secp256k1Point, felt252> {
} else {
// Extract X coordinate and determine parity from last byte.
if pk_bytes_uncompressed.len() != 65 {
return Result::Err('Invalid public key length');
return Result::Err(Error::INVALID_PUBKEY_LEN);
}
let pub_key: u256 = u256_from_byte_array_with_offset(@pk_bytes_uncompressed, 1, 32);
let parity = !(pk_bytes_uncompressed[64] & 1 == 0);
Expand Down Expand Up @@ -403,13 +410,33 @@ pub fn parse_schnorr_pub_key(pk_bytes: @ByteArray) -> Result<Secp256k1Point, fel
// This function extracts the `r` and `s` values from a DER-encoded ECDSA signature (`sig_bytes`).
// The function performs various checks to ensure the integrity and validity of the signature.
pub fn parse_signature(sig_bytes: @ByteArray) -> Result<Signature, felt252> {
let mut sig_len: usize = sig_bytes.len() - constants::HASH_TYPE_LEN;
if sig_bytes[0] != 0x30 {
return Result::Err('invalid sig fmt: no header');
}
let mut sig_len: usize = sig_bytes[1].into();
if sig_len + 2 > sig_bytes.len() || sig_len + 2 < constants::MIN_SIG_LEN {
return Result::Err('invalid sig fmt: bad length');
}

let mut start = 0;
let sig_bytes = @sub_byte_array(sig_bytes, ref start, sig_len + 2);
if sig_bytes[2] != 0x02 {
return Result::Err('invalid sig fmt: no 1st marker');
}

let mut r_len: usize = sig_bytes[3].into();
if r_len <= 0 || r_len > sig_len - 5 {
return Result::Err('invalid sig fmt: bogus R length');
}

let mut s_len: usize = sig_bytes[r_len + 5].into();
if s_len <= 0 || s_len > sig_len - r_len - 4 {
return Result::Err('invalid sig fmt: bogus S length');
}

let mut r_offset = 4;
let mut s_offset = 6 + r_len;
let order: u256 = Secp256Trait::<Secp256k1Point>::get_curve_size();

let mut i = 0;

//Strip leading zero
Expand Down Expand Up @@ -451,9 +478,11 @@ pub fn parse_signature(sig_bytes: @ByteArray) -> Result<Signature, felt252> {
if s_sig == 0 {
return Result::Err('invalid sig: S is zero');
}
if sig_len != r_len + s_len + 6 {

if sig_bytes.len() != sig_bytes[3].into() + sig_bytes[sig_bytes[3].into() + 5].into() + 6 {
return Result::Err('invalid sig: bad final length');
}

return Result::Ok(Signature { r: r_sig, s: s_sig, y_parity: false, });
}

Expand Down Expand Up @@ -497,27 +526,32 @@ pub fn parse_base_sig_and_pk<
ref vm: Engine<T>, pk_bytes: @ByteArray, sig_bytes: @ByteArray
) -> Result<(Secp256k1Point, Signature, u32), felt252> {
let verify_der = vm.has_flag(ScriptFlags::ScriptVerifyDERSignatures);
let strict_encoding = vm.has_flag(ScriptFlags::ScriptVerifyStrictEncoding) || verify_der;
let verify_strict_encoding = vm.has_flag(ScriptFlags::ScriptVerifyStrictEncoding);
let strict_encoding = verify_strict_encoding || verify_der;

if sig_bytes.len() == 0 {
return if strict_encoding {
Result::Err(Error::SCRIPT_ERR_SIG_DER)
} else {
Result::Err('empty signature')
};
}

// TODO: strct encoding
let hash_type_offset: usize = sig_bytes.len() - 1;
let hash_type: u32 = sig_bytes[hash_type_offset].into();
if let Result::Err(e) = check_hash_type_encoding(ref vm, hash_type) {
return if verify_der {
Result::Err(Error::SCRIPT_ERR_SIG_DER)
} else {
Result::Err(e)
};

if strict_encoding {
if let Result::Err(e) = check_hash_type_encoding(ref vm, hash_type) {
return if verify_der {
Result::Err(Error::SCRIPT_ERR_SIG_DER)
} else {
Result::Err(e)
};
}
}
if let Result::Err(e) = check_signature_encoding(ref vm, sig_bytes, strict_encoding) {
return if verify_der {

if let Result::Err(e) = check_signature_encoding(ref vm, sig_bytes) {
return if strict_encoding {
Result::Err(Error::SCRIPT_ERR_SIG_DER)
} else {
Result::Err(e)
Expand All @@ -532,15 +566,12 @@ pub fn parse_base_sig_and_pk<
};
}

let pub_key = match parse_pub_key(pk_bytes) {
Result::Ok(key) => key,
Result::Err(e) => if verify_der {
return Result::Err(Error::SCRIPT_ERR_SIG_DER);
} else {
return Result::Err(e);
},
};
let mut start = 0;
if hash_type_offset < 1 {
return Result::Err('invalid hash offset');
}

let sig_bytes = @sub_byte_array(sig_bytes, ref start, hash_type_offset);
let sig = match parse_signature(sig_bytes) {
Result::Ok(signature) => signature,
Result::Err(e) => if verify_der {
Expand All @@ -550,6 +581,10 @@ pub fn parse_base_sig_and_pk<
},
};

let pub_key = match parse_pub_key(pk_bytes) {
Result::Ok(key) => key,
Result::Err(e) => { return Result::Err(e); }
};
Result::Ok((pub_key, sig, hash_type))
}

Expand Down
Loading

0 comments on commit d509dfe

Please sign in to comment.