Skip to content

180726/nits #224

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

Merged
merged 1 commit into from
Jul 28, 2018
Merged

180726/nits #224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/memory.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@
#endif


#if (MEM_PAGE_LEN != SHA256_DIGEST_LENGTH)
#error "Incompatible macro values"
#endif


static uint8_t MEM_unlocked = DEFAULT_unlocked;
static uint8_t MEM_erased = DEFAULT_erased;
static uint8_t MEM_setup = DEFAULT_setup;
Expand Down Expand Up @@ -116,6 +121,7 @@ static uint8_t memory_eeprom(uint8_t *write_b, uint8_t *read_b, const int32_t ad


// Encrypted storage
// `write_b` and `read_b` must be length `MEM_PAGE_LEN`
static uint8_t memory_eeprom_crypt(const uint8_t *write_b, uint8_t *read_b,
const int32_t addr)
{
Expand Down Expand Up @@ -199,14 +205,20 @@ static uint8_t memory_eeprom_crypt(const uint8_t *write_b, uint8_t *read_b,
if (!dec) {
goto err;
}
memcpy(read_b, utils_hex_to_uint8(dec), MEM_PAGE_LEN);
if (read_b) {
memcpy(read_b, utils_hex_to_uint8(dec), MEM_PAGE_LEN);
Copy link

Choose a reason for hiding this comment

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

it would be nice if the args of memory_eeprom_crypt func were documented. For instance, I see it takes const uint8_t *write_b and uint8_t *read_b but no length. Looking at the code, I assume the length is always MEM_PAGE_LEN but would be nicer to confirm this reading a function doc comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK
Added the comment. Thanks.

}
utils_zero(dec, dec_len);
free(dec);

utils_zero(mempass, MEM_PAGE_LEN);
utils_clear_buffers();
return DBB_OK;
err:
if (read_b) {
// Randomize return value on error
hmac_sha256(mempass, MEM_PAGE_LEN, read_b, MEM_PAGE_LEN, read_b);
Copy link

Choose a reason for hiding this comment

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

So, the length of read_b is MEM_PAGE_LEN which matches the length of hmac, the last arg in the call to hmac_sha256 here: it's SHA256_DIGEST_LENGTH. But what happens if/when you copy this code to a newer version where the length of read_b or rather MEM_PAGE_LEN isn't equal to SHA256_DIGEST_LENGTH? I guess I'm a bit concerned this might be a "good" place for a future possible bug to sneak in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. All the different macro lengths get a bit messy. How about something like:

#if (SHA256_DIGEST_LENGTH != MEM_PAGE_LEN)
#error "Incompatible macro values"
#endif

which will abort at compile time.

Copy link

Choose a reason for hiding this comment

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

That would be perfect! I'm always happy to fail at compile time rather than later.

Copy link
Member Author

Choose a reason for hiding this comment

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

commit added

}
utils_zero(mempass, MEM_PAGE_LEN);
utils_clear_buffers();
return DBB_ERROR;
Expand Down Expand Up @@ -254,7 +266,7 @@ static void memory_scramble_rn(void)
}


uint8_t memory_setup(void)
void memory_setup(void)
Copy link

Choose a reason for hiding this comment

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

Github doesn't allow commenting unchanged lines so I'll do it here. I see a couple calls to memory_eeprom which returns either DBB_OK or DBB_ERROR. Should those also be checked and call HardFault_Handler if the result is DBB_ERROR?

I didn't look up other functions. Maybe there are others.

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK
This is planned in a future commit that will be submitted after the current PRs are merged.

{
if (memory_read_setup()) {
// One-time setup on factory install
Expand All @@ -264,7 +276,7 @@ uint8_t memory_setup(void)
// Return packet [Count(1) || Return Code (1) || CRC (2)]
uint8_t ataes_ret[4] = {0};
if (ataes_process(ataes_cmd, sizeof(ataes_cmd), ataes_ret, 4) != DBB_OK) {
return DBB_ERROR;
HardFault_Handler();
}
#endif
uint32_t c = 0x00000000;
Expand All @@ -280,7 +292,6 @@ uint8_t memory_setup(void)
memory_u2f_count_read();
}
memory_scramble_default_aeskeys();
return DBB_OK;
}


Expand Down
12 changes: 6 additions & 6 deletions src/memory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@
// User Zones: 0x0000 to 0x0FFF
// Do NOT change address locations.
// Otherwise problems will occur after a firmware update.
#define MEM_ERASED_ADDR 0x0000// Zone 0
#define MEM_SETUP_ADDR 0x0002
#define MEM_ACCESS_ERR_ADDR 0x0004
#define MEM_PIN_ERR_ADDR 0x0006
#define MEM_UNLOCKED_ADDR 0x0008
#define MEM_ERASED_ADDR 0x0000// (uint8_t) Zone 0
#define MEM_SETUP_ADDR 0x0002// (uint8_t)
#define MEM_ACCESS_ERR_ADDR 0x0004// (uint16_t)
#define MEM_PIN_ERR_ADDR 0x0006// (uint16_t)
#define MEM_UNLOCKED_ADDR 0x0008// (uint8_t)
#define MEM_EXT_FLAGS_ADDR 0x000A// (uint32_t) 32 possible extension flags
#define MEM_U2F_COUNT_ADDR 0x0010// (uint32_t)
#define MEM_NAME_ADDR 0x0100// (32 bytes) Zone 1
Expand Down Expand Up @@ -78,7 +78,7 @@ typedef enum PASSWORD_ID {
} PASSWORD_ID;


uint8_t memory_setup(void);
void memory_setup(void);
void memory_reset_u2f(void);
void memory_reset_hww(void);
void memory_erase_hww_seed(void);
Expand Down