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

x/crypto/ssh: test cipher implementations against known good input/output data #25214

Open
mundaym opened this Issue May 2, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@mundaym
Copy link
Member

mundaym commented May 2, 2018

If I apply the following patch to the cipher.go file the tests still pass. It would be nice to catch this kind of error so we can refactor this code and have some confidence we haven't broken it.

diff --git a/ssh/cipher.go b/ssh/cipher.go
index 67b0126..d99ffc7 100644
--- a/ssh/cipher.go
+++ b/ssh/cipher.go
@@ -16,7 +16,7 @@ import (
        "hash"
        "io"
        "io/ioutil"
-       "math/bits"
+       _ "math/bits"
 
        "golang.org/x/crypto/internal/chacha20"
        "golang.org/x/crypto/poly1305"
@@ -666,7 +666,7 @@ func newChaCha20Cipher(key, unusedIV, unusedMACKey []byte, unusedAlgs directionA
 }
 
 func (c *chacha20Poly1305Cipher) readPacket(seqNum uint32, r io.Reader) ([]byte, error) {
-       nonce := [3]uint32{0, 0, bits.ReverseBytes32(seqNum)}
+       nonce := [3]uint32{1, 2, 3}
        s := chacha20.New(c.contentKey, nonce)
        var polyKey [32]byte
        s.XORKeyStream(polyKey[:], polyKey[:])
@@ -724,7 +724,7 @@ func (c *chacha20Poly1305Cipher) readPacket(seqNum uint32, r io.Reader) ([]byte,
 }
 
 func (c *chacha20Poly1305Cipher) writePacket(seqNum uint32, w io.Writer, rand io.Reader, payload []byte) error {
-       nonce := [3]uint32{0, 0, bits.ReverseBytes32(seqNum)}
+       nonce := [3]uint32{1, 2, 3}
        s := chacha20.New(c.contentKey, nonce)
        var polyKey [32]byte
        s.XORKeyStream(polyKey[:], polyKey[:])

@gopherbot gopherbot added this to the Unreleased milestone May 2, 2018

@sabin-rapan

This comment has been minimized.

Copy link
Contributor

sabin-rapan commented May 3, 2018

I think I am missing something, but the underscore before importing math/bits makes the build fail.

@mundaym

This comment has been minimized.

Copy link
Member Author

mundaym commented May 3, 2018

@sabin-rapan are you using an old version of the compiler? I think math/bits was introduced in Go 1.9. Either way you can also just comment out the line, the _ is just there to make the file compile without an unused import error.

@sabin-rapan

This comment has been minimized.

Copy link
Contributor

sabin-rapan commented May 5, 2018

@mundaym you are right. It was my bad.
Since most cipher suite RFCs provide test vectors (see appendix A for chacha20) we could implement a test function for each cipher. I could start working on this implementation.
Maybe this idea should be extended to other cipher suites?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.