1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
|
From fa98f46741f818913a8c11b877520a548715131f Mon Sep 17 00:00:00 2001
From: Russ Cox <rsc@golang.org>
Date: Mon, 13 Jul 2020 13:27:22 -0400
Subject: [PATCH] net/http: synchronize "100 Continue" write and Handler writes
The expectContinueReader writes to the connection on the first
Request.Body read. Since a Handler might be doing a read in parallel or
before a write, expectContinueReader needs to synchronize with the
ResponseWriter, and abort if a response already went out.
The tests will land in a separate CL.
Fixes #34902
Fixes CVE-2020-15586
Change-Id: Icdd8dd539f45e8863762bd378194bb4741e875fc
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/793350
Reviewed-by: Filippo Valsorda <valsorda@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/242598
Run-TryBot: Katie Hockman <katie@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Upstream-Status: Backport
CVE: CVE-2020-15586
Signed-off-by: Li Zhou <li.zhou@windriver.com>
---
src/net/http/server.go | 43 +++++++++++++++++++++++++++++++++++-------
1 file changed, 36 insertions(+), 7 deletions(-)
diff --git a/src/net/http/server.go b/src/net/http/server.go
index a995a50658..d41b5f6f48 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -425,6 +425,16 @@ type response struct {
wants10KeepAlive bool // HTTP/1.0 w/ Connection "keep-alive"
wantsClose bool // HTTP request has Connection "close"
+ // canWriteContinue is a boolean value accessed as an atomic int32
+ // that says whether or not a 100 Continue header can be written
+ // to the connection.
+ // writeContinueMu must be held while writing the header.
+ // These two fields together synchronize the body reader
+ // (the expectContinueReader, which wants to write 100 Continue)
+ // against the main writer.
+ canWriteContinue atomicBool
+ writeContinueMu sync.Mutex
+
w *bufio.Writer // buffers output in chunks to chunkWriter
cw chunkWriter
@@ -515,6 +525,7 @@ type atomicBool int32
func (b *atomicBool) isSet() bool { return atomic.LoadInt32((*int32)(b)) != 0 }
func (b *atomicBool) setTrue() { atomic.StoreInt32((*int32)(b), 1) }
+func (b *atomicBool) setFalse() { atomic.StoreInt32((*int32)(b), 0) }
// declareTrailer is called for each Trailer header when the
// response header is written. It notes that a header will need to be
@@ -878,21 +889,27 @@ type expectContinueReader struct {
resp *response
readCloser io.ReadCloser
closed bool
- sawEOF bool
+ sawEOF atomicBool
}
func (ecr *expectContinueReader) Read(p []byte) (n int, err error) {
if ecr.closed {
return 0, ErrBodyReadAfterClose
}
- if !ecr.resp.wroteContinue && !ecr.resp.conn.hijacked() {
- ecr.resp.wroteContinue = true
- ecr.resp.conn.bufw.WriteString("HTTP/1.1 100 Continue\r\n\r\n")
- ecr.resp.conn.bufw.Flush()
+ w := ecr.resp
+ if !w.wroteContinue && w.canWriteContinue.isSet() && !w.conn.hijacked() {
+ w.wroteContinue = true
+ w.writeContinueMu.Lock()
+ if w.canWriteContinue.isSet() {
+ w.conn.bufw.WriteString("HTTP/1.1 100 Continue\r\n\r\n")
+ w.conn.bufw.Flush()
+ w.canWriteContinue.setFalse()
+ }
+ w.writeContinueMu.Unlock()
}
n, err = ecr.readCloser.Read(p)
if err == io.EOF {
- ecr.sawEOF = true
+ ecr.sawEOF.setTrue()
}
return
}
@@ -1311,7 +1328,7 @@ func (cw *chunkWriter) writeHeader(p []byte) {
// because we don't know if the next bytes on the wire will be
// the body-following-the-timer or the subsequent request.
// See Issue 11549.
- if ecr, ok := w.req.Body.(*expectContinueReader); ok && !ecr.sawEOF {
+ if ecr, ok := w.req.Body.(*expectContinueReader); ok && !ecr.sawEOF.isSet() {
w.closeAfterReply = true
}
@@ -1561,6 +1578,17 @@ func (w *response) write(lenData int, dataB []byte, dataS string) (n int, err er
}
return 0, ErrHijacked
}
+
+ if w.canWriteContinue.isSet() {
+ // Body reader wants to write 100 Continue but hasn't yet.
+ // Tell it not to. The store must be done while holding the lock
+ // because the lock makes sure that there is not an active write
+ // this very moment.
+ w.writeContinueMu.Lock()
+ w.canWriteContinue.setFalse()
+ w.writeContinueMu.Unlock()
+ }
+
if !w.wroteHeader {
w.WriteHeader(StatusOK)
}
@@ -1872,6 +1900,7 @@ func (c *conn) serve(ctx context.Context) {
if req.ProtoAtLeast(1, 1) && req.ContentLength != 0 {
// Wrap the Body reader with one that replies on the connection
req.Body = &expectContinueReader{readCloser: req.Body, resp: w}
+ w.canWriteContinue.setTrue()
}
} else if req.Header.get("Expect") != "" {
w.sendExpectationFailed()
--
2.17.1
|
