commit 5791268c391c9388d664c654e2278a50463c2c74 Author: David Fifield david@bamsoftware.com Date: Wed Oct 8 15:39:45 2014 -0700

Don't escape, just panic in formatline.

Backslash escaping in formatline was conflicting with pt-spec.txt's specified backslash escaping for SMETHOD ARGS.

https://trac.torproject.org/projects/tor/ticket/13370

Arguably we could just ignore any illegal bytes, but I made it panic in order to make any application bugs more obvious. --- pt.go | 56 +++++++++++++++++++++++++++++-------------- pt_test.go | 77 +++++++++++++++++++++++++++++++++++++----------------------- 2 files changed, 86 insertions(+), 47 deletions(-)

diff --git a/pt.go b/pt.go index 62dfc38..b02b9c2 100644 --- a/pt.go +++ b/pt.go @@ -201,35 +201,57 @@ func getenvRequired(key string) (string, error) { return value, nil }

-// Escape a string so it contains no byte values over 127 and doesn't contain -// any of the characters '\x00' or '\n'. -func escape(s string) string { - var buf bytes.Buffer - for _, b := range []byte(s) { - if b == '\n' { - buf.WriteString("\n") - } else if b == '\' { - buf.WriteString("\\") - } else if 0 < b && b < 128 { - buf.WriteByte(b) - } else { - fmt.Fprintf(&buf, "\x%02x", b) +// Returns true iff keyword contains only bytes allowed in a PT→Tor output line +// keyword. +// <KeywordChar> ::= <any US-ASCII alphanumeric, dash, and underscore> +func keywordIsSafe(keyword string) bool { + for _, b := range []byte(keyword) { + if b >= '0' && b <= '9' { + continue + } + if b >= 'A' && b <= 'Z' { + continue + } + if b >= 'a' && b <= 'z' { + continue } + if b == '-' || b == '_' { + continue + } + return false } - return buf.String() + return true +} + +// Returns true iff arg contains only bytes allowed in a PT→Tor output line arg. +// <ArgChar> ::= <any US-ASCII character but NUL or NL> +func argIsSafe(arg string) bool { + for _, b := range []byte(arg) { + if b >= '\x80' || b == '\x00' || b == '\n' { + return false + } + } + return true }

func formatline(keyword string, v ...string) string { var buf bytes.Buffer + if !keywordIsSafe(keyword) { + panic(fmt.Sprintf("keyword %q contains forbidden bytes", keyword)) + } buf.WriteString(keyword) for _, x := range v { - buf.WriteString(" " + escape(x)) + if !argIsSafe(x) { + panic(fmt.Sprintf("arg %q contains forbidden bytes", x)) + } + buf.WriteString(" " + x) } return buf.String() }

-// Print a pluggable transports protocol line to Stdout. The line consists of an -// unescaped keyword, followed by any number of escaped strings. +// Print a pluggable transports protocol line to Stdout. The line consists of a +// keyword followed by any number of space-separated arg strings. Panics if +// there are forbidden bytes in the keyword or the args (pt-spec.txt 2.2.1). func line(keyword string, v ...string) { fmt.Fprintln(Stdout, formatline(keyword, v...)) } diff --git a/pt_test.go b/pt_test.go index 456313a..983bf55 100644 --- a/pt_test.go +++ b/pt_test.go @@ -13,43 +13,60 @@ import ( "testing" )

-func stringIsSafe(s string) bool { - for _, c := range []byte(s) { - if c == '\x00' || c == '\n' || c > 127 { - return false +func TestKeywordIsSafe(t *testing.T) { + tests := [...]struct { + keyword string + expected bool + }{ + {"", true}, + {"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ-_", true}, + {"CMETHOD", true}, + {"CMETHOD:", false}, + {"a b c", false}, + {"CMETHOD\x7f", false}, + {"CMETHOD\x80", false}, + {"CMETHOD\x81", false}, + {"CMETHOD\xff", false}, + {"CMÉTHOD", false}, + } + + for _, input := range tests { + isSafe := keywordIsSafe(input.keyword) + if isSafe != input.expected { + t.Errorf("keywordIsSafe(%q) → %v (expected %v)", + input.keyword, isSafe, input.expected) } } - return true }

-func TestEscape(t *testing.T) { - tests := [...]string{ - "", - "abc", - "a\nb", - "a\b", - "ab\", - "ab\\n", - "ab\n\", +func TestArgIsSafe(t *testing.T) { + tests := [...]struct { + arg string + expected bool + }{ + {"", true}, + {"abc", true}, + {"127.0.0.1:8000", true}, + {"étude", false}, + {"a\nb", false}, + {"a\b", true}, + {"ab\", true}, + {"ab\\n", false}, + {"ab\n\", false}, + {"abc\x7f", true}, + {"abc\x80", false}, + {"abc\x81", false}, + {"abc\xff", false}, + {"abc\xff", false}, + {"var=GVsbG8\=", true}, }

- check := func(input string) { - output := escape(input) - if stringIsSafe(input) && input != output { - t.Errorf("escape(%q) → %q despite being safe", input, output) - } - if !stringIsSafe(output) { - t.Errorf("escape(%q) → %q is not safe", input, output) - } - } for _, input := range tests { - check(input) - } - for b := 0; b < 256; b++ { - // check one-byte string with each byte value 0–255 - check(string([]byte{byte(b)})) - // check UTF-8 encoding of each character 0–255 - check(string(b)) + isSafe := argIsSafe(input.arg) + if isSafe != input.expected { + t.Errorf("argIsSafe(%q) → %v (expected %v)", + input.arg, isSafe, input.expected) + } } }