From 7f95a85fd474b93e9f6441fa7f582f9bd089aeb7 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 28 Apr 2020 13:28:38 +0200 Subject: [PATCH] signer, log: properly escape character sequences (#20987) * signer: properly handle terminal escape characters * log: use strconv conversion instead of custom escape function * log: remove relection tests for nil --- log/format.go | 48 +++++++---------------------------- signer/core/cliui.go | 14 +++++++--- signer/core/signed_data.go | 12 ++++----- signer/fourbyte/abi.go | 14 +++++----- signer/fourbyte/validation.go | 4 +-- 5 files changed, 35 insertions(+), 57 deletions(-) diff --git a/log/format.go b/log/format.go index a1b5dac62..b63e60a01 100644 --- a/log/format.go +++ b/log/format.go @@ -358,49 +358,19 @@ func formatLogfmtValue(value interface{}, term bool) string { } } -var stringBufPool = sync.Pool{ - New: func() interface{} { return new(bytes.Buffer) }, -} - +// escapeString checks if the provided string needs escaping/quoting, and +// calls strconv.Quote if needed func escapeString(s string) string { - needsQuotes := false - needsEscape := false + needsQuoting := false for _, r := range s { - if r <= ' ' || r == '=' || r == '"' { - needsQuotes = true - } - if r == '\\' || r == '"' || r == '\n' || r == '\r' || r == '\t' { - needsEscape = true + // We quote everything below " (0x34) and above~ (0x7E), plus equal-sign + if r <= '"' || r > '~' || r == '=' { + needsQuoting = true + break } } - if !needsEscape && !needsQuotes { + if !needsQuoting { return s } - e := stringBufPool.Get().(*bytes.Buffer) - e.WriteByte('"') - for _, r := range s { - switch r { - case '\\', '"': - e.WriteByte('\\') - e.WriteByte(byte(r)) - case '\n': - e.WriteString("\\n") - case '\r': - e.WriteString("\\r") - case '\t': - e.WriteString("\\t") - default: - e.WriteRune(r) - } - } - e.WriteByte('"') - var ret string - if needsQuotes { - ret = e.String() - } else { - ret = string(e.Bytes()[1 : e.Len()-1]) - } - e.Reset() - stringBufPool.Put(e) - return ret + return strconv.Quote(s) } diff --git a/signer/core/cliui.go b/signer/core/cliui.go index 1e033299a..65114ac4b 100644 --- a/signer/core/cliui.go +++ b/signer/core/cliui.go @@ -85,10 +85,19 @@ func (ui *CommandlineUI) confirm() bool { return false } +// sanitize quotes and truncates 'txt' if longer than 'limit'. If truncated, +// and ellipsis is added after the quoted string +func sanitize(txt string, limit int) string { + if len(txt) > limit { + return fmt.Sprintf("%q...", txt[:limit]) + } + return fmt.Sprintf("%q", txt) +} + func showMetadata(metadata Metadata) { fmt.Printf("Request context:\n\t%v -> %v -> %v\n", metadata.Remote, metadata.Scheme, metadata.Local) fmt.Printf("\nAdditional HTTP header data, provided by the external caller:\n") - fmt.Printf("\tUser-Agent: %v\n\tOrigin: %v\n", metadata.UserAgent, metadata.Origin) + fmt.Printf("\tUser-Agent: %v\n\tOrigin: %v\n", sanitize(metadata.UserAgent, 200), sanitize(metadata.Origin, 100)) } // ApproveTx prompt the user for confirmation to request to sign Transaction @@ -113,7 +122,6 @@ func (ui *CommandlineUI) ApproveTx(request *SignTxRequest) (SignTxResponse, erro if request.Transaction.Data != nil { d := *request.Transaction.Data if len(d) > 0 { - fmt.Printf("data: %v\n", hexutil.Encode(d)) } } @@ -145,7 +153,7 @@ func (ui *CommandlineUI) ApproveSignData(request *SignDataRequest) (SignDataResp for _, nvt := range request.Messages { fmt.Printf("\u00a0\u00a0%v\n", strings.TrimSpace(nvt.Pprint(1))) } - fmt.Printf("raw data: \n%q\n", request.Rawdata) + fmt.Printf("raw data: \n\t%q\n", request.Rawdata) fmt.Printf("data hash: %v\n", request.Hash) fmt.Printf("-------------------------------------------\n") showMetadata(request.Meta) diff --git a/signer/core/signed_data.go b/signer/core/signed_data.go index 3a827afa2..dc41fadd9 100644 --- a/signer/core/signed_data.go +++ b/signer/core/signed_data.go @@ -827,23 +827,23 @@ func (t Types) validate() error { } for i, typeObj := range typeArr { if len(typeObj.Type) == 0 { - return fmt.Errorf("type %v:%d: empty Type", typeKey, i) + return fmt.Errorf("type %q:%d: empty Type", typeKey, i) } if len(typeObj.Name) == 0 { - return fmt.Errorf("type %v:%d: empty Name", typeKey, i) + return fmt.Errorf("type %q:%d: empty Name", typeKey, i) } if typeKey == typeObj.Type { - return fmt.Errorf("type '%s' cannot reference itself", typeObj.Type) + return fmt.Errorf("type %q cannot reference itself", typeObj.Type) } if typeObj.isReferenceType() { if _, exist := t[typeObj.typeName()]; !exist { - return fmt.Errorf("reference type '%s' is undefined", typeObj.Type) + return fmt.Errorf("reference type %q is undefined", typeObj.Type) } if !typedDataReferenceTypeRegexp.MatchString(typeObj.Type) { - return fmt.Errorf("unknown reference type '%s", typeObj.Type) + return fmt.Errorf("unknown reference type %q", typeObj.Type) } } else if !isPrimitiveTypeValid(typeObj.Type) { - return fmt.Errorf("unknown type '%s'", typeObj.Type) + return fmt.Errorf("unknown type %q", typeObj.Type) } } } diff --git a/signer/fourbyte/abi.go b/signer/fourbyte/abi.go index 007204c0f..d8fbabd3b 100644 --- a/signer/fourbyte/abi.go +++ b/signer/fourbyte/abi.go @@ -85,7 +85,7 @@ var selectorRegexp = regexp.MustCompile(`^([^\)]+)\(([A-Za-z0-9,\[\]]*)\)`) // parseSelector converts a method selector into an ABI JSON spec. The returned // data is a valid JSON string which can be consumed by the standard abi package. -func parseSelector(selector string) ([]byte, error) { +func parseSelector(unescapedSelector string) ([]byte, error) { // Define a tiny fake ABI struct for JSON marshalling type fakeArg struct { Type string `json:"type"` @@ -95,10 +95,10 @@ func parseSelector(selector string) ([]byte, error) { Type string `json:"type"` Inputs []fakeArg `json:"inputs"` } - // Validate the selector and extract it's components - groups := selectorRegexp.FindStringSubmatch(selector) + // Validate the unescapedSelector and extract it's components + groups := selectorRegexp.FindStringSubmatch(unescapedSelector) if len(groups) != 3 { - return nil, fmt.Errorf("invalid selector %s (%v matches)", selector, len(groups)) + return nil, fmt.Errorf("invalid selector %q (%v matches)", unescapedSelector, len(groups)) } name := groups[1] args := groups[2] @@ -115,7 +115,7 @@ func parseSelector(selector string) ([]byte, error) { // parseCallData matches the provided call data against the ABI definition and // returns a struct containing the actual go-typed values. -func parseCallData(calldata []byte, abidata string) (*decodedCallData, error) { +func parseCallData(calldata []byte, unescapedAbidata string) (*decodedCallData, error) { // Validate the call data that it has the 4byte prefix and the rest divisible by 32 bytes if len(calldata) < 4 { return nil, fmt.Errorf("invalid call data, incomplete method signature (%d bytes < 4)", len(calldata)) @@ -127,9 +127,9 @@ func parseCallData(calldata []byte, abidata string) (*decodedCallData, error) { return nil, fmt.Errorf("invalid call data; length should be a multiple of 32 bytes (was %d)", len(argdata)) } // Validate the called method and upack the call data accordingly - abispec, err := abi.JSON(strings.NewReader(abidata)) + abispec, err := abi.JSON(strings.NewReader(unescapedAbidata)) if err != nil { - return nil, fmt.Errorf("invalid method signature (%s): %v", abidata, err) + return nil, fmt.Errorf("invalid method signature (%q): %v", unescapedAbidata, err) } method, err := abispec.MethodById(sigdata) if err != nil { diff --git a/signer/fourbyte/validation.go b/signer/fourbyte/validation.go index fd13e0a63..baec32f72 100644 --- a/signer/fourbyte/validation.go +++ b/signer/fourbyte/validation.go @@ -98,7 +98,7 @@ func (db *Database) ValidateCallData(selector *string, data []byte, messages *co if info, err := verifySelector(*selector, data); err != nil { messages.Warn(fmt.Sprintf("Transaction contains data, but provided ABI signature could not be matched: %v", err)) } else { - messages.Info(info.String()) + messages.Info(fmt.Sprintf("Transaction invokes the following method: %q", info.String())) db.AddSelector(*selector, data[:4]) } return @@ -112,6 +112,6 @@ func (db *Database) ValidateCallData(selector *string, data []byte, messages *co if info, err := verifySelector(embedded, data); err != nil { messages.Warn(fmt.Sprintf("Transaction contains data, but provided ABI signature could not be verified: %v", err)) } else { - messages.Info(info.String()) + messages.Info(fmt.Sprintf("Transaction invokes the following method: %q", info.String())) } }