Conversation
cjpatton
left a comment
There was a problem hiding this comment.
Thanks for the patch! This ends up being a pretty big change since we need to update the internal serialization APIs. Ultimately I think the right thing to do is to replace the customer serialization logic with cryptobyte. Would you be interested in pivoting your PR to do this instead?
|
Sure! I've not written much Go outside of personal projects years ago, so let me read the docs and rethink it. I'll push when I have something that seems to work |
Sounds good. Some considerations:
|
|
I'm not sure if I'm missing something, but it seems like |
2f8d60e to
31097ce
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
31097ce to
97154d4
Compare
Ugh, you're right. That sucks :) I'll review the PR as is then. |
Because of the use of uint16, >65535 byte values put into the header will wrap around silently and cause decryption to later fail with a confusing "too short" message. This fixes encryption so that it rejects loudly and this can't silently occur (either accidentally or by malicious policy)
Go has a helpful /x/crypto/cryptobyte which includes bounds checking. Unfortunately, it was designed with protocols which mandate big-endian in mind, and these values are little-endian To avoid breaking all the ciphertexts out in the wild we compromise on cryptobyte for bounds checking then doing some little-endian bookkeeping ourselves, which still provides a relatively centralized place instead of a large amount of diffs
97154d4 to
a0a010a
Compare
Because of the use of uint16, >65535 byte values put into the header will wrap around silently and cause decryption to later fail with a confusing "too short" message.
This fixes encryption so that it rejects loudly and this can't silently occur (either accidentally or by malicious policy)
See #695