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

Extending ECC and ECIES #112

Closed
wants to merge 17 commits into from

Conversation

kazu-yamamoto
Copy link
Contributor

This patches extend the rebased ecc branch to support ECDH, P384 and X25521.
I confirmed that this can be use in the tls library.

This also extends PRK which is necessary to implement TLS1.3.

Please give me comments.

This was referenced Nov 21, 2016
Copy link
Member

@vincenthz vincenthz left a comment

Choose a reason for hiding this comment

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

I don't think that's the right thing to do. if you really need the PRK in binary format, you should make it part of ByteArrayAccess / ByteArray class

encodePoint :: Point curve -> ByteString
decodePoint :: ByteString -> Point curve

instance {-# OVERLAPPABLE #-} Show (Point a) where
Copy link
Member

Choose a reason for hiding this comment

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

That seems like the ugly conclusion of having the associated type as newtype instead of types; really shouldn't be here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea on how to make Point an instance of ByteArrayAccess.
Would you show me code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this comment for instance rather than encodePoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point and Scalar should be instance of Eq and Show because upper layer data structures in tls require so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea on how to make Point and Scalar instances of Eq and Show except this approach.

decodeECPoint :: ByteString -> (Integer,Integer)
decodeECPoint mxy = (x,y)
where
xy = B.drop 1 mxy -- dropping 4 (uncompressed)
Copy link
Member

Choose a reason for hiding this comment

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

it really really shouldn't be a drop 1. at the very least it should be a case that check for 4, and put a note for the 2,3 format. Also this shouldn't use ByteString but should do things through the ByteArray / ByteArrayClass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


-- | Pseudo Random Key
data PRK a = PRK (HMAC a) | PRK_NoExpand ScrubbedBytes
deriving (Eq)

instance Show (PRK a) where
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should have a Show instance for security reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

show (PRK hm) = show (hmacGetDigest hm)
show (PRK_NoExpand sb) = show sb

toByteString :: PRK a -> BS.ByteString
Copy link
Member

Choose a reason for hiding this comment

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

Should be ByteArray / ByteArrayAccess not direct use of ByteString

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -110,3 +119,22 @@ foreign import ccall "cryptonite_curve25519_donna"
-> Ptr Word8 -- ^ secret
-> Ptr Word8 -- ^ basepoint
-> IO ()

generateSecretKey :: MonadRandom m => m SecretKey
generateSecretKey = return $ unsafeDoIO $ do
Copy link
Member

Choose a reason for hiding this comment

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

you really don't want to put things into a bytestring and then moved it to a Scrubbed bytes. it does defeat the purpose. getRandomBytes should gives you Scrubbed Bytes too if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kazu-yamamoto
Copy link
Contributor Author

Now PRK is an instance of ByteArrayAccess.

@kazu-yamamoto
Copy link
Contributor Author

All issues except one, which I cannot solve, are fixed.
Please merge this PR and fix the last issue by yourself.

@kazu-yamamoto
Copy link
Contributor Author

kazu-yamamoto commented Nov 30, 2016

Note that those patches are tested with the tls13 branch of my fork of tls.
WAI applications can communicate with Firefox Nightly, Chrome Canary and picotls in TLS 1.3 based on cryptonite with those patches.
P256/P384/P512/X25519 are tested.

@vincenthz
Copy link
Member

now tracked by #114

@vincenthz vincenthz closed this Dec 1, 2016
@kazu-yamamoto kazu-yamamoto deleted the ecc2 branch June 5, 2023 05:19
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.

2 participants