From 0a089c8f328bff1c9b1fdaa30ab1282f28e251f7 Mon Sep 17 00:00:00 2001 From: Qingyang Hu <103950869+qingyang-hu@users.noreply.github.com> Date: Thu, 27 Jun 2024 12:56:42 -0400 Subject: [PATCH 1/3] GODRIVER-3240 [master] Code hardening --- bson/extjson_wrappers.go | 4 +- bson/uint_codec.go | 8 +++- bson/value_reader.go | 12 +++--- etc/run-atlas-test.sh | 4 +- .../cmd/testatlas/{main.go => atlas_test.go} | 18 ++++++--- internal/logger/io_sink.go | 7 +++- mongo/options/clientoptions.go | 15 ++++++- x/bsonx/bsoncore/bsoncore.go | 40 +++++++++---------- 8 files changed, 65 insertions(+), 43 deletions(-) rename internal/cmd/testatlas/{main.go => atlas_test.go} (82%) diff --git a/bson/extjson_wrappers.go b/bson/extjson_wrappers.go index 69ac78ccb5..e98b6428bc 100644 --- a/bson/extjson_wrappers.go +++ b/bson/extjson_wrappers.go @@ -92,9 +92,9 @@ func (ejv *extJSONValue) parseBinary() (b []byte, subType byte, err error) { return nil, 0, fmt.Errorf("$binary subType value should be string, but instead is %s", val.t) } - i, err := strconv.ParseInt(val.v.(string), 16, 64) + i, err := strconv.ParseUint(val.v.(string), 16, 8) if err != nil { - return nil, 0, fmt.Errorf("invalid $binary subType string: %s", val.v.(string)) + return nil, 0, fmt.Errorf("invalid $binary subType string: %q: %w", val.v.(string), err) } subType = byte(i) diff --git a/bson/uint_codec.go b/bson/uint_codec.go index 3bd752ad02..b8b4be398c 100644 --- a/bson/uint_codec.go +++ b/bson/uint_codec.go @@ -123,11 +123,15 @@ func (uic *uintCodec) decodeType(dc DecodeContext, vr ValueReader, t reflect.Typ return reflect.ValueOf(uint64(i64)), nil case reflect.Uint: - if i64 < 0 || int64(uint(i64)) != i64 { // Can we fit this inside of an uint + if i64 < 0 { + return emptyValue, fmt.Errorf("%d overflows uint", i64) + } + v := uint64(i64) + if v > math.MaxUint { // Can we fit this inside of an uint return emptyValue, fmt.Errorf("%d overflows uint", i64) } - return reflect.ValueOf(uint(i64)), nil + return reflect.ValueOf(uint(v)), nil default: return emptyValue, ValueDecoderError{ Name: "UintDecodeValue", diff --git a/bson/value_reader.go b/bson/value_reader.go index ac15a8044b..b6e72424d5 100644 --- a/bson/value_reader.go +++ b/bson/value_reader.go @@ -839,7 +839,7 @@ func (vr *valueReader) peekLength() (int32, error) { } idx := vr.offset - return (int32(vr.d[idx]) | int32(vr.d[idx+1])<<8 | int32(vr.d[idx+2])<<16 | int32(vr.d[idx+3])<<24), nil + return int32(binary.LittleEndian.Uint32(vr.d[idx:])), nil } func (vr *valueReader) readLength() (int32, error) { return vr.readi32() } @@ -851,7 +851,7 @@ func (vr *valueReader) readi32() (int32, error) { idx := vr.offset vr.offset += 4 - return (int32(vr.d[idx]) | int32(vr.d[idx+1])<<8 | int32(vr.d[idx+2])<<16 | int32(vr.d[idx+3])<<24), nil + return int32(binary.LittleEndian.Uint32(vr.d[idx:])), nil } func (vr *valueReader) readu32() (uint32, error) { @@ -861,7 +861,7 @@ func (vr *valueReader) readu32() (uint32, error) { idx := vr.offset vr.offset += 4 - return (uint32(vr.d[idx]) | uint32(vr.d[idx+1])<<8 | uint32(vr.d[idx+2])<<16 | uint32(vr.d[idx+3])<<24), nil + return binary.LittleEndian.Uint32(vr.d[idx:]), nil } func (vr *valueReader) readi64() (int64, error) { @@ -871,8 +871,7 @@ func (vr *valueReader) readi64() (int64, error) { idx := vr.offset vr.offset += 8 - return int64(vr.d[idx]) | int64(vr.d[idx+1])<<8 | int64(vr.d[idx+2])<<16 | int64(vr.d[idx+3])<<24 | - int64(vr.d[idx+4])<<32 | int64(vr.d[idx+5])<<40 | int64(vr.d[idx+6])<<48 | int64(vr.d[idx+7])<<56, nil + return int64(binary.LittleEndian.Uint64(vr.d[idx:])), nil } func (vr *valueReader) readu64() (uint64, error) { @@ -882,6 +881,5 @@ func (vr *valueReader) readu64() (uint64, error) { idx := vr.offset vr.offset += 8 - return uint64(vr.d[idx]) | uint64(vr.d[idx+1])<<8 | uint64(vr.d[idx+2])<<16 | uint64(vr.d[idx+3])<<24 | - uint64(vr.d[idx+4])<<32 | uint64(vr.d[idx+5])<<40 | uint64(vr.d[idx+6])<<48 | uint64(vr.d[idx+7])<<56, nil + return binary.LittleEndian.Uint64(vr.d[idx:]), nil } diff --git a/etc/run-atlas-test.sh b/etc/run-atlas-test.sh index 140b8734fe..5ddfcba78c 100644 --- a/etc/run-atlas-test.sh +++ b/etc/run-atlas-test.sh @@ -7,5 +7,5 @@ set +x # Get the atlas secrets. . ${DRIVERS_TOOLS}/.evergreen/secrets_handling/setup-secrets.sh drivers/atlas_connect -echo "Running cmd/testatlas/main.go" -go run ./internal/cmd/testatlas/main.go "$ATLAS_REPL" "$ATLAS_SHRD" "$ATLAS_FREE" "$ATLAS_TLS11" "$ATLAS_TLS12" "$ATLAS_SERVERLESS" "$ATLAS_SRV_REPL" "$ATLAS_SRV_SHRD" "$ATLAS_SRV_FREE" "$ATLAS_SRV_TLS11" "$ATLAS_SRV_TLS12" "$ATLAS_SRV_SERVERLESS" >> test.suite +echo "Running cmd/testatlas" +go test -v -run ^TestAtlas$ go.mongodb.org/mongo-driver/internal/cmd/testatlas -args "$ATLAS_REPL" "$ATLAS_SHRD" "$ATLAS_FREE" "$ATLAS_TLS11" "$ATLAS_TLS12" "$ATLAS_SERVERLESS" "$ATLAS_SRV_REPL" "$ATLAS_SRV_SHRD" "$ATLAS_SRV_FREE" "$ATLAS_SRV_TLS11" "$ATLAS_SRV_TLS12" "$ATLAS_SRV_SERVERLESS" >> test.suite diff --git a/internal/cmd/testatlas/main.go b/internal/cmd/testatlas/atlas_test.go similarity index 82% rename from internal/cmd/testatlas/main.go rename to internal/cmd/testatlas/atlas_test.go index 1bf2f8faff..8637511f3c 100644 --- a/internal/cmd/testatlas/main.go +++ b/internal/cmd/testatlas/atlas_test.go @@ -11,6 +11,8 @@ import ( "errors" "flag" "fmt" + "os" + "testing" "time" "go.mongodb.org/mongo-driver/bson" @@ -19,15 +21,19 @@ import ( "go.mongodb.org/mongo-driver/mongo/options" ) -func main() { +func TestMain(m *testing.M) { flag.Parse() + os.Exit(m.Run()) +} + +func TestAtlas(t *testing.T) { uris := flag.Args() ctx := context.Background() - fmt.Printf("Running atlas tests for %d uris\n", len(uris)) + t.Logf("Running atlas tests for %d uris\n", len(uris)) for idx, uri := range uris { - fmt.Printf("Running test %d\n", idx) + t.Logf("Running test %d\n", idx) // Set a low server selection timeout so we fail fast if there are errors. clientOpts := options.Client(). @@ -36,18 +42,18 @@ func main() { // Run basic connectivity test. if err := runTest(ctx, clientOpts); err != nil { - panic(fmt.Sprintf("error running test with TLS at index %d: %v", idx, err)) + t.Fatalf("error running test with TLS at index %d: %v", idx, err) } // Run the connectivity test with InsecureSkipVerify to ensure SNI is done correctly even if verification is // disabled. clientOpts.TLSConfig.InsecureSkipVerify = true if err := runTest(ctx, clientOpts); err != nil { - panic(fmt.Sprintf("error running test with tlsInsecure at index %d: %v", idx, err)) + t.Fatalf("error running test with tlsInsecure at index %d: %v", idx, err) } } - fmt.Println("Finished!") + t.Logf("Finished!") } func runTest(ctx context.Context, clientOpts *options.ClientOptions) error { diff --git a/internal/logger/io_sink.go b/internal/logger/io_sink.go index c5ff1474b4..0a6c1bdcab 100644 --- a/internal/logger/io_sink.go +++ b/internal/logger/io_sink.go @@ -9,6 +9,7 @@ package logger import ( "encoding/json" "io" + "math" "sync" "time" ) @@ -36,7 +37,11 @@ func NewIOSink(out io.Writer) *IOSink { // Info will write a JSON-encoded message to the io.Writer. func (sink *IOSink) Info(_ int, msg string, keysAndValues ...interface{}) { - kvMap := make(map[string]interface{}, len(keysAndValues)/2+2) + mapSize := len(keysAndValues) / 2 + if math.MaxInt-mapSize >= 2 { + mapSize += 2 + } + kvMap := make(map[string]interface{}, mapSize) kvMap[KeyTimestamp] = time.Now().UnixNano() kvMap[KeyMessage] = msg diff --git a/mongo/options/clientoptions.go b/mongo/options/clientoptions.go index b5fe1931ea..0ec15537cd 100644 --- a/mongo/options/clientoptions.go +++ b/mongo/options/clientoptions.go @@ -15,6 +15,7 @@ import ( "errors" "fmt" "io/ioutil" + "math" "net" "net/http" "strings" @@ -1166,7 +1167,19 @@ func addClientCertFromSeparateFiles(cfg *tls.Config, keyFile, certFile, keyPassw return "", err } - data := make([]byte, 0, len(keyData)+len(certData)+1) + keySize := len(keyData) + if keySize > 64*1024*1024 { + return "", errors.New("X.509 key must be less than 64 MiB") + } + certSize := len(certData) + if certSize > 64*1024*1024 { + return "", errors.New("X.509 certificate must be less than 64 MiB") + } + dataSize := keySize + certSize + 1 + if dataSize > math.MaxInt { + return "", errors.New("size overflow") + } + data := make([]byte, 0, dataSize) data = append(data, keyData...) data = append(data, '\n') data = append(data, certData...) diff --git a/x/bsonx/bsoncore/bsoncore.go b/x/bsonx/bsoncore/bsoncore.go index 6dc520dcec..25ed144de4 100644 --- a/x/bsonx/bsoncore/bsoncore.go +++ b/x/bsonx/bsoncore/bsoncore.go @@ -8,6 +8,7 @@ package bsoncore import ( "bytes" + "encoding/binary" "fmt" "math" "strconv" @@ -705,17 +706,16 @@ func ReserveLength(dst []byte) (int32, []byte) { // UpdateLength updates the length at index with length and returns the []byte. func UpdateLength(dst []byte, index, length int32) []byte { - dst[index] = byte(length) - dst[index+1] = byte(length >> 8) - dst[index+2] = byte(length >> 16) - dst[index+3] = byte(length >> 24) + binary.LittleEndian.PutUint32(dst[index:], uint32(length)) return dst } func appendLength(dst []byte, l int32) []byte { return appendi32(dst, l) } func appendi32(dst []byte, i32 int32) []byte { - return append(dst, byte(i32), byte(i32>>8), byte(i32>>16), byte(i32>>24)) + b := []byte{0, 0, 0, 0} + binary.LittleEndian.PutUint32(b, uint32(i32)) + return append(dst, b...) } // ReadLength reads an int32 length from src and returns the length and the remaining bytes. If @@ -733,27 +733,26 @@ func readi32(src []byte) (int32, []byte, bool) { if len(src) < 4 { return 0, src, false } - return (int32(src[0]) | int32(src[1])<<8 | int32(src[2])<<16 | int32(src[3])<<24), src[4:], true + return int32(binary.LittleEndian.Uint32(src)), src[4:], true } func appendi64(dst []byte, i64 int64) []byte { - return append(dst, - byte(i64), byte(i64>>8), byte(i64>>16), byte(i64>>24), - byte(i64>>32), byte(i64>>40), byte(i64>>48), byte(i64>>56), - ) + b := []byte{0, 0, 0, 0, 0, 0, 0, 0} + binary.LittleEndian.PutUint64(b, uint64(i64)) + return append(dst, b...) } func readi64(src []byte) (int64, []byte, bool) { if len(src) < 8 { return 0, src, false } - i64 := (int64(src[0]) | int64(src[1])<<8 | int64(src[2])<<16 | int64(src[3])<<24 | - int64(src[4])<<32 | int64(src[5])<<40 | int64(src[6])<<48 | int64(src[7])<<56) - return i64, src[8:], true + return int64(binary.LittleEndian.Uint64(src)), src[8:], true } func appendu32(dst []byte, u32 uint32) []byte { - return append(dst, byte(u32), byte(u32>>8), byte(u32>>16), byte(u32>>24)) + b := []byte{0, 0, 0, 0} + binary.LittleEndian.PutUint32(b, u32) + return append(dst, b...) } func readu32(src []byte) (uint32, []byte, bool) { @@ -761,23 +760,20 @@ func readu32(src []byte) (uint32, []byte, bool) { return 0, src, false } - return (uint32(src[0]) | uint32(src[1])<<8 | uint32(src[2])<<16 | uint32(src[3])<<24), src[4:], true + return binary.LittleEndian.Uint32(src), src[4:], true } func appendu64(dst []byte, u64 uint64) []byte { - return append(dst, - byte(u64), byte(u64>>8), byte(u64>>16), byte(u64>>24), - byte(u64>>32), byte(u64>>40), byte(u64>>48), byte(u64>>56), - ) + b := []byte{0, 0, 0, 0, 0, 0, 0, 0} + binary.LittleEndian.PutUint64(b, u64) + return append(dst, b...) } func readu64(src []byte) (uint64, []byte, bool) { if len(src) < 8 { return 0, src, false } - u64 := (uint64(src[0]) | uint64(src[1])<<8 | uint64(src[2])<<16 | uint64(src[3])<<24 | - uint64(src[4])<<32 | uint64(src[5])<<40 | uint64(src[6])<<48 | uint64(src[7])<<56) - return u64, src[8:], true + return binary.LittleEndian.Uint64(src), src[8:], true } // keep in sync with readcstringbytes From 1c6eb2811bf517edfb35edc3428900b81605147a Mon Sep 17 00:00:00 2001 From: Qingyang Hu Date: Thu, 27 Jun 2024 14:20:20 -0400 Subject: [PATCH 2/3] Manually update files. --- bson/default_value_decoders.go | 2 +- mongo/writeconcern/writeconcern.go | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/bson/default_value_decoders.go b/bson/default_value_decoders.go index 2075c0ad3d..ff331b6412 100644 --- a/bson/default_value_decoders.go +++ b/bson/default_value_decoders.go @@ -292,7 +292,7 @@ func intDecodeType(dc DecodeContext, vr ValueReader, t reflect.Type) (reflect.Va case reflect.Int64: return reflect.ValueOf(i64), nil case reflect.Int: - if int64(int(i64)) != i64 { // Can we fit this inside of an int + if i64 > math.MaxInt { // Can we fit this inside of an int return emptyValue, fmt.Errorf("%d overflows int", i64) } diff --git a/mongo/writeconcern/writeconcern.go b/mongo/writeconcern/writeconcern.go index 2e4d2ade16..8a18c175e1 100644 --- a/mongo/writeconcern/writeconcern.go +++ b/mongo/writeconcern/writeconcern.go @@ -13,6 +13,7 @@ package writeconcern import ( "errors" "fmt" + "math" "time" "go.mongodb.org/mongo-driver/bson" @@ -169,6 +170,9 @@ func (wc *WriteConcern) MarshalBSONValue() (bson.Type, []byte, error) { return 0, nil, ErrInconsistent } + if w > math.MaxInt32 { + return 0, nil, fmt.Errorf("%d overflows int32", w) + } elems = bsoncore.AppendInt32Element(elems, "w", int32(w)) case string: elems = bsoncore.AppendStringElement(elems, "w", w) From 194b9bddffadc31cb489df0fe0277cfc0e7c9f7b Mon Sep 17 00:00:00 2001 From: Steven Silvester Date: Fri, 28 Jun 2024 13:33:58 -0500 Subject: [PATCH 3/3] Update mongo/options/clientoptions.go Co-authored-by: Matt Dale <9760375+matthewdale@users.noreply.github.com> --- mongo/options/clientoptions.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mongo/options/clientoptions.go b/mongo/options/clientoptions.go index 0ec15537cd..05c00f5d22 100644 --- a/mongo/options/clientoptions.go +++ b/mongo/options/clientoptions.go @@ -1175,11 +1175,11 @@ func addClientCertFromSeparateFiles(cfg *tls.Config, keyFile, certFile, keyPassw if certSize > 64*1024*1024 { return "", errors.New("X.509 certificate must be less than 64 MiB") } - dataSize := keySize + certSize + 1 + dataSize := int64(keySize) + int64(certSize) + 1 if dataSize > math.MaxInt { return "", errors.New("size overflow") } - data := make([]byte, 0, dataSize) + data := make([]byte, 0, int(dataSize)) data = append(data, keyData...) data = append(data, '\n') data = append(data, certData...)