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.

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

EncryptKey/DecryptKey return whole PB message, not just value #7

Closed
karelbilek opened this issue Mar 16, 2019 · 6 comments
Closed

EncryptKey/DecryptKey return whole PB message, not just value #7

karelbilek opened this issue Mar 16, 2019 · 6 comments

Comments

@karelbilek
Copy link

EncryptKey/DecryptKey return whole PB message, not just value

This code just returns bytes from the PB message

func (trezor *TrezorBase) CipherKeyValue(path string, isToEncrypt bool, keyName string, data, iv []byte, askOnEncode, askOnDecode bool) ([]byte, messages.MessageType) {
result, msgType := trezor.call(trezor.Client.CipherKeyValue(isToEncrypt, keyName, data, tesoro.StringToBIP32Path(path), iv, askOnEncode, askOnDecode))
return []byte(result), messages.MessageType(msgType)
}

and that's used directly as a key here

decryptedKey, msgType := trezor.CipherKeyValue(path, false, trezorKeyname, []byte(encryptedKeyhexValue), nonce, false, true)
switch msgType {
case messages.MessageType_MessageType_Success, messages.MessageType_MessageType_CipheredKeyValue:
case messages.MessageType_MessageType_Failure:
return nil, fmt.Errorf(`Got an error from a trezor device: %v (the trezor device is busy?)`, string(decryptedKey)) // if an error occurs then the error description is returned into "decryptedKey" as a string
default:
return nil, fmt.Errorf("Got an unexpected behaviour from a trezor device: %v: %v", msgType, string(encryptedKey))
}
return decryptedKey, nil

but it's never de-marshaled from protobuf.

https://github.com/trezor/trezor-common/blob/04bac52951764d5516206f5359cfc5f5c24eb38f/protob/messages-crypto.proto#L28-L30

@karelbilek
Copy link
Author

I am currently re-writing xaionaro-go/cryptoWallet to use my fork of trezord-go as a library here

trezor/trezord-go#147

which will cause the code here to simplify a lot (I am basically removing all the intermediary classes) and improve the code here. (And I am also removing the dependency on tesoro, which is unmaintained.)

@karelbilek
Copy link
Author

Similarly, Ping result is not de-marshaled, as isn't Features. (I wonder how could it work for you :D )

@karelbilek
Copy link
Author

I see JSON unmarshaling here, which makes no sense, as JSON is not used anywhere in trezor

err := json.Unmarshal([]byte(str), &response)

@xaionaro
Copy link
Member

xaionaro commented Mar 17, 2019

Hello. I have no time on Trezors right now, so the only thing I can do is review and merge patches :(

I see JSON unmarshaling here, which makes no sense, as JSON is not used anywhere in trezor

If this's true then it's very strange. Because I remember for sure that the answer was in JSON. May be the device returns it that way or something. Or, may be it's just obsolete.

Similarly, Ping result is not de-marshaled, as isn't Features. (I wonder how could it work for you :D )

Well, it really worked, too :)
I cannot answer anything here because of no time on investigations... Sorry :(

but it's never de-marshaled from protobuf.

Yep, seems to be a mistake.

@xaionaro
Copy link
Member

xaionaro commented Mar 17, 2019

UPD:

I've just looked into the code a little because I couldn't believe I was so careless. And it seems that actually everything is OK. The unmarshaling was done here:

https://github.com/conejoninja/tesoro/blob/master/tesoro.go#L864

And JSON messages are about there, too.

@karelbilek
Copy link
Author

OH OK! I looked at wrong place. In that case, closing.

(as I said, I am rewriting this library + tesoro. I will make you a PR once it's done, but basically, at that point, this library will be basically almost empty.)

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

No branches or pull requests

2 participants