]> Cypherpunks repositories - gostls13.git/commitdiff
[release-branch.go1.17] encoding/pem: fix stack overflow in Decode
authorJulie Qiu <julie@golang.org>
Tue, 1 Mar 2022 16:19:38 +0000 (10:19 -0600)
committerDmitri Shuralyov <dmitshur@golang.org>
Tue, 12 Apr 2022 14:42:58 +0000 (14:42 +0000)
Previously, Decode called decodeError, a recursive function that was
prone to stack overflows when given a large PEM file containing errors.

Credit to Juho Nurminen of Mattermost who reported the error.

Fixes CVE-2022-24675
Updates #51853
Fixes #52036

Change-Id: Iffe768be53c8ddc0036fea0671d290f8f797692c
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1391157
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Filippo Valsorda <valsorda@google.com>
(cherry picked from commit 794ea5e828010e8b68493b2fc6d2963263195a02)
Reviewed-on: https://go-review.googlesource.com/c/go/+/399816
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

src/encoding/pem/pem.go
src/encoding/pem/pem_test.go

index a7272da5ad3a81575c3930fd405aafb824375004..1bee1c12d286f9d17108c126e10b0e21de963a32 100644 (file)
@@ -87,123 +87,97 @@ func Decode(data []byte) (p *Block, rest []byte) {
        // pemStart begins with a newline. However, at the very beginning of
        // the byte array, we'll accept the start string without it.
        rest = data
-       if bytes.HasPrefix(data, pemStart[1:]) {
-               rest = rest[len(pemStart)-1 : len(data)]
-       } else if i := bytes.Index(data, pemStart); i >= 0 {
-               rest = rest[i+len(pemStart) : len(data)]
-       } else {
-               return nil, data
-       }
-
-       typeLine, rest := getLine(rest)
-       if !bytes.HasSuffix(typeLine, pemEndOfLine) {
-               return decodeError(data, rest)
-       }
-       typeLine = typeLine[0 : len(typeLine)-len(pemEndOfLine)]
-
-       p = &Block{
-               Headers: make(map[string]string),
-               Type:    string(typeLine),
-       }
-
        for {
-               // This loop terminates because getLine's second result is
-               // always smaller than its argument.
-               if len(rest) == 0 {
+               if bytes.HasPrefix(rest, pemStart[1:]) {
+                       rest = rest[len(pemStart)-1:]
+               } else if i := bytes.Index(rest, pemStart); i >= 0 {
+                       rest = rest[i+len(pemStart) : len(rest)]
+               } else {
                        return nil, data
                }
-               line, next := getLine(rest)
 
-               i := bytes.IndexByte(line, ':')
-               if i == -1 {
-                       break
+               var typeLine []byte
+               typeLine, rest = getLine(rest)
+               if !bytes.HasSuffix(typeLine, pemEndOfLine) {
+                       continue
                }
+               typeLine = typeLine[0 : len(typeLine)-len(pemEndOfLine)]
 
-               // TODO(agl): need to cope with values that spread across lines.
-               key, val := line[:i], line[i+1:]
-               key = bytes.TrimSpace(key)
-               val = bytes.TrimSpace(val)
-               p.Headers[string(key)] = string(val)
-               rest = next
-       }
+               p = &Block{
+                       Headers: make(map[string]string),
+                       Type:    string(typeLine),
+               }
 
-       var endIndex, endTrailerIndex int
+               for {
+                       // This loop terminates because getLine's second result is
+                       // always smaller than its argument.
+                       if len(rest) == 0 {
+                               return nil, data
+                       }
+                       line, next := getLine(rest)
 
-       // If there were no headers, the END line might occur
-       // immediately, without a leading newline.
-       if len(p.Headers) == 0 && bytes.HasPrefix(rest, pemEnd[1:]) {
-               endIndex = 0
-               endTrailerIndex = len(pemEnd) - 1
-       } else {
-               endIndex = bytes.Index(rest, pemEnd)
-               endTrailerIndex = endIndex + len(pemEnd)
-       }
+                       i := bytes.IndexByte(line, ':')
+                       if i == -1 {
+                               break
+                       }
 
-       if endIndex < 0 {
-               return decodeError(data, rest)
-       }
+                       // TODO(agl): need to cope with values that spread across lines.
+                       key, val := line[:i], line[i+1:]
+                       key = bytes.TrimSpace(key)
+                       val = bytes.TrimSpace(val)
+                       p.Headers[string(key)] = string(val)
+                       rest = next
+               }
 
-       // After the "-----" of the ending line, there should be the same type
-       // and then a final five dashes.
-       endTrailer := rest[endTrailerIndex:]
-       endTrailerLen := len(typeLine) + len(pemEndOfLine)
-       if len(endTrailer) < endTrailerLen {
-               return decodeError(data, rest)
-       }
+               var endIndex, endTrailerIndex int
 
-       restOfEndLine := endTrailer[endTrailerLen:]
-       endTrailer = endTrailer[:endTrailerLen]
-       if !bytes.HasPrefix(endTrailer, typeLine) ||
-               !bytes.HasSuffix(endTrailer, pemEndOfLine) {
-               return decodeError(data, rest)
-       }
+               // If there were no headers, the END line might occur
+               // immediately, without a leading newline.
+               if len(p.Headers) == 0 && bytes.HasPrefix(rest, pemEnd[1:]) {
+                       endIndex = 0
+                       endTrailerIndex = len(pemEnd) - 1
+               } else {
+                       endIndex = bytes.Index(rest, pemEnd)
+                       endTrailerIndex = endIndex + len(pemEnd)
+               }
 
-       // The line must end with only whitespace.
-       if s, _ := getLine(restOfEndLine); len(s) != 0 {
-               return decodeError(data, rest)
-       }
+               if endIndex < 0 {
+                       continue
+               }
 
-       base64Data := removeSpacesAndTabs(rest[:endIndex])
-       p.Bytes = make([]byte, base64.StdEncoding.DecodedLen(len(base64Data)))
-       n, err := base64.StdEncoding.Decode(p.Bytes, base64Data)
-       if err != nil {
-               return decodeError(data, rest)
-       }
-       p.Bytes = p.Bytes[:n]
+               // After the "-----" of the ending line, there should be the same type
+               // and then a final five dashes.
+               endTrailer := rest[endTrailerIndex:]
+               endTrailerLen := len(typeLine) + len(pemEndOfLine)
+               if len(endTrailer) < endTrailerLen {
+                       continue
+               }
+
+               restOfEndLine := endTrailer[endTrailerLen:]
+               endTrailer = endTrailer[:endTrailerLen]
+               if !bytes.HasPrefix(endTrailer, typeLine) ||
+                       !bytes.HasSuffix(endTrailer, pemEndOfLine) {
+                       continue
+               }
 
-       // the -1 is because we might have only matched pemEnd without the
-       // leading newline if the PEM block was empty.
-       _, rest = getLine(rest[endIndex+len(pemEnd)-1:])
+               // The line must end with only whitespace.
+               if s, _ := getLine(restOfEndLine); len(s) != 0 {
+                       continue
+               }
 
-       return
-}
+               base64Data := removeSpacesAndTabs(rest[:endIndex])
+               p.Bytes = make([]byte, base64.StdEncoding.DecodedLen(len(base64Data)))
+               n, err := base64.StdEncoding.Decode(p.Bytes, base64Data)
+               if err != nil {
+                       continue
+               }
+               p.Bytes = p.Bytes[:n]
 
-func decodeError(data, rest []byte) (*Block, []byte) {
-       // If we get here then we have rejected a likely looking, but
-       // ultimately invalid PEM block. We need to start over from a new
-       // position. We have consumed the preamble line and will have consumed
-       // any lines which could be header lines. However, a valid preamble
-       // line is not a valid header line, therefore we cannot have consumed
-       // the preamble line for the any subsequent block. Thus, we will always
-       // find any valid block, no matter what bytes precede it.
-       //
-       // For example, if the input is
-       //
-       //    -----BEGIN MALFORMED BLOCK-----
-       //    junk that may look like header lines
-       //   or data lines, but no END line
-       //
-       //    -----BEGIN ACTUAL BLOCK-----
-       //    realdata
-       //    -----END ACTUAL BLOCK-----
-       //
-       // we've failed to parse using the first BEGIN line
-       // and now will try again, using the second BEGIN line.
-       p, rest := Decode(rest)
-       if p == nil {
-               rest = data
+               // the -1 is because we might have only matched pemEnd without the
+               // leading newline if the PEM block was empty.
+               _, rest = getLine(rest[endIndex+len(pemEnd)-1:])
+               return p, rest
        }
-       return p, rest
 }
 
 const pemLineLength = 64
index b2b6b15e736553295c60d008ba4c6e6b0a4f18b2..c94b5ca53b56887d6258f89b65d8459649c7e8ce 100644 (file)
@@ -107,6 +107,12 @@ const pemMissingEndingSpace = `
 dGVzdA==
 -----ENDBAR-----`
 
+const pemMissingEndLine = `
+-----BEGIN FOO-----
+Header: 1`
+
+var pemRepeatingBegin = strings.Repeat("-----BEGIN \n", 10)
+
 var badPEMTests = []struct {
        name  string
        input string
@@ -131,14 +137,34 @@ var badPEMTests = []struct {
                "missing ending space",
                pemMissingEndingSpace,
        },
+       {
+               "repeating begin",
+               pemRepeatingBegin,
+       },
+       {
+               "missing end line",
+               pemMissingEndLine,
+       },
 }
 
 func TestBadDecode(t *testing.T) {
        for _, test := range badPEMTests {
-               result, _ := Decode([]byte(test.input))
+               result, rest := Decode([]byte(test.input))
                if result != nil {
                        t.Errorf("unexpected success while parsing %q", test.name)
                }
+               if string(rest) != test.input {
+                       t.Errorf("unexpected rest: %q; want = %q", rest, test.input)
+               }
+       }
+}
+
+func TestCVE202224675(t *testing.T) {
+       // Prior to CVE-2022-24675, this input would cause a stack overflow.
+       input := []byte(strings.Repeat("-----BEGIN \n", 10000000))
+       result, rest := Decode(input)
+       if result != nil || !reflect.DeepEqual(rest, input) {
+               t.Errorf("Encode of %#v decoded as %#v", input, rest)
        }
 }