Skip to content

Commit

Permalink
simplify string escaping/unescaping
Browse files Browse the repository at this point in the history
fix escape/unescape edge cases detected by go-fuzz
escape filename in uri for DownloadFileByName
  • Loading branch information
armhold committed Oct 10, 2017
1 parent cad56a0 commit 4df8235
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 58 deletions.
2 changes: 1 addition & 1 deletion base/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -1038,7 +1038,7 @@ func mkRange(offset, size int64) string {

// DownloadFileByName wraps b2_download_file_by_name.
func (b *Bucket) DownloadFileByName(ctx context.Context, name string, offset, size int64) (*FileReader, error) {
uri := fmt.Sprintf("%s/file/%s/%s", b.b2.downloadURI, b.Name, name)
uri := fmt.Sprintf("%s/file/%s/%s", b.b2.downloadURI, b.Name, escape(name))
req, err := http.NewRequest("GET", uri, nil)
if err != nil {
return nil, err
Expand Down
73 changes: 73 additions & 0 deletions base/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -533,3 +533,76 @@ func TestEscapes(t *testing.T) {
}
}
}

func TestUploadDownloadFilenameEscaping(t *testing.T) {
filename := "file%foo.txt"

id := os.Getenv(apiID)
key := os.Getenv(apiKey)

if id == "" || key == "" {
t.Skipf("B2_ACCOUNT_ID or B2_SECRET_KEY unset; skipping integration tests")
}
ctx := context.Background()

// b2_authorize_account
b2, err := AuthorizeAccount(ctx, id, key, UserAgent("blazer-base-test"))
if err != nil {
t.Fatal(err)
}

// b2_create_bucket
bname := id + "-" + bucketName
bucket, err := b2.CreateBucket(ctx, bname, "", nil, nil)
if err != nil {
t.Fatal(err)
}

defer func() {
// b2_delete_bucket
if err := bucket.DeleteBucket(ctx); err != nil {
t.Error(err)
}
}()

if err != nil {

This comment has been minimized.

Copy link
@kurin

kurin Oct 10, 2017

This seems to be checking a function call that never happened.

This comment has been minimized.

Copy link
@armhold

armhold Oct 10, 2017

Author Owner

Ah, good catch. Looks like I left some mess behind while refactoring. I will fix before doing a PR.

t.Fatalf("error unescaping string: %s\n", err)
}

// b2_get_upload_url
ue, err := bucket.GetUploadURL(ctx)
if err != nil {
t.Fatal(err)
}

// b2_upload_file
smallFile := io.LimitReader(zReader{}, 128)
hash := sha1.New()
buf := &bytes.Buffer{}
w := io.MultiWriter(hash, buf)
if _, err := io.Copy(w, smallFile); err != nil {
t.Error(err)
}
smallSHA1 := fmt.Sprintf("%x", hash.Sum(nil))
file, err := ue.UploadFile(ctx, buf, buf.Len(), filename, "application/octet-stream", smallSHA1, nil)
if err != nil {
t.Fatal(err)
}

defer func() {
// b2_delete_file_version
if err := file.DeleteFileVersion(ctx); err != nil {
t.Error(err)
}
}()

// b2_download_file_by_name
fr, err := bucket.DownloadFileByName(ctx, filename, 0, 0)
if err != nil {
t.Fatal(err)
}
lbuf := &bytes.Buffer{}
if _, err := io.Copy(lbuf, fr); err != nil {
t.Fatal(err)
}
}
61 changes: 4 additions & 57 deletions base/strings.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,67 +15,14 @@
package base

import (
"bytes"
"errors"
"fmt"
"net/url"
"strings"
)

func noEscape(c byte) bool {
switch c {
case '.', '_', '-', '/', '~', '!', '$', '\'', '(', ')', '*', ';', '=', ':', '@':
return true
}
return false
}

func escape(s string) string {
// cribbed from url.go, kinda
b := &bytes.Buffer{}
for i := 0; i < len(s); i++ {
switch c := s[i]; {
case c == '/':
b.WriteByte(c)
case 'a' <= c && c <= 'z' || 'A' <= c && c <= 'Z' || '0' <= c && c <= '9':
b.WriteByte(c)
case noEscape(c):
b.WriteByte(c)
default:
fmt.Fprintf(b, "%%%X", c)
}
}
return b.String()
return strings.Replace(url.QueryEscape(s), "%2F", "/", -1)
}

func unescape(s string) (string, error) {
b := &bytes.Buffer{}
for i := 0; i < len(s); i++ {
c := s[i]
switch c {
case '/':
b.WriteString("/")
case '+':
b.WriteString(" ")
case '%':
if len(s)-i < 3 {
return "", errors.New("unescape: bad encoding")
}
b.WriteByte(unhex(s[i+1])<<4 | unhex(s[i+2]))
i += 2
default:
b.WriteByte(c)
}
}
return b.String(), nil
}

func unhex(c byte) byte {
switch {
case '0' <= c && c <= '9':
return c - '0'
case 'a' <= c && c <= 'f':
return c - 'a' + 10
case 'A' <= c && c <= 'F':
return c - 'A' + 10
}
return 0
return url.QueryUnescape(s)
}
52 changes: 52 additions & 0 deletions base/strings_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package base

import (
"testing"
"fmt"
)

func TestEncodeDecode(t *testing.T) {
// crashes identified by go-fuzz
origs := []string{
"&\x020000",
"&\x020000\x9c",
"&\x020\x9c0",
"&\x0230j",
"&\x02\x98000",
"&\x02\x983\xc8j00",
"00\x000",
"00\x0000",
"00\x0000000000000",
"\x11\x030",
}

for _, orig := range origs {
escaped := escape(orig)
unescaped, err := unescape(escaped)
if err != nil {
t.Errorf("%s: orig: %#v, escaped: %#v, unescaped: %#v\n", err.Error(), orig, escaped, unescaped)
continue
}

if unescaped != orig {
t.Errorf("expected: %#v, got: %#v", orig, unescaped)
}
}
}

// hook for go-fuzz: https://github.com/dvyukov/go-fuzz
func Fuzz(data []byte) int {
orig := string(data)
escaped := escape(orig)

unescaped, err := unescape(escaped)
if err != nil {
return 0
}

if unescaped != orig {
panic(fmt.Sprintf("unescaped: \"%#v\", != orig: \"%#v\"", unescaped, orig))
}

return 1
}

0 comments on commit 4df8235

Please sign in to comment.