Merge pull request #18 from dubyte/fix_http_trasversal_vulnerability

fix: http trasversal vulnerability #17

.github/workflows/go.yml

@@ -16,7 +16,7 @@ jobs:
     - name: Set up Go
       uses: actions/setup-go@v4
       with:
-        go-version: 1.21      
+        go-version: 1.22     
 
     - name: Build
       run: go build -v ./...

.github/workflows/release.yml

@@ -21,7 +21,7 @@ jobs:
         name: Set up Go
         uses: actions/setup-go@v4
         with:
-          go-version: 1.21 
+          go-version: 1.22 
       - 
         name: Test
         run: go test -v ./...

CHANGELOG.md

@@ -5,6 +5,26 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [1.0.6] - 2024-03-12
+
+### Security
+
+- Fix HTTP directory traversal vulnerability.
+- Dir parameter will be used as trusted root.
+
+### Changed
+
+- Trusted root is obtained from dir parameter after get the absolut and canonical path.
+- Module go version changed to 1.22
+
+### Fixed
+
+- Fix README.md error marked by linter
+
+### Added
+
+- Adding Make file
+
 ## [1.0.5] - 2024-01-30
 
 ### Changed

Makefile

@@ -0,0 +1,10 @@
+.DEFAULT_GOAL := build
+.PHONY: fmt vet build
+fmt:
+		go fmt ./...
+
+vet: fmt 
+		go vet ./...
+
+build: vet
+		go build .

README.md

@@ -1,50 +1,59 @@
 # dir2opds - serve books from a directory
+
  dir2opds inspects the given folder and serves an OPDS 1.1 compliant server.
 
-# Overview
- There are good options to serve books using OPDS. Calibre is good for
- that, but if your server is headless, installing Calibre doesn't seem to
- be the best option.
+## Overview
+
+There are good options for serving books using OPDS. Calibre is a popular
+choice, but if you have a headless server, installing Calibre might not be
+the best option.
+
+That's where calibre2opds comes in. However, if you have a large number of
+books and don't want to create a Calibre library, dir2opds can help you
+set up an OPDS server from a directory with one condition:
+
+- A folder should contain either only folders or only files.
 
- That is why calibre2opds exists, but if you have too many books and
- you don't want to create a Calibre library, dir2opds could help you to
- have an OPDS server from a directory with one condition:
+## Change log
 
- - A folder should have only folders or only files.
+- [Changelog](CHANGELOG.md)
 
-# Change log
-  - [Changelog](CHANGELOG.md)
+## Installation
 
-# Installation
-    go install github.com/dubyte/dir2opds@latest
+```bash
+go install github.com/dubyte/dir2opds@latest
+```
+
+## Usage
 
-# Usage
 ```bash
 Usage of dir2opds:
   -calibre
-    	Hide files stored by calibre
+      Hide files stored by calibre
   -debug
-    	If it is set, it will log the requests
+      If it is set, it will log the requests
   -dir string
-    	A directory with books (default "./books")
+      A directory with books (default "./books")
   -host string
-    	The server will listen in this host (default "0.0.0.0")
+      The server will listen in this host (default "0.0.0.0")
   -port string
-    	The server will listen in this port (default "8080")
-
+      The server will listen in this port (default "8080")
 ```
 
-# Tested on:
-   - Moon+ reader
+## Tested on
 
-# More information
-  - http://opds-spec.org
+- Moon+ reader
 
-# Binary release
-  - https://github.com/dubyte/dir2opds/releases
+## More information
 
+- <http://opds-spec.org>
+
+## Binary release
+
+- <https://github.com/dubyte/dir2opds/releases>
+
+### Raspberry pi deployment using binary release
 
-## Raspberry pi deployment using binary release
 ```bash
 cd && mkdir dir2opds && cd dir2opds
 
@@ -64,6 +73,7 @@ sudo systemctl start dir2opds.service
 ```
 
 /etc/systemd/system/dir2opds.service
+
 ```ini
 [Unit]
 Description=dir2opds
@@ -80,5 +90,11 @@ ExecStart=/home/pi/dir2opds/dir2opds -dir <FULL PATH OF BOOKS FOLDER> -port 8080
 WantedBy=multi-user.target
 ```
 
