Re: golang-1.7 / CVE-2019-9514 / CVE-2019-9512
I have a patch to fix this. As attached.
I had to apply this patch manually be hand, but I didn't notice any
issues.
I also noticed that tests failed when I built this in a Docker container
without IPv6 support. So I added a tiny change to disable this IPv6 test
if IPv6 is not supported on host (same as with the other IPv6 tests).
All tests pass now, with and without IPv6 support.
What is Debian LTS policy concerning Debian salsa git repos? Should I
push these changes to the git repo in new a debian/stretch branch? Ask
the maintainer for permission? Or what? I don't want to accidentally
tread on any toes here, by asking when I shouldn't or not asking when I
should.
--
Brian May <bam@debian.org>
diff -Nru golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/changelog golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/changelog
--- golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/changelog 2016-10-15 21:31:25.000000000 +1100
+++ golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/changelog 2020-12-04 09:10:25.000000000 +1100
@@ -1,3 +1,12 @@
+golang-golang-x-net-dev (1:0.0+git20161013.8b4af36+dfsg-3+deb9u1) stretch-security; urgency=medium
+
+ * Non-maintainer upload by the LTS Security Team.
+ * Fix CVE-2019-9512 and CVE-2019-9514: Prevent DOS attack by limiting unread
+ control frames.
+ * Fix test error when building on host without IPv6 support.
+
+ -- Brian May <bam@debian.org> Fri, 04 Dec 2020 09:10:25 +1100
+
golang-golang-x-net-dev (1:0.0+git20161013.8b4af36+dfsg-3) unstable; urgency=medium
[ Paul Tagliamonte ]
diff -Nru golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/0001-http2-limit-number-of-control-frames-in-server-send-.patch golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/0001-http2-limit-number-of-control-frames-in-server-send-.patch
--- golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/0001-http2-limit-number-of-control-frames-in-server-send-.patch 1970-01-01 10:00:00.000000000 +1000
+++ golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/0001-http2-limit-number-of-control-frames-in-server-send-.patch 2020-12-04 09:10:25.000000000 +1100
@@ -0,0 +1,163 @@
+From: Brian May <brian@linuxpenguins.xyz>
+Date: Fri, 4 Dec 2020 09:03:10 +1100
+Subject: http2: limit number of control frames in server send queue
+
+An attacker could cause servers to queue an unlimited number of PING
+ACKs or RST_STREAM frames by soliciting them and not reading them, until
+the program runs out of memory.
+
+Limit control frames in the queue to a few thousands (matching the limit
+imposed by other vendors) by counting as they enter and exit the scheduler,
+so the protection will work with any WriteScheduler.
+
+Once the limit is exceeded, close the connection, as we have no way to
+communicate with the peer.
+
+This addresses CVE-2019-9512 and CVE-2019-9514.
+
+Fixes golang/go#33606
+---
+ http2/server.go | 32 ++++++++++++++++++++++++++++++++
+ http2/server_test.go | 26 ++++++++++++++++++++++++++
+ http2/writesched.go | 6 ++++++
+ 3 files changed, 64 insertions(+)
+
+diff --git a/http2/server.go b/http2/server.go
+index c986bc1..2f61ef6 100644
+--- a/http2/server.go
++++ b/http2/server.go
+@@ -64,6 +64,7 @@ const (
+ firstSettingsTimeout = 2 * time.Second // should be in-flight with preface anyway
+ handlerChunkWriteSize = 4 << 10
+ defaultMaxStreams = 250 // TODO: make this 100 as the GFE seems to?
++ maxQueuedControlFrames = 10000;
+ )
+
+ var (
+@@ -130,6 +131,15 @@ func (s *Server) maxConcurrentStreams() uint32 {
+ return defaultMaxStreams
+ }
+
++// maxQueuedControlFrames is the maximum number of control frames like
++// SETTINGS, PING and RST_STREAM that will be queued for writing before
++// the connection is closed to prevent memory exhaustion attacks.
++func (s *Server) maxQueuedControlFrames() int {
++ // TODO: if anybody asks, add a Server field, and remember to define the
++ // behavior of negative values.
++ return maxQueuedControlFrames
++}
++
+ // ConfigureServer adds HTTP/2 support to a net/http Server.
+ //
+ // The configuration conf may be nil.
+@@ -373,6 +383,7 @@ type serverConn struct {
+ sawFirstSettings bool // got the initial SETTINGS frame after the preface
+ needToSendSettingsAck bool
+ unackedSettings int // how many SETTINGS have we sent without ACKs?
++ queuedControlFrames int // control frames in the writeSched queue
+ clientMaxStreams uint32 // SETTINGS_MAX_CONCURRENT_STREAMS from client (our PUSH_PROMISE limit)
+ advMaxStreams uint32 // our SETTINGS_MAX_CONCURRENT_STREAMS advertised the client
+ curOpenStreams uint32 // client's number of open streams
+@@ -713,6 +724,14 @@ func (sc *serverConn) serve() {
+ case fn := <-sc.testHookCh:
+ fn(loopNum)
+ }
++
++ // If the peer is causing us to generate a lot of control frames,
++ // but not reading them from us, assume they are trying to make us
++ // run out of memory.
++ if sc.queuedControlFrames > sc.srv.maxQueuedControlFrames() {
++ sc.vlogf("http2: too many control frames in send queue, closing connection")
++ return
++ }
+ }
+ }
+
+@@ -840,6 +859,14 @@ func (sc *serverConn) writeFrame(wm frameWriteMsg) {
+ }
+
+ if !ignoreWrite {
++ if wm.isControl() {
++ sc.queuedControlFrames++
++ // For extra safety, detect wraparounds, which should not happen,
++ // and pull the plug.
++ if sc.queuedControlFrames < 0 {
++ sc.conn.Close()
++ }
++ }
+ sc.writeSched.add(wm)
+ }
+ sc.scheduleFrameWrite()
+@@ -966,6 +993,9 @@ func (sc *serverConn) scheduleFrameWrite() {
+ }
+ if !sc.inGoAway {
+ if wm, ok := sc.writeSched.take(); ok {
++ if wm.isControl() {
++ sc.queuedControlFrames--
++ }
+ sc.startFrameWrite(wm)
+ return
+ }
+@@ -1213,6 +1243,8 @@ func (sc *serverConn) processSettings(f *SettingsFrame) error {
+ if err := f.ForeachSetting(sc.processSetting); err != nil {
+ return err
+ }
++ // TODO: judging by RFC 7540, Section 6.5.3 each SETTINGS frame should be
++ // acknowledged individually, even if multiple are received before the ACK.
+ sc.needToSendSettingsAck = true
+ sc.scheduleFrameWrite()
+ return nil
+diff --git a/http2/server_test.go b/http2/server_test.go
+index 879e821..9230f5e 100644
+--- a/http2/server_test.go
++++ b/http2/server_test.go
+@@ -1026,6 +1026,32 @@ func TestServer_Ping(t *testing.T) {
+ }
+ }
+
++func TestServer_MaxQueuedControlFrames(t *testing.T) {
++ if testing.Short() {
++ t.Skip("skipping in short mode")
++ }
++
++ st := newServerTester(t, nil)
++ defer st.Close()
++ st.greet()
++
++ const extraPings = 500000 // enough to fill the TCP buffers
++
++ for i := 0; i < maxQueuedControlFrames+extraPings; i++ {
++ pingData := [8]byte{1, 2, 3, 4, 5, 6, 7, 8}
++ if err := st.fr.WritePing(false, pingData); err != nil {
++ if i == 0 {
++ t.Fatal(err)
++ }
++ // We expect the connection to get closed by the server when the TCP
++ // buffer fills up and the write queue reaches MaxQueuedControlFrames.
++ t.Logf("sent %d PING frames", i)
++ return
++ }
++ }
++ t.Errorf("unexpected success sending all PING frames")
++}
++
+ func TestServer_RejectsLargeFrames(t *testing.T) {
+ if runtime.GOOS == "windows" {
+ t.Skip("see golang.org/issue/13434")
+diff --git a/http2/writesched.go b/http2/writesched.go
+index c24316c..8b3d94e 100644
+--- a/http2/writesched.go
++++ b/http2/writesched.go
+@@ -36,6 +36,12 @@ func (wm frameWriteMsg) String() string {
+ return fmt.Sprintf("[frameWriteMsg stream=%d, ch=%v, type: %v]", streamID, wm.done != nil, des)
+ }
+
++// isControl reports whether wr is a control frame for MaxQueuedControlFrames
++// purposes. That includes non-stream frames and RST_STREAM frames.
++func (wm frameWriteMsg) isControl() bool {
++ return wm.stream == nil
++}
++
+ // writeScheduler tracks pending frames to write, priorities, and decides
+ // the next one to use. It is not thread-safe.
+ type writeScheduler struct {
diff -Nru golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/0002-Ensure-we-skip-all-IPv6-tests.patch golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/0002-Ensure-we-skip-all-IPv6-tests.patch
--- golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/0002-Ensure-we-skip-all-IPv6-tests.patch 1970-01-01 10:00:00.000000000 +1000
+++ golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/0002-Ensure-we-skip-all-IPv6-tests.patch 2020-12-04 09:10:25.000000000 +1100
@@ -0,0 +1,22 @@
+From: Brian May <brian@linuxpenguins.xyz>
+Date: Mon, 7 Dec 2020 08:28:17 +1100
+Subject: Ensure we skip all IPv6 tests
+
+---
+ ipv6/bpf_test.go | 3 +++
+ 1 file changed, 3 insertions(+)
+
+diff --git a/ipv6/bpf_test.go b/ipv6/bpf_test.go
+index 03d478d..8253e1f 100644
+--- a/ipv6/bpf_test.go
++++ b/ipv6/bpf_test.go
+@@ -18,6 +18,9 @@ func TestBPF(t *testing.T) {
+ if runtime.GOOS != "linux" {
+ t.Skipf("not supported on %s", runtime.GOOS)
+ }
++ if !supportsIPv6 {
++ t.Skip("ipv6 is not supported")
++ }
+
+ l, err := net.ListenPacket("udp6", "[::1]:0")
+ if err != nil {
diff -Nru golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/series golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/series
--- golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/series 1970-01-01 10:00:00.000000000 +1000
+++ golang-golang-x-net-dev-0.0+git20161013.8b4af36+dfsg/debian/patches/series 2020-12-04 09:10:25.000000000 +1100
@@ -0,0 +1,2 @@
+0001-http2-limit-number-of-control-frames-in-server-send-.patch
+0002-Ensure-we-skip-all-IPv6-tests.patch
Reply to: