Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

Pad z value to proper size after P-521 scalar multiplication. #245

Merged
merged 2 commits into from
Jun 3, 2019

Conversation

csstaub
Copy link
Collaborator

@csstaub csstaub commented May 31, 2019

Fixes issue #228. After calling ScalarMult for P-521, the output can sometimes be 65 bytes long instead of 66 bytes. This happens when the first bit of the computed value is zero. Calling z.Bytes() on the big integer will then omit the leading zero, giving us a 65-byte value.

This subsequently causes the shared secret computation to be incorrect as the input into the KDF function should always be 66 bytes which is the full length for a P-521 coordinate value.

Fixes issue #228. After calling ScalarMult for P-521, the output can
sometimes be 65 bytes long instead of 66 bytes. This happens when the
first bit of the computed value is zero. Calling z.Bytes() on the big
integer will then omit the leading zero, giving us a 65-byte value.

This subsequently causes the shared secret computation to be incorrect
as the input into the KDF function should always be 66 bytes which is
the full length for a P-521 coordinate value.
@csstaub
Copy link
Collaborator Author

csstaub commented May 31, 2019

Will cherry-pick this into v2 after merge to master, seems important enough to fix there too.


octSize := dSize(priv.Curve)
if len(zBytes) != octSize {
zBytes = append(bytes.Repeat([]byte{0}, octSize-len(zBytes)), zBytes...)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we care about any timing info leak here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly, though I’m not sure how we’d fix it, and regardless P-521 in Go isn’t constant time anyway (only P-256 is afaik).

Copy link
Contributor

@mbyczkowski mbyczkowski left a comment

Choose a reason for hiding this comment

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

Woah @csstaub, great sleuthing! 🙌


octSize := dSize(priv.Curve)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comment from #228 (comment) would be great here

@csstaub csstaub merged commit c768d60 into master Jun 3, 2019
@csstaub csstaub deleted the cs/fix-228 branch June 3, 2019 17:24
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.

3 participants