De-packetizing-rebuffering of Read() w/HMAC working, yay

demo/client/client.go

@@ -129,8 +129,6 @@ func main() {
 	_, err = conn.Write(rec.cmd)
 	_, err = conn.Write(rec.authCookie)
 
-	conn.EnableHMAC()
-	
 	//client reader (from server) goroutine
 	wg.Add(1)
 	go func() {
@@ -153,6 +151,7 @@ func main() {
 				os.Exit(1)
 			}
 		}
+
 		if isInteractive {
 			log.Println("[Got EOF]")
 			wg.Done() // server hung up, close WaitGroup to exit client

demo/server/server.go

@@ -178,6 +178,8 @@ func main() {
 			var len1, len2, len3, len4 uint32
 			
 			n, err := fmt.Fscanf(c, "%d %d %d %d\n", &len1, &len2, &len3, &len4)
+			log.Printf("cmdSpec read:%d %d %d %d\n", len1, len2, len3, len4)
+			
 			if err != nil || n < 4 {
 				log.Println("[Bad cmdSpec fmt]")
 				return err
@@ -211,8 +213,6 @@ func main() {
 				return err
 			}
 
-			conn.EnableHMAC()
-			
 			log.Printf("[cmdSpec: op:%c who:%s cmd:%s auth:****]\n",
 				rec.op[0], string(rec.who), string(rec.cmd))
 

hkexnet.go

@@ -24,9 +24,11 @@ package herradurakex
 import (
 	"bytes"
 	"crypto/cipher"
+	"encoding/binary"
 	"encoding/hex"
 	"fmt"
 	"hash"
+	"io"
 	"log"
 	"math/big"
 	"net"
@@ -40,17 +42,13 @@ import (
 type Conn struct {
 	c          net.Conn // which also implements io.Reader, io.Writer, ...
 	h          *HerraduraKEx
-	hmacOn     bool          // turned on once channel param negotiation is done
 	cipheropts uint32        // post-KEx cipher/hmac options
 	opts       uint32        // post-KEx protocol options (caller-defined)
 	r          cipher.Stream //read cipherStream
 	rm         hash.Hash
 	w          cipher.Stream //write cipherStream
 	wm         hash.Hash
-}
+	dBuf       *bytes.Buffer //decrypt buffer for Read()
-
-func (c *Conn) EnableHMAC() {
-	c.hmacOn = true
 }
 
 // ConnOpts returns the cipher/hmac options value, which is sent to the
@@ -134,7 +132,7 @@ func Dial(protocol string, ipport string, extensions ...string) (hc *Conn, err e
 		return nil, err
 	}
 	// Init hkexnet.Conn hc over net.Conn c
-	hc = &Conn{c: c, h: New(0, 0)}
+	hc = &Conn{c: c, h: New(0, 0), dBuf: new(bytes.Buffer)}
 	hc.applyConnExtensions(extensions...)
 
 	// Send hkexnet.Conn parameters to remote side
@@ -266,7 +264,7 @@ func (hl HKExListener) Accept() (hc Conn, err error) {
 	}
 	log.Println("[Accepted]")
 
-	hc = Conn{c: c, h: New(0, 0)}
+	hc = Conn{c: c, h: New(0, 0), dBuf: new(bytes.Buffer)}
 
 	// Read in hkexnet.Conn parameters over raw Conn c
 	// d is value for Herradura key exchange
@@ -301,47 +299,70 @@ func (hl HKExListener) Accept() (hc Conn, err error) {
 // See go doc io.Reader
 func (c Conn) Read(b []byte) (n int, err error) {
 	//log.Printf("[Decrypting...]\r\n")
-	var hIn []byte = make([]byte, 1, 1)
+	log.Printf("Read() requests %d bytes\n", len(b))
-
+	for {
-	if c.hmacOn {
+		log.Printf("c.dBuf.Len(): %d\n", c.dBuf.Len())
-		_ = hIn
+		if c.dBuf.Len() >= 1 /* len(b) */ {
-		//_, _ = io.ReadFull(c.c, hIn)
+			break
-		//if e != nil {
-		//	panic(e)
-		//}
-	}
-	n, err = c.c.Read(b)
-
-	// Normal client 'exit' from interactive session will cause
-	// (on server side) err.Error() == "<iface/addr info ...>: use of closed network connection"
-	if err != nil && err.Error() != "EOF" {
-		if !strings.HasSuffix(err.Error(), "use of closed network connection") {
-			log.Println("unexpected Read() err:", err)
-		} else {
-			log.Println("[Client hung up]")
 		}
-	}
 
-	log.Printf("  <:ctext:\r\n%s\r\n", hex.Dump(b[:n])) //EncodeToString(b[:n])) // print only used portion
+		var hmacIn uint8
+		var payloadLen uint32
 
-	db := bytes.NewBuffer(b[:n])
+		// Read the hmac LSB and payload len first
-	// The StreamReader acts like a pipe, decrypting
+		err = binary.Read(c.c, binary.BigEndian, &hmacIn)
-	// whatever is available and forwarding the result
+		if err != nil && err.Error() != "EOF" {
-	// to the parameter of Read() as a normal io.Reader
+			panic(err)
-	rs := &cipher.StreamReader{S: c.r, R: db}
+		}
-	// FIXME: Possibly the bug here -- Read() may get grouped writes from
-	// server side, causing loss of hmac sync. -rlm 2018-01-16
-	n, err = rs.Read(b)
-	log.Printf("  <-ptext:\r\n%s\r\n", hex.Dump(b[:n])) //EncodeToString(b[:n]))
 
-    // Re-calculate hmac, compare with received value
+		err = binary.Read(c.c, binary.BigEndian, &payloadLen)
-	if c.hmacOn {
+		if err != nil {
-		c.rm.Write(b[:n])
+			panic(err)
+		}
+		if payloadLen > 16384 {
+			panic("Insane payloadLen")
+		}
+		log.Println("payloadLen:", payloadLen)
+		var payloadBytes = make([]byte, payloadLen)
+		n, err = io.ReadFull(c.c, payloadBytes)
+		log.Print(" << Read ", n, " payloadBytes")
+
+		// Normal client 'exit' from interactive session will cause
+		// (on server side) err.Error() == "<iface/addr info ...>: use of closed network connection"
+		if err != nil && err.Error() != "EOF" {
+			if !strings.HasSuffix(err.Error(), "use of closed network connection") {
+				log.Println("unexpected Read() err:", err)
+			} else {
+				log.Println("[Client hung up]")
+			}
+		}
+
+		log.Printf("  <:ctext:\r\n%s\r\n", hex.Dump(payloadBytes[:n])) //EncodeToString(b[:n])) // print only used portion
+
+		db := bytes.NewBuffer(payloadBytes[:n]) //copying payloadBytes to db
+		// The StreamReader acts like a pipe, decrypting
+		// whatever is available and forwarding the result
+		// to the parameter of Read() as a normal io.Reader
+		rs := &cipher.StreamReader{S: c.r, R: db}
+		// The caller isn't necessarily reading the full payload so we need
+		// to decrypt ot an intermediate buffer, draining it on demand of caller
+		decryptN, err := rs.Read(payloadBytes)
+		log.Printf("  <-ptext:\r\n%s\r\n", hex.Dump(payloadBytes[:n])) //EncodeToString(b[:n]))
+		if err != nil {
+			panic(err)
+		}
+		c.dBuf.Write(payloadBytes)
+		log.Printf("c.dBuf: %s\n", hex.Dump(c.dBuf.Bytes()))
+
+		// Re-calculate hmac, compare with received value
+		c.rm.Write(payloadBytes)
 		hTmp := c.rm.Sum(nil)[0]
-		log.Printf("<%04x) HMAC:(i)%02x (c)%02x\r\n", len(b[:n]), hIn, hTmp)
+		log.Printf("<%04x) HMAC:(i)%02x (c)%02x\r\n", decryptN, hmacIn, hTmp)
 	}
-
+	log.Printf("Read() got %d bytes\n", c.dBuf.Len())
-	return
+	copy(b, c.dBuf.Next(len(b)))
+	log.Printf("As Read() returns, c.dBuf is %d long: %s\n", c.dBuf.Len(), hex.Dump(c.dBuf.Bytes()))
+	return len(b), nil
 }
 
 // Write a byte slice
@@ -349,24 +370,18 @@ func (c Conn) Read(b []byte) (n int, err error) {
 // See go doc io.Writer
 func (c Conn) Write(b []byte) (n int, err error) {
 	//log.Printf("[Encrypting...]\r\n")
-	//var pLen uint32
+	var hmacOut uint8
-	var hTmp = make([]byte, 1, 1)
+	var payloadLen uint32
 
 	log.Printf("  :>ptext:\r\n%s\r\n", hex.Dump(b)) //EncodeToString(b))
 
-	if c.hmacOn {
+	payloadLen = uint32(len(b))
-		_ = hTmp
-		//pLen = uint32(len(b))
-		//_ = binary.Write(c.c, binary.BigEndian, &pLen)
 
-		c.wm.Write(b)
+	// Calculate hmac on payload
-		hTmp[0] = c.wm.Sum(nil)[0]
+	c.wm.Write(b)
-		//_, e := c.c.Write(hTmp)
+	hmacOut = uint8(c.wm.Sum(nil)[0])
-		//if e != nil {
+
-		//	panic(e)
+	log.Printf("  (%04x> HMAC(o):%02x\r\n", payloadLen, hmacOut)
-		//}
-		log.Printf("  (%04x> HMAC(o):%02x\r\n", len(b) /*pLen*/, hTmp)
-	}
 
 	var wb bytes.Buffer
 	// The StreamWriter acts like a pipe, forwarding whatever is
@@ -376,7 +391,11 @@ func (c Conn) Write(b []byte) (n int, err error) {
 	if err != nil {
 		panic(err)
 	}
-	log.Printf("  ->ctext:\r\n%s\r\n", hex.Dump(wb.Bytes())) //EncodeToString(b)) // print only used portion
+	log.Printf("  ->ctext:\r\n%s\r\n", hex.Dump(wb.Bytes()))
+
+	// Write hmac LSB, payloadLen followed by payload
+	_ = binary.Write(c.c, binary.BigEndian, hmacOut)
+	_ = binary.Write(c.c, binary.BigEndian, payloadLen)
 
 	n, err = c.c.Write(wb.Bytes())
 	if err != nil {