Skip to content
This repository has been archived by the owner on Nov 16, 2021. It is now read-only.

Compute pubkey only on demand. #61

Merged
merged 1 commit into from Jul 4, 2016
Merged

Conversation

jhoenicke
Copy link
Contributor

@jhoenicke jhoenicke commented Jun 29, 2016

I'm not completely happy with this pull request, but I opened it to be able to discuss this.
This pull request changes the API and needs more changes to trezor-mcu to work, see the corresponding pull request trezor/trezor-mcu#97.

The goal of this pull request is to reduce the number of times we compute the public key. This is both for performance reasons and to reduce the side-channel risk. I'm confident that the current implementation is side-channel resistant but if we can avoid computing a public key, we should avoid it. For example for u2f we currently compute 10 public keys, including the master public key for the nist256p hierarchy. With this patch u2f only computes the R value of the signature from the private nonce.

Things I'm not happy with:

  • Most functions on hdnode are no longer const, since they may write the public key to the hdnode.
  • The previous point is especially true for ed25519 sign, which needs the public key.
  • The hdnode becomes stateful. If we compute a hdnode, then use a function that needs the public key and then access the public key, it works. If we later remove the function, the public key would be zero, which is a hard to find bug.
  • The unit tests don't test the statefulness of hdnode.
  • We now set the first byte of the 33 byte ed25519 public key to 1, to mark the presence of the public key. Currently the Trezor API sets it to 0. (Officially, an ed25519 public key is 32 byte).

The public key is always 33 byte. NIST and SECP public keys use the usual 02/03 prefix byte to mark even/odd compressed public key. ed25519 instead uses a 01 prefix, just to distinguish unset public key.

Remove fingerprint from hdnode structure (if you need it, call
hdnode_fingerprint on the parent hdnode).
Only compute public_key, when hdnode_fill_public_key is called.
@jhoenicke
Copy link
Contributor Author

The check that failed is a timeout in test_codepoints. Is there an easy way to increase the timeout limit for one test?

@prusnak
Copy link
Member

prusnak commented Jun 30, 2016

http://check.sourceforge.net/doc/check_html/check_4.html#Test-Timeouts says:

The timeout for a specific test case, which may contain multiple unit tests, can be changed with the tcase_set_timeout() function.

@prusnak
Copy link
Member

prusnak commented Jul 4, 2016

I like the change because of the performance upgrade. (I was thinking about this in the past, but did not want to break things). Let's get this in.

@prusnak prusnak merged commit ab81351 into trezor:master Jul 4, 2016
@prusnak
Copy link
Member

prusnak commented Jul 4, 2016

test_codepoints timeout increased in dc16759

@prusnak
Copy link
Member

prusnak commented Jul 4, 2016

I also reintroduced uint32_t field to HDNode structure in 9a8df5a where fingerprint used to be, so we won't break binary compatibility of people using the struct directly (we do this in storage for example).

@jhoenicke
Copy link
Contributor Author

I was under the impression that this isn't a problem, since the storage node stores a HDNodeType (protobuf structure).

@prusnak
Copy link
Member

prusnak commented Jul 4, 2016

Ah, yeah. I got that mixed up. Sorry. Reverted.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants