Skip to content

Commit e197800

Browse files
author
Andreas Auernhammer
authored
s3v4: read and verify S3 signature v4 chunks separately (#11801)
This commit fixes a security issue in the signature v4 chunked reader. Before, the reader returned unverified data to the caller and would only verify the chunk signature once it has encountered the end of the chunk payload. Now, the chunk reader reads the entire chunk into an in-memory buffer, verifies the signature and then returns data to the caller. In general, this is a common security problem. We verifying data streams, the verifier MUST NOT return data to the upper layers / its callers as long as it has not verified the current data chunk / data segment: ``` func (r *Reader) Read(buffer []byte) { if err := r.readNext(r.internalBuffer); err != nil { return err } if err := r.verify(r.internalBuffer); err != nil { return err } copy(buffer, r.internalBuffer) } ```
1 parent 980311f commit e197800

File tree

1 file changed

+165
-132
lines changed

1 file changed

+165
-132
lines changed

Diff for: cmd/streaming-signature-v4.go

+165-132
Original file line numberDiff line numberDiff line change
@@ -166,158 +166,191 @@ func newSignV4ChunkedReader(req *http.Request) (io.ReadCloser, APIErrorCode) {
166166
seedDate: seedDate,
167167
region: region,
168168
chunkSHA256Writer: sha256.New(),
169-
state: readChunkHeader,
169+
buffer: make([]byte, 64*1024),
170170
}, ErrNone
171171
}
172172

173173
// Represents the overall state that is required for decoding a
174174
// AWS Signature V4 chunked reader.
175175
type s3ChunkedReader struct {
176-
reader *bufio.Reader
177-
cred auth.Credentials
178-
seedSignature string
179-
seedDate time.Time
180-
region string
181-
state chunkState
182-
lastChunk bool
183-
chunkSignature string
176+
reader *bufio.Reader
177+
cred auth.Credentials
178+
seedSignature string
179+
seedDate time.Time
180+
region string
181+
184182
chunkSHA256Writer hash.Hash // Calculates sha256 of chunk data.
185-
n uint64 // Unread bytes in chunk
183+
buffer []byte
184+
offset int
186185
err error
187186
}
188187

189-
// Read chunk reads the chunk token signature portion.
190-
func (cr *s3ChunkedReader) readS3ChunkHeader() {
191-
// Read the first chunk line until CRLF.
192-
var hexChunkSize, hexChunkSignature []byte
193-
hexChunkSize, hexChunkSignature, cr.err = readChunkLine(cr.reader)
194-
if cr.err != nil {
195-
return
196-
}
197-
// <hex>;token=value - converts the hex into its uint64 form.
198-
cr.n, cr.err = parseHexUint(hexChunkSize)
199-
if cr.err != nil {
200-
return
201-
}
202-
if cr.n == 0 {
203-
cr.err = io.EOF
204-
}
205-
// Save the incoming chunk signature.
206-
cr.chunkSignature = string(hexChunkSignature)
207-
}
208-
209-
type chunkState int
210-
211-
const (
212-
readChunkHeader chunkState = iota
213-
readChunkTrailer
214-
readChunk
215-
verifyChunk
216-
eofChunk
217-
)
218-
219-
func (cs chunkState) String() string {
220-
stateString := ""
221-
switch cs {
222-
case readChunkHeader:
223-
stateString = "readChunkHeader"
224-
case readChunkTrailer:
225-
stateString = "readChunkTrailer"
226-
case readChunk:
227-
stateString = "readChunk"
228-
case verifyChunk:
229-
stateString = "verifyChunk"
230-
case eofChunk:
231-
stateString = "eofChunk"
232-
233-
}
234-
return stateString
235-
}
236-
237188
func (cr *s3ChunkedReader) Close() (err error) {
238189
return nil
239190
}
240191

