runtime error: slice bounds out of range

vladtreny commented 2 years ago

Hello,

Sometimes unexpectedly get the following error

panic: runtime error: slice bounds out of range [:1378] with capacity 1315

github.com/google/certificate-transparency-go/asn1.parseField({0x9a98e0?, 0xc004b1b800?, 0xb?}, {0xc0119ec100, 0x562, 0x523}, 0x0, {0x0, 0x0, 0x0, ...})
#011/root/go/pkg/mod/github.com/google/certificate-transparency-go@v1.1.3/asn1/asn1.go:940 +0x31de
github.com/google/certificate-transparency-go/asn1.UnmarshalWithParams({0xc0119ec100, 0x562, 0x523}, {0x937a20?, 0xc004b1b800?}, {0x0, 0x0})
#011/root/go/pkg/mod/github.com/google/certificate-transparency-go@v1.1.3/asn1/asn1.go:1190 +0x1b7
github.com/google/certificate-transparency-go/asn1.Unmarshal(...)
#011/root/go/pkg/mod/github.com/google/certificate-transparency-go@v1.1.3/asn1/asn1.go:1183
github.com/google/certificate-transparency-go/x509.ParseCertificate({0xc0119ec100, 0x562, 0x523})
#011/root/go/pkg/mod/github.com/google/certificate-transparency-go@v1.1.3/x509/x509.go:2116 +0x95

pav-kv commented 2 years ago

@vladtreny Thanks for the report. Could you clarify the conditions under which you get this error? Are you running the CTFE server, or using x509 package standalone?

vladtreny commented 2 years ago

Hello, I get it from x509 package standalone on high concurrent usage, like 20k goroutines at the same time processing ssls. If see a possibility to reproduce it, will share

mhutchinson commented 2 years ago

I'm not familiar with this code, but I thought I'd take a look for anything obviously unsafe here. The very odd thing is that there's literally a check before the slice function that checks that the parameters are safe. Unless there's something wrong with invalidLength that I'm not seeing, the only other explanation I can see is that the array backing bytes somehow gets smaller. My guess here is that there are concurrent modifications to the input byte slice into the ParseCertificate function.

@vladtreny is it possible that you have code outside of these calls that modifies the slice passed in?

// invalidLength reports whether offset + length > sliceLength, or if the
// addition would overflow.
func invalidLength(offset, length, sliceLength int) bool {
    return offset+length < offset || offset+length > sliceLength
}

... {
    ...
    if invalidLength(offset, t.length, len(bytes)) {
        err = SyntaxError{"data truncated", params.name}
        return
    }
    innerBytes := bytes[offset : offset+t.length] // BOOM

vladtreny commented 2 years ago

Hello, I process a big array of ssls (like 7 billion).

I do not get this error on 1000 goroutines. But get it on 10k-20k using the same array. Maybe hitting OS limits somewhere, but it is a powerful sever and also tried huge os limits.

On every goroutine, I just parse cert and then loop the data, do not change slices

                cert, err := x509.ParseCertificate(crt.Certificate)
                if err != nil {
                    switch err.(type) {
                    case nil, x509.NonFatalErrors:
                        // ignore
                    default:
                        continue
                    }
                }

Anyway thank you so much for this library, works much better than included in go.

mhutchinson commented 2 years ago

It may be worthwhile to run this with the -race flag to use the go tooling to detect race conditions: https://go.dev/doc/articles/race_detector

google / certificate-transparency-go

runtime error: slice bounds out of range #941