Skip to content

Commit

Permalink
[Issue #220] - dcRUSTy - Fix DOS vulnerability related to scanning sy…
Browse files Browse the repository at this point in the history
…mlinks.

* [Issue #220] - dcRUSTy - Fix bug that crashed test on Windows
* [Issue #220] - dcRUSTy - Implement utility function wrapper for ioutil.ReadFile which skips following symlink
  • Loading branch information
dcRUSTy authored Aug 13, 2020
1 parent 440eee6 commit 0855689
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 7 deletions.
4 changes: 2 additions & 2 deletions directory_hook.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package main

import (
"io/ioutil"
"talisman/gitrepo"
"talisman/utility"

log "github.com/Sirupsen/logrus"

Expand Down Expand Up @@ -35,5 +35,5 @@ func (p *DirectoryHook) GetFilesFromDirectory(globPattern string) []gitrepo.Addi

func ReadFile(filepath string) ([]byte, error) {
log.Debugf("reading file %s", filepath)
return ioutil.ReadFile(filepath)
return utility.SafeReadFile(filepath)
}
2 changes: 1 addition & 1 deletion git_testing/git_testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func Init(gitRoot string) *GitTesting {
os.MkdirAll(gitRoot, 0777)
testingRepo := &GitTesting{gitRoot}
testingRepo.ExecCommand("git", "init", ".")
gitConfigFileObject, _ := ioutil.TempFile("/tmp", "gitConfigForTalismanTests")
gitConfigFileObject, _ := ioutil.TempFile(os.TempDir(), "gitConfigForTalismanTests")
gitConfigFile = gitConfigFileObject.Name()
testingRepo.CreateFileWithContents(gitConfigFile, `[user]
email = [email protected]
Expand Down
4 changes: 2 additions & 2 deletions gitrepo/gitrepo.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ package gitrepo
import (
"fmt"
log "github.com/Sirupsen/logrus"
"io/ioutil"
"os"
"os/exec"
"path"
"path/filepath"
"regexp"
"strings"
"talisman/utility"
)

//FilePath represents the absolute path of an added file
Expand Down Expand Up @@ -161,7 +161,7 @@ func NewScannerAddition(filePath string, commits []string, content []byte) Addit
func (repo GitRepo) ReadRepoFile(fileName string) ([]byte, error) {
path := filepath.Join(repo.root, fileName)
log.Debugf("reading file %s", path)
return ioutil.ReadFile(path)
return utility.SafeReadFile(path)
}

//ReadRepoFileOrNothing returns the contents of the supplied relative filename by locating it in the git repo.
Expand Down
3 changes: 1 addition & 2 deletions utility/sha_256_hasher.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package utility
import (
"crypto/sha256"
"encoding/hex"
"io/ioutil"
)

type SHA256Hasher interface {
Expand All @@ -20,7 +19,7 @@ func (DefaultSHA256Hasher) CollectiveSHA256Hash(paths []string) string {
concatBytes := hashByte(&sbyte)
nameByte := []byte(path)
nameHash := hashByte(&nameByte)
fileBytes, _ := ioutil.ReadFile(path)
fileBytes, _ := SafeReadFile(path)
fileHash := hashByte(&fileBytes)
finHash = concatBytes + fileHash + nameHash
}
Expand Down
17 changes: 17 additions & 0 deletions utility/utility.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package utility

import (
"fmt"
log "github.com/Sirupsen/logrus"
"io"
"io/ioutil"
"os"
Expand Down Expand Up @@ -89,3 +90,19 @@ func Dir(src string, dst string) error {
}
return nil
}

func IsFileSymlink(path string) bool {
fileMetadata, err := os.Lstat(path)
if err != nil {
return false
}
return fileMetadata.Mode()&os.ModeSymlink != 0
}

func SafeReadFile(path string) ([]byte, error) {
if IsFileSymlink(path) {
log.Debug("Symlink was detected! Not following symlink ", path)
return []byte{}, nil
}
return ioutil.ReadFile(path)
}
39 changes: 39 additions & 0 deletions utility/utility_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package utility

import (
"github.com/stretchr/testify/assert"
"io/ioutil"
"os"
"testing"
)

func TestShouldReadNormalFileCorrectly(t *testing.T) {
tempFile, _ := ioutil.TempFile(os.TempDir(), "somefile")
dataToBeWrittenInFile := []byte{0, 1, 2, 3}
tempFile.Write(dataToBeWrittenInFile)
tempFile.Close()

readDataFromFileUsingIoutilDotReadFile, _ := ioutil.ReadFile(tempFile.Name())
readDataFromFileUsingSafeFileRead, _ := SafeReadFile(tempFile.Name())
os.Remove(tempFile.Name())

assert.Equal(t, readDataFromFileUsingIoutilDotReadFile, dataToBeWrittenInFile)
assert.Equal(t, readDataFromFileUsingSafeFileRead, dataToBeWrittenInFile)
}

func TestShouldNotReadSymbolicLinkTargetFile(t *testing.T) {
tempFile, _ := ioutil.TempFile(os.TempDir(), "somefile")
dataToBeWrittenInFile := []byte{0, 1, 2, 3}
tempFile.Write(dataToBeWrittenInFile)
tempFile.Close()
symlinkFileName := tempFile.Name() + "symlink"
os.Symlink(tempFile.Name(), symlinkFileName)

readDataFromSymlinkUsingIoutilDotReadFile, _ := ioutil.ReadFile(symlinkFileName)
readDataFromSymlinkUsingSafeFileRead, _ := SafeReadFile(symlinkFileName)
os.Remove(symlinkFileName)
os.Remove(tempFile.Name())

assert.Equal(t, readDataFromSymlinkUsingIoutilDotReadFile, dataToBeWrittenInFile)
assert.Equal(t, readDataFromSymlinkUsingSafeFileRead, []byte{})
}

0 comments on commit 0855689

Please sign in to comment.