-# How to contribute
-  - [Contributing](CONTRIBUTING.md)
+## How to contribute
+
+- [Contributing](CONTRIBUTING.md)
+
+## Special thanks
+
+- @clach04: for testing and report missing content type for comics.
+- @masked-owl: for reporting security issue about http transversal.

go.mod

@@ -1,6 +1,6 @@
 module github.com/dubyte/dir2opds
 
-go 1.21
+go 1.22
 
 require (
 	github.com/lann/builder v0.0.0-20150808151131-f22ce00fd939

internal/service/service.go

@@ -6,6 +6,8 @@ package service
 import (
 	"bytes"
 	"encoding/xml"
+	"errors"
+	"fmt"
 	"log"
 	"mime"
 	"net/http"
@@ -35,7 +37,7 @@ const (
 )
 
 type OPDS struct {
-	DirRoot          string
+	TrustedRoot      string
 	IsCalibreLibrary bool
 }
 
@@ -62,14 +64,22 @@ func (s OPDS) Handler(w http.ResponseWriter, req *http.Request) error {
 		return err
 	}
 
-	fPath := filepath.Join(s.DirRoot, urlPath)
+	fPath := filepath.Join(s.TrustedRoot, urlPath)
+
+	// verifyPath avoid the http transversal by checking the path is under DirRoot
+	_, err = verifyPath(fPath, s.TrustedRoot)
+	if err != nil {
+		log.Printf("fPath %q err: %s", fPath, err)
+		w.WriteHeader(http.StatusNotFound)
+		return nil
+	}
 
 	log.Printf("urlPath:'%s'", urlPath)
 
 	if _, err := os.Stat(fPath); err != nil {
 		log.Printf("fPath err: %s", err)
 		w.WriteHeader(http.StatusNotFound)
-		return nil
+		return err
 	}
 
 	log.Printf("fPath:'%s'", fPath)
@@ -191,3 +201,28 @@ func timeNowFunc() func() time.Time {
 	t := time.Now()
 	return func() time.Time { return t }
 }
+
+// verify path use a trustedRoot to avoid http transversal
+// from https://www.stackhawk.com/blog/golang-path-traversal-guide-examples-and-prevention/
+func verifyPath(path, trustedRoot string) (string, error) {
+	// clean is already used upstream but leaving this
+	// to keep the functionality of the function as close as possible to the blog.
+	c := filepath.Clean(path)
+
+	// get the canonical path
+	r, err := filepath.EvalSymlinks(c)
+	if err != nil {
+		fmt.Println("Error " + err.Error())
+		return c, errors.New("unsafe or invalid path specified")
+	}
+
+	if !inTrustedRoot(r, trustedRoot) {
+		return r, errors.New("unsafe or invalid path specified")
+	}
+
+	return r, nil
+}
+
+func inTrustedRoot(path string, trustedRoot string) bool {
+	return strings.HasPrefix(path, trustedRoot)
+}

internal/service/service_test.go

@@ -23,11 +23,13 @@ func TestHandler(t *testing.T) {
 		input             string
 		want              string
 		WantedContentType string
+		wantedStatusCode  int
 	}{
-		"feed (dir of dirs )":           {input: "/", want: feed, WantedContentType: "application/atom+xml;profile=opds-catalog;kind=navigation"},
-		"acquisitionFeed(dir of files)": {input: "/mybook", want: acquisitionFeed, WantedContentType: "application/atom+xml;profile=opds-catalog;kind=acquisition"},
-		"servingAFile":                  {input: "/mybook/mybook.txt", want: "Fixture", WantedContentType: "text/plain; charset=utf-8"},
-		"serving file with spaces":      {input: "/mybook/mybook%20copy.txt", want: "Fixture", WantedContentType: "text/plain; charset=utf-8"},
+		"feed (dir of dirs )":                 {input: "/", want: feed, WantedContentType: "application/atom+xml;profile=opds-catalog;kind=navigation", wantedStatusCode: 200},
+		"acquisitionFeed(dir of files)":       {input: "/mybook", want: acquisitionFeed, WantedContentType: "application/atom+xml;profile=opds-catalog;kind=acquisition", wantedStatusCode: 200},
+		"servingAFile":                        {input: "/mybook/mybook.txt", want: "Fixture", WantedContentType: "text/plain; charset=utf-8", wantedStatusCode: 200},
+		"serving file with spaces":            {input: "/mybook/mybook%20copy.txt", want: "Fixture", WantedContentType: "text/plain; charset=utf-8", wantedStatusCode: 200},
+		"http trasversal vulnerability check": {input: "/../../../../mybook", want: feed, WantedContentType: "application/atom+xml;profile=opds-catalog;kind=navigation", wantedStatusCode: 404},
 	}
 
 	for name, tc := range tests {
@@ -50,7 +52,10 @@ func TestHandler(t *testing.T) {
 			require.NoError(t, err)
 
 			// verify
-			assert.Equal(t, 200, resp.StatusCode)
+			require.Equal(t, tc.wantedStatusCode, resp.StatusCode)
+			if tc.wantedStatusCode != http.StatusOK {
+				return
+			}
 			assert.Equal(t, tc.WantedContentType, resp.Header.Get("Content-Type"))
 			assert.Equal(t, tc.want, string(body))
 		})

main.go

@@ -23,6 +23,7 @@ import (
 	"io"
 	"log"
 	"net/http"
+	"path/filepath"
 
 	"github.com/dubyte/dir2opds/internal/service"
 )
@@ -44,8 +45,18 @@ func main() {
 	}
 
 	fmt.Println(startValues())
+	var err error
 
-	s := service.OPDS{DirRoot: *dirRoot, IsCalibreLibrary: *calibre}
+	// Use the absoluteCanonical path of the dir parm as the trustedRoot.
+	// helpfull avoid http trasversal. https://github.com/dubyte/dir2opds/issues/17
+	*dirRoot, err = absoluteCanonicalPath(*dirRoot)
+	if err != nil {
+		log.Fatal(err)
+	}
+
+	log.Printf("%q will be used as your trusted root", *dirRoot)
+
+	s := service.OPDS{TrustedRoot: *dirRoot, IsCalibreLibrary: *calibre}
 
 	http.HandleFunc("/", errorHandler(s.Handler))
 
@@ -66,3 +77,20 @@ func errorHandler(f func(http.ResponseWriter, *http.Request) error) http.Handler
 		}
 	}
 }
