Repository: pion/dtls
Pull request #324 (2020-12-02 23:42:37)

Add cipher suite TLS_PSK_WITH_AES_128_CBC_SHA256
Modify CBC to support other Hash as well

Sean-Der: I don't need a review, this is @bocajim work and I just added a test!
codecov[bot]: # [Codecov](https://codecov.io/gh/pion/dtls/pull/324?src=pr&el=h1) Report
> Merging [#324](https://codecov.io/gh/pion/dtls/pull/324?src=pr&el=desc) (8882f34) into [master](https://codecov.io/gh/pion/dtls/commit/bfec2a8240c592d42523f5cf4e8b89c61051f94a?el=desc) (bfec2a8) will **decrease** coverage by `0.94%`.
> The diff coverage is `81.25%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/dtls/pull/324/graphs/tree.svg?width=650&height=150&src=pr&token=bcCB2jB88I)](https://codecov.io/gh/pion/dtls/pull/324?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #324      +/-   ##
==========================================
- Coverage   73.82%   72.87%   -0.95%     
==========================================
  Files          77       76       -1     
  Lines        3484     3447      -37     
==========================================
- Hits         2572     2512      -60     
- Misses        636      661      +25     
+ Partials      276      274       -2     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `72.87% <81.25%> (+0.11%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/dtls/pull/324?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [cipher\_suite.go](https://codecov.io/gh/pion/dtls/pull/324/diff?src=pr&el=tree#diff-Y2lwaGVyX3N1aXRlLmdv) | `80.76% <50.00%> (-1.24%)` | :arrow_down: |
| [cipher\_suite\_tls\_psk\_with\_aes\_128\_cbc\_sha256.go](https://codecov.io/gh/pion/dtls/pull/324/diff?src=pr&el=tree#diff-Y2lwaGVyX3N1aXRlX3Rsc19wc2tfd2l0aF9hZXNfMTI4X2NiY19zaGEyNTYuZ28=) | `81.08% <81.08%> (ø)` | |
| [...pher\_suite\_tls\_ecdhe\_ecdsa\_with\_aes\_256\_cbc\_sha.go](https://codecov.io/gh/pion/dtls/pull/324/diff?src=pr&el=tree#diff-Y2lwaGVyX3N1aXRlX3Rsc19lY2RoZV9lY2RzYV93aXRoX2Flc18yNTZfY2JjX3NoYS5nbw==) | `81.08% <100.00%> (+1.08%)` | :arrow_up: |
| [crypto\_cbc.go](https://codecov.io/gh/pion/dtls/pull/324/diff?src=pr&el=tree#diff-Y3J5cHRvX2NiYy5nbw==) | `69.49% <100.00%> (+0.52%)` | :arrow_up: |
| [prf.go](https://codecov.io/gh/pion/dtls/pull/324/diff?src=pr&el=tree#diff-cHJmLmdv) | `57.28% <100.00%> (ø)` | |
| [errors.go](https://codecov.io/gh/pion/dtls/pull/324/diff?src=pr&el=tree#diff-ZXJyb3JzLmdv) | `76.08% <0.00%> (-8.70%)` | :arrow_down: |
| [config.go](https://codecov.io/gh/pion/dtls/pull/324/diff?src=pr&el=tree#diff-Y29uZmlnLmdv) | `90.47% <0.00%> (-4.77%)` | :arrow_down: |
| [handshaker.go](https://codecov.io/gh/pion/dtls/pull/324/diff?src=pr&el=tree#diff-aGFuZHNoYWtlci5nbw==) | `69.34% <0.00%> (-3.65%)` | :arrow_down: |
| [conn.go](https://codecov.io/gh/pion/dtls/pull/324/diff?src=pr&el=tree#diff-Y29ubi5nbw==) | `78.61% <0.00%> (-2.18%)` | :arrow_down: |
| [errors\_errno.go](https://codecov.io/gh/pion/dtls/pull/324/diff?src=pr&el=tree#diff-ZXJyb3JzX2Vycm5vLmdv) | | |
| ... and [2 more](https://codecov.io/gh/pion/dtls/pull/324/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/dtls/pull/324?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/dtls/pull/324?src=pr&el=footer). Last update [bfec2a8...8882f34](https://codecov.io/gh/pion/dtls/pull/324?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/README.md b/README.md
index 056b580..86e19d8 100644
--- a/README.md
+++ b/README.md
@@ -53,6 +53,7 @@ We would love contributes that fall under the 'Planned Features' and fixing any
 * TLS_PSK_WITH_AES_128_CCM ([RFC 6655][rfc6655])
 * TLS_PSK_WITH_AES_128_CCM_8 ([RFC 6655][rfc6655])
 * TLS_PSK_WITH_AES_128_GCM_SHA256 ([RFC 5487][rfc5487])
+* TLS_PSK_WITH_AES_128_CBC_SHA256 ([RFC 5487][rfc5487])
 
 [rfc5289]: https://tools.ietf.org/html/rfc5289
 [rfc8422]: https://tools.ietf.org/html/rfc8422
diff --git a/cipher_suite.go b/cipher_suite.go
index 073d2ba..42b607f 100644
--- a/cipher_suite.go
+++ b/cipher_suite.go
@@ -26,6 +26,7 @@ const (
 	TLS_PSK_WITH_AES_128_CCM        CipherSuiteID = 0xc0a4 //nolint:golint,stylecheck
 	TLS_PSK_WITH_AES_128_CCM_8      CipherSuiteID = 0xc0a8 //nolint:golint,stylecheck
 	TLS_PSK_WITH_AES_128_GCM_SHA256 CipherSuiteID = 0x00a8 //nolint:golint,stylecheck
+	TLS_PSK_WITH_AES_128_CBC_SHA256 CipherSuiteID = 0x00ae //nolint:golint,stylecheck
 )
 
 var _ = allCipherSuites() // Necessary until this function isn't only used by Go 1.14
@@ -50,6 +51,8 @@ func (c CipherSuiteID) String() string {
 		return "TLS_PSK_WITH_AES_128_CCM_8"
 	case TLS_PSK_WITH_AES_128_GCM_SHA256:
 		return "TLS_PSK_WITH_AES_128_GCM_SHA256"
+	case TLS_PSK_WITH_AES_128_CBC_SHA256:
+		return "TLS_PSK_WITH_AES_128_CBC_SHA256"
 	default:
 		return fmt.Sprintf("unknown(%v)", uint16(c))
 	}
@@ -106,6 +109,8 @@ func cipherSuiteForID(id CipherSuiteID) cipherSuite {
 		return newCipherSuiteTLSPskWithAes128Ccm8()
 	case TLS_PSK_WITH_AES_128_GCM_SHA256:
 		return &cipherSuiteTLSPskWithAes128GcmSha256{}
+	case TLS_PSK_WITH_AES_128_CBC_SHA256:
+		return &cipherSuiteTLSPskWithAes128CbcSha256{}
 	}
 	return nil
 }
diff --git a/cipher_suite_tls_ecdhe_ecdsa_with_aes_256_cbc_sha.go b/cipher_suite_tls_ecdhe_ecdsa_with_aes_256_cbc_sha.go
index 56543a5..03f5eda 100644
--- a/cipher_suite_tls_ecdhe_ecdsa_with_aes_256_cbc_sha.go
+++ b/cipher_suite_tls_ecdhe_ecdsa_with_aes_256_cbc_sha.go
@@ -1,6 +1,7 @@
 package dtls
 
 import (
+	"crypto/sha1" //nolint: gosec,gci
 	"crypto/sha256"
 	"fmt"
 	"hash"
@@ -52,11 +53,13 @@ func (c *cipherSuiteTLSEcdheEcdsaWithAes256CbcSha) init(masterSecret, clientRand
 		cbc, err = newCryptoCBC(
 			keys.clientWriteKey, keys.clientWriteIV, keys.clientMACKey,
 			keys.serverWriteKey, keys.serverWriteIV, keys.serverMACKey,
+			sha1.New,
 		)
 	} else {
 		cbc, err = newCryptoCBC(
 			keys.serverWriteKey, keys.serverWriteIV, keys.serverMACKey,
 			keys.clientWriteKey, keys.clientWriteIV, keys.clientMACKey,
+			sha1.New,
 		)
 	}
 	c.cbc.Store(cbc)
diff --git a/cipher_suite_tls_psk_with_aes_128_cbc_sha256.go b/cipher_suite_tls_psk_with_aes_128_cbc_sha256.go
new file mode 100644
index 0000000..13b85f4
--- /dev/null
+++ b/cipher_suite_tls_psk_with_aes_128_cbc_sha256.go
@@ -0,0 +1,85 @@
+package dtls
+
+import (
+	"crypto/sha256"
+	"fmt"
+	"hash"
+	"sync/atomic"
+)
+
+type cipherSuiteTLSPskWithAes128CbcSha256 struct {
+	cbc atomic.Value // *cryptoCBC
+}
+
+func (c *cipherSuiteTLSPskWithAes128CbcSha256) certificateType() clientCertificateType {
+	return clientCertificateType(0)
+}
+
+func (c *cipherSuiteTLSPskWithAes128CbcSha256) ID() CipherSuiteID {
+	return TLS_PSK_WITH_AES_128_CBC_SHA256
+}
+
+func (c *cipherSuiteTLSPskWithAes128CbcSha256) String() string {
+	return "TLS_PSK_WITH_AES_128_CBC_SHA256"
+}
+
+func (c *cipherSuiteTLSPskWithAes128CbcSha256) hashFunc() func() hash.Hash {
+	return sha256.New
+}
+
+func (c *cipherSuiteTLSPskWithAes128CbcSha256) isPSK() bool {
+	return true
+}
+
+func (c *cipherSuiteTLSPskWithAes128CbcSha256) isInitialized() bool {
+	return c.cbc.Load() != nil
+}
+
+func (c *cipherSuiteTLSPskWithAes128CbcSha256) init(masterSecret, clientRandom, serverRandom []byte, isClient bool) error {
+	const (
+		prfMacLen = 32
+		prfKeyLen = 16
+		prfIvLen  = 16
+	)
+
+	keys, err := prfEncryptionKeys(masterSecret, clientRandom, serverRandom, prfMacLen, prfKeyLen, prfIvLen, c.hashFunc())
+	if err != nil {
+		return err
+	}
+
+	var cbc *cryptoCBC
+	if isClient {
+		cbc, err = newCryptoCBC(
+			keys.clientWriteKey, keys.clientWriteIV, keys.clientMACKey,
+			keys.serverWriteKey, keys.serverWriteIV, keys.serverMACKey,
+			c.hashFunc(),
+		)
+	} else {
+		cbc, err = newCryptoCBC(
+			keys.serverWriteKey, keys.serverWriteIV, keys.serverMACKey,
+			keys.clientWriteKey, keys.clientWriteIV, keys.clientMACKey,
+			c.hashFunc(),
+		)
+	}
+	c.cbc.Store(cbc)
+
+	return err
+}
+
+func (c *cipherSuiteTLSPskWithAes128CbcSha256) encrypt(pkt *recordLayer, raw []byte) ([]byte, error) {
+	cbc := c.cbc.Load()
+	if cbc == nil { // !c.isInitialized()
+		return nil, fmt.Errorf("%w, unable to decrypt", errCipherSuiteNotInit)
+	}
+
+	return cbc.(*cryptoCBC).encrypt(pkt, raw)
+}
+
+func (c *cipherSuiteTLSPskWithAes128CbcSha256) decrypt(raw []byte) ([]byte, error) {
+	cbc := c.cbc.Load()
+	if cbc == nil { // !c.isInitialized()
+		return nil, fmt.Errorf("%w, unable to decrypt", errCipherSuiteNotInit)
+	}
+
+	return cbc.(*cryptoCBC).decrypt(raw)
+}
diff --git a/crypto_cbc.go b/crypto_cbc.go
index 6a4e7f4..f1863ab 100644
--- a/crypto_cbc.go
+++ b/crypto_cbc.go
@@ -5,7 +5,6 @@ import ( //nolint:gci
 	"crypto/cipher"
 	"crypto/hmac"
 	"crypto/rand"
-	"crypto/sha1" //nolint:gosec
 	"encoding/binary"
 )
 
@@ -19,12 +18,10 @@ type cbcMode interface {
 type cryptoCBC struct {
 	writeCBC, readCBC cbcMode
 	writeMac, readMac []byte
+	h                 hashFunc
 }
 
-// Currently hardcoded to be SHA1 only
-var cryptoCBCMacFunc = sha1.New //nolint:gochecknoglobals
-
-func newCryptoCBC(localKey, localWriteIV, localMac, remoteKey, remoteWriteIV, remoteMac []byte) (*cryptoCBC, error) {
+func newCryptoCBC(localKey, localWriteIV, localMac, remoteKey, remoteWriteIV, remoteMac []byte, h hashFunc) (*cryptoCBC, error) {
 	writeBlock, err := aes.NewCipher(localKey)
 	if err != nil {
 		return nil, err
@@ -41,6 +38,7 @@ func newCryptoCBC(localKey, localWriteIV, localMac, remoteKey, remoteWriteIV, re
 
 		readCBC: cipher.NewCBCDecrypter(readBlock, remoteWriteIV).(cbcMode),
 		readMac: remoteMac,
+		h:       h,
 	}, nil
 }
 
@@ -52,7 +50,7 @@ func (c *cryptoCBC) encrypt(pkt *recordLayer, raw []byte) ([]byte, error) {
 	// Generate + Append MAC
 	h := pkt.recordLayerHeader
 
-	MAC, err := prfMac(h.epoch, h.sequenceNumber, h.contentType, h.protocolVersion, payload, c.writeMac)
+	MAC, err := prfMac(h.epoch, h.sequenceNumber, h.contentType, h.protocolVersion, payload, c.writeMac, c.h)
 	if err != nil {
 		return nil, err
 	}
@@ -89,7 +87,7 @@ func (c *cryptoCBC) encrypt(pkt *recordLayer, raw []byte) ([]byte, error) {
 func (c *cryptoCBC) decrypt(in []byte) ([]byte, error) {
 	body := in[recordLayerHeaderSize:]
 	blockSize := c.readCBC.BlockSize()
-	mac := cryptoCBCMacFunc()
+	mac := c.h()
 
 	var h recordLayerHeader
 	err := h.Unmarshal(in)
@@ -122,7 +120,7 @@ func (c *cryptoCBC) decrypt(in []byte) ([]byte, error) {
 	dataEnd := len(body) - macSize - paddingLen
 
 	expectedMAC := body[dataEnd : dataEnd+macSize]
-	actualMAC, err := prfMac(h.epoch, h.sequenceNumber, h.contentType, h.protocolVersion, body[:dataEnd], c.readMac)
+	actualMAC, err := prfMac(h.epoch, h.sequenceNumber, h.contentType, h.protocolVersion, body[:dataEnd], c.readMac, c.h)
 
 	// Compute Local MAC and compare
 	if paddingGood != 255 || err != nil || !hmac.Equal(actualMAC, expectedMAC) {
diff --git a/prf.go b/prf.go
index 2d649a1..e28c933 100644
--- a/prf.go
+++ b/prf.go
@@ -3,7 +3,6 @@ package dtls
 import ( //nolint:gci
 	"crypto/elliptic"
 	"crypto/hmac"
-	"crypto/sha1" //nolint:gosec
 	"encoding/binary"
 	"fmt"
 	"hash"
@@ -208,8 +207,8 @@ func prfVerifyDataServer(masterSecret, handshakeBodies []byte, h hashFunc) ([]by
 }
 
 // compute the MAC using HMAC-SHA1
-func prfMac(epoch uint16, sequenceNumber uint64, contentType contentType, protocolVersion protocolVersion, payload []byte, key []byte) ([]byte, error) {
-	h := hmac.New(sha1.New, key)
+func prfMac(epoch uint16, sequenceNumber uint64, contentType contentType, protocolVersion protocolVersion, payload []byte, key []byte, hf hashFunc) ([]byte, error) {
+	h := hmac.New(hf, key)
 
 	msg := make([]byte, 13)
 


Test patch:
diff --git a/conn_test.go b/conn_test.go
index 0adf5ed..a2315fb 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -422,14 +422,22 @@ func TestPSK(t *testing.T) {
 	for _, test := range []struct {
 		Name           string
 		ServerIdentity []byte
+		CipherSuites   []CipherSuiteID
 	}{
 		{
 			Name:           "Server identity specified",
 			ServerIdentity: []byte("Test Identity"),
+			CipherSuites:   []CipherSuiteID{TLS_PSK_WITH_AES_128_CCM_8},
 		},
 		{
 			Name:           "Server identity nil",
 			ServerIdentity: nil,
+			CipherSuites:   []CipherSuiteID{TLS_PSK_WITH_AES_128_CCM_8},
+		},
+		{
+			Name:           "TLS_PSK_WITH_AES_128_CBC_SHA256",
+			ServerIdentity: nil,
+			CipherSuites:   []CipherSuiteID{TLS_PSK_WITH_AES_128_CBC_SHA256},
 		},
 	} {
 		test := test
@@ -455,7 +463,7 @@ func TestPSK(t *testing.T) {
 						return []byte{0xAB, 0xC1, 0x23}, nil
 					},
 					PSKIdentityHint: clientIdentity,
-					CipherSuites:    []CipherSuiteID{TLS_PSK_WITH_AES_128_CCM_8},
+					CipherSuites:    test.CipherSuites,
 				}
 
 				c, err := testClient(ctx, ca, conf, false)
@@ -470,7 +478,7 @@ func TestPSK(t *testing.T) {
 					return []byte{0xAB, 0xC1, 0x23}, nil
 				},
 				PSKIdentityHint: test.ServerIdentity,
-				CipherSuites:    []CipherSuiteID{TLS_PSK_WITH_AES_128_CCM_8},
+				CipherSuites:    test.CipherSuites,
 			}
 
 			server, err := testServer(ctx, cb, config, false)