From 8dad5a5f41b1b1b4864be5eb4c456ecdab2fb8ff Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Mon, 2 Apr 2018 12:50:45 +0200 Subject: [PATCH 1/3] Add test for append-only mode --- handlers_test.go | 191 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 191 insertions(+) diff --git a/handlers_test.go b/handlers_test.go index f6cdbaa..b91c012 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -1,7 +1,16 @@ package restserver import ( + "bytes" + "crypto/rand" + "encoding/hex" + "io" + "io/ioutil" + "net/http" + "net/http/httptest" + "os" "path/filepath" + "strings" "testing" ) @@ -58,3 +67,185 @@ func TestIsUserPath(t *testing.T) { } } } + +// declare a few helper functions + +// wantFunc tests the HTTP response in res and calls t.Error() if something is incorrect. +type wantFunc func(t testing.TB, res *httptest.ResponseRecorder) + +// newRequest returns a new HTTP request with the given params. On error, t.Fatal is called. +func newRequest(t testing.TB, method, path string, body io.Reader) *http.Request { + req, err := http.NewRequest(method, path, body) + if err != nil { + t.Fatal(err) + } + return req +} + +// wantCode returns a function which checks that the response has the correct HTTP status code. +func wantCode(code int) wantFunc { + return func(t testing.TB, res *httptest.ResponseRecorder) { + if res.Code != code { + t.Errorf("wrong response code, want %v, got %v", code, res.Code) + } + } +} + +// wantBody returns a function which checks that the response has the data in the body. +func wantBody(body string) wantFunc { + return func(t testing.TB, res *httptest.ResponseRecorder) { + if res.Body == nil { + t.Errorf("body is nil, want %q", body) + return + } + + if !bytes.Equal(res.Body.Bytes(), []byte(body)) { + t.Errorf("wrong response body, want:\n %q\ngot:\n %q", body, res.Body.Bytes()) + } + } +} + +// checkRequest uses f to process the request and runs the checker functions on the result. +func checkRequest(t testing.TB, f http.HandlerFunc, req *http.Request, want []wantFunc) { + rr := httptest.NewRecorder() + f(rr, req) + + for _, fn := range want { + fn(t, rr) + } +} + +// TestRequest is a sequence of HTTP requests with (optional) tests for the response. +type TestRequest struct { + req *http.Request + want []wantFunc +} + +// createOverwriteDeleteSeq returns a sequence which will create a new file at +// path, and then try to overwrite and delete it. +func createOverwriteDeleteSeq(t testing.TB, path string) []TestRequest { + // add a file, try to overwrite and delete it + req := []TestRequest{ + { + req: newRequest(t, "GET", path, nil), + want: []wantFunc{wantCode(http.StatusNotFound)}, + }, + { + req: newRequest(t, "POST", path, strings.NewReader("foobar test config")), + want: []wantFunc{wantCode(http.StatusOK)}, + }, + { + req: newRequest(t, "GET", path, nil), + want: []wantFunc{ + wantCode(http.StatusOK), + wantBody("foobar test config"), + }, + }, + { + req: newRequest(t, "POST", path, strings.NewReader("other config")), + want: []wantFunc{wantCode(http.StatusForbidden)}, + }, + { + req: newRequest(t, "GET", path, nil), + want: []wantFunc{ + wantCode(http.StatusOK), + wantBody("foobar test config"), + }, + }, + { + req: newRequest(t, "DELETE", path, nil), + want: []wantFunc{wantCode(http.StatusForbidden)}, + }, + { + req: newRequest(t, "GET", path, nil), + want: []wantFunc{ + wantCode(http.StatusOK), + wantBody("foobar test config"), + }, + }, + } + return req +} + +// TestResticHandler runs tests on the restic handler code, especially in append-only mode. +func TestResticHandler(t *testing.T) { + buf := make([]byte, 32) + _, err := io.ReadFull(rand.Reader, buf) + if err != nil { + t.Fatal(err) + } + randomID := hex.EncodeToString(buf) + + var tests = []struct { + seq []TestRequest + }{ + {createOverwriteDeleteSeq(t, "/config")}, + {createOverwriteDeleteSeq(t, "/data/"+randomID)}, + { + // ensure we can add and remove lock files + []TestRequest{ + { + req: newRequest(t, "GET", "/locks/"+randomID, nil), + want: []wantFunc{wantCode(http.StatusNotFound)}, + }, + { + req: newRequest(t, "POST", "/locks/"+randomID, strings.NewReader("lock file")), + want: []wantFunc{wantCode(http.StatusOK)}, + }, + { + req: newRequest(t, "GET", "/locks/"+randomID, nil), + want: []wantFunc{ + wantCode(http.StatusOK), + wantBody("lock file"), + }, + }, + { + req: newRequest(t, "POST", "/locks/"+randomID, strings.NewReader("other lock file")), + want: []wantFunc{wantCode(http.StatusForbidden)}, + }, + { + req: newRequest(t, "DELETE", "/locks/"+randomID, nil), + want: []wantFunc{wantCode(http.StatusOK)}, + }, + { + req: newRequest(t, "GET", "/locks/"+randomID, nil), + want: []wantFunc{wantCode(http.StatusNotFound)}, + }, + }, + }, + } + + // setup rclone with a local backend in a temporary directory + tempdir, err := ioutil.TempDir("", "rclone-restic-test-") + if err != nil { + t.Fatal(err) + } + + // make sure the tempdir is properly removed + defer func() { + err := os.RemoveAll(tempdir) + if err != nil { + t.Fatal(err) + } + }() + + // globally set append-only mode and configure path + Config.AppendOnly = true + Config.Path = tempdir + + mux := NewMux() + + // create the repo + checkRequest(t, mux.ServeHTTP, + newRequest(t, "POST", "/?create=true", nil), + []wantFunc{wantCode(http.StatusOK)}) + + for _, test := range tests { + t.Run("", func(t *testing.T) { + for i, seq := range test.seq { + t.Logf("request %v: %v %v", i, seq.req.Method, seq.req.URL.Path) + checkRequest(t, mux.ServeHTTP, seq.req, seq.want) + } + }) + } +} From 0f72176ddd8d2650524e8dac92a7225de32faf33 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Mon, 2 Apr 2018 13:02:16 +0200 Subject: [PATCH 2/3] Refuse writing the config in append-only mode --- handlers.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/handlers.go b/handlers.go index 1a61a8a..de3f328 100644 --- a/handlers.go +++ b/handlers.go @@ -206,22 +206,31 @@ func SaveConfig(w http.ResponseWriter, r *http.Request) { return } - bytes, err := ioutil.ReadAll(r.Body) - if err != nil { + f, err := os.OpenFile(cfg, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0600) + if err != nil && os.IsExist(err) { if Config.Debug { log.Print(err) } - http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest) + http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden) return } - if err := ioutil.WriteFile(cfg, bytes, 0600); err != nil { + _, err = io.Copy(f, r.Body) + if err != nil { if Config.Debug { log.Print(err) } http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) return } + + err = f.Close() + if err != nil { + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) + return + } + + _ = r.Body.Close() } // DeleteConfig removes a config. @@ -473,6 +482,11 @@ func SaveBlob(w http.ResponseWriter, r *http.Request) { } } + if os.IsExist(err) { + http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden) + return + } + if err != nil { if Config.Debug { log.Print(err) From 0f4f747b74581d21b3826b846d6458237609cbeb Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Mon, 2 Apr 2018 13:08:29 +0200 Subject: [PATCH 3/3] Add entry to changelog --- changelog/unreleased/pull-64 | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog/unreleased/pull-64 diff --git a/changelog/unreleased/pull-64 b/changelog/unreleased/pull-64 new file mode 100644 index 0000000..c5f9cd0 --- /dev/null +++ b/changelog/unreleased/pull-64 @@ -0,0 +1,8 @@ +Security: Refuse overwriting config file in append-only mode + +While working on the `rclone serve restic` command we noticed that is currently +possible to overwrite the config file in a repo even if `--append-only` is +specified. The first commit adds proper tests, and the second commit fixes the +issue. + +https://github.com/restic/rest-server/pull/64