+
+// absoluteCanonicalPath returns the canonical path of the absolute path that was passed
+func absoluteCanonicalPath(aPath string) (string, error) {
+	// get absolute path
+	aPath, err := filepath.Abs(aPath)
+	if err != nil {
+		return "", fmt.Errorf("get absolute path %s: %w", aPath, err)
+	}
+
+	// get canonical path
+	aPath, err = filepath.EvalSymlinks(aPath)
+	if err != nil {
+		return "", fmt.Errorf("get connonical path from absolute path %s: %w", aPath, err)
+	}
+
+	return aPath, nil
+}

main_test.go

@@ -6,6 +6,8 @@ import (
 	"log"
 	"net/http"
 	"net/http/httptest"
+	"os"
+	"path"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -55,3 +57,35 @@ func TestErrorHandler(t *testing.T) {
 	// assert
 	assert.Contains(t, buf.String(), `handling "/": scary error`)
 }
+
+func Test_absoluteCannnonicalPath(t *testing.T) {
+	wd, err := os.Getwd()
+	if err != nil {
+		t.Errorf("not possible to get current dir")
+	}
+	type args struct {
+		aPath string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    string
+		wantErr bool
+	}{
+		{name: "dir relative path", args: args{aPath: "./opds"}, want: path.Join(wd, "opds"), wantErr: false},
+		{name: "dir not exists", args: args{aPath: "books"}, want: "", wantErr: true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := absoluteCanonicalPath(tt.args.aPath)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("absoluteCannnonicalPath() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+
+			if got != tt.want {
+				t.Errorf("absoluteCannnonicalPath() = %q, want %q", got, tt.want)
+			}
+		})
+	}
+}