From 632f7d6a2ab6ef13e4fce5fed8a222c7fb6da5c9 Mon Sep 17 00:00:00 2001 From: Thomas von Dein Date: Wed, 1 Mar 2023 19:48:49 +0100 Subject: [PATCH] added basic input validation/cleanup + tests --- upd/api/common.go | 25 +++++++++++++++++++++++++ upd/api/common_test.go | 25 +++++++++++++++++++++++++ upd/api/handlers.go | 36 +++++++++++++++++++++--------------- 3 files changed, 71 insertions(+), 15 deletions(-) diff --git a/upd/api/common.go b/upd/api/common.go index e5bb762..a3f4a0a 100644 --- a/upd/api/common.go +++ b/upd/api/common.go @@ -18,6 +18,7 @@ along with this program. If not, see . package api import ( + "errors" "fmt" "regexp" "strconv" @@ -105,3 +106,27 @@ func IsExpired(start time.Time, duration string) bool { return false } + +/* + Untaint user input, that is: remove all non supported chars. + + wanted is a regexp matching chars we shall leave. Everything else + will be removed. Eg: + + untainted := Untaint(input, `[^a-zA-Z0-9\-]`) + + Returns a new string and an error if the input string has been + modified. It's the callers choice to decide what to do about + it. You may ignore the error and use the untainted string or bail + out. +*/ +func Untaint(input string, wanted string) (string, error) { + re := regexp.MustCompile(wanted) + untainted := re.ReplaceAllString(input, "") + + if len(untainted) != len(input) { + return untainted, errors.New("Invalid input string!") + } + + return untainted, nil +} diff --git a/upd/api/common_test.go b/upd/api/common_test.go index dedc70a..8a515ba 100644 --- a/upd/api/common_test.go +++ b/upd/api/common_test.go @@ -51,3 +51,28 @@ func TestIsExpired(t *testing.T) { }) } } + +func TestUntaint(t *testing.T) { + var tests = []struct { + want string + input string + expect string + wanterr bool + }{ + {`[^a-zA-Z0-9\-]`, "ab23-bb43-beef", "ab23-bb43-beef", false}, + {`[^a-zA-Z0-9\-]`, "`cat passwd`+ab23-bb43-beef", "catpasswdab23-bb43-beef", true}, + } + + for _, tt := range tests { + testname := fmt.Sprintf("untaint-%s-%s", tt.want, tt.expect) + t.Run(testname, func(t *testing.T) { + untainted, err := Untaint(tt.input, tt.want) + if untainted != tt.expect { + t.Errorf("got %s, want %s", untainted, tt.expect) + } + if err != nil && !tt.wanterr { + t.Errorf("got error: %s", err) + } + }) + } +} diff --git a/upd/api/handlers.go b/upd/api/handlers.go index 5595a7f..7443468 100644 --- a/upd/api/handlers.go +++ b/upd/api/handlers.go @@ -56,10 +56,8 @@ func FilePut(c *fiber.Ctx, cfg *cfg.Config, db *Db) (string, error) { return "", err } - //entry := &Upload{Id: id, Uploaded: time.Now()} - entry := &Upload{Id: id, Uploaded: Timestamp{Time: time.Now()}} - // init upload obj + entry := &Upload{Id: id, Uploaded: Timestamp{Time: time.Now()}} // retrieve files, if any files := form.File["upload[]"] @@ -83,7 +81,11 @@ func FilePut(c *fiber.Ctx, cfg *cfg.Config, db *Db) (string, error) { if len(formdata.Expire) == 0 { entry.Expire = "asap" } else { - entry.Expire = formdata.Expire // FIXME: validate + ex, err := Untaint(formdata.Expire, `[dhms0-9]`) // duration or asap allowed + if err != nil { + return "", err + } + entry.Expire = ex } if len(entry.Members) == 1 { @@ -131,10 +133,14 @@ func FilePut(c *fiber.Ctx, cfg *cfg.Config, db *Db) (string, error) { } func FileGet(c *fiber.Ctx, cfg *cfg.Config, db *Db) error { - // deliver a file and delete it after a (configurable?) delay + // deliver a file and delete it if expire is set to asap - id := c.Params("id") - file := c.Params("file") + // we ignore c.Params("file"), cause it may be malign. Also we've + // got it in the db anyway + id, err := Untaint(c.Params("id"), `[^a-zA-Z0-9\-]`) + if err != nil { + return fiber.NewError(403, "Invalid id provided!") + } upload, err := db.Lookup(id) if err != nil { @@ -142,10 +148,7 @@ func FileGet(c *fiber.Ctx, cfg *cfg.Config, db *Db) error { return fiber.NewError(404, "No download with that id could be found!") } - if len(file) == 0 { - // actual file name is optional - file = upload.File - } + file := upload.File filename := filepath.Join(cfg.StorageDir, id, file) @@ -158,10 +161,10 @@ func FileGet(c *fiber.Ctx, cfg *cfg.Config, db *Db) error { err = c.Download(filename, file) go func() { - // check if we need to delete the file now + // check if we need to delete the file now and do it in the background if upload.Expire == "asap" { cleanup(filepath.Join(cfg.StorageDir, id)) - go db.Delete(id) + db.Delete(id) } }() @@ -175,7 +178,10 @@ type Id struct { func FileDelete(c *fiber.Ctx, cfg *cfg.Config, db *Db) error { // delete file, id dir and db entry - id := c.Params("id") + id, err := Untaint(c.Params("id"), `[^a-zA-Z0-9\-]`) + if err != nil { + return fiber.NewError(403, "Invalid id provided!") + } // try: path, body(json), query param if len(id) == 0 { @@ -193,7 +199,7 @@ func FileDelete(c *fiber.Ctx, cfg *cfg.Config, db *Db) error { cleanup(filepath.Join(cfg.StorageDir, id)) - err := db.Delete(id) + err = db.Delete(id) if err != nil { // non existent db entry with that id, or other db error, see logs return fiber.NewError(404, "No upload with that id could be found!")