Bug: generated Unmarshal code can panic, more length checks needed

[akrennmair]

I played a bit with go-fuzz and found the following an issue in gogofast where the generated code can't deal with invalid input and panics.

Given this protobuf file named myproto.proto:

enum FOO { X = 17; }

message Test {
  required string label = 1;
  optional int32 type = 2 [default=77];
  repeated int64 reps = 3;

  message Embedded {
    optional double foo = 1;
  }

  repeated Embedded bar = 4;
}

I generated the corresponding myproto.pb.go with the command protoc --gogofast_out=. myproto.proto.

I then used go-fuzz to fuzz the Unmarshal method of the Test message. It found a number of crashers, from which I then created a unit test as a baseline:

package myproto

import (
    "testing"

    "github.com/gogo/protobuf/proto"
)

func TestGoFuzzCrashers(t *testing.T) {
    testData := []string{
        "\x03\x830\x830\x03\x03:\xc0\xc0\xbd\xbf\xefʽ\xbf\xef\x03",
        "\x03\"\x83\xc0ʃ\xc0\x83\xc0ʃ\x830",
        "\" ÿ\xef\x83\xc0\xc0\xbd\xbf\xef\u007f\x83\x8c00\xd40\x1a\xc0" + "\xbd\xbf\xef\xca\xc0\xca\uf0c300000",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x830\x03\x830\x830\x03\x830\x03\x03\x03\x830\x830" + "\x03\x03\x830\x830\"\x83\xc0\xc0\xbd\xbf\xefʃ\xbf\xef0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x03\x830\x830\x03\x830\x03\x03\x03\x830\x03\"\xe3\xbd" + "\xbfｿｿ\xef0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x830\x03\x830\x830\x03\x830\x03\x03\x03\x830\x830" + "\x03\x03\"\x83\xc0\xc0\xbd\xbf\xefʃ\xbf\xef0",
        "\" 00C\xba0\xdcٝ\xbd\xbf\xef\x83\xc0\xc0\xbd000" + "00000000000000",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x830\x03\x830\x83" + "0\xca0\xa1\xef\xca\xc0ʽ\xbf\xefʽ0",
        "\x03\x830\x830\xca0ｿ�\xbf\uf0c30",
        "\x03\x830\x03\x03:\xc0\xc0\xbd\xbf\xefʽ\xbf\xef\x03",
        "\x03\x03\x830\x03\x03\x03\x03\x03\x03\x830\x03\x03\x830\x830\x03\xfa" + "0\xd9\xe8ʃ\xc0ʃ\xc0\x83\x03",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x03\x830\x830\x03\x830\x03\x03\x03\x830\x830\x03\"" + "㽿ｿｿ\xef0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x03\x830\x03\"㽿ｿｿ\xef0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\xca0\xa1\xef\xca\xc0" + "ʽ\xbf\xefʽ0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x830\x03\x830\x830\x03\x830\x03\x03\x03\x830\x830" + "\x03\x03\x830\"\x83\xc0\xc0\xbd\xbf\xefʃ\xbf\xef0",
        "\x03\x830\x830\x03\x03\x830\xca0\x83\xc0\xc0\xbd\xbf\xefʽ\xbf" + "\xef0",
        "\x03\x03\x830\x03\x03\x03\x03\x830\x03\x03\x830\x03\x03\x830\x830" + "\x03\xfa0\xd9\xe8ʃ\xc0ʃ\xc0\x83\x03",
        "\x03\x830\xca0\xbd\xbf�\xbf\uf0c3\x13",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x03\x03\"㽿ｿｿ\xef0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x03\x830\x830\x03\x830\x03\x03\"㽿ｿ" + "ｿ\xef0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x830\x03\x830\x03\"\x83\xc0\xc0\xbd\xbf\xefʃ\xbf\xef" + "0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x830\x03\x830\x830\x03\x03\"\x83\xc0\xc0\xbd\xbf\xef\xca" + "\x83\xbf\xef0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x03\x03\"㽿ｿｿ\xef0",
        "\xba0ٿ\xbd��!",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\xca0\xa1\xef\xca\xc0\xca" + "\xbd\xbf\xefʽ0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x03\x830\x830\x03\x830\x03\x03\x03\x830\x830\x03\x83" + "0\"㽿ｿｿ\xef0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x03\x830\x830\x03\x03\"㽿ｿｿ" + "\xef0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x830\x03\x830\xca" + "0\xa1\xef\xca\xc0ʽ\xbf\xefʽ0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x03\x830\x830\x03\x830\x03\x03\x03\x03\"㽿\xef" + "\xbd\xbfｿ\xef0",
        "\" ÿ0\x83\xc0\xc0\xbd\xbf\xef0\x83\x8c00\xd40\x1a\xc0" + "\xbd\xbf\xef\xca\xc0\xca\uf0c300000",
        "\" z\xbd\xbf\xef\x83\xc0\xc0\xbd\xbf\xef\u007f0000000" + "00000000000000",
        "\x03\x830\x830\x03\x03\x03\x830\x03\xca0\xa1\xef\xca\xc0ʽ\xbf" + "\xefʽ0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\x830\x830\x03" + "\x03\x830\x830\x03\x830\x830\x03\x830\x03\x03\"\x83\xc0\xc0\xbd" + "\xbf\xefʃ\xbf\xef0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x03\xca0\xa1\xef\xca" + "\xc0ʽ\xbf\xefʽ0",
        "\x03\x830\x830\x03\x03\x03\x830\x830\x03\x03\x830\x03\xca0\xa1" + "\xef\xca\xc0ʽ\xbf\xefʽ0",
        "\x03\x830\x830\x03s\x03\x830\"\x83\xa1\x83\xb1\xca\xc4\xc4\xde\xd2" + "\x03",
        "\"Ϳ\x80\xb4ʃ\xa7\xc0\xca!",
    }

    for idx, testEntry := range testData {
        var test Test

        err := proto.Unmarshal([]byte(testEntry), &test)
        t.Logf("%d. err = %v", idx, err)
    }
}

I then manually fixed the bugs in the generated source code. Out came the following diff:

--- myproto.pb.go.orig  2015-07-29 19:21:23.000000000 +0200
+++ myproto.pb.go   2015-07-29 23:36:13.000000000 +0200
@@ -13,7 +13,10 @@
 */
 package myproto

-import proto "github.com/gogo/protobuf/proto"
+import (
+   "errors"
+   proto "github.com/gogo/protobuf/proto"
+)
 import math "math"

 import github.com_gogo_protobuf_proto "github.com/gogo/protobuf/proto"
@@ -368,6 +371,9 @@
                    break
                }
            }
+           if msglen < 0 {
+               return errInvalidLength
+           }
            postIndex := iNdEx + msglen
            if postIndex > l {
                return io.ErrUnexpectedEOF
@@ -466,6 +472,11 @@

    return nil
 }
+
+var (
+   errInvalidLength = errors.New("invalid length")
+)
+
 func skipMyproto(data []byte) (n int, err error) {
    l := len(data)
    iNdEx := 0
@@ -511,6 +522,9 @@
                    break
                }
            }
+           if length < 0 {
+               return 0, errInvalidLength
+           }
            iNdEx += length
            return iNdEx, nil
        case 3:

I hope you find this information useful and can reproduce the issue with this information. FTR, I haven't conducted a full check of all protobuf data types, or checked any of the other generators.

[awalterschulze]

Awesome work! :)

[awalterschulze]

I added my own fuzzing to the generated tests.
It eventually caught your bugs, but before that it caught many other non existent "length" < 0 checks.
I also fixed all these bugs.
8edb24c
Thank you very much for your efforts :)