TON signing: include address, timestamp & hash#991
Conversation
Refactor TON sign flow to include address and timestamp in the sign-data hash and responses. Added gem_hash dependency and sha256-based sign-data construction (SIGN_DATA_PREFIX + workchain + hash + domain + timestamp + type + payload). TonSignMessageData now carries address and provides build_sign_data_hash; TonSignDataPayload gained encode_for_signing. sign_personal now returns TonSignResult { signature, public_key, timestamp } and callers (chain_signer, gemstone) updated accordingly. Added Address::from_base64_url and base64_to_hex_address helpers and adjusted WalletConnect/validator parsing to include the message "from" address. Tests updated to reflect the new behavior.
|
Review changes with Changed Files
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the TON signing mechanism by embedding crucial contextual information like the sender's address and the signing timestamp directly into the data that gets hashed and signed. This change enhances the integrity and non-repudiation of TON transactions and messages, making them more secure and auditable. The refactoring touches core signing logic, data structures, and integrates new hashing capabilities, ensuring that all components correctly handle the enriched signing data. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the TON signing process to enhance security by incorporating the sender's address and a timestamp into the signed data, and correctly fixes the workchain sign-extension for masterchain addresses. However, a critical security vulnerability has been identified: the new TON address parsing logic in Address::from_base64_url lacks CRC16 checksum validation, which could lead to processing malformed addresses or typos, potentially resulting in unintended operations or loss of funds. Additionally, a critical issue exists in gemstone/src/message/signer.rs regarding potential timestamp mismatches between hash preview and the actual signed hash, along with medium-severity issues related to error handling of system time and ambiguity in base64 decoding.
| SignDigestType::TonPersonal => { | ||
| let (signature, public_key) = ton_sign_personal(&self.message.data, &private_key)?; | ||
| self.get_ton_result(&signature, &public_key) | ||
| let result = ton_sign_personal(&self.message.data, &private_key)?; | ||
| self.get_ton_result(&result) | ||
| } |
There was a problem hiding this comment.
There's a critical issue with how the hash for TON signing is handled. The sign method calls self.hash()? at the beginning, which for TonPersonal computes a hash with a timestamp. However, this branch for TonPersonal ignores that computed hash and instead calls ton_sign_personal. This function re-parses the message and computes a new hash with a new timestamp.
This means the hash computed by self.hash() (e.g., for a preview) will not match the hash that is actually signed. This violates the what-you-see-is-what-you-sign principle.
To fix this, the TonPersonal branch should use the hash variable computed at the start of the sign method, just like other signing types. This will require a refactoring to ensure the timestamp is consistent between hash() and sign().
| pub fn from_base64_url(base64: &str) -> Result<Self, ParseError> { | ||
| use base64::prelude::BASE64_STANDARD_NO_PAD; | ||
|
|
||
| let bytes = BASE64_URL_SAFE_NO_PAD | ||
| .decode(base64) | ||
| .or_else(|_| BASE64_STANDARD_NO_PAD.decode(base64)) | ||
| .map_err(|_| ParseError("Invalid base64".to_string()))?; | ||
|
|
||
| if bytes.len() != 36 { | ||
| return Err(ParseError("Invalid base64 address length".to_string())); | ||
| } | ||
|
|
||
| let workchain = bytes[1] as i8 as i32; | ||
| let hash_part: HashPart = bytes[2..34].try_into().map_err(|_| ParseError("Invalid hash length".to_string()))?; | ||
|
|
||
| Ok(Self { workchain, hash_part }) | ||
| } |
There was a problem hiding this comment.
The Address::from_base64_url function decodes a base64-encoded TON address but critically fails to validate the 2-byte CRC checksum, which is a standard part of the 36-byte TON user-friendly address format. This omission allows the application to accept malformed addresses or addresses with typos, potentially leading to signing messages for unintended accounts or displaying incorrect information. Beyond this, the function's name from_base64_url suggests it only handles URL-safe base64, but it falls back to decoding standard base64 if URL-safe decoding fails. This behavior, while robust, might obscure issues with data sources expected to provide URL-safe base64. If both formats are intentionally supported, this should be clearly documented; otherwise, the fallback should be removed to enforce the correct format. Remediation: Implement CRC16-CCITT validation to verify that the last 2 bytes of the decoded 36-byte array match the CRC16-CCITT checksum of the first 34 bytes.
| pub fn sign_personal(data: &[u8], private_key: &[u8]) -> Result<TonSignResult, SignerError> { | ||
| let ton_data = TonSignMessageData::from_bytes(data)?; | ||
| let digest = ton_data.payload.hash(); | ||
| let timestamp = SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0); |
There was a problem hiding this comment.
Using unwrap_or(0) can hide issues with the system clock. If the clock is set to a time before the UNIX epoch, duration_since will return an error, and this code will produce a timestamp of 0. This could lead to unexpected behavior or signature rejections by servers that validate the timestamp. It would be more robust to handle the error explicitly and return a SignerError.
| let timestamp = SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0); | |
| let timestamp = SystemTime::now().duration_since(UNIX_EPOCH).map_err(|e| SignerError::InvalidInput(e.to_string()))?.as_secs(); |
gemstone/src/message/signer.rs
Outdated
| let string = String::from_utf8(self.message.data.clone())?; | ||
| let ton_data = TonSignMessageData::from_bytes(string.as_bytes())?; | ||
| Ok(ton_data.payload.hash()) | ||
| let timestamp = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0); |
There was a problem hiding this comment.
Using unwrap_or(0) can hide issues with the system clock. If the clock is set to a time before the UNIX epoch, duration_since will return an error, and this code will produce a timestamp of 0. This could lead to unexpected behavior or signature rejections by servers that validate the timestamp. It would be more robust to handle the error explicitly and return a GemstoneError.
| let timestamp = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0); | |
| let timestamp = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map_err(|e| GemstoneError::from(e.to_string()))?.as_secs(); |
gemstone/src/message/signer.rs
Outdated
| let ton_data = TonSignMessageData::from_bytes(string.as_bytes())?; | ||
| Ok(ton_data.payload.hash()) | ||
| let timestamp = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0); | ||
| Ok(ton_data.build_sign_data_hash(timestamp)?) |
There was a problem hiding this comment.
will this timestamp be used in the end?
There was a problem hiding this comment.
yes, the timestamp is required by TON sign data spec
Introduce explicit timestamp support for TON personal signatures and refactor related APIs. sign_personal now accepts a timestamp and TonSignMessageData::build_sign_data_hash/encode_for_signing were renamed to hash/encode respectively. The timestamp is produced in ChainSigner and MessageSigner (via a new current_timestamp helper) and threaded through signing calls. Address base64 decoding now falls back to standard no-pad, and tests were updated to assert deterministic signature/public key outputs.
gemstone/src/message/signer.rs
Outdated
| impl MessageSigner { | ||
| fn get_ton_result(&self, result: &TonSignResult) -> Result<String, GemstoneError> { | ||
| let string = String::from_utf8(self.message.data.clone())?; | ||
| let ton_data = TonSignMessageData::from_bytes(string.as_bytes())?; |
There was a problem hiding this comment.
| let ton_data = TonSignMessageData::from_bytes(string.as_bytes())?; | |
| let data = TonSignMessageData::from_bytes(string.as_bytes())?; |
| fn sign_message(&self, message: &[u8], private_key: &[u8]) -> Result<String, SignerError> { | ||
| let (signature, _public_key) = sign_personal(message, private_key)?; | ||
| Ok(base64::Engine::encode(&base64::engine::general_purpose::STANDARD, signature)) | ||
| let timestamp = SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0); |
There was a problem hiding this comment.
| let timestamp = SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0); | |
| let timestamp = SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_secs())?; |
never unwrap for such cases.
crates/gem_ton/src/signer/types.rs
Outdated
| mod tests { | ||
| use super::*; | ||
|
|
||
| const TEST_ADDRESS: &str = "UQBY1cVPu4SIr36q0M3HWcqPb_efyVVRBsEzmwN-wKQDR6zg"; |
There was a problem hiding this comment.
should be added to testkit.rs, see other examples
3 participants
WalletConnect updated TON signData verification https://github.com/reown-com/web-examples/pull/1006/changes to validate the structured hash per TON Connect spec instead of accepting raw bytes. Without this change, TON personal_sign requests via WalletConnect fail validation on dApp side.
Changes
payload)