Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feat/m2 parser #1

Merged
merged 5 commits into from
Jan 2, 2024
Merged

Feat/m2 parser #1

merged 5 commits into from
Jan 2, 2024

Conversation

chcmedeiros
Copy link
Contributor

No description provided.

@chcmedeiros chcmedeiros requested a review from ftheirs December 28, 2023 12:35
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@chcmedeiros chcmedeiros marked this pull request as ready for review December 28, 2023 12:38
ftheirs
ftheirs reviewed Dec 28, 2023
View reviewed changes
app/src/parser_impl.c Outdated Show resolved Hide resolved
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 28, 2023
View reviewed changes
app/src/tx_cchain.c Fixed Show fixed Hide fixed
app/src/tx_cchain.c Fixed Show fixed Hide fixed
app/src/tx_pchain.c Fixed Show fixed Hide fixed
app/src/tx_pchain.c Fixed Show fixed Hide fixed
@chcmedeiros chcmedeiros requested a review from ftheirs December 29, 2023 13:53
ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/parser_impl.c Outdated

static parser_error_t parser_verify_codec(parser_context_t *ctx) {
uint16_t codec = 0;
read_u16(ctx, &codec);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing CHECK_ERROR here

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/parser_print_common.c Outdated
#include "zxformat.h"
#include "zxmacros.h"

#define ALPHABET_ENCODE "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is not used

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/parser_print_common.c Outdated
const char *hrp = "";
switch (network_id) {
case songbird:
hrp = " song";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the space part of the HRP?

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/parser_print_common.c Outdated
hrp = " song";
break;
case coston:
hrp = " costwo";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/parser_print_common.c Outdated
// Calculate SHA256 checksum
uint8_t checksum[CX_SHA256_SIZE] = {0};
#if defined(TARGET_NANOS) || defined(TARGET_NANOS2) || defined(TARGET_NANOX) || defined(TARGET_STAX)
cx_hash_sha256(nodeId, NODE_ID_LEN, checksum, CX_SHA256_SIZE);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing check that return code is CX_OK

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/tx_cchain.c

static parser_error_t parser_handle_cchain_export(parser_context_t *c, parser_tx_t *v) {
// Get destination chain
v->tx.c_export_tx.destination_chain = c->buffer + c->offset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check that c->offset + BLOCKCHAIN_ID_LEN <= c->bufferLen

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/tx_cchain.c
}

// Pointer to inputs
v->tx.c_export_tx.evm_inputs.ins = c->buffer + c->offset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar that above. Check that c->offset < c->bufferLen

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/tx_cchain.c

// Pointer to outputs
if (v->tx.c_export_tx.secp_outs.n_outs > 0) {
v->tx.c_export_tx.secp_outs.outs = c->buffer + c->offset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/tx_cchain.c

static parser_error_t parser_handle_cchain_import(parser_context_t *c, parser_tx_t *v) {
// Get source chain
v->tx.c_import_tx.source_chain = c->buffer + c->offset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same checks mentioned for parser_handle_cchain_export

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/tx_pchain.c

// Pointer to outputs
if (outputs->n_outs > 0) {
outputs->outs = c->buffer + c->offset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check that c->offset < c->bufferLen

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/tx_pchain.c

// Pointer to inputs
if (inputs->n_ins > 0) {
inputs->ins = c->buffer + c->offset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check that c->offset < c->bufferLen

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/tx_pchain.c
CHECK_ERROR(parser_base_tx(c, &v->tx.p_export_tx.base_secp_ins, &v->tx.p_export_tx.base_secp_outs));

// Get destination chain
v->tx.p_export_tx.destination_chain = c->buffer + c->offset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same checks mentioned for parser_handle_cchain_export

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/tx_pchain.c
CHECK_ERROR(parser_base_tx(c, &v->tx.p_import_tx.base_secp_ins, &v->tx.p_import_tx.base_secp_outs));

// Get source chain
v->tx.p_import_tx.source_chain = c->buffer + c->offset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same checks mentioned for parser_handle_cchain_export

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/tx_pchain.c
CHECK_ERROR(parser_base_tx(c, &v->tx.add_del_val_tx.base_secp_ins, &v->tx.add_del_val_tx.base_secp_outs));

// Node ID
v->tx.add_del_val_tx.node_id = c->buffer + c->offset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check that c->offset < c->bufferLen

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
app/src/tx_pchain.c
}

// Pointer to outputs
v->tx.add_del_val_tx.staked_outs.outs = c->buffer + c->offset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check that c->offset < c->bufferLen

ftheirs
ftheirs reviewed Dec 29, 2023
View reviewed changes
docs/APDUSPEC.md Outdated
| Path[2] | byte (4) | Derivation Path Data | ? |
| Path[3] | byte (4) | Derivation Path Data | ? |
| Path[4] | byte (4) | Derivation Path Data | ? |
| L | byte (1) | Bytes in payload | 0x0 |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L should be variable or ???

ftheirs
ftheirs approved these changes Dec 29, 2023
View reviewed changes
Copy link

@ftheirs ftheirs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good! Just minor checks that might be found by the fuzzer 🚀

@chcmedeiros
Copy link
Contributor Author

@ftheirs just added an extra context verification macro and two verification functions to use throughout the code

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 29, 2023
View reviewed changes
app/src/crypto_helper.c Fixed Show fixed Hide fixed
app/src/parser_print_common.c Fixed Show fixed Hide fixed
@chcmedeiros chcmedeiros merged commit 38950ca into main Jan 2, 2024
39 of 43 checks passed
@chcmedeiros chcmedeiros deleted the feat/m2-parser branch January 2, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ftheirs ftheirs ftheirs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chcmedeiros @ftheirs