http2: reject large SETTINGS frames or those with duplicates

Per private report.

This isn't actually in the spec, but there's also no need to be so
permissive here.

Change-Id: I2b464778cc502ca7a99ea533622afea8f943eb7d
Reviewed-on: https://go-review.googlesource.com/124735
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

http2/frame.go

@@ -733,32 +733,67 @@ func (f *SettingsFrame) IsAck() bool {
 	return f.FrameHeader.Flags.Has(FlagSettingsAck)
 }
 
-func (f *SettingsFrame) Value(s SettingID) (v uint32, ok bool) {
+func (f *SettingsFrame) Value(id SettingID) (v uint32, ok bool) {
 	f.checkValid()
-	buf := f.p
-	for len(buf) > 0 {
-		settingID := SettingID(binary.BigEndian.Uint16(buf[:2]))
-		if settingID == s {
-			return binary.BigEndian.Uint32(buf[2:6]), true
+	for i := 0; i < f.NumSettings(); i++ {
+		if s := f.Setting(i); s.ID == id {
+			return s.Val, true
 		}
-		buf = buf[6:]
 	}
 	return 0, false
 }
 
+// Setting returns the setting from the frame at the given 0-based index.
+// The index must be >= 0 and less than f.NumSettings().
+func (f *SettingsFrame) Setting(i int) Setting {
+	buf := f.p
+	return Setting{
+		ID:  SettingID(binary.BigEndian.Uint16(buf[i*6 : i*6+2])),
+		Val: binary.BigEndian.Uint32(buf[i*6+2 : i*6+6]),
+	}
+}
+
+func (f *SettingsFrame) NumSettings() int { return len(f.p) / 6 }
+
+// HasDuplicates reports whether f contains any duplicate setting IDs.
+func (f *SettingsFrame) HasDuplicates() bool {
+	num := f.NumSettings()
+	if num == 0 {
+		return false
+	}
+	// If it's small enough (the common case), just do the n^2
+	// thing and a map allocation.
+	if num < 10 {
+		for i := 0; i < num; i++ {
+			idi := f.Setting(i).ID
+			for j := i + 1; j < num; j++ {
+				idj := f.Setting(j).ID
+				if idi == idj {
+					return true
+				}
+			}
+		}
+		return false
+	}
+	seen := map[SettingID]bool{}
+	for i := 0; i < num; i++ {
+		id := f.Setting(i).ID
+		if seen[id] {
+			return true
+		}
+		seen[id] = true
+	}
+	return false
+}
+
 // ForeachSetting runs fn for each setting.
 // It stops and returns the first error.
 func (f *SettingsFrame) ForeachSetting(fn func(Setting) error) error {
 	f.checkValid()
-	buf := f.p
-	for len(buf) > 0 {
-		if err := fn(Setting{
-			SettingID(binary.BigEndian.Uint16(buf[:2])),
-			binary.BigEndian.Uint32(buf[2:6]),
-		}); err != nil {
+	for i := 0; i < f.NumSettings(); i++ {
+		if err := fn(f.Setting(i)); err != nil {
 			return err
 		}
-		buf = buf[6:]
 	}
 	return nil
 }

http2/frame_test.go

@@ -1189,3 +1189,47 @@ func encodeHeaderRaw(t *testing.T, pairs ...string) []byte {
 	var he hpackEncoder
 	return he.encodeHeaderRaw(t, pairs...)
 }
+
+func TestSettingsDuplicates(t *testing.T) {
+	tests := []struct {
+		settings []Setting
+		want     bool
+	}{
+		{nil, false},
+		{[]Setting{{ID: 1}}, false},
+		{[]Setting{{ID: 1}, {ID: 2}}, false},
+		{[]Setting{{ID: 1}, {ID: 2}}, false},
+		{[]Setting{{ID: 1}, {ID: 2}, {ID: 3}}, false},
+		{[]Setting{{ID: 1}, {ID: 2}, {ID: 3}}, false},
+		{[]Setting{{ID: 1}, {ID: 2}, {ID: 3}, {ID: 4}}, false},
+
+		{[]Setting{{ID: 1}, {ID: 2}, {ID: 3}, {ID: 2}}, true},
+		{[]Setting{{ID: 4}, {ID: 2}, {ID: 3}, {ID: 4}}, true},
+
+		{[]Setting{
+			{ID: 1}, {ID: 2}, {ID: 3}, {ID: 4},
+			{ID: 5}, {ID: 6}, {ID: 7}, {ID: 8},
+			{ID: 9}, {ID: 10}, {ID: 11}, {ID: 12},
+		}, false},
+
+		{[]Setting{
+			{ID: 1}, {ID: 2}, {ID: 3}, {ID: 4},
+			{ID: 5}, {ID: 6}, {ID: 7}, {ID: 8},
+			{ID: 9}, {ID: 10}, {ID: 11}, {ID: 11},
+		}, true},
+	}
+	for i, tt := range tests {
+		fr, _ := testFramer()
+		fr.WriteSettings(tt.settings...)
+		f, err := fr.ReadFrame()
+		if err != nil {
+			t.Fatalf("%d. ReadFrame: %v", i, err)
+		}
+		sf := f.(*SettingsFrame)
+		got := sf.HasDuplicates()
+		if got != tt.want {
+			t.Errorf("%d. HasDuplicates = %v; want %v", i, got, tt.want)
+		}
+	}
+
+}

http2/server.go

@@ -1487,6 +1487,12 @@ func (sc *serverConn) processSettings(f *SettingsFrame) error {
 		}
 		return nil
 	}
+	if f.NumSettings() > 100 || f.HasDuplicates() {
+		// This isn't actually in the spec, but hang up on
+		// suspiciously large settings frames or those with
+		// duplicate entries.
+		return ConnectionError(ErrCodeProtocol)
+	}
 	if err := f.ForeachSetting(sc.processSetting); err != nil {
 		return err
 	}