Skip to content

Commit 9f9c9e0

Browse files
am0o0am0o0
committed
fix issues according to codereview
1 parent e239d76 commit 9f9c9e0

2 files changed

Lines changed: 125 additions & 124 deletions

File tree

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

Lines changed: 125 additions & 119 deletions
Original file line numberDiff line numberDiff line change
@@ -1,211 +1,217 @@
11
import go
22

33
/**
4-
* Provide File system access sinks of [fasthttp](https://github.com/valyala/fasthttp) web framework
4+
* The File system access sinks of [fasthttp](https://github.com/valyala/fasthttp) web framework
55
*/
66
class FastHttpFileSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
7+
int pathArg;
8+
79
FastHttpFileSystemAccess() {
8-
exists(DataFlow::Method mcn |
10+
exists(Method m |
911
(
10-
mcn.hasQualifiedName("github.com/valyala/fasthttp.RequestCtx", ["SendFileBytes", "SendFile"]) or
11-
mcn.hasQualifiedName("github.com/valyala/fasthttp.Response", ["SendFile"])
12+
m.hasQualifiedName(package("github.com/valyala/fasthttp", ""), "RequestCtx",
13+
["SendFileBytes", "SendFile"])
14+
or
15+
m.hasQualifiedName(package("github.com/valyala/fasthttp", ""), "Response", "SendFile")
1216
) and
13-
this = mcn.getACall()
17+
this = m.getACall() and
18+
pathArg = 0
1419
)
1520
or
16-
exists(DataFlow::Function f |
17-
f.hasQualifiedName("github.com/valyala/fasthttp",
21+
exists(Function f |
22+
f.hasQualifiedName(package("github.com/valyala/fasthttp", ""),
1823
[
1924
"ServeFile", "ServeFileUncompressed", "ServeFileBytes", "ServeFileBytesUncompressed",
2025
"SaveMultipartFile"
2126
]) and
22-
this = f.getACall()
27+
this = f.getACall() and
28+
pathArg = 1
2329
)
2430
}
2531

26-
override DataFlow::Node getAPathArgument() {
27-
this.getTarget().getName() =
28-
[
29-
"ServeFile", "ServeFileUncompressed", "ServeFileBytes", "ServeFileBytesUncompressed",
30-
"SaveMultipartFile"
31-
] and
32-
result = this.getArgument(1)
33-
or
34-
this.getTarget().getName() = ["SendFile", "SendFileBytes"] and
35-
result = this.getArgument(0)
36-
}
32+
override DataFlow::Node getAPathArgument() { result = this.getArgument(pathArg) }
3733
}
3834

3935
/**
40-
* Provide File system access sinks of `net/http` package
36+
* The File system access sinks of `net/http` package
4137
*/
4238
class HttpServeFile extends FileSystemAccess::Range, DataFlow::CallNode {
39+
int pathArg;
40+
4341
HttpServeFile() {
44-
exists(DataFlow::Function mcn |
45-
mcn.hasQualifiedName("net/http", "ServeFile") and
46-
this = mcn.getACall()
42+
exists(Function f |
43+
f.hasQualifiedName("net/http", "ServeFile") and
44+
this = f.getACall() and
45+
pathArg = 2
4746
)
4847
}
4948

50-
override DataFlow::Node getAPathArgument() { result = this.getArgument(2) }
49+
override DataFlow::Node getAPathArgument() { result = this.getArgument(pathArg) }
5150
}
5251

5352
/**
54-
* Provide File system access sinks of [beego](https://github.com/beego/beego) web framework
53+
* The File system access sinks of [beego](https://github.com/beego/beego) web framework
5554
*/
5655
class BeegoFileSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
56+
int pathArg;
57+
5758
BeegoFileSystemAccess() {
58-
exists(DataFlow::Method mcn |
59+
exists(Method m |
5960
(
60-
mcn.hasQualifiedName("github.com/beego/beego/v2/server/web/context.BeegoOutput", "Download") or
61-
mcn.hasQualifiedName("github.com/beego/beego/v2/server/web.Controller",
62-
"SaveToFileWithBuffer")
61+
m.hasQualifiedName(package("github.com/beego/beego", "server/web/context"), "BeegoOutput",
62+
"Download") and
63+
pathArg = 0
64+
or
65+
m.hasQualifiedName(package("github.com/beego/beego", "server/web"), "Controller",
66+
"SaveToFileWithBuffer") and
67+
pathArg = 1
6368
) and
64-
this = mcn.getACall()
69+
this = m.getACall()
6570
)
6671
}
6772

68-
override DataFlow::Node getAPathArgument() {
69-
this.getTarget()
70-
.hasQualifiedName("github.com/beego/beego/v2/server/web/context.BeegoOutput", "Download") and
71-
result = this.getArgument(0)
72-
or
73-
this.getTarget()
74-
.hasQualifiedName("github.com/beego/beego/v2/server/web.Controller", "SaveToFileWithBuffer") and
75-
result = this.getArgument(1)
76-
}
73+
override DataFlow::Node getAPathArgument() { result = this.getArgument(pathArg) }
7774
}
7875

7976
/**
8077
* Provide File system access sinks of [beego](https://github.com/beego/beego) web framework
8178
*/
8279
class EchoFileSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
80+
int pathArg;
81+
8382
EchoFileSystemAccess() {
84-
exists(DataFlow::Method mcn |
85-
mcn.hasQualifiedName("github.com/labstack/echo/v4.Context", ["Attachment", "File"]) and
86-
this = mcn.getACall()
83+
exists(Method m |
84+
m.hasQualifiedName(package("github.com/labstack/echo", ""), "Context", ["Attachment", "File"]) and
85+
this = m.getACall() and
86+
pathArg = 0
8787
)
8888
}
8989

90-
override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
90+
override DataFlow::Node getAPathArgument() { result = this.getArgument(pathArg) }
9191
}
9292

9393
/**
94-
* Provide File system access sinks of [gin](https://github.com/gin-gonic/gin) web framework
94+
* The File system access sinks of [gin](https://github.com/gin-gonic/gin) web framework
9595
*/
9696
class GinFileSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
97+
int pathArg;
98+
9799
GinFileSystemAccess() {
98-
exists(DataFlow::Method mcn |
99-
mcn.hasQualifiedName("github.com/gin-gonic/gin.Context",
100-
["File", "FileAttachment", "SaveUploadedFile"]) and
101-
this = mcn.getACall()
100+
exists(Method m |
101+
(
102+
m.hasQualifiedName(package("github.com/gin-gonic/gin", ""), "Context",
103+
["File", "FileAttachment"]) and
104+
pathArg = 0
105+
or
106+
m.hasQualifiedName(package("github.com/gin-gonic/gin", ""), "Context", "SaveUploadedFile") and
107+
pathArg = 1
108+
) and
109+
this = m.getACall()
102110
)
103111
}
104112

105-
override DataFlow::Node getAPathArgument() {
106-
this.getTarget().getName() = ["File", "FileAttachment"] and result = this.getArgument(0)
107-
or
108-
this.getTarget().getName() = "SaveUploadedFile" and result = this.getArgument(1)
109-
}
113+
override DataFlow::Node getAPathArgument() { result = this.getArgument(pathArg) }
110114
}
111115

112116
/**
113-
* Provide File system access sinks of [iris](https://github.com/kataras/iris) web framework
117+
* The File system access sinks of [iris](https://github.com/kataras/iris) web framework
114118
*/
115119
class IrisFileSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
120+
int pathArg;
121+
116122
IrisFileSystemAccess() {
117-
exists(DataFlow::Method mcn |
118-
mcn.hasQualifiedName("github.com/kataras/iris/v12/context.Context",
119-
["SendFile", "ServeFile", "SendFileWithRate", "ServeFileWithRate", "UploadFormFiles"]) and
120-
this = mcn.getACall()
121-
or
122-
mcn.hasQualifiedName("github.com/kataras/iris/v12/context.Context", "SaveFormFile") and
123-
this = mcn.getACall()
123+
exists(Method m |
124+
(
125+
m.hasQualifiedName(package("github.com/kataras/iris", "context"), "Context",
126+
["SendFile", "ServeFile", "SendFileWithRate", "ServeFileWithRate", "UploadFormFiles"]) and
127+
pathArg = 0
128+
or
129+
m.hasQualifiedName(package("github.com/kataras/iris", "context"), "Context", "SaveFormFile") and
130+
pathArg = 1
131+
) and
132+
this = m.getACall()
124133
)
125134
}
126135

127-
override DataFlow::Node getAPathArgument() {
128-
this.getTarget().getName() =
129-
["SendFile", "ServeFile", "SendFileWithRate", "ServeFileWithRate", "UploadFormFiles"] and
130-
result = this.getArgument(0)
131-
or
132-
this.getTarget().getName() = "SaveFormFile" and result = this.getArgument(1)
133-
}
136+
override DataFlow::Node getAPathArgument() { result = this.getArgument(pathArg) }
134137
}
135138

136139
/**
137-
* Provide File system access sinks of [fiber](https://github.com/gofiber/fiber) web framework
140+
* The File system access sinks of [fiber](https://github.com/gofiber/fiber) web framework
138141
*/
139142
class FiberSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
143+
int pathArg;
144+
140145
FiberSystemAccess() {
141-
exists(DataFlow::Method mcn |
142-
mcn.hasQualifiedName("github.com/gofiber/fiber/v2.Ctx", ["Attachment", "SendFile"]) and
143-
this = mcn.getACall()
144-
or
145-
mcn.hasQualifiedName("github.com/gofiber/fiber/v2.Ctx", "SaveFile") and
146-
this = mcn.getACall()
146+
exists(Method m |
147+
(
148+
m.hasQualifiedName(package("github.com/gofiber/fiber", ""), "Ctx", "SendFile") and
149+
pathArg = 0
150+
or
151+
m.hasQualifiedName(package("github.com/gofiber/fiber", ""), "Ctx", "SaveFile") and
152+
pathArg = 1
153+
) and
154+
this = m.getACall()
147155
)
148156
}
149157

150-
override DataFlow::Node getAPathArgument() {
151-
this.getTarget().getName() = ["Attachment", "SendFile"] and result = this.getArgument(0)
152-
or
153-
this.getTarget().getName() = "SaveFile" and result = this.getArgument(1)
154-
}
158+
override DataFlow::Node getAPathArgument() { result = this.getArgument(pathArg) }
159+
}
160+
161+
predicate test(Function f) {
162+
f.hasQualifiedName("github.com/valyala/fasthttp",
163+
["WriteReader", "SafeWriteReader", "WriteFile", "ReadFile", "ReadDir"])
164+
155165
}
166+
string aferoPackage() { result = "github.com/valyala/fasthttp" }
156167

157168
/**
158169
* Provide File system access sinks of [afero](https://github.com/spf13/afero) filesystem framework
159-
* The Only Type that is not vulnerable to path traversal is `afero.IOFS`
170+
* The Types that are not vulnerable: `afero.BasePathFs` and `afero.IOFS`
160171
*/
161172
class AferoSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {
173+
int pathArg;
174+
162175
AferoSystemAccess() {
163-
exists(DataFlow::Function mcn |
164-
mcn.hasQualifiedName("github.com/spf13/afero",
176+
// utility functions
177+
exists(Function f |
178+
f.hasQualifiedName("github.com/valyala/fasthttp",
165179
["WriteReader", "SafeWriteReader", "WriteFile", "ReadFile", "ReadDir"]) and
166-
this = mcn.getACall()
180+
this = f.getACall() and
181+
pathArg = 1
167182
)
168183
or
169-
exists(DataFlow::Method mcn |
170-
mcn.hasQualifiedName("github.com/spf13/afero.Afero",
171-
["ReadFile", "ReadDir", "WriteReader", "WriteFile", "SafeWriteReader"]) and
172-
this = mcn.getACall()
173-
or
174-
mcn.hasQualifiedName("github.com/spf13/afero.HttpFs", ["Open", "OpenFile", "Create"]) and
175-
this = mcn.getACall()
176-
or
177-
mcn.hasQualifiedName("github.com/spf13/afero.RegexpFs",
178-
["Create", "Open", "Remove", "OpenFile"]) and
179-
this = mcn.getACall()
180-
or
181-
mcn.hasQualifiedName("github.com/spf13/afero.ReadOnlyFs",
182-
["Create", "Open", "Remove", "OpenFile", "ReadDir", "ReadlinkIfPossible"]) and
183-
this = mcn.getACall()
184+
// afero FS Types
185+
exists(Method f |
186+
f.hasQualifiedName(package(aferoPackage(), ""), "HttpFs",
187+
["Create", "Open", "OpenFile", "Remove", "RemoveAll"]) and
188+
this = f.getACall() and
189+
pathArg = 0
184190
or
185-
mcn.hasQualifiedName("github.com/spf13/afero.OsFs",
186-
["Create", "Open", "Remove", "RemoveAll", "OpenFile", "ReadDir", "ReadlinkIfPossible"]) and
187-
this = mcn.getACall()
191+
f.hasQualifiedName(package(aferoPackage(), ""), "RegexpFs",
192+
["Create", "Open", "OpenFile", "Remove", "RemoveAll", "Mkdir", "MkdirAll"]) and
193+
this = f.getACall() and
194+
pathArg = 0
188195
or
189-
mcn.hasQualifiedName("github.com/spf13/afero.OsFs",
190-
["Create", "Open", "Remove", "RemoveAll", "OpenFile", "ReadDir", "ReadlinkIfPossible"]) and
191-
this = mcn.getACall()
196+
f.hasQualifiedName(package(aferoPackage(), ""), "ReadOnlyFs",
197+
["Create", "Open", "OpenFile", "ReadDir", "ReadlinkIfPossible", "Mkdir", "MkdirAll"]) and
198+
this = f.getACall() and
199+
pathArg = 0
192200
or
193-
mcn.hasQualifiedName("github.com/spf13/afero.MemMapFs",
194-
["Create", "Open", "OpenFile", "Remove", "RemoveAll"]) and
195-
this = mcn.getACall()
201+
f.hasQualifiedName(package(aferoPackage(), ""), "OsFs",
202+
[
203+
"Create", "Open", "OpenFile", "ReadlinkIfPossible", "Remove", "RemoveAll", "Mkdir",
204+
"MkdirAll"
205+
]) and
206+
this = f.getACall() and
207+
pathArg = 0
196208
or
197-
mcn.hasQualifiedName("github.com/spf13/afero.BasePathFs",
198-
["Create", "Open", "OpenFile", "Remove", "RemoveAll", "ReadlinkIfPossible"]) and
199-
this = mcn.getACall()
209+
f.hasQualifiedName(package(aferoPackage(), ""), "MemMapFs",
210+
["Create", "Open", "OpenFile", "Remove", "RemoveAll", "Mkdir", "MkdirAll"]) and
211+
this = f.getACall() and
212+
pathArg = 0
200213
)
201214
}
202215

203-
override DataFlow::Node getAPathArgument() {
204-
if
205-
this.getTarget()
206-
.hasQualifiedName("github.com/spf13/afero",
207-
["WriteReader", "SafeWriteReader", "WriteFile", "ReadFile", "ReadDir"])
208-
then result = this.getArgument(1)
209-
else result = this.getArgument(0)
210-
}
216+
override DataFlow::Node getAPathArgument() { result = this.getArgument(pathArg) }
211217
}

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

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -95,15 +95,10 @@ func Echo() {
9595

9696
func Fiber() {
9797
app := fiber.New()
98-
app.Get("/a", func(c *fiber.Ctx) error {
99-
filepath := c.Params("filepath")
100-
return c.SendFile(filepath)
101-
})
10298
app.Get("/b", func(c *fiber.Ctx) error {
10399
filepath := c.Params("filepath")
104100
header, _ := c.FormFile("f")
105101
_ = c.SaveFile(header, filepath)
106-
c.Attachment(filepath)
107102
return c.SendFile(filepath)
108103
})
109104
_ = app.Listen(":3000")

0 commit comments

Comments
 (0)