241192
// Read - implements `io.Reader`, which transparently decodes
242193
// the incoming AWS Signature V4 streaming signature.
243194
func (cr *s3ChunkedReader) Read(buf []byte) (n int, err error) {
195+
// First, if there is any unread data, copy it to the client
196+
// provided buffer.
197+
if cr.offset > 0 {
198+
n = copy(buf, cr.buffer[cr.offset:])
199+
if n == len(buf) {
200+
cr.offset += n
201+
return n, nil
202+
}
203+
cr.offset = 0
204+
buf = buf[n:]
205+
}
206+
207+
// Now, we read one chunk from the underlying reader.
208+
// A chunk has the following format:
209+
// <chunk-size-as-hex> + ";chunk-signature=" + <signature-as-hex> + "\r\n" + <payload> + "\r\n"
210+
//
211+
// Frist, we read the chunk size but fail if it is larger
212+
// than 1 MB. We must not accept arbitrary large chunks.
213+
// One 1 MB is a reasonable max limit.
214+
//
215+
// Then we read the signature and payload data. We compute the SHA256 checksum
216+
// of the payload and verify that it matches the expected signature value.
217+
//
218+
// The last chunk is *always* 0-sized. So, we must only return io.EOF if we have encountered
219+
// a chunk with a chunk size = 0. However, this chunk still has a signature and we must
220+
// verify it.
221+
const MaxSize = 1 << 20 // 1 MB
222+
var size int
244223
for {
245-
switch cr.state {
246-
case readChunkHeader:
247-
cr.readS3ChunkHeader()
248-
// If we're at the end of a chunk.
249-
if cr.n == 0 && cr.err == io.EOF {
250-
cr.state = readChunkTrailer
251-
cr.lastChunk = true
252-
continue
253-
}
254-
if cr.err != nil {
255-
return 0, cr.err
256-
}
257-
cr.state = readChunk
258-
case readChunkTrailer:
259-
cr.err = readCRLF(cr.reader)
260-
if cr.err != nil {
261-
return 0, errMalformedEncoding
262-
}
263-
cr.state = verifyChunk
264-
case readChunk:
265-
// There is no more space left in the request buffer.
266-
if len(buf) == 0 {
267-
return n, nil
268-
}
269-
rbuf := buf
270-
// The request buffer is larger than the current chunk size.
271-
// Read only the current chunk from the underlying reader.
272-
if uint64(len(rbuf)) > cr.n {
273-
rbuf = rbuf[:cr.n]
274-
}
275-
var n0 int
276-
n0, cr.err = cr.reader.Read(rbuf)
277-
if cr.err != nil {
278-
// We have lesser than chunk size advertised in chunkHeader, this is 'unexpected'.
279-
if cr.err == io.EOF {
280-
cr.err = io.ErrUnexpectedEOF
281-
}
282-
return 0, cr.err
283-
}
284-
285-
// Calculate sha256.
286-
cr.chunkSHA256Writer.Write(rbuf[:n0])
287-
// Update the bytes read into request buffer so far.
288-
n += n0
289-
buf = buf[n0:]
290-
// Update bytes to be read of the current chunk before verifying chunk's signature.
291-
cr.n -= uint64(n0)
292-
293-
// If we're at the end of a chunk.
294-
if cr.n == 0 {
295-
cr.state = readChunkTrailer
296-
continue
297-
}
298-
case verifyChunk:
299-
// Calculate the hashed chunk.
300-
hashedChunk := hex.EncodeToString(cr.chunkSHA256Writer.Sum(nil))
301-
// Calculate the chunk signature.
302-
newSignature := getChunkSignature(cr.cred, cr.seedSignature, cr.region, cr.seedDate, hashedChunk)
303-
if !compareSignatureV4(cr.chunkSignature, newSignature) {
304-
// Chunk signature doesn't match we return signature does not match.
305-
cr.err = errSignatureMismatch
306-
return 0, cr.err
307-
}
308-
// Newly calculated signature becomes the seed for the next chunk
309-
// this follows the chaining.
310-
cr.seedSignature = newSignature
311-
cr.chunkSHA256Writer.Reset()
312-
if cr.lastChunk {
313-
cr.state = eofChunk
314-
} else {
315-
cr.state = readChunkHeader
316-
}
317-
case eofChunk:
318-
return n, io.EOF
224+
b, err := cr.reader.ReadByte()
225+
if err == io.EOF {
226+
err = io.ErrUnexpectedEOF
319227
}
228+
if err != nil {
229+
cr.err = err
230+
return n, cr.err
231+
}
232+
if b == ';' { // separating character
233+
break
234+
}
235+
236+
// Manually deserialize the size since AWS specified
237+
// the chunk size to be of variable width. In particular,
238+
// a size of 16 is encoded as `10` while a size of 64 KB
239+
// is `10000`.
240+
switch {
241+
case b >= '0' && b <= '9':
242+
size = size<<4 | int(b-'0')
243+
case b >= 'a' && b <= 'f':
244+
size = size<<4 | int(b-('a'-10))
245+
case b >= 'A' && b <= 'F':
246+
size = size<<4 | int(b-('A'-10))
247+
default:
248+
cr.err = errMalformedEncoding
249+
return n, cr.err
250+
}
251+
if size > MaxSize {
252+
cr.err = errMalformedEncoding
253+
return n, cr.err
254+
}
255+
}
256+
257+
// Now, we read the signature of the following payload and expect:
258+
// chunk-signature=" + <signature-as-hex> + "\r\n"
259+
//
260+
// The signature is 64 bytes long (hex-encoded SHA256 hash) and
261+
// starts with a 16 byte header: len("chunk-signature=") + 64 == 80.
262+
var signature [80]byte
263+
_, err = io.ReadFull(cr.reader, signature[:])
264+
if err == io.EOF {
265+
err = io.ErrUnexpectedEOF
320266
}
267+
if err != nil {
268+
cr.err = err
269+
return n, cr.err
270+
}
271+
if !bytes.HasPrefix(signature[:], []byte("chunk-signature=")) {
272+
cr.err = errMalformedEncoding
273+
return n, cr.err
274+
}
275+
b, err := cr.reader.ReadByte()
276+
if err == io.EOF {
277+
err = io.ErrUnexpectedEOF
278+
}
279+
if err != nil {
280+
cr.err = err
281+
return n, cr.err
282+
}
283+
if b != '\r' {
284+
cr.err = errMalformedEncoding
285+
return n, cr.err
286+
}
287+
b, err = cr.reader.ReadByte()
288+
if err == io.EOF {
289+
err = io.ErrUnexpectedEOF
290+
}
291+
if err != nil {
292+
cr.err = err
293+
return n, cr.err
294+
}
295+
if b != '\n' {
296+
cr.err = errMalformedEncoding
297+
return n, cr.err
298+
}
299+
300+
if cap(cr.buffer) < size {
301+
cr.buffer = make([]byte, size)
302+
} else {
303+
cr.buffer = cr.buffer[:size]
304+
}
305+
306+
// Now, we read the payload and compute its SHA-256 hash.
307+
_, err = io.ReadFull(cr.reader, cr.buffer)
308+
if err == io.EOF && size != 0 {
309+
err = io.ErrUnexpectedEOF
310+
}
311+
if err != nil && err != io.EOF {
312+
cr.err = err
313+
return n, cr.err
314+
}
315+
b, err = cr.reader.ReadByte()
316+
if b != '\r' {
317+
cr.err = errMalformedEncoding
318+
return n, cr.err
319+
}
320+
b, err = cr.reader.ReadByte()
321+
if err == io.EOF {
322+
err = io.ErrUnexpectedEOF
323+
}
324+
if err != nil {
325+
cr.err = err
326+
return n, cr.err
327+
}
328+
if b != '\n' {
329+
cr.err = errMalformedEncoding
330+
return n, cr.err
331+
}
332+
333+
// Once we have read the entire chunk successfully, we verify
334+
// that the received signature matches our computed signature.
335+
cr.chunkSHA256Writer.Write(cr.buffer)
336+
newSignature := getChunkSignature(cr.cred, cr.seedSignature, cr.region, cr.seedDate, hex.EncodeToString(cr.chunkSHA256Writer.Sum(nil)))
337+
if !compareSignatureV4(string(signature[16:]), newSignature) {
338+
cr.err = errSignatureMismatch
339+
return n, cr.err
340+
}
341+
cr.seedSignature = newSignature
342+
cr.chunkSHA256Writer.Reset()
343+
344+
// If the chunk size is zero we return io.EOF. As specified by AWS,
345+
// only the last chunk is zero-sized.
346+
if size == 0 {
347+
cr.err = io.EOF
348+
return n, cr.err
349+
}
350+
351+
cr.offset = copy(buf, cr.buffer)
352+
n += cr.offset
353+
return n, err
321354
}
322355

323356
// readCRLF - check if reader only has '\r\n' CRLF character.

0 commit comments

Comments
 (0)