From f558bdebed0bc55f12500670953a0aeb832fa5e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20R=C3=BCth?= Date: Thu, 11 May 2017 19:54:49 +0200 Subject: [PATCH 1/4] Added parsing of PUBS tag and filtering according to KEXS This now allows quic-go to contact servers announcing multiple KEXS methods, e.g. like Akamai. --- handshake/server_config_client.go | 36 +++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/handshake/server_config_client.go b/handshake/server_config_client.go index 1c99d0be..15d2f702 100644 --- a/handshake/server_config_client.go +++ b/handshake/server_config_client.go @@ -57,7 +57,6 @@ func (s *serverConfigClient) parseValues(tagMap map[Tag][]byte) error { s.ID = scfgID // KEXS - // TODO: allow for P256 in the list // TODO: setup Key Exchange kexs, ok := tagMap[TagKEXS] if !ok { @@ -66,7 +65,15 @@ func (s *serverConfigClient) parseValues(tagMap map[Tag][]byte) error { if len(kexs)%4 != 0 { return qerr.Error(qerr.CryptoInvalidValueLength, "KEXS") } - if !bytes.Equal(kexs, []byte("C255")) { + c255Foundat := -1 + + for i := 0; i < len(kexs)/4; i++ { + if bytes.Equal(kexs[4*i:4*i+4], []byte("C255")) { + c255Foundat = i + break + } + } + if (c255Foundat < 0) { return qerr.Error(qerr.CryptoNoSupport, "KEXS") } @@ -90,12 +97,29 @@ func (s *serverConfigClient) parseValues(tagMap map[Tag][]byte) error { } // PUBS - // TODO: save this value pubs, ok := tagMap[TagPUBS] if !ok { return qerr.Error(qerr.CryptoMessageParameterNotFound, "PUBS") } - if len(pubs) != 35 { + + var pubs_kexs []struct{Length uint32; Value []byte} + var last_len uint32 + + for i := 0; i < len(pubs)-3; i += int(last_len)+3 { + // the PUBS value is always prepended by 3 byte little endian length field + + err := binary.Read(bytes.NewReader([]byte{pubs[i], pubs[i+1], pubs[i+2], 0x00}), binary.LittleEndian, &last_len); + if err != nil { + return qerr.Error(qerr.CryptoInvalidValueLength, "PUBS") + } + pubs_kexs = append(pubs_kexs, struct{Length uint32; Value []byte}{last_len, pubs[i+3:i+3+int(last_len)]}) + } + + if c255Foundat >= len(pubs_kexs) { + return qerr.Error(qerr.CryptoInvalidValueLength, "KEXS not in PUBS") + } + + if pubs_kexs[c255Foundat].Length != 32 { return qerr.Error(qerr.CryptoInvalidValueLength, "PUBS") } @@ -105,8 +129,8 @@ func (s *serverConfigClient) parseValues(tagMap map[Tag][]byte) error { return err } - // the PUBS value is always prepended by []byte{0x20, 0x00, 0x00} - s.sharedSecret, err = s.kex.CalculateSharedKey(pubs[3:]) + + s.sharedSecret, err = s.kex.CalculateSharedKey(pubs_kexs[c255Foundat].Value) if err != nil { return err } From 6a0b2d04d67dd59bfb531c9a4d7dbd4d625f381d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20R=C3=BCth?= Date: Thu, 11 May 2017 21:15:52 +0200 Subject: [PATCH 2/4] Added tests to check if PUBS is invalid and made sure a valid PUBS is requested by default --- handshake/server_config_client.go | 10 +++++++++- handshake/server_config_client_test.go | 8 +++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/handshake/server_config_client.go b/handshake/server_config_client.go index 15d2f702..fe871310 100644 --- a/handshake/server_config_client.go +++ b/handshake/server_config_client.go @@ -110,13 +110,21 @@ func (s *serverConfigClient) parseValues(tagMap map[Tag][]byte) error { err := binary.Read(bytes.NewReader([]byte{pubs[i], pubs[i+1], pubs[i+2], 0x00}), binary.LittleEndian, &last_len); if err != nil { + return qerr.Error(qerr.CryptoInvalidValueLength, "PUBS not decodable") + } + if last_len == 0 { return qerr.Error(qerr.CryptoInvalidValueLength, "PUBS") } + + if i+3+int(last_len) > len(pubs) { + return qerr.Error(qerr.CryptoInvalidValueLength, "PUBS") + } + pubs_kexs = append(pubs_kexs, struct{Length uint32; Value []byte}{last_len, pubs[i+3:i+3+int(last_len)]}) } if c255Foundat >= len(pubs_kexs) { - return qerr.Error(qerr.CryptoInvalidValueLength, "KEXS not in PUBS") + return qerr.Error(qerr.CryptoMessageParameterNotFound, "KEXS not in PUBS") } if pubs_kexs[c255Foundat].Length != 32 { diff --git a/handshake/server_config_client_test.go b/handshake/server_config_client_test.go index 34d314ab..ce73f80b 100644 --- a/handshake/server_config_client_test.go +++ b/handshake/server_config_client_test.go @@ -15,7 +15,7 @@ func getDefaultServerConfigClient() map[Tag][]byte { TagSCID: bytes.Repeat([]byte{'F'}, 16), TagKEXS: []byte("C255"), TagAEAD: []byte("AESG"), - TagPUBS: bytes.Repeat([]byte{0}, 35), + TagPUBS: append([]byte{0x20, 0x00, 0x00}, bytes.Repeat([]byte{0}, 32)...), TagOBIT: bytes.Repeat([]byte{0}, 8), TagEXPY: []byte{0x0, 0x6c, 0x57, 0x78, 0, 0, 0, 0}, // 2033-12-24 } @@ -184,6 +184,12 @@ var _ = Describe("Server Config", func() { Expect(err).To(MatchError("CryptoInvalidValueLength: PUBS")) }) + It("rejects PUBS values that have a zero length", func() { + tagMap[TagPUBS] = bytes.Repeat([]byte{0}, 100) // completely wrong length + err := scfg.parseValues(tagMap) + Expect(err).To(MatchError("CryptoInvalidValueLength: PUBS")) + }) + It("errors if the PUBS is missing", func() { delete(tagMap, TagPUBS) err := scfg.parseValues(tagMap) From 7465ee128d521cf7ea4ad86aa33e6a547c7c315e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20R=C3=BCth?= Date: Fri, 12 May 2017 14:14:27 +0200 Subject: [PATCH 3/4] removed parenthesis and added more detailed error description --- handshake/server_config_client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/handshake/server_config_client.go b/handshake/server_config_client.go index fe871310..e3ee4f7d 100644 --- a/handshake/server_config_client.go +++ b/handshake/server_config_client.go @@ -73,8 +73,8 @@ func (s *serverConfigClient) parseValues(tagMap map[Tag][]byte) error { break } } - if (c255Foundat < 0) { - return qerr.Error(qerr.CryptoNoSupport, "KEXS") + if c255Foundat < 0 { + return qerr.Error(qerr.CryptoNoSupport, "KEXS: Could not find C255, other key exchanges are not supported") } // AEAD From 4983119be5af2fdff624c4bb547aa4644a024b73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20R=C3=BCth?= Date: Fri, 12 May 2017 15:02:38 +0200 Subject: [PATCH 4/4] added test to verify C255 KEXS's PUBs must not be the first thing in the public values --- handshake/server_config_client_test.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/handshake/server_config_client_test.go b/handshake/server_config_client_test.go index ce73f80b..509c6bba 100644 --- a/handshake/server_config_client_test.go +++ b/handshake/server_config_client_test.go @@ -124,7 +124,7 @@ var _ = Describe("Server Config", func() { It("rejects KEXS values other than C255", func() { tagMap[TagKEXS] = []byte("P256") err := scfg.parseValues(tagMap) - Expect(err).To(MatchError("CryptoNoSupport: KEXS")) + Expect(err).To(MatchError("CryptoNoSupport: KEXS: Could not find C255, other key exchanges are not supported")) }) It("errors if the KEXS is missing", func() { @@ -190,6 +190,19 @@ var _ = Describe("Server Config", func() { Expect(err).To(MatchError("CryptoInvalidValueLength: PUBS")) }) + It("ensure that C255 Pubs must not be at the first index", func() { + serverKex, err := crypto.NewCurve25519KEX() + Expect(err).ToNot(HaveOccurred()) + tagMap[TagKEXS] = []byte("P256C255") // have another KEXS before C255 + // 3 byte len + 1 byte empty + C255 + tagMap[TagPUBS] = append([]byte{0x01, 0x00, 0x00, 0x00}, append([]byte{0x20, 0x00, 0x00}, serverKex.PublicKey()...)...) + err = scfg.parseValues(tagMap) + Expect(err).ToNot(HaveOccurred()) + sharedSecret, err := serverKex.CalculateSharedKey(scfg.kex.PublicKey()) + Expect(err).ToNot(HaveOccurred()) + Expect(scfg.sharedSecret).To(Equal(sharedSecret)) + }) + It("errors if the PUBS is missing", func() { delete(tagMap, TagPUBS) err := scfg.parseValues(tagMap)