ber.go: Protocol Validation and Defense

This commit attempts to address possible `panic` conditions due to access outside of the bounds of the slice.  There are specifically three changes:
 * A couple log messages are commented out due to them attempting to do an out of bounds view into the slice.
 * Existing protocol validation was fixed.  There was an attempt to guard against these conditions, however in many cases there was an off by one error, or other conditions had not been considered.
 * A PR from the parent fork which attempts to bring in a protocol fix for lengths between `0x80` and `0xFF` was also included as part of this: mozilla-services#68

The logic for slice access has been reviewed carefully in an attempt to make sure we are defensive, but still allowing the nuance of the PKCS7 structure.  In addition extensive fuzzing has been conducted on these changes.

ber.go

@@ -5,8 +5,6 @@ import (
 	"errors"
 )
 
-var encodeIndent = 0
-
 type asn1Object interface {
 	EncodeTo(writer *bytes.Buffer) error
 }
@@ -18,15 +16,13 @@ type asn1Structured struct {
 
 func (s asn1Structured) EncodeTo(out *bytes.Buffer) error {
 	//fmt.Printf("%s--> tag: % X\n", strings.Repeat("| ", encodeIndent), s.tagBytes)
-	encodeIndent++
 	inner := new(bytes.Buffer)
 	for _, obj := range s.content {
 		err := obj.EncodeTo(inner)
 		if err != nil {
 			return err
 		}
 	}
-	encodeIndent--
 	out.Write(s.tagBytes)
 	encodeLength(out, inner.Len())
 	out.Write(inner.Bytes())
@@ -67,10 +63,6 @@ func ber2der(ber []byte) ([]byte, error) {
 	}
 	obj.EncodeTo(out)
 
-	// if offset < len(ber) {
-	//	return nil, fmt.Errorf("ber2der: Content longer than expected. Got %d, expected %d", offset, len(ber))
-	//}
-
 	return out.Bytes(), nil
 }
 
@@ -149,14 +141,14 @@ func readObject(ber []byte, offset int) (asn1Object, int, error) {
 		for ber[offset] >= 0x80 {
 			tag = tag*128 + ber[offset] - 0x80
 			offset++
-			if offset > berLen {
+			if offset >= berLen {
 				return nil, 0, errors.New("ber2der: cannot move offset forward, end of ber data reached")
 			}
 		}
 		// jvehent 20170227: this doesn't appear to be used anywhere...
 		//tag = tag*128 + ber[offset] - 0x80
 		offset++
-		if offset > berLen {
+		if offset >= berLen {
 			return nil, 0, errors.New("ber2der: cannot move offset forward, end of ber data reached")
 		}
 	}
@@ -172,7 +164,10 @@ func readObject(ber []byte, offset int) (asn1Object, int, error) {
 	var length int
 	l := ber[offset]
 	offset++
-	if offset > berLen {
+	if l >= 0x80 && offset >= berLen {
+		// if indefinite or multibyte length, we need to verify there is at least one more byte available
+		// otherwise we need to be flexible here for length == 0 conditions
+		// validation that the length is available is done after the length is correctly parsed
 		return nil, 0, errors.New("ber2der: cannot move offset forward, end of ber data reached")
 	}
 	indefinite := false
@@ -184,17 +179,20 @@ func readObject(ber []byte, offset int) (asn1Object, int, error) {
 		if numberOfBytes == 4 && (int)(ber[offset]) > 0x7F {
 			return nil, 0, errors.New("ber2der: BER tag length is negative")
 		}
-		if (int)(ber[offset]) == 0x0 {
+		if offset + numberOfBytes > berLen {
+			// == condition is not checked here, this allows for a more descreptive error when the parsed length is
+			// compared with the remaining available bytes (`contentEnd > berLen`)
+			return nil, 0, errors.New("ber2der: cannot move offset forward, end of ber data reached")
+		}
+		if (int)(ber[offset]) == 0x0 && (numberOfBytes == 1 || ber[offset+1] <= 0x7F)  {
+			// `numberOfBytes == 1` is an important conditional to avoid a potential out of bounds panic with `ber[offset+1]`
 			return nil, 0, errors.New("ber2der: BER tag length has leading zero")
 		}
 		debugprint("--> (compute length) indicator byte: %x\n", l)
-		debugprint("--> (compute length) length bytes: % X\n", ber[offset:offset+numberOfBytes])
+		//debugprint("--> (compute length) length bytes: %x\n", ber[offset:offset+numberOfBytes])
 		for i := 0; i < numberOfBytes; i++ {
 			length = length*256 + (int)(ber[offset])
 			offset++
-			if offset > berLen {
-				return nil, 0, errors.New("ber2der: cannot move offset forward, end of ber data reached")
-			}
 		}
 	} else if l == 0x80 {
 		indefinite = true
@@ -206,12 +204,12 @@ func readObject(ber []byte, offset int) (asn1Object, int, error) {
 	}
 	//fmt.Printf("--> length        : %d\n", length)
 	contentEnd := offset + length
-	if contentEnd > len(ber) {
+	if contentEnd > berLen {
 		return nil, 0, errors.New("ber2der: BER tag length is more than available data")
 	}
 	debugprint("--> content start : %d\n", offset)
 	debugprint("--> content end   : %d\n", contentEnd)
-	debugprint("--> content       : % X\n", ber[offset:contentEnd])
+	//debugprint("--> content       : %x\n", ber[offset:contentEnd])
 	var obj asn1Object
 	if indefinite && kind == 0 {
 		return nil, 0, errors.New("ber2der: Indefinite form tag must have constructed encoding")

ber_test.go

@@ -44,7 +44,12 @@ func TestBer2Der_Negatives(t *testing.T) {
 		Input         []byte
 		ErrorContains string
 	}{
-		{[]byte{0x30, 0x85}, "tag length too long"},
+		{[]byte{}, "input ber is empty"},
+		{[]byte{0x30}, "cannot move offset forward, end of ber data reached"},
+		{[]byte{0x30, 0x08}, "BER tag length is more than available dat"},
+		{[]byte{0x30, 0x81}, "cannot move offset forward, end of ber data reached"},
+		{[]byte{0x30, 0x81, 0x00}, "BER tag length has leading zero"},
+		{[]byte{0x30, 0x85, 0x00}, "tag length too long"},
 		{[]byte{0x30, 0x84, 0x80, 0x0, 0x0, 0x0}, "length is negative"},
 		{[]byte{0x30, 0x82, 0x0, 0x1}, "length has leading zero"},
 		{[]byte{0x30, 0x80, 0x1, 0x2, 0x1, 0x2}, "Invalid BER format"},