Skip to content

Commit

Permalink
feat: apply some code cleanup and fixes (#122)
Browse files Browse the repository at this point in the history
* fix: remove FIXMEs

* fix: remove commented code

* feat: add check over rx buffer

* fix: add check fow overflow on chacha code

* feat: bump version

* fix: add missing include

* feat: update snapshots
  • Loading branch information
emmanuelm41 authored Oct 8, 2024
1 parent 8992a45 commit f7f1cc3
Show file tree
Hide file tree
Showing 18 changed files with 23 additions and 76 deletions.
2 changes: 1 addition & 1 deletion app/Makefile.version
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ APPVERSION_M=4
# This is the minor version
APPVERSION_N=2
# This is the patch version
APPVERSION_P=0
APPVERSION_P=1
1 change: 0 additions & 1 deletion app/rust/src/bolos/blake2b.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,6 @@ pub fn blake2s_diversification(tag: &[u8]) -> [u8; 32] {
pub const GH_FIRST_BLOCK: &[u8; 64] =
b"096b36a5804bfacef1691e173c366a47ff5ba84a44f26ddd7e8d9f79d5b42df0";

// FIXME: not using bolos blake!?
let h = Blake2sParams::new()
.hash_length(32)
.personal(KEY_DIVERSIFICATION_PERSONALIZATION)
Expand Down
60 changes: 0 additions & 60 deletions app/rust/src/refactor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,63 +9,3 @@ pub extern "C" fn blake2b_prf(input_ptr: *const [u8; 128], out_ptr: *mut [u8; 32
let output = unsafe { &mut *out_ptr }; //ovk, cv, cmu, epk
output.copy_from_slice(&hash);
}

// #[inline(never)]
// pub fn prf_sessionkey(data: &[u8]) -> [u8; 32] {
// crate::bolos::heartbeat();
// blake2b32_with_personalization(PRF_SESSION_PERSONALIZATION, &data)
// }

// pub fn generate_esk() -> [u8; 32] {
// let mut buffer = [0u8; 64];
// Trng.fill_bytes(&mut buffer);
// let esk = Fr::from_bytes_wide(&buffer);
// esk.to_bytes()
// }

//epk
// pub fn derive_public(esk: &[u8; 32], g_d: &[u8; 32]) -> [u8; 32] {
// let p = AffinePoint::from_bytes(*g_d).unwrap();
// let q = p.to_niels().multiply_bits(esk);
// let t = AffinePoint::from(q);
// t.to_bytes()
// }

// pub fn prf_ock(ovk: &[u8; 32], cv: &[u8; 32], cmu: &[u8; 32], epk: &[u8; 32]) -> [u8; 32] {
// let mut ock_input = [0u8; 128];
// ock_input[0..32].copy_from_slice(ovk);
// ock_input[32..64].copy_from_slice(cv);
// ock_input[64..96].copy_from_slice(cmu);
// ock_input[96..128].copy_from_slice(epk);
// crate::bolos::heartbeat();
// blake2b32_with_personalization(PRF_OCK_PERSONALIZATION, &ock_input)
// }

// #[no_mangle]
// pub extern "C" fn pubkey_gen(scalar_ptr: *const [u8; 32], output_ptr: *mut [u8; 32]) {
// let scalar = unsafe { &*scalar_ptr };
// let output = unsafe { &mut *output_ptr };
// let v = constants::SESSION_KEY_BASE.multiply_bits(scalar);
// output.copy_from_slice(&extended_to_bytes(&v));
// }

// #[no_mangle]
// pub extern "C" fn sessionkey_agree(
// scalar_ptr: *const [u8; 32],
// point_ptr: *const [u8; 32],
// output_ptr: *mut [u8; 32],
// ) {
// let scalar = unsafe { &*scalar_ptr }; //ovk, cv, cmu, epk
// let point = unsafe { &*point_ptr };
//
// let epk = sapling_ka_agree(scalar, point);
// let sessionkey = prf_sessionkey(&epk);
//
// let output = unsafe { &mut *output_ptr }; //ovk, cv, cmu, epk
// output[0..32].copy_from_slice(&sessionkey);
// }
// pub fn verify_bindingsig_keys(rcmsum: &[u8; 32], valuecommitsum: &[u8; 32]) -> bool {
// let v = bytes_to_extended(*valuecommitsum);
// let r = VALUE_COMMITMENT_RANDOM_BASE.multiply_bits(rcmsum);
// v == r
// }
2 changes: 0 additions & 2 deletions app/rust/src/sapling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ pub fn sapling_asknsk_to_ivk(ask: &AskBytes, nsk: &NskBytes) -> IvkBytes {
let ak = sapling_ask_to_ak(ask);
let nk = sapling_nsk_to_nk(nsk);

// FIXME: not using bolos blake!?
let h = Blake2sParams::new()
.hash_length(32)
.personal(CRH_IVK_PERSONALIZATION)
Expand All @@ -42,7 +41,6 @@ pub fn sapling_asknsk_to_ivk(ask: &AskBytes, nsk: &NskBytes) -> IvkBytes {

#[inline(never)]
pub fn sapling_aknk_to_ivk(ak: &AkBytes, nk: &NkBytes) -> IvkBytes {
// FIXME: not using bolos blake!?
let h = Blake2sParams::new()
.hash_length(32)
.personal(CRH_IVK_PERSONALIZATION)
Expand Down
4 changes: 1 addition & 3 deletions app/rust/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ pub fn diversifier_zero() -> Diversifier {
[0u8; 11]
}

// FIXME: This is not good design. Mayeb something like
pub type DiversifierList4 = [u8; 44];
pub type DiversifierList10 = [u8; 110];

Expand All @@ -33,7 +32,7 @@ pub type DkBytes = [u8; 32];
pub type NfBytes = [u8; 32];

// This can be between 32 and 252 bytes
// FIXME: move to 64 to align with ed25519 private key?
// TODO: move to 64 to align with ed25519 private key?
pub type Zip32Seed = [u8; 32];

pub type Zip32Path = [u32];
Expand Down Expand Up @@ -105,7 +104,6 @@ create_ztruct! {
pub diversifier: Diversifier,
pub value: [u8; 8],
pub rcm: DkBytes,
// FIXME: why an additional byte?
pub memotype: u8
}
}
1 change: 0 additions & 1 deletion app/rust/src/zip32.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,6 @@ fn zip32_sapling_derive_child(
// NOTE: WARNING: CURRENTLY COMPUTING NON-HARDENED PATHS DO NOT FIT IN MEMORY
LittleEndian::write_u32(&mut le_i, c);

// FIXME: Duplicated work?
let s_k = &ik.spending_key();

let ask = zip32_sapling_ask_m(s_k);
Expand Down
13 changes: 7 additions & 6 deletions app/src/crypto.c
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,7 @@ zxerr_t crypto_checkspend_sapling(
}

// NOTE: This use is probably correct
io_seproxyhal_io_heartbeat();
io_seproxyhal_io_heartbeat();
compute_nullifier(tmp_buf->ncm_full, notepos, tmp.step4.nsk, tmp_buf->nf);
if (MEMCMP(tmp_buf->nf, start_spenddata + INDEX_SPEND_NF + i * SPEND_TX_LEN, NULLIFIER_SIZE) != 0) {
CHECK_ZXERROR_AND_CLEAN(zxerr_unknown)
Expand Down Expand Up @@ -952,8 +952,8 @@ zxerr_t crypto_checkencryptions_sapling(uint8_t *buffer, uint16_t bufferLen, con
MEMZERO(tmp->step2.chachanonce, CHACHA_NONCE_SIZE);
// encrypt the previously obtained encoding, and store it in
// step2.compactoutput (reusing the same memory for input and output)
chacha(tmp->step2.compactout, tmp->step2.compactout, COMPACT_OUT_SIZE, tmp->step2.sharedkey, tmp->step2.chachanonce,
1);
CHECK_ZXERR(chacha(tmp->step2.compactout, tmp->step2.compactout, COMPACT_OUT_SIZE, tmp->step2.sharedkey,
tmp->step2.chachanonce, 1));
io_seproxyhal_io_heartbeat();
CHECK_APP_CANARY()
// check that the computed encryption is the same as that provided in the
Expand Down Expand Up @@ -985,7 +985,8 @@ zxerr_t crypto_checkencryptions_sapling(uint8_t *buffer, uint16_t bufferLen, con
// tmp->step6.encciph = tmp->step5.pkd || tmp->step5.esk
// encrypt that, using as encryption key the output of the blake2b PRF
// store resulting ciphertext in tmp->step6.encciph
chacha(tmp->step6.encciph, tmp->step6.encciph, ENC_CIPHER_SIZE, tmp->step6.outkey, tmp->step6.chachanonce, 1);
CHECK_ZXERR(chacha(tmp->step6.encciph, tmp->step6.encciph, ENC_CIPHER_SIZE, tmp->step6.outkey,
tmp->step6.chachanonce, 1));
CHECK_APP_CANARY()

// check that the computed encryption is the same as that provided in the
Expand All @@ -1012,8 +1013,8 @@ zxerr_t crypto_checkencryptions_sapling(uint8_t *buffer, uint16_t bufferLen, con
// tmp->step4b.encciph = tmp->step3b.encciph_part1 ||
// tmp->step3b.encciph_part2 encrypt and compare computed encryption to
// that provided in the transaction data
chacha(tmp->step4b.encciph, tmp->step4b.encciph, ENC_CIPHER_SIZE, tmp->step4b.outkey, tmp->step4b.chachanonce,
1);
CHECK_ZXERR(chacha(tmp->step4b.encciph, tmp->step4b.encciph, ENC_CIPHER_SIZE, tmp->step4b.outkey,
tmp->step4b.chachanonce, 1));
io_seproxyhal_io_heartbeat();
if (MEMCMP(tmp->step4b.encciph, start_outputdata + INDEX_OUTPUT_OUT + i * OUTPUT_TX_LEN, ENC_CIPHER_SIZE) != 0) {
return zxerr_unknown;
Expand Down
8 changes: 7 additions & 1 deletion app/src/crypto/chacha.c
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ void chacha_core(uint8_t *output, const uint32_t *input) {
}
}

void chacha(uint8_t *out, const uint8_t *in, size_t in_len, const uint8_t *key, const uint8_t *nonce, uint32_t counter) {
zxerr_t chacha(uint8_t *out, const uint8_t *in, size_t in_len, const uint8_t *key, const uint8_t *nonce, uint32_t counter) {
uint32_t input[16];
uint8_t buf[64];
size_t todo, i;
Expand All @@ -110,6 +110,10 @@ void chacha(uint8_t *out, const uint8_t *in, size_t in_len, const uint8_t *key,
input[14] = U8TO32_LITTLE(nonce + 4);
input[15] = U8TO32_LITTLE(nonce + 8);
while (in_len > 0) {
if (input[12] == UINT32_MAX) {
return zxerr_out_of_bounds;
}

todo = sizeof(buf);
if (in_len < todo) {
todo = in_len;
Expand All @@ -124,4 +128,6 @@ void chacha(uint8_t *out, const uint8_t *in, size_t in_len, const uint8_t *key,
in_len -= todo;
input[12]++;
}

return zxerr_ok;
}
3 changes: 2 additions & 1 deletion app/src/crypto/chacha.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include <stddef.h>
#include <stdint.h>
#include "zxmacros.h"

#if defined(__cplusplus)
extern "C" {
Expand All @@ -25,7 +26,7 @@ extern "C" {
// as part of XChaCha20.
void CRYPTO_hchacha20(uint8_t out[32], const uint8_t key[32], const uint8_t nonce[16]);

void chacha(uint8_t *out, const uint8_t *in, size_t in_len, const uint8_t *key, const uint8_t *nonce, uint32_t counter);
zxerr_t chacha(uint8_t *out, const uint8_t *in, size_t in_len, const uint8_t *key, const uint8_t *nonce, uint32_t counter);

#if defined(__cplusplus)
} // extern C
Expand Down
5 changes: 5 additions & 0 deletions app/src/handlers/handler_addr.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ __Z_INLINE void handleGetAddrSecp256K1(volatile uint32_t *flags, volatile uint32
ZEMU_LOGF(100, "----[handleGetAddrSecp256K1]\n");
*tx = 0;

if (rx < APDU_MIN_LENGTH) {
ZEMU_LOGF(100, "rx: %d\n", rx);
THROW(APDU_CODE_COMMAND_NOT_ALLOWED);
}

extractHDPathTransparent(rx, OFFSET_DATA);

uint8_t requireConfirmation = G_io_apdu_buffer[OFFSET_P1];
Expand Down
Binary file modified tests_zemu/snapshots/fl-mainmenu/00004.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/s-mainmenu/00004.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/s-mainmenu/00010.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/sp-mainmenu/00004.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/sp-mainmenu/00010.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/st-mainmenu/00004.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/x-mainmenu/00004.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified tests_zemu/snapshots/x-mainmenu/00010.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit f7f1cc3

Please sign in to comment.