Skip to content

Commit

Permalink
fix: prevent directory traversal attack when writing request files
Browse files Browse the repository at this point in the history
  • Loading branch information
nalgeon committed Jan 19, 2024
1 parent 02473a2 commit bca91d7
Show file tree
Hide file tree
Showing 5 changed files with 176 additions and 3 deletions.
13 changes: 10 additions & 3 deletions internal/engine/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"io"
"os"
"os/exec"
"path/filepath"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -54,7 +53,10 @@ func (e *Docker) Exec(req Request) Execution {
if e.cmd.Entry != "" {
// write request files to the temp directory
err = e.writeFiles(dir, req.Files)
if err != nil {
var argErr ArgumentError
if errors.As(err, &argErr) {
return Fail(req.ID, err)
} else if err != nil {
err = NewExecutionError("write files to temp dir", err)
return Fail(req.ID, err)
}
Expand Down Expand Up @@ -171,7 +173,12 @@ func (e *Docker) writeFiles(dir string, files Files) error {
if name == "" {
name = e.cmd.Entry
}
path := filepath.Join(dir, name)
var path string
path, err = fileio.JoinDir(dir, name)
if err != nil {
err = NewArgumentError(fmt.Sprintf("files[%s]", name), err)
return false
}
err = fileio.WriteFile(path, content, 0444)
return err == nil
})
Expand Down
24 changes: 24 additions & 0 deletions internal/engine/docker_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package engine

import (
"fmt"
"strings"
"testing"

Expand Down Expand Up @@ -231,6 +232,29 @@ func TestDockerRun(t *testing.T) {
t.Errorf("Stderr: unexpected value: %s", out.Stderr)
}
})

t.Run("directory traversal attack", func(t *testing.T) {
mem.Clear()
const fileName = "../../opt/codapi/codapi"
engine := NewDocker(dockerCfg, "python", "run")
req := Request{
ID: "http_42",
Sandbox: "python",
Command: "run",
Files: map[string]string{
"": "print('hello world')",
fileName: "hehe",
},
}
out := engine.Exec(req)
if out.OK {
t.Error("OK: expected false")
}
want := fmt.Sprintf("files[%s]: invalid name", fileName)
if out.Stderr != want {
t.Errorf("Stderr: unexpected value: %s", out.Stderr)
}
})
}

func TestDockerExec(t *testing.T) {
Expand Down
19 changes: 19 additions & 0 deletions internal/engine/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,25 @@ func (err ExecutionError) Unwrap() error {
return err.inner
}

// An ArgumentError is returned if code execution failed
// due to the invalid value of the request agrument.
type ArgumentError struct {
name string
reason error
}

func NewArgumentError(name string, reason error) ArgumentError {
return ArgumentError{name: name, reason: reason}
}

func (err ArgumentError) Error() string {
return err.name + ": " + err.reason.Error()
}

func (err ArgumentError) Unwrap() error {
return err.reason
}

// Files are a collection of files to be executed by the engine.
type Files map[string]string

Expand Down
29 changes: 29 additions & 0 deletions internal/fileio/fileio.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,35 @@ func WriteFile(path, content string, perm fs.FileMode) (err error) {
return os.WriteFile(path, data, perm)
}

// JoinDir joins a directory path with a relative file path,
// making sure that the resulting path is still inside the directory.
// Returns an error otherwise.
func JoinDir(dir string, name string) (string, error) {
if dir == "" {
return "", errors.New("invalid dir")
}

cleanName := filepath.Clean(name)
if cleanName == "" {
return "", errors.New("invalid name")
}
if cleanName == "." || cleanName == "/" || filepath.IsAbs(cleanName) {
return "", errors.New("invalid name")
}

path := filepath.Join(dir, cleanName)

dirPrefix := filepath.Clean(dir)
if dirPrefix != "/" {
dirPrefix += string(os.PathSeparator)
}
if !strings.HasPrefix(path, dirPrefix) {
return "", errors.New("invalid name")
}

return path, nil
}

// MkdirTemp creates a new temporary directory with given permissions
// and returns the pathname of the new directory.
func MkdirTemp(perm fs.FileMode) (string, error) {
Expand Down
94 changes: 94 additions & 0 deletions internal/fileio/fileio_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,100 @@ func TestWriteFile(t *testing.T) {
})
}

func TestJoinDir(t *testing.T) {
tests := []struct {
name string
dir string
filename string
want string
wantErr bool
}{
{
name: "regular join",
dir: "/home/user",
filename: "docs/report.txt",
want: "/home/user/docs/report.txt",
wantErr: false,
},
{
name: "join with dot",
dir: "/home/user",
filename: ".",
want: "",
wantErr: true,
},
{
name: "join with absolute path",
dir: "/home/user",
filename: "/etc/passwd",
want: "",
wantErr: true,
},
{
name: "join with parent directory",
dir: "/home/user",
filename: "../user2/docs/report.txt",
want: "",
wantErr: true,
},
{
name: "empty directory",
dir: "",
filename: "report.txt",
want: "",
wantErr: true,
},
{
name: "empty filename",
dir: "/home/user",
filename: "",
want: "",
wantErr: true,
},
{
name: "directory with trailing slash",
dir: "/home/user/",
filename: "docs/report.txt",
want: "/home/user/docs/report.txt",
wantErr: false,
},
{
name: "filename with leading slash",
dir: "/home/user",
filename: "/docs/report.txt",
want: "",
wantErr: true,
},
{
name: "root directory",
dir: "/",
filename: "report.txt",
want: "/report.txt",
wantErr: false,
},
{
name: "dot dot slash filename",
dir: "/home/user",
filename: "..",
want: "",
wantErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := JoinDir(tt.dir, tt.filename)
if (err != nil) != tt.wantErr {
t.Errorf("JoinDir() error = %v, wantErr %v", err, tt.wantErr)
return
}
if got != tt.want {
t.Errorf("JoinDir() = %v, want %v", got, tt.want)
}
})
}
}

func TestMkdirTemp(t *testing.T) {
t.Run("default permissions", func(t *testing.T) {
const perm = 0755
Expand Down

0 comments on commit bca91d7

Please sign in to comment.