hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-01-29 22:32:58 +01:00
Code Issues Packages Projects Releases Wiki Activity

Include os.Readlink as a probable sanitiser.

Browse Source 
A couple of projects seem to walk links one unit at a time, rather than just throwing `EvalSymlinks` at the whole potentially suspect path.
This commit is contained in:
Chris Smowton
2020-11-10 16:34:01 +00:00
parent 2193642c6e
commit 500d78dafa
3 changed files with 45 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
ql/src/Security/CWE-022/UnsafeUnzipSymlink.ql
View File

@@ -93,14 +93,18 @@ class SymlinkSink extends DataFlow::Node {
}
/**
* An argument to `path/filepath.EvalSymlinks`, taken as a sink for detecting target paths
* that are likely safe to extract to.
* An argument to `path/filepath.EvalSymlinks` or `os.Readlink`, taken as a sink for detecting target
* paths that are likely safe to extract to.
*/
class EvalSymlinksSink extends DataFlow::Node {
EvalSymlinksSink() {
this =
any(DataFlow::CallNode n | n.getTarget().hasQualifiedName("path/filepath", "EvalSymlinks"))
.getArgument(0)
exists(DataFlow::CallNode n |
n.getTarget().hasQualifiedName("path/filepath", "EvalSymlinks")
or
n.getTarget().hasQualifiedName("os", "Readlink")
|
this = n.getArgument(0)
)
}
}

30
ql/test/query-tests/Security/CWE-022/UnsafeUnzipSymlinkGood.go
View File

@@ -48,3 +48,33 @@ func unzipSymlinkGoodZip(f io.ReaderAt, target string) {
}
}
}
func isRelGoodReadlink(candidate, target string) bool {
// GOOD: resolves symbolic links before checking
// that `candidate` does not escape from `target`.
// Note this is not actually safe (using Readlink
// to resolve everything is not simple), so I just
// make some token use of it here.
if filepath.IsAbs(candidate) {
return false
}
realpath, err := os.Readlink(filepath.Join(target, candidate))
if err != nil {
return false
}
relpath, err := filepath.Rel(target, realpath)
return err == nil && !strings.HasPrefix(filepath.Clean(relpath), "..")
}
func unzipSymlinkGoodReadlink(f io.Reader, target string) {
r := tar.NewReader(f)
for {
header, err := r.Next()
if err != nil {
break
}
if isRelGoodReadlink(header.Linkname, target) && isRelGoodReadlink(header.Name, target) {
os.Symlink(header.Linkname, header.Name)
}
}
}

6
ql/test/query-tests/Security/CWE-022/ZipSlip.expected
View File

@@ -1,8 +1,13 @@
edges
| UnsafeUnzipSymlinkGood.go:52:24:52:32 | definition of candidate : string | UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join |
| UnsafeUnzipSymlinkGood.go:76:70:76:80 | selection of Name : string | UnsafeUnzipSymlinkGood.go:52:24:52:32 | definition of candidate : string |
| ZipSlip.go:12:24:12:29 | selection of Name : string | ZipSlip.go:14:20:14:20 | p |
| tarslip.go:14:23:14:33 | selection of Name : string | tarslip.go:14:14:14:34 | call to Dir |
| tst.go:24:11:24:16 | selection of Name : string | tst.go:29:20:29:23 | path |
nodes
| UnsafeUnzipSymlinkGood.go:52:24:52:32 | definition of candidate : string | semmle.label | definition of candidate : string |
| UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join | semmle.label | call to Join |
| UnsafeUnzipSymlinkGood.go:76:70:76:80 | selection of Name : string | semmle.label | selection of Name : string |
| ZipSlip.go:12:24:12:29 | selection of Name : string | semmle.label | selection of Name : string |
| ZipSlip.go:14:20:14:20 | p | semmle.label | p |
| tarslip.go:14:14:14:34 | call to Dir | semmle.label | call to Dir |
@@ -10,6 +15,7 @@ nodes
| tst.go:24:11:24:16 | selection of Name : string | semmle.label | selection of Name : string |
| tst.go:29:20:29:23 | path | semmle.label | path |
#select
| UnsafeUnzipSymlinkGood.go:76:70:76:80 | selection of Name | UnsafeUnzipSymlinkGood.go:76:70:76:80 | selection of Name : string | UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join | Unsanitized archive entry, which may contain '..', is used in a $@. | UnsafeUnzipSymlinkGood.go:61:31:61:62 | call to Join | file system operation |
| ZipSlip.go:12:24:12:29 | selection of Name | ZipSlip.go:12:24:12:29 | selection of Name : string | ZipSlip.go:14:20:14:20 | p | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.go:14:20:14:20 | p | file system operation |
| tarslip.go:14:23:14:33 | selection of Name | tarslip.go:14:23:14:33 | selection of Name : string | tarslip.go:14:14:14:34 | call to Dir | Unsanitized archive entry, which may contain '..', is used in a $@. | tarslip.go:14:14:14:34 | call to Dir | file system operation |
| tst.go:24:11:24:16 | selection of Name | tst.go:24:11:24:16 | selection of Name : string | tst.go:29:20:29:23 | path | Unsanitized archive entry, which may contain '..', is used in a $@. | tst.go:29:20:29:23 | path | file system operation |