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

Jump to bottom

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

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

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

mundaym opened this issue May 2, 2018 · 3 comments
Labels
help wanted Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Unreleased

Comments

@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[:])
@mundaym mundaym added Testing An issue that has been verified to require only test changes, not just a test failure. help wanted labels May 2, 2018
@gopherbot gopherbot added this to the Unreleased milestone May 2, 2018
@sabin-rapan
Copy link
Contributor

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

@mundaym
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
Copy link
Contributor

@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
Assignees
No one assigned
Labels
help wanted Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@gopherbot @mundaym @sabin-rapan