-
Notifications
You must be signed in to change notification settings - Fork 889
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GODRIVER-3240 Code hardening. #1684
Changes from 3 commits
26a05fc
40319b3
0fb85cf
5ad5a60
839ae99
9b5bafd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,6 +8,7 @@ package bsonrw | |||||||||
|
||||||||||
import ( | ||||||||||
"encoding/base64" | ||||||||||
"encoding/binary" | ||||||||||
"errors" | ||||||||||
"fmt" | ||||||||||
"math" | ||||||||||
|
@@ -95,12 +96,14 @@ 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) | ||||||||||
b := []byte{0, 0, 0, 0, 0, 0, 0, 0} | ||||||||||
binary.LittleEndian.PutUint64(b, i) | ||||||||||
subType = b[0] | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we already apply the bounds check in the
Suggested change
|
||||||||||
stFound = true | ||||||||||
default: | ||||||||||
return nil, 0, fmt.Errorf("invalid key in $binary object: %s", key) | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -15,6 +15,7 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||
"errors" | ||||||||||||||||||||||||||||||||||||||||||||
"fmt" | ||||||||||||||||||||||||||||||||||||||||||||
"io/ioutil" | ||||||||||||||||||||||||||||||||||||||||||||
"math" | ||||||||||||||||||||||||||||||||||||||||||||
"net" | ||||||||||||||||||||||||||||||||||||||||||||
"net/http" | ||||||||||||||||||||||||||||||||||||||||||||
"strings" | ||||||||||||||||||||||||||||||||||||||||||||
|
@@ -1177,7 +1178,17 @@ func addClientCertFromSeparateFiles(cfg *tls.Config, keyFile, certFile, keyPassw | |||||||||||||||||||||||||||||||||||||||||||
return "", err | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
data := make([]byte, 0, len(keyData)+len(certData)+1) | ||||||||||||||||||||||||||||||||||||||||||||
dataSize := len(keyData) | ||||||||||||||||||||||||||||||||||||||||||||
certSize := len(certData) | ||||||||||||||||||||||||||||||||||||||||||||
if math.MaxInt-dataSize < certSize { | ||||||||||||||||||||||||||||||||||||||||||||
return "", errors.New("size overflow") | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
dataSize += certSize | ||||||||||||||||||||||||||||||||||||||||||||
if math.MaxInt-dataSize < 1 { | ||||||||||||||||||||||||||||||||||||||||||||
return "", errors.New("size overflow") | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
dataSize++ | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can simplify the logic here by adding some additional data size validation. AFAIK there's no case where an X.509 key and cert should be anywhere near 64MiB, so we can avoid the integer overflow and check for reasonable data size at the same time.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
data := make([]byte, 0, dataSize) | ||||||||||||||||||||||||||||||||||||||||||||
data = append(data, keyData...) | ||||||||||||||||||||||||||||||||||||||||||||
data = append(data, '\n') | ||||||||||||||||||||||||||||||||||||||||||||
data = append(data, certData...) | ||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a more accurate error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems better to keep the error message consistent with other unsigned types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable. We can fix them all at the same time. Consider this comment resolved.