Skip to content

Commit 73803ea

Browse files
am0o0am0o0
committed
fix tests
add missed afero sinks
1 parent cea44e2 commit 73803ea

File tree

8 files changed

+105
-1852
lines changed

8 files changed

+105
-1852
lines changed

‎go/ql/lib/semmle/go/security/FileSystemAccess.qll

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -134,29 +134,29 @@ module Afero {
134134
*/
135135
class AferoSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
136136
AferoSystemAccess() {
137-
exists(Method f |
138-
f.hasQualifiedName(aferoPackage(), "HttpFs",
137+
exists(Method m |
138+
m.hasQualifiedName(aferoPackage(), "HttpFs",
139139
["Create", "Open", "OpenFile", "Remove", "RemoveAll"]) and
140-
this = f.getACall()
140+
this = m.getACall()
141141
or
142-
f.hasQualifiedName(aferoPackage(), "RegexpFs",
142+
m.hasQualifiedName(aferoPackage(), "RegexpFs",
143143
["Create", "Open", "OpenFile", "Remove", "RemoveAll", "Mkdir", "MkdirAll"]) and
144-
this = f.getACall()
144+
this = m.getACall()
145145
or
146-
f.hasQualifiedName(aferoPackage(), "ReadOnlyFs",
146+
m.hasQualifiedName(aferoPackage(), "ReadOnlyFs",
147147
["Create", "Open", "OpenFile", "ReadDir", "ReadlinkIfPossible", "Mkdir", "MkdirAll"]) and
148-
this = f.getACall()
148+
this = m.getACall()
149149
or
150-
f.hasQualifiedName(aferoPackage(), "OsFs",
150+
m.hasQualifiedName(aferoPackage(), "OsFs",
151151
[
152152
"Create", "Open", "OpenFile", "ReadlinkIfPossible", "Remove", "RemoveAll", "Mkdir",
153153
"MkdirAll"
154154
]) and
155-
this = f.getACall()
155+
this = m.getACall()
156156
or
157-
f.hasQualifiedName(aferoPackage(), "MemMapFs",
157+
m.hasQualifiedName(aferoPackage(), "MemMapFs",
158158
["Create", "Open", "OpenFile", "Remove", "RemoveAll", "Mkdir", "MkdirAll"]) and
159-
this = f.getACall()
159+
this = m.getACall()
160160
)
161161
}
162162

@@ -166,20 +166,33 @@ module Afero {
166166
/**
167167
* The File system access sinks of [afero](https://github.com/spf13/afero) framework utility functions
168168
*
169+
* Afero Type is basically is an wrapper around utility functions which make them like a method, look at [here](https://github.com/spf13/afero/blob/cf95922e71986c0116204b6eeb3b345a01ffd842/ioutil.go#L61)
170+
*
169171
* The Types that are not vulnerable: `afero.BasePathFs` and `afero.IOFS`
170172
*/
171173
class AferoUtilityFunctionSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
174+
int pathArg;
175+
172176
AferoUtilityFunctionSystemAccess() {
173177
// utility functions
174178
exists(Function f |
175179
f.hasQualifiedName(aferoPackage(),
176180
["WriteReader", "SafeWriteReader", "WriteFile", "ReadFile", "ReadDir"]) and
177181
this = f.getACall() and
182+
pathArg = 1 and
178183
not aferoSanitizer(this.getArgument(0))
179184
)
185+
or
186+
exists(Method m |
187+
m.hasQualifiedName(aferoPackage(), "Afero",
188+
["WriteReader", "SafeWriteReader", "WriteFile", "ReadFile", "ReadDir"]) and
189+
this = m.getACall() and
190+
pathArg = 0 and
191+
not aferoSanitizer(this.getReceiver())
192+
)
180193
}
181194

182-
override DataFlow::Node getAPathArgument() { result = this.getArgument(1) }
195+
override DataFlow::Node getAPathArgument() { result = this.getArgument(pathArg) }
183196
}
184197

185198
/**
@@ -193,8 +206,8 @@ module Afero {
193206
*/
194207
predicate aferoSanitizer(DataFlow::Node n) {
195208
exists(Function f |
196-
f.hasQualifiedName(aferoPackage(), "NewBasePathFs") and
197-
DataFlow::localFlow(f.getACall(), n)
209+
f.hasQualifiedName(aferoPackage(), ["NewBasePathFs", "NewIOFS"]) and
210+
TaintTracking::localTaint(f.getACall(), n)
198211
)
199212
}
200213

@@ -208,7 +221,7 @@ module Afero {
208221
predicate additionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
209222
exists(StructLit st | st.getType().hasQualifiedName(aferoPackage(), "Afero") |
210223
n1.asExpr() = st.getAChildExpr().(KeyValueExpr).getAChildExpr() and
211-
n2.asExpr() = st
224+
n2.asExpr() = st.getParent()
212225
)
213226
}
214227
}

‎go/ql/test/library-tests/semmle/go/security/FileSystemAccess/FileSystemAccess.expected

Lines changed: 0 additions & 28 deletions
This file was deleted.

‎go/ql/test/library-tests/semmle/go/security/FileSystemAccess/FileSystemAccess.ql

Lines changed: 0 additions & 4 deletions
This file was deleted.

‎go/ql/test/library-tests/semmle/go/security/FileSystemAccess/Query.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
testFailures
2+
failures

‎go/ql/test/library-tests/semmle/go/security/FileSystemAccess/Query.ql

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
import go
2+
import TestUtilities.InlineExpectationsTest
3+
4+
module FileSystemAccessTest implements TestSig {
5+
string getARelevantTag() { result = ["FileSystemAccess", "succ", "pred"] }
6+
7+
predicate hasActualResult(Location location, string element, string tag, string value) {
8+
exists(FileSystemAccess fsa |
9+
fsa.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
10+
location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
11+
element = fsa.getAPathArgument().toString() and
12+
value = fsa.getAPathArgument().toString() and
13+
tag = "FileSystemAccess"
14+
)
15+
or
16+
exists(DataFlow::Node succ, DataFlow::Node pred | Afero::additionalTaintStep(pred, succ) |
17+
succ.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
18+
location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
19+
element = succ.toString() and
20+
value = succ.toString() and
21+
tag = "succ"
22+
or
23+
pred.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
24+
location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
25+
element = pred.toString() and
26+
value = pred.toString() and
27+
tag = "pred"
28+
)
29+
}
30+
}
31+
32+
import MakeTest<FileSystemAccessTest>

‎go/ql/test/library-tests/semmle/go/security/FileSystemAccess/main.go

Lines changed: 43 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,10 @@ package main
44
//go:generate depstubber -vendor github.com/beego/beego/v2/server/web/context BeegoOutput,Context
55
//go:generate depstubber -vendor github.com/gin-gonic/gin Context Default
66
//go:generate depstubber -vendor github.com/gofiber/fiber/v2 Ctx New
7-
//go:generate depstubber -vendor github.com/kataras/iris/v12 Context
7+
//go:generate depstubber -vendor github.com/kataras/iris/v12/context Context
88
//go:generate depstubber -vendor github.com/labstack/echo/v4 Context New
99
//go:generate depstubber -vendor github.com/spf13/afero Afero,RegexpFs,HttpFs,ReadOnlyFs,MemMapFs,OsFs,BasePathFs WriteReader,SafeWriteReader,WriteFile,ReadFile,ReadDir,NewOsFs,NewRegexpFs,NewReadOnlyFs,NewCacheOnReadFs,,NewHttpFs,NewBasePathFs,NewIOFS
1010

11-
1211
import (
1312
"fmt"
1413
beego "github.com/beego/beego/v2/server/web"
@@ -29,68 +28,67 @@ func main() {
2928

3029
func BeegoController(beegoController beego.Controller) {
3130
beegoOutput := BeegoContext.BeegoOutput{}
32-
beegoOutput.Download("filepath", "license.txt")
31+
beegoOutput.Download("filepath", "license.txt") // $ FileSystemAccess="filepath"
3332
buffer := make([]byte, 10)
34-
_ = beegoController.SaveToFileWithBuffer("filenameExistsInForm", "filepath", buffer)
33+
_ = beegoController.SaveToFileWithBuffer("filenameExistsInForm", "filepath", buffer) // $ FileSystemAccess="filepath"
3534
}
3635

3736
func Afero(writer http.ResponseWriter, request *http.Request) {
3837
filepath := request.URL.Query()["filepath"][0]
3938
//osFS := afero.NewMemMapFs()
4039
// OR
4140
osFS := afero.NewOsFs()
42-
fmt.Println(osFS.MkdirAll("tmp/b", 0755))
43-
fmt.Println(afero.WriteFile(osFS, "tmp/a", []byte("this is me a !"), 0755))
44-
fmt.Println(afero.WriteFile(osFS, "tmp/b/c", []byte("this is me c !"), 0755))
45-
fmt.Println(afero.WriteFile(osFS, "tmp/d", []byte("this is me d !"), 0755))
46-
content, _ := afero.ReadFile(osFS, filepath)
41+
fmt.Println(osFS.MkdirAll(filepath, 0755)) // $ FileSystemAccess=filepath
42+
fmt.Println(afero.WriteFile(osFS, filepath, []byte("this is me a !"), 0755)) // $ FileSystemAccess=filepath
43+
content, _ := afero.ReadFile(osFS, filepath) // $ FileSystemAccess=filepath
4744
fmt.Println(string(content))
48-
fmt.Println(osFS.Open(filepath))
49-
// BAD
50-
fmt.Println(afero.SafeWriteReader(osFS, filepath, os.Stdout))
51-
fmt.Println(afero.WriteReader(osFS, filepath, os.Stdout))
45+
fmt.Println(osFS.Open(filepath)) // $ FileSystemAccess=filepath
46+
// NOT OK
47+
fmt.Println(afero.SafeWriteReader(osFS, filepath, os.Stdout)) // $ FileSystemAccess=filepath
48+
fmt.Println(afero.WriteReader(osFS, filepath, os.Stdout)) // $ FileSystemAccess=filepath
5249

53-
// RegexpFs ==> BAD
50+
// RegexpFs ==> NOT OK
5451
fmt.Println("RegexpFs:")
5552
regex, _ := regexp.Compile(".*")
5653
regexpFs := afero.NewRegexpFs(osFS, regex)
57-
fmt.Println(afero.ReadFile(regexpFs, filepath))
54+
fmt.Println(afero.ReadFile(regexpFs, filepath)) // $ FileSystemAccess=filepath
5855

59-
// ReadOnlyFS ==> BAD
56+
// ReadOnlyFS ==> NOT OK
6057
fmt.Println("ReadOnlyFS:")
6158
readOnlyFS := afero.NewReadOnlyFs(osFS)
62-
fmt.Println(afero.ReadFile(readOnlyFS, filepath))
59+
fmt.Println(afero.ReadFile(readOnlyFS, filepath)) // $ FileSystemAccess=filepath
6360

64-
// CacheOnReadFs ==> BAD
61+
// CacheOnReadFs ==> NOT OK
6562
fmt.Println("CacheOnReadFs:")
6663
cacheOnReadFs := afero.NewCacheOnReadFs(osFS, osFS, 10)
67-
fmt.Println(afero.ReadFile(cacheOnReadFs, filepath))
64+
fmt.Println(afero.ReadFile(cacheOnReadFs, filepath)) // $ FileSystemAccess=filepath
6865

69-
// HttpFS ==> BAD
66+
// HttpFS ==> NOT OK
7067
fmt.Println("HttpFS:")
7168
httpFs := afero.NewHttpFs(osFS)
72-
httpFile, _ := httpFs.Open(filepath)
69+
httpFile, _ := httpFs.Open(filepath) // $ FileSystemAccess=filepath
7370
tmpbytes := make([]byte, 30)
7471
fmt.Println(httpFile.Read(tmpbytes))
7572
fmt.Println(string(tmpbytes))
7673

77-
// osFS ==> BAD
74+
// osFS ==> NOT OK
7875
fmt.Println("Afero:")
79-
afs := &afero.Afero{Fs: osFS}
80-
fmt.Println(afs.ReadFile(filepath))
76+
afs := &afero.Afero{Fs: osFS} // $ succ=&... pred=osFS
77+
fmt.Println(afs.ReadFile(filepath)) // $ FileSystemAccess=filepath
8178

82-
// BasePathFs ==> BAD
79+
// BasePathFs ==> OK
8380
fmt.Println("Afero:")
84-
basePathFs0 := &afero.Afero{Fs: afero.NewBasePathFs(osFS, "tmp")}
85-
fmt.Println(basePathFs0.ReadFile(filepath))
81+
newBasePathFs := afero.NewBasePathFs(osFS, "tmp")
82+
basePathFs0 := &afero.Afero{Fs: newBasePathFs} // $ succ=&... pred=newBasePathFs
83+
fmt.Println(basePathFs0.ReadFile(filepath)) // $ SPURIOUS: FileSystemAccess=filepath
8684

87-
// IOFS ==> GOOD
85+
// IOFS ==> OK
8886
fmt.Println("IOFS:")
8987
ioFS := afero.NewIOFS(osFS)
9088
fmt.Println(ioFS.ReadFile(filepath))
9189
fmt.Println(ioFS.Open(filepath))
9290

93-
// BasePathFs ==> GOOD
91+
// BasePathFs ==> OK
9492
fmt.Println("BasePathFs:")
9593
basePathFs := afero.NewBasePathFs(osFS, "tmp")
9694
fmt.Println(afero.ReadFile(basePathFs, filepath))
@@ -100,11 +98,13 @@ func Afero(writer http.ResponseWriter, request *http.Request) {
10098
func Echo() {
10199
e := echo.New()
102100
e.GET("/", func(c echo.Context) error {
103-
return c.File(c.QueryParam("filePath"))
101+
filepath := c.QueryParam("filePath")
102+
return c.File(filepath) // $ FileSystemAccess=filepath
104103
})
105104

106105
e.GET("/attachment", func(c echo.Context) error {
107-
return c.Attachment(c.QueryParam("filePath"), "file name in response")
106+
filepath := c.QueryParam("filePath")
107+
return c.Attachment(filepath, "file name in response") // $ FileSystemAccess=filepath
108108
})
109109
_ = e.Start(":1323")
110110
}
@@ -114,32 +114,32 @@ func Fiber() {
114114
app.Get("/b", func(c *fiber.Ctx) error {
115115
filepath := c.Params("filepath")
116116
header, _ := c.FormFile("f")
117-
_ = c.SaveFile(header, filepath)
118-
return c.SendFile(filepath)
117+
_ = c.SaveFile(header, filepath) // $ FileSystemAccess=filepath
118+
return c.SendFile(filepath) // $ FileSystemAccess=filepath
119119
})
120120
_ = app.Listen(":3000")
121121
}
122122

123123
func IrisTest(ctx context.Context) {
124124
filepath := ctx.URLParam("filepath")
125-
_ = ctx.SendFile(filepath, "file")
126-
_ = ctx.SendFileWithRate(filepath, "file", 0, 0)
127-
_ = ctx.ServeFile(filepath)
128-
_ = ctx.ServeFileWithRate(filepath, 0, 0)
129-
_, _, _ = ctx.UploadFormFiles(filepath, nil)
125+
_ = ctx.SendFile(filepath, "file") // $ FileSystemAccess=filepath
126+
_ = ctx.SendFileWithRate(filepath, "file", 0, 0) // $ FileSystemAccess=filepath
127+
_ = ctx.ServeFile(filepath) // $ FileSystemAccess=filepath
128+
_ = ctx.ServeFileWithRate(filepath, 0, 0) // $ FileSystemAccess=filepath
129+
_, _, _ = ctx.UploadFormFiles(filepath, nil) // $ FileSystemAccess=filepath
130130
_, fileHeader, _ := ctx.FormFile("file")
131-
_, _ = ctx.SaveFormFile(fileHeader, filepath)
131+
_, _ = ctx.SaveFormFile(fileHeader, filepath) // $ FileSystemAccess=filepath
132132

133133
}
134134
func Gin() {
135135
router := gin.Default()
136136
router.POST("/FormUploads", func(c *gin.Context) {
137137
filepath := c.Query("filepath")
138-
c.File(filepath)
139-
http.ServeFile(c.Writer, c.Request, filepath)
140-
c.FileAttachment(filepath, "file name in response")
138+
c.File(filepath) // $ FileSystemAccess=filepath
139+
http.ServeFile(c.Writer, c.Request, filepath) // $ FileSystemAccess=filepath
140+
c.FileAttachment(filepath, "file name in response") // $ FileSystemAccess=filepath
141141
file, _ := c.FormFile("afile")
142-
_ = c.SaveUploadedFile(file, filepath)
142+
_ = c.SaveUploadedFile(file, filepath) // $ FileSystemAccess=filepath
143143
})
144144
_ = router.Run()
145145
}

0 commit comments

Comments
 (0)