From fee0355ea0320844aae95ff0abd6fbd38a3179c3 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 18 Feb 2021 11:48:36 -0800 Subject: [PATCH 01/18] Update actions to use go 1.16 --- .github/workflows/codeqltest.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/codeqltest.yml b/.github/workflows/codeqltest.yml index aebf8277e35..75a43937dc8 100644 --- a/.github/workflows/codeqltest.yml +++ b/.github/workflows/codeqltest.yml @@ -7,10 +7,10 @@ jobs: runs-on: ubuntu-latest steps: - - name: Set up Go 1.14 + - name: Set up Go 1.16 uses: actions/setup-go@v1 with: - go-version: 1.14 + go-version: 1.16 id: go - name: Set up CodeQL CLI @@ -52,10 +52,10 @@ jobs: name: Test MacOS runs-on: macOS-latest steps: - - name: Set up Go 1.14 + - name: Set up Go 1.16 uses: actions/setup-go@v1 with: - go-version: 1.14 + go-version: 1.16 id: go - name: Set up CodeQL CLI @@ -85,10 +85,10 @@ jobs: name: Test Windows runs-on: windows-latest steps: - - name: Set up Go 1.14 + - name: Set up Go 1.16 uses: actions/setup-go@v1 with: - go-version: 1.14 + go-version: 1.16 id: go - name: Set up CodeQL CLI From 42939a70b85000362ca7b2d89f5ee80e7d61f954 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 18 Feb 2021 11:48:48 -0800 Subject: [PATCH 02/18] Update go.mod to 1.16 --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index dc9f6ed3450..50a9c825682 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/github/codeql-go -go 1.14 +go 1.16 require ( golang.org/x/mod v0.3.0 From fc9bc68829ba8263d32acb8ab6f518968ad5258f Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 18 Feb 2021 11:49:00 -0800 Subject: [PATCH 03/18] Add change note for go 1.16 --- change-notes/2021-02-18-go-116.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change-notes/2021-02-18-go-116.md diff --git a/change-notes/2021-02-18-go-116.md b/change-notes/2021-02-18-go-116.md new file mode 100644 index 00000000000..353b5afa189 --- /dev/null +++ b/change-notes/2021-02-18-go-116.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The extractor now supports Go 1.16 and the new `io/fs` library that was introduced. From 62ae3ec7c5abdddeb192468660648c729ae102fd Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 18 Feb 2021 11:49:18 -0800 Subject: [PATCH 04/18] Add extractor test for go 1.16 --- ql/test/extractor-tests/go1.16/embed.expected | 1 + ql/test/extractor-tests/go1.16/embed.ql | 5 +++++ ql/test/extractor-tests/go1.16/embeddedfile.go | 15 +++++++++++++++ ql/test/extractor-tests/go1.16/file | 2 ++ 4 files changed, 23 insertions(+) create mode 100644 ql/test/extractor-tests/go1.16/embed.expected create mode 100644 ql/test/extractor-tests/go1.16/embed.ql create mode 100644 ql/test/extractor-tests/go1.16/embeddedfile.go create mode 100644 ql/test/extractor-tests/go1.16/file diff --git a/ql/test/extractor-tests/go1.16/embed.expected b/ql/test/extractor-tests/go1.16/embed.expected new file mode 100644 index 00000000000..a2ba87f5c22 --- /dev/null +++ b/ql/test/extractor-tests/go1.16/embed.expected @@ -0,0 +1 @@ +| embeddedfile.go:9:5:9:5 | e | diff --git a/ql/test/extractor-tests/go1.16/embed.ql b/ql/test/extractor-tests/go1.16/embed.ql new file mode 100644 index 00000000000..5880ea94707 --- /dev/null +++ b/ql/test/extractor-tests/go1.16/embed.ql @@ -0,0 +1,5 @@ +import go + +from Variable v +where exists(v.getDeclaration()) +select v diff --git a/ql/test/extractor-tests/go1.16/embeddedfile.go b/ql/test/extractor-tests/go1.16/embeddedfile.go new file mode 100644 index 00000000000..16caf1b08a5 --- /dev/null +++ b/ql/test/extractor-tests/go1.16/embeddedfile.go @@ -0,0 +1,15 @@ +package main + +import ( + _ "embed" + "fmt" +) + +//go:embed file +var e string = "hi" + +func main() { + fmt.Println(e) + e = "why" + fmt.Println(e) +} diff --git a/ql/test/extractor-tests/go1.16/file b/ql/test/extractor-tests/go1.16/file new file mode 100644 index 00000000000..1abe9e98695 --- /dev/null +++ b/ql/test/extractor-tests/go1.16/file @@ -0,0 +1,2 @@ +file +contents From a327fb7e972512f250ed6ca63bef7c85b99fb466 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 18 Feb 2021 11:49:54 -0800 Subject: [PATCH 05/18] Add support for go 1.16 frameworks --- ql/src/semmle/go/frameworks/Stdlib.qll | 1 + ql/src/semmle/go/frameworks/stdlib/IoFs.qll | 100 ++++++++++++++++++++ ql/src/semmle/go/frameworks/stdlib/Os.qll | 2 + 3 files changed, 103 insertions(+) create mode 100644 ql/src/semmle/go/frameworks/stdlib/IoFs.qll diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll index 9701ee8f944..063214fb570 100644 --- a/ql/src/semmle/go/frameworks/Stdlib.qll +++ b/ql/src/semmle/go/frameworks/Stdlib.qll @@ -40,6 +40,7 @@ import semmle.go.frameworks.stdlib.Fmt import semmle.go.frameworks.stdlib.Html import semmle.go.frameworks.stdlib.HtmlTemplate import semmle.go.frameworks.stdlib.Io +import semmle.go.frameworks.stdlib.IoFs import semmle.go.frameworks.stdlib.IoIoutil import semmle.go.frameworks.stdlib.Log import semmle.go.frameworks.stdlib.Mime diff --git a/ql/src/semmle/go/frameworks/stdlib/IoFs.qll b/ql/src/semmle/go/frameworks/stdlib/IoFs.qll new file mode 100644 index 00000000000..310d6da5749 --- /dev/null +++ b/ql/src/semmle/go/frameworks/stdlib/IoFs.qll @@ -0,0 +1,100 @@ +/** + * Provides classes modeling security-relevant aspects of the 'io/fs' package. + */ + +import go + +/** + * Provides classes modeling security-relevant aspects of the 'io/fs' package. + */ +module IoFs { + /** Gets the package name `io/fs`. */ + string packagePath() { result = "io/fs" } + + private class FunctionModels extends TaintTracking::FunctionModel { + FunctionInput inp; + FunctionOutput outp; + + FunctionModels() { + //signature: func Glob(fsys FS, pattern string) (matches []string, err error) + this.hasQualifiedName(packagePath(), "Glob") and + (inp.isParameter(0) and outp.isResult(0)) + or + //signature: func ReadFile(fsys FS, name string) ([]byte, error) + this.hasQualifiedName(packagePath(), "ReadFile") and + (inp.isParameter(0) and outp.isResult(0)) + or + //signature: func ReadDir(fsys FS, name string) ([]DirEntry, error) + this.hasQualifiedName(packagePath(), "ReadDir") and + (inp.isParameter(0) and outp.isResult(0)) + or + //signature: func Sub(fsys FS, dir string) (FS, error) + this.hasQualifiedName(packagePath(), "Sub") and + (inp.isParameter(0) and outp.isResult(0)) + } + + override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { + input = inp and output = outp + } + } + + /** + * Models a step from `fs` to `path` and `d` in + * `fs.WalkDir(fs, "root", func(path string, d DirEntry, err error) {}` + */ + private class WalkDirStep extends TaintTracking::AdditionalTaintStep { + override predicate step(DataFlow::Node pred, DataFlow::Node succ) { + //signature: func WalkDir(fsys FS, root string, fn WalkDirFunc) error + exists(DataFlow::CallNode call, DataFlow::FunctionNode f | + call.getTarget().hasQualifiedName(packagePath(), "WalkDir") and + f.getASuccessor*() = call.getArgument(2) + | + pred = call.getArgument(0) and + succ = f.getParameter([0, 1]) + ) + } + } + + private class MethodModels extends TaintTracking::FunctionModel, Method { + FunctionInput inp; + FunctionOutput outp; + + MethodModels() { + //signature: func (DirEntry).Name() string + this.implements(packagePath(), "DirEntry", "Name") and + (inp.isReceiver() and outp.isResult()) + or + //signature: func (DirEntry).Info() (FileInfo, error) + this.implements(packagePath(), "DirEntry", "Info") and + (inp.isReceiver() and outp.isResult(0)) + or + //signature: func (FS).Open(name string) (File, error) + this.implements(packagePath(), "FS", "Open") and + (inp.isReceiver() and outp.isResult(0)) + or + //signature: func (GlobFS).Glob(pattern string) ([]string, error) + this.implements(packagePath(), "GlobFS", "Glob") and + (inp.isReceiver() and outp.isResult(0)) + or + //signature: func (ReadDirFS).ReadDir(name string) ([]DirEntry, error) + this.implements(packagePath(), "ReadDirFS", "ReadDir") and + (inp.isReceiver() and outp.isResult(0)) + or + //signature: func (ReadFileFS).ReadFile(name string) ([]byte, error) + this.implements(packagePath(), "ReadFileFS", "ReadFile") and + (inp.isReceiver() and outp.isResult(0)) + or + //signature: func (SubFS).Sub(dir string) (FS, error) + this.implements(packagePath(), "SubFS", "Sub") and + (inp.isReceiver() and outp.isResult(0)) + or + //signature: func (File).Read([]byte) (int, error) + this.implements(packagePath(), "File", "Read") and + (inp.isReceiver() and outp.isParameter(0)) + } + + override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { + input = inp and output = outp + } + } +} diff --git a/ql/src/semmle/go/frameworks/stdlib/Os.qll b/ql/src/semmle/go/frameworks/stdlib/Os.qll index ce5c5581a2b..3b2ea652e93 100644 --- a/ql/src/semmle/go/frameworks/stdlib/Os.qll +++ b/ql/src/semmle/go/frameworks/stdlib/Os.qll @@ -53,6 +53,8 @@ module Os { fn = "Symlink" and pathidx in [0 .. 1] or fn = "Truncate" and pathidx = 0 + or + fn = "DirFS" and pathidx = 0 ) } From adc2f08b76087c7a19c4ecad909537a884ea33ef Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 18 Feb 2021 11:50:11 -0800 Subject: [PATCH 06/18] Add tests for go 1.16 libraries --- .../go/frameworks/StdlibTaintFlow/IoFs.go | 97 +++++++++++++++++++ .../go/frameworks/StdlibTaintFlow/Os.go | 29 +++++- .../frameworks/StdlibTaintFlow/test.expected | 0 .../go/frameworks/StdlibTaintFlow/test.ql | 17 ++++ 4 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/IoFs.go create mode 100644 ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/test.expected create mode 100644 ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/test.ql diff --git a/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/IoFs.go b/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/IoFs.go new file mode 100644 index 00000000000..9eea2dc516f --- /dev/null +++ b/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/IoFs.go @@ -0,0 +1,97 @@ +package main + +import ( + "io/fs" +) + +func walkDirCallback(path string, d fs.DirEntry, _ error) error { + sink(14, path) + sink(15, d) + return nil +} + +func steps() { + { + source := newSource(0).(fs.FS) + out, _ := fs.Glob(source, "*") + sink(0, out) + } + { + source := newSource(1).(fs.FS) + out, _ := fs.ReadFile(source, "filename") + sink(1, out) + } + { + source := newSource(2).(fs.FS) + out, _ := fs.ReadDir(source, "dirname") + sink(2, out) + } + { + source := newSource(3).(fs.FS) + out, _ := fs.Sub(source, "dirname") + sink(3, out) + } + { + source := newSource(4).(fs.FS) + fs.WalkDir(source, ".", func(_ string, d fs.DirEntry, _ error) error { + sink(4, d) + return nil + }) + } + { + source := newSource(5).(fs.FS) + fs.WalkDir(source, ".", func(path string, _ fs.DirEntry, _ error) error { + sink(5, path) + return nil + }) + } + { + source := newSource(6).(fs.DirEntry) + out := source.Name() + sink(6, out) + } + { + source := newSource(7).(fs.DirEntry) + out, _ := source.Info() + sink(7, out) + } + { + source := newSource(8).(fs.FS) + out, _ := source.Open("filename") + sink(8, out) + } + { + source := newSource(9).(fs.GlobFS) + out, _ := source.Glob("*") + sink(9, out) + } + { + source := newSource(10).(fs.ReadDirFS) + out, _ := source.ReadDir("dirname") + sink(10, out) + } + { + source := newSource(11).(fs.ReadFileFS) + out, _ := source.ReadFile("filename") + sink(11, out) + } + { + source := newSource(12).(fs.SubFS) + out, _ := source.Sub("dirname") + sink(12, out) + } + { + source := newSource(13).(fs.File) + var out []byte + source.Read(out) + sink(13, out) + } + { + source := newSource(14).(fs.FS) + fs.WalkDir(source, ".", walkDirCallback) + } + { + source := newSource(15).(fs.FS) + fs.WalkDir(source, ".", walkDirCallback) + } +} diff --git a/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Os.go b/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Os.go index 221d6868cd5..9736cddfdc8 100644 --- a/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Os.go +++ b/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Os.go @@ -5,6 +5,7 @@ package main import ( "os" "syscall" + "time" ) func TaintStepTest_OsExpand_B0I0O0(sourceCQL interface{}) interface{} { @@ -21,7 +22,7 @@ func TaintStepTest_OsExpandEnv_B0I0O0(sourceCQL interface{}) interface{} { func TaintStepTest_OsNewFile_B0I0O0(sourceCQL interface{}) interface{} { fromUintptr784 := sourceCQL.(uintptr) - intoFile957 := os.NewFile(fromUintptr784, "") + intoFile957 := os.NewFile(fromUintptr784, "") // $fsaccess="" return intoFile957 } @@ -149,3 +150,29 @@ func RunAllTaints_Os() { sink(11, out) } } + +func fsAccesses() { + var path, path1 string + var time time.Time + os.Chdir(path) // $fsaccess=path + os.Chmod(path, 0600) // $fsaccess=path + os.Chown(path, 1000, 1000) // $fsaccess=path + os.Chtimes(path, time, time) // $fsaccess=path + os.Create(path) // $fsaccess=path + os.Lchown(path, 1000, 1000) // $fsaccess=path + os.Link(path, path1) // $fsaccess=path $fsaccess=path1 + os.Lstat(path) // $fsaccess=path + os.Mkdir(path, 0600) // $fsaccess=path + os.MkdirAll(path, 0600) // $fsaccess=path + os.NewFile(124, path) // $fsaccess=path + os.Open(path) // $fsaccess=path + os.OpenFile(path, os.O_RDONLY, 0600) // $fsaccess=path + os.Readlink(path) // $fsaccess=path + os.Remove(path) // $fsaccess=path + os.RemoveAll(path) // $fsaccess=path + os.Rename(path, path1) // $fsaccess=path $fsaccess=path1 + os.Stat(path) // $fsaccess=path + os.Symlink(path, path1) // $fsaccess=path $fsaccess=path1 + os.Truncate(path, 1000) // $fsaccess=path + os.DirFS(path) // $fsaccess=path +} diff --git a/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/test.expected b/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/test.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/test.ql b/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/test.ql new file mode 100644 index 00000000000..30b7a2b4797 --- /dev/null +++ b/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/test.ql @@ -0,0 +1,17 @@ +import go +import TestUtilities.InlineExpectationsTest + +class FileSystemAccessTest extends InlineExpectationsTest { + FileSystemAccessTest() { this = "FileSystemAccess" } + + override string getARelevantTag() { result = "fsaccess" } + + override predicate hasActualResult(string file, int line, string element, string tag, string value) { + exists(FileSystemAccess f | + f.hasLocationInfo(file, line, _, _, _) and + element = f.toString() and + value = f.getAPathArgument().toString() and + tag = "fsaccess" + ) + } +} From 4056ac4ab5edf9073beaa0eb9794c98ee0db1d06 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 18 Feb 2021 12:28:19 -0800 Subject: [PATCH 07/18] os.FileInfo -> io/fs.FileInfo --- ql/src/semmle/go/security/StoredXssCustomizations.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/semmle/go/security/StoredXssCustomizations.qll b/ql/src/semmle/go/security/StoredXssCustomizations.qll index c5d6832adef..fbe2029b71c 100644 --- a/ql/src/semmle/go/security/StoredXssCustomizations.qll +++ b/ql/src/semmle/go/security/StoredXssCustomizations.qll @@ -51,7 +51,7 @@ module StoredXss { ) or // A call to os.FileInfo.Name - exists(Method m | m.implements("os", "FileInfo", "Name") | + exists(Method m | m.implements("io/fs", "FileInfo", "Name") | m = this.(DataFlow::CallNode).getTarget() ) } From 41cacd579fe5bab83c3394efcc6a2b27eabbc909 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 18 Feb 2021 15:17:46 -0800 Subject: [PATCH 08/18] Model moved io/ioutil functions --- ql/src/semmle/go/frameworks/stdlib/Io.qll | 8 ++++++++ ql/src/semmle/go/frameworks/stdlib/Os.qll | 10 ++++++++++ .../semmle/go/frameworks/StdlibTaintFlow/Io.go | 10 ++++++++++ .../semmle/go/frameworks/StdlibTaintFlow/Os.go | 7 ++++++- 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/ql/src/semmle/go/frameworks/stdlib/Io.qll b/ql/src/semmle/go/frameworks/stdlib/Io.qll index 512098b8c1e..968f45dfe9b 100644 --- a/ql/src/semmle/go/frameworks/stdlib/Io.qll +++ b/ql/src/semmle/go/frameworks/stdlib/Io.qll @@ -61,6 +61,14 @@ module Io { // signature: func WriteString(w Writer, s string) (n int, err error) hasQualifiedName("io", "WriteString") and (inp.isParameter(1) and outp.isParameter(0)) + or + // signature: func NopCloser(r io.Reader) io.ReadCloser + hasQualifiedName("io", "NopCloser") and + (inp.isParameter(0) and outp.isResult()) + or + // signature: func ReadAll(r io.Reader) ([]byte, error) + hasQualifiedName("io", "ReadAll") and + (inp.isParameter(0) and outp.isResult(0)) } override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { diff --git a/ql/src/semmle/go/frameworks/stdlib/Os.qll b/ql/src/semmle/go/frameworks/stdlib/Os.qll index 3b2ea652e93..a5b94f3a471 100644 --- a/ql/src/semmle/go/frameworks/stdlib/Os.qll +++ b/ql/src/semmle/go/frameworks/stdlib/Os.qll @@ -55,6 +55,16 @@ module Os { fn = "Truncate" and pathidx = 0 or fn = "DirFS" and pathidx = 0 + or + fn = "ReadDir" and pathidx = 0 + or + fn = "ReadFile" and pathidx = 0 + or + fn = "MkdirTemp" and pathidx in [0 .. 1] + or + fn = "CreateTemp" and pathidx in [0 .. 1] + or + fn = "WriteFile" and pathidx = 0 ) } diff --git a/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Io.go b/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Io.go index e5bc3ca1cfd..d06e9b6d818 100644 --- a/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Io.go +++ b/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Io.go @@ -302,4 +302,14 @@ func RunAllTaints_Io() { out := TaintStepTest_IoWriterToWriteTo_B0I0O0(source) sink(24, out) } + { + source := newSource(25).(io.Reader) + out := io.NopCloser(source) + sink(25, out) + } + { + source := newSource(26).(io.Reader) + out, _ := io.ReadAll(source) + sink(26, out) + } } diff --git a/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Os.go b/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Os.go index 9736cddfdc8..a930c2d435f 100644 --- a/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Os.go +++ b/ql/test/library-tests/semmle/go/frameworks/StdlibTaintFlow/Os.go @@ -152,7 +152,7 @@ func RunAllTaints_Os() { } func fsAccesses() { - var path, path1 string + var path, path1, part string var time time.Time os.Chdir(path) // $fsaccess=path os.Chmod(path, 0600) // $fsaccess=path @@ -175,4 +175,9 @@ func fsAccesses() { os.Symlink(path, path1) // $fsaccess=path $fsaccess=path1 os.Truncate(path, 1000) // $fsaccess=path os.DirFS(path) // $fsaccess=path + os.ReadDir(path) // $fsaccess=path + os.ReadFile(path) // $fsaccess=path + os.MkdirTemp(path, part) // $fsaccess=path $fsaccess=part + os.CreateTemp(path, part) // $fsaccess=path $fsaccess=part + os.WriteFile(path, []byte{}, 0600) // $fsaccess=path } From 82849fe91a21f3ef4700b745ee46219835bd41cb Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 18 Feb 2021 15:27:01 -0800 Subject: [PATCH 09/18] Explicitly set GO111MODULE=off --- extractor/cli/go-autobuilder/go-autobuilder.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/extractor/cli/go-autobuilder/go-autobuilder.go b/extractor/cli/go-autobuilder/go-autobuilder.go index be3b78e8271..fde561ee2d4 100644 --- a/extractor/cli/go-autobuilder/go-autobuilder.go +++ b/extractor/cli/go-autobuilder/go-autobuilder.go @@ -549,10 +549,13 @@ func main() { install = exec.Command("glide", "install") log.Println("Installing dependencies using `glide install`") } else { + // explicitly set go module support if depMode == GoGetWithModules { - // enable go modules if used os.Setenv("GO111MODULE", "on") + } else if depMode == GoGetNoModules { + os.Setenv("GO111MODULE", "off") } + // get dependencies install = exec.Command("go", "get", "-v", "./...") log.Println("Installing dependencies using `go get -v ./...`.") From 23103fd8e0c706ef8bbd74f6e3977a05ddb313d4 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 18 Feb 2021 15:39:35 -0800 Subject: [PATCH 10/18] Add support for 'path/filepath.WalkDir' --- .../semmle/go/security/StoredXssCustomizations.qll | 9 +++------ .../query-tests/Security/CWE-079/StoredXss.expected | 12 ++++++++---- ql/test/query-tests/Security/CWE-079/stored.go | 12 ++++++++++++ 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/ql/src/semmle/go/security/StoredXssCustomizations.qll b/ql/src/semmle/go/security/StoredXssCustomizations.qll index fbe2029b71c..973635b754f 100644 --- a/ql/src/semmle/go/security/StoredXssCustomizations.qll +++ b/ql/src/semmle/go/security/StoredXssCustomizations.qll @@ -42,12 +42,9 @@ module StoredXss { class FileNameSource extends Source { FileNameSource() { // the second parameter to a filepath.Walk function - exists(DataFlow::ParameterNode prm, DataFlow::FunctionNode f, DataFlow::CallNode walkCall | - prm = this and - f.getParameter(0) = prm - | - walkCall.getTarget().hasQualifiedName("path/filepath", "Walk") and - walkCall.getArgument(1) = f.getASuccessor*() + exists(DataFlow::FunctionNode f, Function walkFn | this = f.getParameter(0) | + walkFn.hasQualifiedName("path/filepath", ["Walk", "WalkDir"]) and + walkFn.getACall().getArgument(1) = f.getASuccessor*() ) or // A call to os.FileInfo.Name diff --git a/ql/test/query-tests/Security/CWE-079/StoredXss.expected b/ql/test/query-tests/Security/CWE-079/StoredXss.expected index c9de068a66d..5c4a507e557 100644 --- a/ql/test/query-tests/Security/CWE-079/StoredXss.expected +++ b/ql/test/query-tests/Security/CWE-079/StoredXss.expected @@ -1,11 +1,15 @@ edges | StoredXss.go:13:21:13:31 | call to Name : string | StoredXss.go:13:21:13:36 | ...+... | -| stored.go:16:3:16:28 | ... := ...[0] : pointer type | stored.go:28:22:28:25 | name | +| stored.go:18:3:18:28 | ... := ...[0] : pointer type | stored.go:30:22:30:25 | name | +| stored.go:59:30:59:33 | definition of path : string | stored.go:61:22:61:25 | path | nodes | StoredXss.go:13:21:13:31 | call to Name : string | semmle.label | call to Name : string | | StoredXss.go:13:21:13:36 | ...+... | semmle.label | ...+... | -| stored.go:16:3:16:28 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type | -| stored.go:28:22:28:25 | name | semmle.label | name | +| stored.go:18:3:18:28 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type | +| stored.go:30:22:30:25 | name | semmle.label | name | +| stored.go:59:30:59:33 | definition of path : string | semmle.label | definition of path : string | +| stored.go:61:22:61:25 | path | semmle.label | path | #select | StoredXss.go:13:21:13:36 | ...+... | StoredXss.go:13:21:13:31 | call to Name : string | StoredXss.go:13:21:13:36 | ...+... | Stored cross-site scripting vulnerability due to $@. | StoredXss.go:13:21:13:31 | call to Name | stored value | -| stored.go:28:22:28:25 | name | stored.go:16:3:16:28 | ... := ...[0] : pointer type | stored.go:28:22:28:25 | name | Stored cross-site scripting vulnerability due to $@. | stored.go:16:3:16:28 | ... := ...[0] | stored value | +| stored.go:30:22:30:25 | name | stored.go:18:3:18:28 | ... := ...[0] : pointer type | stored.go:30:22:30:25 | name | Stored cross-site scripting vulnerability due to $@. | stored.go:18:3:18:28 | ... := ...[0] | stored value | +| stored.go:61:22:61:25 | path | stored.go:59:30:59:33 | definition of path : string | stored.go:61:22:61:25 | path | Stored cross-site scripting vulnerability due to $@. | stored.go:59:30:59:33 | definition of path | stored value | diff --git a/ql/test/query-tests/Security/CWE-079/stored.go b/ql/test/query-tests/Security/CWE-079/stored.go index 005a8e5f635..807ae7e1d44 100644 --- a/ql/test/query-tests/Security/CWE-079/stored.go +++ b/ql/test/query-tests/Security/CWE-079/stored.go @@ -3,8 +3,10 @@ package main import ( "database/sql" "io" + "io/fs" "log" "net/http" + "path/filepath" ) var db *sql.DB @@ -51,3 +53,13 @@ func storedserve2() { } }) } + +func storedserve3() { + http.HandleFunc("/user", func(w http.ResponseWriter, r *http.Request) { + filepath.WalkDir(".", func(path string, _ fs.DirEntry, err error) error { + // BAD: filenames are considered to be untrusted + io.WriteString(w, path) + return nil + }) + }) +} From 65e6da9b0ede96f3b9ad48339e638cbc93e7d42f Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 19 Feb 2021 09:40:50 -0800 Subject: [PATCH 11/18] Actions: Add change note checker Co-authored-by: Taus --- .github/workflows/check-change-note.yml | 33 +++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 .github/workflows/check-change-note.yml diff --git a/.github/workflows/check-change-note.yml b/.github/workflows/check-change-note.yml new file mode 100644 index 00000000000..d28756aef14 --- /dev/null +++ b/.github/workflows/check-change-note.yml @@ -0,0 +1,33 @@ +on: + pull_request_target: + types: [labeled, unlabeled, opened, synchronize, reopened, ready_for_review] + paths: + - "*/ql/src/**/*.ql" + - "*/ql/src/**/*.qll" + - "!**/experimental/**" + +jobs: + check-change-note: + runs-on: ubuntu-latest + steps: + - name: Check if change note file is present + uses: dorny/paths-filter@7c0f15b688b020e95e00f15c61299b022f08ca95 # v2.8.0 + id: paths_filter + with: + filters: | + change_note: + - '**/change-notes/*.md' + - name: Get PR labels + id: pr-labels + uses: joerick/pr-labels-action@0a4cc4ee0ab557ec0b1ae1157fa6fa7f9f4c494b # v1.0.6 + - name: Fail if change note is missing + uses: actions/github-script@v3 + if: | + github.event.pull_request.draft == false && + steps.paths_filter.outputs.change_note == 'false' && + !contains(steps.pr-labels.outputs.labels, ' no-change-note-required ') + with: + github-token: ${{secrets.GITHUB_TOKEN}} + script: | + core.setFailed('No change note found.' + + ' Either add one, or add the `no-change-note-required` label.') From 17cd04c6b2694fae1b1f5acce73085c1777482b8 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 19 Feb 2021 10:20:29 -0800 Subject: [PATCH 12/18] Avoid attempting to build i386 darwin binaries --- Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index dd178602e86..29bb3321927 100644 --- a/Makefile +++ b/Makefile @@ -115,7 +115,8 @@ ql/src/go.dbscheme.stats: ql/src/go.dbscheme build/stats/src.stamp extractor test: all build/testdb/check-upgrade-path codeql test run ql/test --search-path . --consistency-queries ql/test/consistency - env GOARCH=386 codeql$(EXE) test run ql/test/query-tests/Security/CWE-681 --search-path . --consistency-queries ql/test/consistency + # use GOOS=linux because GOOS=darwin GOARCH=386 is no longer supported + env GOOS=linux GOARCH=386 codeql$(EXE) test run ql/test/query-tests/Security/CWE-681 --search-path . --consistency-queries ql/test/consistency cd extractor; go test -mod=vendor ./... | grep -vF "[no test files]" .PHONY: build/testdb/check-upgrade-path From 2bcf73c9fbea730266c29dd5015da8d85eec537a Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 22 Feb 2021 11:38:13 +0000 Subject: [PATCH 13/18] Add new module path for beego Beego moved from astaxie/beego to beego/beego on 13 Dec 2020. The old location still works but is not being updated. --- ql/src/semmle/go/frameworks/Beego.qll | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/ql/src/semmle/go/frameworks/Beego.qll b/ql/src/semmle/go/frameworks/Beego.qll index 246c8cc6b6b..a893da684ed 100644 --- a/ql/src/semmle/go/frameworks/Beego.qll +++ b/ql/src/semmle/go/frameworks/Beego.qll @@ -1,6 +1,6 @@ /** * Provides classes for working with untrusted flow sources, sinks and taint propagators - * from the `github.com/astaxie/beego` package. + * from the `github.com/beego/beego` package. */ import go @@ -8,21 +8,25 @@ import semmle.go.security.Xss private import semmle.go.security.SafeUrlFlowCustomizations module Beego { - /** Gets the package name `github.com/astaxie/beego`. */ + /** Gets the module path `github.com/astaxie/beego` or `github.com/beego/beego`. */ bindingset[result] - string packagePath() { result = package("github.com/astaxie/beego", "") } + string modulePath() { result = ["github.com/astaxie/beego", "github.com/beego/beego"] } - /** Gets the context subpackage name `github.com/astaxie/beego/context`. */ + /** Gets the path for the root package of beego. */ bindingset[result] - string contextPackagePath() { result = package("github.com/astaxie/beego", "context") } + string packagePath() { result = package(modulePath(), "") } - /** Gets the logs subpackage name `github.com/astaxie/beego/logs`. */ + /** Gets the path for the context package of beego. */ bindingset[result] - string logsPackagePath() { result = package("github.com/astaxie/beego", "logs") } + string contextPackagePath() { result = package(modulePath(), "context") } - /** Gets the utils subpackage name `github.com/astaxie/beego/utils`. */ + /** Gets the path for the logs package of beego. */ bindingset[result] - string utilsPackagePath() { result = package("github.com/astaxie/beego", "utils") } + string logsPackagePath() { result = package(modulePath(), "logs") } + + /** Gets the path for the utils package of beego. */ + bindingset[result] + string utilsPackagePath() { result = package(modulePath(), "utils") } /** * `BeegoInput` sources of untrusted data. From 083512acef774117cb209822d36ae491e928ed5d Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 22 Feb 2021 15:07:56 +0000 Subject: [PATCH 14/18] Add extra module path for xmlpath package --- ql/src/semmle/go/frameworks/XPath.qll | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/ql/src/semmle/go/frameworks/XPath.qll b/ql/src/semmle/go/frameworks/XPath.qll index 52ec9c82054..97edd0216e7 100644 --- a/ql/src/semmle/go/frameworks/XPath.qll +++ b/ql/src/semmle/go/frameworks/XPath.qll @@ -102,12 +102,12 @@ module XPath { private class GoXmlpathXmlpathXPathExpressionString extends Range { GoXmlpathXmlpathXPathExpressionString() { exists(Function f, string name | name.matches("Compile%") | - f.hasQualifiedName(package("github.com/go-xmlpath/xmlpath", ""), name) and + f.hasQualifiedName(XmlPath::packagePath(), name) and this = f.getACall().getArgument(0) ) or exists(Function f, string name | name.matches("MustCompile%") | - f.hasQualifiedName(package("github.com/go-xmlpath/xmlpath", ""), name) and + f.hasQualifiedName(XmlPath::packagePath(), name) and this = f.getACall().getArgument(0) ) } @@ -164,3 +164,11 @@ module XPath { } } } + +module XmlPath { + /** Gets the package name `github.com/go-xmlpath/xmlpath` or `gopkg.in/xmlpath`. */ + bindingset[result] + string packagePath() { + result = package(["github.com/go-xmlpath/xmlpath", "gopkg.in/xmlpath"], "") + } +} From 370afe33830817ef50f76c8e1b5f7d16a45490c3 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 22 Feb 2021 14:34:11 +0000 Subject: [PATCH 15/18] Fix incorrect calls to `package()` --- ql/src/semmle/go/frameworks/SQL.qll | 2 +- ql/src/semmle/go/frameworks/WebSocket.qll | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ql/src/semmle/go/frameworks/SQL.qll b/ql/src/semmle/go/frameworks/SQL.qll index 3c351969d88..191f833d45d 100644 --- a/ql/src/semmle/go/frameworks/SQL.qll +++ b/ql/src/semmle/go/frameworks/SQL.qll @@ -206,7 +206,7 @@ module SQL { private class SqlxSink extends SQL::QueryString::Range { SqlxSink() { exists(Method meth, string name, int n | - meth.hasQualifiedName(package("github.com/jmoiron", "sqlx"), ["DB", "Tx"], name) and + meth.hasQualifiedName(package("github.com/jmoiron/sqlx", ""), ["DB", "Tx"], name) and this = meth.getACall().getArgument(n) | name = ["Select", "Get"] and n = 1 diff --git a/ql/src/semmle/go/frameworks/WebSocket.qll b/ql/src/semmle/go/frameworks/WebSocket.qll index 39e635ac58a..02ab3940210 100644 --- a/ql/src/semmle/go/frameworks/WebSocket.qll +++ b/ql/src/semmle/go/frameworks/WebSocket.qll @@ -319,5 +319,5 @@ module NhooyrWebSocket { module GobwasWs { /** Gets the package name `github.com/gobwas/ws`. */ bindingset[result] - string packagePath() { result = package("github.com/gobwas", "ws") } + string packagePath() { result = package("github.com/gobwas/ws", "") } } From f32b4883bf47314a7dd9de9a86c3ed3a69e0cd82 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 22 Feb 2021 14:47:41 +0000 Subject: [PATCH 16/18] Make use of URLs in comments more consistent --- ql/src/semmle/go/frameworks/GoRestfulHttp.qll | 2 +- ql/src/semmle/go/frameworks/XPath.qll | 40 +++++++++++++++---- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/ql/src/semmle/go/frameworks/GoRestfulHttp.qll b/ql/src/semmle/go/frameworks/GoRestfulHttp.qll index 2219c334f3f..05ad3095376 100644 --- a/ql/src/semmle/go/frameworks/GoRestfulHttp.qll +++ b/ql/src/semmle/go/frameworks/GoRestfulHttp.qll @@ -1,7 +1,7 @@ import go /** - * Provides models of the go-restful library (`https://github.com/emicklei/go-restful`). + * Provides models of the [go-restful library](https://github.com/emicklei/go-restful). */ private module GoRestfulHttp { /** Gets the package name `github.com/emicklei/go-restful`. */ diff --git a/ql/src/semmle/go/frameworks/XPath.qll b/ql/src/semmle/go/frameworks/XPath.qll index 97edd0216e7..70651c9ae75 100644 --- a/ql/src/semmle/go/frameworks/XPath.qll +++ b/ql/src/semmle/go/frameworks/XPath.qll @@ -28,7 +28,10 @@ module XPath { */ abstract class Range extends DataFlow::Node { } - /** An XPath expression string used in an API function of the https://github.com/antchfx/xpath package. */ + /** + * An XPath expression string used in an API function of the + * [XPath](https://github.com/antchfx/xpath) package. + */ private class AntchfxXpathXPathExpressionString extends Range { AntchfxXpathXPathExpressionString() { exists(Function f, string name | name.matches("Compile%") | @@ -48,7 +51,10 @@ module XPath { } } - /** An XPath expression string used in an API function of the https://github.com/antchfx/htmlquery package. */ + /** + * An XPath expression string used in an API function of the + * [htmlquery](https://github.com/antchfx/htmlquery) package. + */ private class AntchfxHtmlqueryXPathExpressionString extends Range { AntchfxHtmlqueryXPathExpressionString() { exists(Function f, string name | name.matches("Find%") | @@ -63,7 +69,10 @@ module XPath { } } - /** An XPath expression string used in an API function of the https://github.com/antchfx/xmlquery package. */ + /** + * An XPath expression string used in an API function of the + * [xmlquery](https://github.com/antchfx/xmlquery) package. + */ private class AntchfxXmlqueryXPathExpressionString extends Range { AntchfxXmlqueryXPathExpressionString() { exists(Function f, string name | name.matches("Find%") | @@ -83,7 +92,10 @@ module XPath { } } - /** An XPath expression string used in an API function of the https://github.com/antchfx/jsonquery package. */ + /** + * An XPath expression string used in an API function of the + * [jsonquery](https://github.com/antchfx/jsonquery) package. + */ private class AntchfxJsonqueryXPathExpressionString extends Range { AntchfxJsonqueryXPathExpressionString() { exists(Function f, string name | name.matches("Find%") | @@ -98,7 +110,10 @@ module XPath { } } - /** An XPath expression string used in an API function of the https://github.com/go-xmlpath/xmlpath package. */ + /** + * An XPath expression string used in an API function of the + * [xmlpath](https://github.com/go-xmlpath/xmlpath) package. + */ private class GoXmlpathXmlpathXPathExpressionString extends Range { GoXmlpathXmlpathXPathExpressionString() { exists(Function f, string name | name.matches("Compile%") | @@ -113,7 +128,10 @@ module XPath { } } - /** An XPath expression string used in an API function of the https://github.com/ChrisTrenkamp/goxpath package. */ + /** + * An XPath expression string used in an API function of the + * [goxpath](https://github.com/ChrisTrenkamp/goxpath) package. + */ private class ChrisTrenkampGoxpathXPathExpressionString extends Range { ChrisTrenkampGoxpathXPathExpressionString() { exists(Function f, string name | name.matches("Parse%") | @@ -128,7 +146,10 @@ module XPath { } } - /** An XPath expression string used in an API function of the https://github.com/santhosh-tekuri/xpathparser package. */ + /** + * An XPath expression string used in an API function of the + * [xpathparser](https://github.com/santhosh-tekuri/xpathparser) package. + */ private class SanthoshTekuriXpathparserXPathExpressionString extends Range { SanthoshTekuriXpathparserXPathExpressionString() { exists(Function f, string name | name.matches("Parse%") | @@ -143,7 +164,10 @@ module XPath { } } - /** An XPath expression string used in an API function of the https://github.com/jbowtie/gokogiri package. */ + /** + * An XPath expression string used in an API function of the + * [gokogiri]https://github.com/jbowtie/gokogiri) package. + */ private class JbowtieGokogiriXPathExpressionString extends Range { JbowtieGokogiriXPathExpressionString() { exists(Function f, string name | name.matches("Compile%") | From ff317e63de46971b078fab45dc90ac31270f7ea9 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 22 Feb 2021 14:47:54 +0000 Subject: [PATCH 17/18] Remove `http://` in package path --- ql/src/semmle/go/security/ExternalAPIs.qll | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ql/src/semmle/go/security/ExternalAPIs.qll b/ql/src/semmle/go/security/ExternalAPIs.qll index 64a535e9f98..2404a7de949 100644 --- a/ql/src/semmle/go/security/ExternalAPIs.qll +++ b/ql/src/semmle/go/security/ExternalAPIs.qll @@ -17,9 +17,7 @@ private import Logrus abstract class SafeExternalAPIFunction extends Function { } private predicate isDefaultSafePackage(Package package) { - package.getPath() in [ - "time", "unicode/utf8", package("http://gopkg.in/go-playground/validator", "") - ] + package.getPath() in ["time", "unicode/utf8", package("gopkg.in/go-playground/validator", "")] } /** The default set of "safe" external APIs. */ From f3969372a4df455948af39ac88981fb9189ac82c Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 23 Feb 2021 08:59:31 -0800 Subject: [PATCH 18/18] Add shati-patel to CODEOWNERS --- CODEOWNERS | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CODEOWNERS b/CODEOWNERS index d076d9576d4..72a3954cfb5 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -1 +1,6 @@ * @github/codeql-go + +# Documentation +**/*.qhelp @shati-patel +# Exclude experimental +**/experimental/**/*.qhelp @github/codeql-go