From 4d4129313ac3ebbdb64b51bb3c0889cf269df122 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 24 Aug 2020 17:04:31 +0100 Subject: [PATCH 01/18] Fix tests for `Gorestful`. --- .../HTTP/Gorestful/gorestful.expected | 28 ++--- .../go/frameworks/HTTP/Gorestful/gorestful.go | 2 + .../frameworks/HTTP/Gorestful/gorestful_v2.go | 2 + .../github.com/emicklei/go-restful/stub.go | 117 +++++++++++++++++- .../github.com/emicklei/go-restful/v3/stub.go | 115 ++++++++++++++++- 5 files changed, 243 insertions(+), 21 deletions(-) diff --git a/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful.expected b/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful.expected index ee3137d73ee..4c252dadb93 100644 --- a/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful.expected +++ b/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful.expected @@ -1,14 +1,14 @@ -| gorestful.go:13:15:13:47 | index expression | gorestful.go:13:15:13:44 | call to QueryParameters : slice type | gorestful.go:13:15:13:47 | index expression | This command depends on $@. | gorestful.go:13:15:13:44 | call to QueryParameters | a user-provided value | -| gorestful.go:14:15:14:43 | call to QueryParameter | gorestful.go:14:15:14:43 | call to QueryParameter | gorestful.go:14:15:14:43 | call to QueryParameter | This command depends on $@. | gorestful.go:14:15:14:43 | call to QueryParameter | a user-provided value | -| gorestful.go:16:15:16:17 | val | gorestful.go:15:12:15:39 | call to BodyParameter : tuple type | gorestful.go:16:15:16:17 | val | This command depends on $@. | gorestful.go:15:12:15:39 | call to BodyParameter | a user-provided value | -| gorestful.go:17:15:17:44 | call to HeaderParameter | gorestful.go:17:15:17:44 | call to HeaderParameter | gorestful.go:17:15:17:44 | call to HeaderParameter | This command depends on $@. | gorestful.go:17:15:17:44 | call to HeaderParameter | a user-provided value | -| gorestful.go:18:15:18:42 | call to PathParameter | gorestful.go:18:15:18:42 | call to PathParameter | gorestful.go:18:15:18:42 | call to PathParameter | This command depends on $@. | gorestful.go:18:15:18:42 | call to PathParameter | a user-provided value | -| gorestful.go:19:15:19:45 | index expression | gorestful.go:19:15:19:38 | call to PathParameters : map type | gorestful.go:19:15:19:45 | index expression | This command depends on $@. | gorestful.go:19:15:19:38 | call to PathParameters | a user-provided value | -| gorestful.go:22:15:22:21 | selection of cmd | gorestful.go:21:21:21:24 | &... : pointer type | gorestful.go:22:15:22:21 | selection of cmd | This command depends on $@. | gorestful.go:21:21:21:24 | &... | a user-provided value | -| gorestful_v2.go:13:15:13:47 | index expression | gorestful_v2.go:13:15:13:44 | call to QueryParameters : slice type | gorestful_v2.go:13:15:13:47 | index expression | This command depends on $@. | gorestful_v2.go:13:15:13:44 | call to QueryParameters | a user-provided value | -| gorestful_v2.go:14:15:14:43 | call to QueryParameter | gorestful_v2.go:14:15:14:43 | call to QueryParameter | gorestful_v2.go:14:15:14:43 | call to QueryParameter | This command depends on $@. | gorestful_v2.go:14:15:14:43 | call to QueryParameter | a user-provided value | -| gorestful_v2.go:16:15:16:17 | val | gorestful_v2.go:15:12:15:39 | call to BodyParameter : tuple type | gorestful_v2.go:16:15:16:17 | val | This command depends on $@. | gorestful_v2.go:15:12:15:39 | call to BodyParameter | a user-provided value | -| gorestful_v2.go:17:15:17:44 | call to HeaderParameter | gorestful_v2.go:17:15:17:44 | call to HeaderParameter | gorestful_v2.go:17:15:17:44 | call to HeaderParameter | This command depends on $@. | gorestful_v2.go:17:15:17:44 | call to HeaderParameter | a user-provided value | -| gorestful_v2.go:18:15:18:42 | call to PathParameter | gorestful_v2.go:18:15:18:42 | call to PathParameter | gorestful_v2.go:18:15:18:42 | call to PathParameter | This command depends on $@. | gorestful_v2.go:18:15:18:42 | call to PathParameter | a user-provided value | -| gorestful_v2.go:19:15:19:45 | index expression | gorestful_v2.go:19:15:19:38 | call to PathParameters : map type | gorestful_v2.go:19:15:19:45 | index expression | This command depends on $@. | gorestful_v2.go:19:15:19:38 | call to PathParameters | a user-provided value | -| gorestful_v2.go:22:15:22:21 | selection of cmd | gorestful_v2.go:21:21:21:24 | &... : pointer type | gorestful_v2.go:22:15:22:21 | selection of cmd | This command depends on $@. | gorestful_v2.go:21:21:21:24 | &... | a user-provided value | +| gorestful.go:15:15:15:47 | index expression | gorestful.go:15:15:15:44 | call to QueryParameters : slice type | gorestful.go:15:15:15:47 | index expression | This command depends on $@. | gorestful.go:15:15:15:44 | call to QueryParameters | a user-provided value | +| gorestful.go:16:15:16:43 | call to QueryParameter | gorestful.go:16:15:16:43 | call to QueryParameter | gorestful.go:16:15:16:43 | call to QueryParameter | This command depends on $@. | gorestful.go:16:15:16:43 | call to QueryParameter | a user-provided value | +| gorestful.go:18:15:18:17 | val | gorestful.go:17:12:17:39 | call to BodyParameter : tuple type | gorestful.go:18:15:18:17 | val | This command depends on $@. | gorestful.go:17:12:17:39 | call to BodyParameter | a user-provided value | +| gorestful.go:19:15:19:44 | call to HeaderParameter | gorestful.go:19:15:19:44 | call to HeaderParameter | gorestful.go:19:15:19:44 | call to HeaderParameter | This command depends on $@. | gorestful.go:19:15:19:44 | call to HeaderParameter | a user-provided value | +| gorestful.go:20:15:20:42 | call to PathParameter | gorestful.go:20:15:20:42 | call to PathParameter | gorestful.go:20:15:20:42 | call to PathParameter | This command depends on $@. | gorestful.go:20:15:20:42 | call to PathParameter | a user-provided value | +| gorestful.go:21:15:21:45 | index expression | gorestful.go:21:15:21:38 | call to PathParameters : map type | gorestful.go:21:15:21:45 | index expression | This command depends on $@. | gorestful.go:21:15:21:38 | call to PathParameters | a user-provided value | +| gorestful.go:24:15:24:21 | selection of cmd | gorestful.go:23:21:23:24 | &... : pointer type | gorestful.go:24:15:24:21 | selection of cmd | This command depends on $@. | gorestful.go:23:21:23:24 | &... | a user-provided value | +| gorestful_v2.go:15:15:15:47 | index expression | gorestful_v2.go:15:15:15:44 | call to QueryParameters : slice type | gorestful_v2.go:15:15:15:47 | index expression | This command depends on $@. | gorestful_v2.go:15:15:15:44 | call to QueryParameters | a user-provided value | +| gorestful_v2.go:16:15:16:43 | call to QueryParameter | gorestful_v2.go:16:15:16:43 | call to QueryParameter | gorestful_v2.go:16:15:16:43 | call to QueryParameter | This command depends on $@. | gorestful_v2.go:16:15:16:43 | call to QueryParameter | a user-provided value | +| gorestful_v2.go:18:15:18:17 | val | gorestful_v2.go:17:12:17:39 | call to BodyParameter : tuple type | gorestful_v2.go:18:15:18:17 | val | This command depends on $@. | gorestful_v2.go:17:12:17:39 | call to BodyParameter | a user-provided value | +| gorestful_v2.go:19:15:19:44 | call to HeaderParameter | gorestful_v2.go:19:15:19:44 | call to HeaderParameter | gorestful_v2.go:19:15:19:44 | call to HeaderParameter | This command depends on $@. | gorestful_v2.go:19:15:19:44 | call to HeaderParameter | a user-provided value | +| gorestful_v2.go:20:15:20:42 | call to PathParameter | gorestful_v2.go:20:15:20:42 | call to PathParameter | gorestful_v2.go:20:15:20:42 | call to PathParameter | This command depends on $@. | gorestful_v2.go:20:15:20:42 | call to PathParameter | a user-provided value | +| gorestful_v2.go:21:15:21:45 | index expression | gorestful_v2.go:21:15:21:38 | call to PathParameters : map type | gorestful_v2.go:21:15:21:45 | index expression | This command depends on $@. | gorestful_v2.go:21:15:21:38 | call to PathParameters | a user-provided value | +| gorestful_v2.go:24:15:24:21 | selection of cmd | gorestful_v2.go:23:21:23:24 | &... : pointer type | gorestful_v2.go:24:15:24:21 | selection of cmd | This command depends on $@. | gorestful_v2.go:23:21:23:24 | &... | a user-provided value | diff --git a/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful.go b/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful.go index fa2a1959aa8..95efc52f483 100644 --- a/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful.go +++ b/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful.go @@ -1,5 +1,7 @@ package gorestfultest +//go:generate depstubber -vendor github.com/emicklei/go-restful/v3 Request,Response + import ( restful "github.com/emicklei/go-restful/v3" "os/exec" diff --git a/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful_v2.go b/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful_v2.go index 884c037b052..3ddf3871625 100644 --- a/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful_v2.go +++ b/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/gorestful_v2.go @@ -1,5 +1,7 @@ package gorestfultest +//go:generate depstubber -vendor github.com/emicklei/go-restful Request,Response + import ( restful "github.com/emicklei/go-restful" "os/exec" diff --git a/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/vendor/github.com/emicklei/go-restful/stub.go b/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/vendor/github.com/emicklei/go-restful/stub.go index 7cde3962a7f..7fd52cf1eee 100644 --- a/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/vendor/github.com/emicklei/go-restful/stub.go +++ b/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/vendor/github.com/emicklei/go-restful/stub.go @@ -1,16 +1,23 @@ // Code generated by depstubber. DO NOT EDIT. -// This is a simple stub for github.com/emicklei/go-restful/v3, strictly for use in testing. +// This is a simple stub for github.com/emicklei/go-restful, strictly for use in testing. // See the LICENSE file for information about the licensing of the original library. -// Source: github.com/emicklei/go-restful/v3 (exports: Request; functions: ) +// Source: github.com/emicklei/go-restful (exports: Request,Response; functions: ) -// Package gorestfulstub is a stub of github.com/emicklei/go-restful, generated by depstubber. -package gorestfulstub +// Package gopkg is a stub of github.com/emicklei/go-restful, generated by depstubber. +package gopkg import ( + bufio "bufio" + net "net" http "net/http" ) +type EntityReaderWriter interface { + Read(_ *Request, _ interface{}) error + Write(_ *Response, _ int, _ interface{}) error +} + type Request struct { Request *http.Request } @@ -52,3 +59,105 @@ func (_ *Request) ReadEntity(_ interface{}) error { } func (_ *Request) SetAttribute(_ string, _ interface{}) {} + +type Response struct { + ResponseWriter http.ResponseWriter +} + +func (_ Response) AddHeader(_ string, _ string) Response { + return Response{} +} + +func (_ Response) CloseNotify() <-chan bool { + return nil +} + +func (_ Response) ContentLength() int { + return 0 +} + +func (_ Response) Error() error { + return nil +} + +func (_ Response) Header() http.Header { + return nil +} + +func (_ Response) InternalServerError() Response { + return Response{} +} + +func (_ Response) StatusCode() int { + return 0 +} + +func (_ *Response) EntityWriter() (EntityReaderWriter, bool) { + return nil, false +} + +func (_ *Response) Flush() {} + +func (_ *Response) Hijack() (net.Conn, *bufio.ReadWriter, error) { + return nil, nil, nil +} + +func (_ *Response) PrettyPrint(_ bool) {} + +func (_ *Response) SetRequestAccepts(_ string) {} + +func (_ *Response) Write(_ []byte) (int, error) { + return 0, nil +} + +func (_ *Response) WriteAsJson(_ interface{}) error { + return nil +} + +func (_ *Response) WriteAsXml(_ interface{}) error { + return nil +} + +func (_ *Response) WriteEntity(_ interface{}) error { + return nil +} + +func (_ *Response) WriteError(_ int, _ error) error { + return nil +} + +func (_ *Response) WriteErrorString(_ int, _ string) error { + return nil +} + +func (_ *Response) WriteHeader(_ int) {} + +func (_ *Response) WriteHeaderAndEntity(_ int, _ interface{}) error { + return nil +} + +func (_ *Response) WriteHeaderAndJson(_ int, _ interface{}, _ string) error { + return nil +} + +func (_ *Response) WriteHeaderAndXml(_ int, _ interface{}) error { + return nil +} + +func (_ *Response) WriteJson(_ interface{}, _ string) error { + return nil +} + +func (_ *Response) WriteServiceError(_ int, _ ServiceError) error { + return nil +} + +type ServiceError struct { + Code int + Message string + Header http.Header +} + +func (_ ServiceError) Error() string { + return "" +} diff --git a/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/vendor/github.com/emicklei/go-restful/v3/stub.go b/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/vendor/github.com/emicklei/go-restful/v3/stub.go index 71d25f095e8..6aab549f432 100644 --- a/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/vendor/github.com/emicklei/go-restful/v3/stub.go +++ b/ql/test/library-tests/semmle/go/frameworks/HTTP/Gorestful/vendor/github.com/emicklei/go-restful/v3/stub.go @@ -2,15 +2,22 @@ // This is a simple stub for github.com/emicklei/go-restful/v3, strictly for use in testing. // See the LICENSE file for information about the licensing of the original library. -// Source: github.com/emicklei/go-restful/v3 (exports: Request; functions: ) +// Source: github.com/emicklei/go-restful/v3 (exports: Request,Response; functions: ) -// Package gorestfulstub is a stub of github.com/emicklei/go-restful/v3, generated by depstubber. -package gorestfulstub +// Package gopkg is a stub of github.com/emicklei/go-restful/v3, generated by depstubber. +package gopkg import ( + bufio "bufio" + net "net" http "net/http" ) +type EntityReaderWriter interface { + Read(_ *Request, _ interface{}) error + Write(_ *Response, _ int, _ interface{}) error +} + type Request struct { Request *http.Request } @@ -52,3 +59,105 @@ func (_ *Request) ReadEntity(_ interface{}) error { } func (_ *Request) SetAttribute(_ string, _ interface{}) {} + +type Response struct { + ResponseWriter http.ResponseWriter +} + +func (_ Response) AddHeader(_ string, _ string) Response { + return Response{} +} + +func (_ Response) CloseNotify() <-chan bool { + return nil +} + +func (_ Response) ContentLength() int { + return 0 +} + +func (_ Response) Error() error { + return nil +} + +func (_ Response) Header() http.Header { + return nil +} + +func (_ Response) InternalServerError() Response { + return Response{} +} + +func (_ Response) StatusCode() int { + return 0 +} + +func (_ *Response) EntityWriter() (EntityReaderWriter, bool) { + return nil, false +} + +func (_ *Response) Flush() {} + +func (_ *Response) Hijack() (net.Conn, *bufio.ReadWriter, error) { + return nil, nil, nil +} + +func (_ *Response) PrettyPrint(_ bool) {} + +func (_ *Response) SetRequestAccepts(_ string) {} + +func (_ *Response) Write(_ []byte) (int, error) { + return 0, nil +} + +func (_ *Response) WriteAsJson(_ interface{}) error { + return nil +} + +func (_ *Response) WriteAsXml(_ interface{}) error { + return nil +} + +func (_ *Response) WriteEntity(_ interface{}) error { + return nil +} + +func (_ *Response) WriteError(_ int, _ error) error { + return nil +} + +func (_ *Response) WriteErrorString(_ int, _ string) error { + return nil +} + +func (_ *Response) WriteHeader(_ int) {} + +func (_ *Response) WriteHeaderAndEntity(_ int, _ interface{}) error { + return nil +} + +func (_ *Response) WriteHeaderAndJson(_ int, _ interface{}, _ string) error { + return nil +} + +func (_ *Response) WriteHeaderAndXml(_ int, _ interface{}) error { + return nil +} + +func (_ *Response) WriteJson(_ interface{}, _ string) error { + return nil +} + +func (_ *Response) WriteServiceError(_ int, _ ServiceError) error { + return nil +} + +type ServiceError struct { + Code int + Message string + Header http.Header +} + +func (_ ServiceError) Error() string { + return "" +} From c06531d9c05c70bdc61ce6be53170f14d762aac9 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 24 Aug 2020 17:06:01 +0100 Subject: [PATCH 02/18] Fix tests for `InsecureHostKeyCallback`. --- .../Security/CWE-322/InsecureHostKeyCallback.expected | 4 ++-- .../Security/CWE-322/InsecureHostKeyCallbackExample.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.expected b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.expected index 0e851d743b2..2953f3a9310 100644 --- a/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.expected +++ b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.expected @@ -6,7 +6,7 @@ edges | InsecureHostKeyCallbackExample.go:58:39:58:46 | definition of callback : HostKeyCallback | InsecureHostKeyCallbackExample.go:62:20:62:27 | callback | | InsecureHostKeyCallbackExample.go:58:39:58:46 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:62:20:62:27 | callback | | InsecureHostKeyCallbackExample.go:68:48:68:55 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:78:28:78:35 | callback | -| InsecureHostKeyCallbackExample.go:94:3:94:45 | ... := ...[0] : HostKeyCallback | InsecureHostKeyCallbackExample.go:95:28:95:35 | callback | +| InsecureHostKeyCallbackExample.go:94:3:94:43 | ... := ...[0] : HostKeyCallback | InsecureHostKeyCallbackExample.go:95:28:95:35 | callback | | InsecureHostKeyCallbackExample.go:102:22:105:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:107:35:107:50 | insecureCallback : signature type | | InsecureHostKeyCallbackExample.go:103:3:105:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:102:22:105:4 | type conversion : signature type | | InsecureHostKeyCallbackExample.go:107:35:107:50 | insecureCallback : signature type | InsecureHostKeyCallbackExample.go:58:39:58:46 | definition of callback : signature type | @@ -32,7 +32,7 @@ nodes | InsecureHostKeyCallbackExample.go:76:28:76:54 | call to InsecureIgnoreHostKey | semmle.label | call to InsecureIgnoreHostKey | | InsecureHostKeyCallbackExample.go:78:28:78:35 | callback | semmle.label | callback | | InsecureHostKeyCallbackExample.go:92:28:92:54 | call to InsecureIgnoreHostKey | semmle.label | call to InsecureIgnoreHostKey | -| InsecureHostKeyCallbackExample.go:94:3:94:45 | ... := ...[0] : HostKeyCallback | semmle.label | ... := ...[0] : HostKeyCallback | +| InsecureHostKeyCallbackExample.go:94:3:94:43 | ... := ...[0] : HostKeyCallback | semmle.label | ... := ...[0] : HostKeyCallback | | InsecureHostKeyCallbackExample.go:95:28:95:35 | callback | semmle.label | callback | | InsecureHostKeyCallbackExample.go:102:22:105:4 | type conversion : signature type | semmle.label | type conversion : signature type | | InsecureHostKeyCallbackExample.go:103:3:105:3 | function literal : signature type | semmle.label | function literal : signature type | diff --git a/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallbackExample.go b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallbackExample.go index 0c7f9466488..d13bda30a5e 100644 --- a/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallbackExample.go +++ b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallbackExample.go @@ -91,7 +91,7 @@ func potentialInsecureSSHClientConfigUsingKnownHosts(x bool) { if x { config.HostKeyCallback = ssh.InsecureIgnoreHostKey() // OK } else { - callback, err := knownhosts.New("somefile") + callback, _ := knownhosts.New("somefile") config.HostKeyCallback = callback } } From bdcb1f233cfceb6d61fc45caf74a7fd5f60c2f55 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 24 Aug 2020 08:43:11 +0100 Subject: [PATCH 03/18] Prevent misoptimisation in `StringOps`. --- ql/src/semmle/go/StringOps.qll | 45 ++++++++++++++----- .../go/dataflow/internal/DataFlowUtil.qll | 7 +++ 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/ql/src/semmle/go/StringOps.qll b/ql/src/semmle/go/StringOps.qll index dfc8ceb3c9f..69008e02221 100644 --- a/ql/src/semmle/go/StringOps.qll +++ b/ql/src/semmle/go/StringOps.qll @@ -78,15 +78,25 @@ module StringOps { } /** - * An expression of form `strings.Index(A, B) === 0`. + * Holds if `eq` is of the form `nd == 0` or `nd != 0`. + */ + pragma[noinline] + private predicate comparesToZero(DataFlow::EqualityTestNode eq, DataFlow::Node nd) { + exists(DataFlow::Node zero | + eq.hasOperands(globalValueNumber(nd).getANode(), zero) and + zero.getIntValue() = 0 + ) + } + + /** + * An expression of form `strings.Index(A, B) == 0`. */ private class HasPrefix_IndexOfEquals extends Range, DataFlow::EqualityTestNode { DataFlow::CallNode indexOf; HasPrefix_IndexOfEquals() { - indexOf.getTarget().hasQualifiedName("strings", "Index") and - getAnOperand() = globalValueNumber(indexOf).getANode() and - getAnOperand().getIntValue() = 0 + comparesToZero(this, indexOf) and + indexOf.getTarget().hasQualifiedName("strings", "Index") } override DataFlow::Node getBaseString() { result = indexOf.getArgument(0) } @@ -97,19 +107,30 @@ module StringOps { } /** - * A comparison of form `x[0] === 'k'` for some rune literal `k`. + * Holds if `eq` is of the form `str[0] == rhs` or `str[0] != rhs`. + */ + pragma[noinline] + private predicate comparesFirstCharacter( + DataFlow::EqualityTestNode eq, DataFlow::Node str, DataFlow::Node rhs + ) { + exists(DataFlow::ElementReadNode read | + eq.hasOperands(globalValueNumber(read).getANode(), rhs) and + str = read.getBase() and + str.getType().getUnderlyingType() instanceof StringType and + read.getIndex().getIntValue() = 0 + ) + } + + /** + * A comparison of form `x[0] == 'k'` for some rune literal `k`. */ private class HasPrefix_FirstCharacter extends Range, DataFlow::EqualityTestNode { - DataFlow::ElementReadNode read; + DataFlow::Node base; DataFlow::Node runeLiteral; - HasPrefix_FirstCharacter() { - read.getBase().getType().getUnderlyingType() instanceof StringType and - read.getIndex().getIntValue() = 0 and - eq(_, globalValueNumber(read).getANode(), runeLiteral) - } + HasPrefix_FirstCharacter() { comparesFirstCharacter(this, base, runeLiteral) } - override DataFlow::Node getBaseString() { result = read.getBase() } + override DataFlow::Node getBaseString() { result = base } override DataFlow::Node getSubstring() { result = runeLiteral } diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index ed6958c73d3..4d4479482de 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -669,6 +669,13 @@ class BinaryOperationNode extends Node { /** Gets the operator of this operation. */ string getOperator() { result = op } + + /** Holds if `x` and `y` are the operands of this operation, in either order. */ + predicate hasOperands(Node x, Node y) { + x = getAnOperand() and + y = getAnOperand() and + x != y + } } /** From 4c82ad60647d85fe7399cc3add92cfe455021791 Mon Sep 17 00:00:00 2001 From: Max Schaefer <54907921+max-schaefer@users.noreply.github.com> Date: Tue, 25 Aug 2020 07:36:14 +0100 Subject: [PATCH 04/18] Apply suggestions from code review Co-authored-by: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> --- ql/src/semmle/go/StringOps.qll | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ql/src/semmle/go/StringOps.qll b/ql/src/semmle/go/StringOps.qll index 69008e02221..f844d945072 100644 --- a/ql/src/semmle/go/StringOps.qll +++ b/ql/src/semmle/go/StringOps.qll @@ -67,7 +67,7 @@ module StringOps { } /** - * An expression of form `strings.HasPrefix(A, B)`. + * An expression of the form `strings.HasPrefix(A, B)`. */ private class StringsHasPrefix extends Range, DataFlow::CallNode { StringsHasPrefix() { getTarget().hasQualifiedName("strings", "HasPrefix") } @@ -89,7 +89,7 @@ module StringOps { } /** - * An expression of form `strings.Index(A, B) == 0`. + * An expression of the form `strings.Index(A, B) == 0`. */ private class HasPrefix_IndexOfEquals extends Range, DataFlow::EqualityTestNode { DataFlow::CallNode indexOf; @@ -122,7 +122,7 @@ module StringOps { } /** - * A comparison of form `x[0] == 'k'` for some rune literal `k`. + * A comparison of the form `x[0] == 'k'` for some rune literal `k`. */ private class HasPrefix_FirstCharacter extends Range, DataFlow::EqualityTestNode { DataFlow::Node base; @@ -138,7 +138,7 @@ module StringOps { } /** - * A comparison of form `x[:len(y)] === y`. + * A comparison of the form `x[:len(y)] == y`. */ private class HasPrefix_Substring extends Range, DataFlow::EqualityTestNode { DataFlow::SliceNode slice; From 944b69066e547db99afc4ec62369aac1a6646320 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 26 Aug 2020 07:20:24 +0100 Subject: [PATCH 05/18] Add change note for github/codeql-go#125 --- change-notes/2020-04-30-syscall-functions.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change-notes/2020-04-30-syscall-functions.md diff --git a/change-notes/2020-04-30-syscall-functions.md b/change-notes/2020-04-30-syscall-functions.md new file mode 100644 index 00000000000..caa2cf56654 --- /dev/null +++ b/change-notes/2020-04-30-syscall-functions.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The query "Command built from user-controlled sources" has been improved to recognize methods from the `syscall` library, which may lead to more alerts. From d4a377b7cc593864e99da81badb0a0a150cffc56 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 26 Aug 2020 07:21:05 +0100 Subject: [PATCH 06/18] Add change note for https://github.com/github/codeql-go/pull/107 The model for websocket was included in another change note --- change-notes/2020-05-20-request-forgery-sanitizers.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change-notes/2020-05-20-request-forgery-sanitizers.md diff --git a/change-notes/2020-05-20-request-forgery-sanitizers.md b/change-notes/2020-05-20-request-forgery-sanitizers.md new file mode 100644 index 00000000000..cb1fdcb5acf --- /dev/null +++ b/change-notes/2020-05-20-request-forgery-sanitizers.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The query "Uncontrolled data used in network request" is now more precise, which may reduce the number of false positives. From 210208b0030e540edab8b3d344f19be82434b0fe Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 26 Aug 2020 07:46:56 +0100 Subject: [PATCH 07/18] Add change note for https://github.com/github/codeql-go/pull/226 --- change-notes/2020-06-19-switch-block-without-test.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change-notes/2020-06-19-switch-block-without-test.md diff --git a/change-notes/2020-06-19-switch-block-without-test.md b/change-notes/2020-06-19-switch-block-without-test.md new file mode 100644 index 00000000000..17f20f6840f --- /dev/null +++ b/change-notes/2020-06-19-switch-block-without-test.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* A bug has been fixed that could cause the incorrect analysis of control flow around switch statements. From ad6c94e8f9f65b3af41d1a6141ba2cee21b793c6 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 26 Aug 2020 07:58:19 +0100 Subject: [PATCH 08/18] Add change note for https://github.com/github/codeql-go/pull/251 --- change-notes/2020-06-26-taint-model-tar-zip.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 change-notes/2020-06-26-taint-model-tar-zip.md diff --git a/change-notes/2020-06-26-taint-model-tar-zip.md b/change-notes/2020-06-26-taint-model-tar-zip.md new file mode 100644 index 00000000000..10cdaa41b46 --- /dev/null +++ b/change-notes/2020-06-26-taint-model-tar-zip.md @@ -0,0 +1,3 @@ +lgtm,codescanning +* Modeling of the `archive/tar` and `archive/zip` packages has been added, which may lead to more + results from the security queries. From 7fd5e7e9780fb8a18fe3b1b9a613a8ee75a19469 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 26 Aug 2020 10:54:18 +0100 Subject: [PATCH 09/18] Add change note for https://github.com/github/codeql-go/pull/277 --- change-notes/2020-07-06-repo-with-file-url-origin.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change-notes/2020-07-06-repo-with-file-url-origin.md diff --git a/change-notes/2020-07-06-repo-with-file-url-origin.md b/change-notes/2020-07-06-repo-with-file-url-origin.md new file mode 100644 index 00000000000..f84d4c995a2 --- /dev/null +++ b/change-notes/2020-07-06-repo-with-file-url-origin.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* A bug has been fixed that caused the autobuilder to not work on repositories with a `file://` URL as `origin`. From 28c69743a4eee6ce51aa604cbb1df2cf3140f747 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Mon, 24 Aug 2020 00:42:47 -0700 Subject: [PATCH 10/18] Add workaround for go 1.14 explicit vendoring requirement This only applies for module files for which no Go version has been specified; Go will assume these should be parsed with the latest Go version, which will cause them to fail if the vendor directory has been generated with an old version of Go, as the vendor/modules.txt will not meet the new requirements for consistency. --- .../cli/go-autobuilder/go-autobuilder.go | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/extractor/cli/go-autobuilder/go-autobuilder.go b/extractor/cli/go-autobuilder/go-autobuilder.go index 4df3d2c77e0..185e7a088c9 100644 --- a/extractor/cli/go-autobuilder/go-autobuilder.go +++ b/extractor/cli/go-autobuilder/go-autobuilder.go @@ -141,6 +141,12 @@ const ( Glide ) +// addVersionToMod add a go version directive, e.g. `go 1.14` to a `go.mod` file. +func addVersionToMod(goMod []byte, version string) bool { + cmd := exec.Command("go", "mod", "edit", "-go="+version) + return run(cmd) +} + func main() { if len(os.Args) > 1 { usage() @@ -184,6 +190,34 @@ func main() { // if a vendor/modules.txt file exists, we assume that there are vendored Go dependencies, and // skip the dependency installation step and run the extractor with `-mod=vendor` hasVendor := util.FileExists("vendor/modules.txt") + if hasVendor { + // fix go vendor issues with go versions >= 1.14 when no go version is specified in the go.mod + // if this is the case, and dependencies were vendored with an old go version (and therefore + // do not contain a '## explicit' annotation, the go command will fail and refuse to do any + // work + // + // we work around this by adding an explicit go version of 1.13, which is the last version + // where this is not an issue + if depMode == GoGetWithModules { + goMod, err := ioutil.ReadFile("go.mod") + if err != nil { + log.Println("Failed to read go.mod to check for missing Go version") + } else if versionRe := regexp.MustCompile(`(?m)^go[ \t\r]+[0-9]+\.[0-9]+$`); !versionRe.Match(goMod) { + // if the go.mod does not contain a version line + modulesTxt, err := ioutil.ReadFile("vendor/modules.txt") + if err != nil { + log.Println("Failed to read vendor/modules.txt to check for mismatched Go version") + } else if explicitRe := regexp.MustCompile("(?m)^## explicit$"); !explicitRe.Match(modulesTxt) { + // and the modules.txt does not contain an explicit annotation + log.Println("Adding a version directive to the go.mod file as the modules.txt does not have explicit annotations") + if !addVersionToMod(goMod, "1.13") { + log.Println("Failed to add a version to the go.mod file to fix explicitly required package bug; not using vendored dependencies") + hasVendor = false + } + } + } + } + } // if `LGTM_INDEX_NEED_GOPATH` is set, it overrides the value for `needGopath` inferred above if needGopathOverride := os.Getenv("LGTM_INDEX_NEED_GOPATH"); needGopathOverride != "" { From 852ae9397b6f3d17bffe026c778e179e3ef91fb9 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 25 Aug 2020 00:57:03 -0700 Subject: [PATCH 11/18] autobuilder: Test for vendor inconsistency --- .../cli/go-autobuilder/go-autobuilder.go | 83 +++++++++++++++---- 1 file changed, 67 insertions(+), 16 deletions(-) diff --git a/extractor/cli/go-autobuilder/go-autobuilder.go b/extractor/cli/go-autobuilder/go-autobuilder.go index 185e7a088c9..b8e0b919196 100644 --- a/extractor/cli/go-autobuilder/go-autobuilder.go +++ b/extractor/cli/go-autobuilder/go-autobuilder.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "golang.org/x/mod/semver" "io/ioutil" "log" "net/url" @@ -44,12 +45,17 @@ variable is 32. fmt.Fprintf(os.Stderr, "Usage:\n\n %s\n", os.Args[0]) } +var goVersion = "" + func getEnvGoVersion() string { - gover, err := exec.Command("go", "version").CombinedOutput() - if err != nil { - log.Fatalf("Unable to run the go command, is it installed?\nError: %s", err.Error()) + if goVersion == "" { + gover, err := exec.Command("go", "version").CombinedOutput() + if err != nil { + log.Fatalf("Unable to run the go command, is it installed?\nError: %s", err.Error()) + } + goVersion = strings.Fields(string(gover))[2] } - return strings.Fields(string(gover))[2] + return goVersion } func run(cmd *exec.Cmd) bool { @@ -141,6 +147,40 @@ const ( Glide ) +// ModMode corresponds to the possible values of the -mod flag for the Go compiler +type ModMode int + +const ( + ModUnset ModMode = iota + ModReadonly + ModMod + ModVendor +) + +func (m ModMode) String() string { + switch m { + case ModUnset: + return "" + case ModReadonly: + return "-mod=readonly" + case ModMod: + return "-mod=mod" + case ModVendor: + return "-mod=vendor" + } + return "" +} + +// modModIfSupported returns `ModMod` if that flag is supported, or `ModUnset` if it is not, in +// which case the behavior should be identical to `ModMod`. +func modModIfSupported() ModMode { + if semver.Compare(getEnvGoVersion(), "1.14") < 0 { + return ModUnset + } else { + return ModMod + } +} + // addVersionToMod add a go version directive, e.g. `go 1.14` to a `go.mod` file. func addVersionToMod(goMod []byte, version string) bool { cmd := exec.Command("go", "mod", "edit", "-go="+version) @@ -174,6 +214,7 @@ func main() { // determine how to install dependencies and whether a GOPATH needs to be set up before // extraction depMode := GoGetNoModules + modMode := ModUnset needGopath := true if util.FileExists("go.mod") { depMode = GoGetWithModules @@ -189,8 +230,10 @@ func main() { // if a vendor/modules.txt file exists, we assume that there are vendored Go dependencies, and // skip the dependency installation step and run the extractor with `-mod=vendor` - hasVendor := util.FileExists("vendor/modules.txt") - if hasVendor { + if util.FileExists("vendor/modules.txt") { + modMode = ModVendor + } + if modMode == ModVendor { // fix go vendor issues with go versions >= 1.14 when no go version is specified in the go.mod // if this is the case, and dependencies were vendored with an old go version (and therefore // do not contain a '## explicit' annotation, the go command will fail and refuse to do any @@ -212,7 +255,7 @@ func main() { log.Println("Adding a version directive to the go.mod file as the modules.txt does not have explicit annotations") if !addVersionToMod(goMod, "1.13") { log.Println("Failed to add a version to the go.mod file to fix explicitly required package bug; not using vendored dependencies") - hasVendor = false + modMode = modModIfSupported() } } } @@ -336,7 +379,7 @@ func main() { tryBuild("build.sh", "./build.sh") if !buildSucceeded { - if hasVendor { + if modMode == ModVendor { log.Printf("Skipping dependency installation because a Go vendor directory was found.") } else { // automatically determine command to install dependencies @@ -422,6 +465,20 @@ func main() { run(install) } + if modMode == ModVendor { + // test if running `go` with -mod=vendor works, and if it doesn't, try to fallback to -mod=mod + // or not set if the go version < 1.14. + vendorCheckCmd := exec.Command("go", "list", "-mod=vendor", "./...") + outp, err := vendorCheckCmd.CombinedOutput() + if err != nil { + badVendorRe := regexp.MustCompile(`(?m)^go: inconsistent vendoring in .*:$`) + if badVendorRe.Match(outp) { + modMode = modModIfSupported() + log.Println("The vendor directory is not consistent with the go.mod; not using vendored dependencies.") + } + } + } + // extract mypath, err := os.Executable() if err != nil { @@ -438,14 +495,8 @@ func main() { } var cmd *exec.Cmd - // check for `vendor/modules.txt` and not just `vendor` in order to distinguish non-go vendor dirs - if depMode == GoGetWithModules && hasVendor { - log.Printf("Running extractor command '%s -mod=vendor ./...' from directory '%s'.\n", extractor, cwd) - cmd = exec.Command(extractor, "-mod=vendor", "./...") - } else { - log.Printf("Running extractor command '%s ./...' from directory '%s'.\n", extractor, cwd) - cmd = exec.Command(extractor, "./...") - } + log.Printf("Running extractor command '%s %s ./...' from directory '%s'.\n", extractor, modMode, cwd) + cmd = exec.Command(extractor, modMode.String(), "./...") cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr err = cmd.Run() From 70d425d317c45eac562472eeb6bdd98d123658c3 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 25 Aug 2020 03:34:34 -0700 Subject: [PATCH 12/18] autobuilder: move vendor check before dependency installation check This means dependency installation is still attempted when a vendor directory is inconsistent. --- .../cli/go-autobuilder/go-autobuilder.go | 42 +++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/extractor/cli/go-autobuilder/go-autobuilder.go b/extractor/cli/go-autobuilder/go-autobuilder.go index b8e0b919196..75503aaeeec 100644 --- a/extractor/cli/go-autobuilder/go-autobuilder.go +++ b/extractor/cli/go-autobuilder/go-autobuilder.go @@ -187,6 +187,18 @@ func addVersionToMod(goMod []byte, version string) bool { return run(cmd) } +// checkVendor tests to see whether a vendor directory is inconsistent according to the go frontend +func checkVendor() bool { + vendorCheckCmd := exec.Command("go", "list", "-mod=vendor", "./...") + outp, err := vendorCheckCmd.CombinedOutput() + if err != nil { + badVendorRe := regexp.MustCompile(`(?m)^go: inconsistent vendoring in .*:$`) + return !badVendorRe.Match(outp) + } + + return true +} + func main() { if len(os.Args) > 1 { usage() @@ -379,6 +391,15 @@ func main() { tryBuild("build.sh", "./build.sh") if !buildSucceeded { + if modMode == ModVendor { + // test if running `go` with -mod=vendor works, and if it doesn't, try to fallback to -mod=mod + // or not set if the go version < 1.14. + if !checkVendor() { + modMode = modModIfSupported() + log.Println("The vendor directory is not consistent with the go.mod; not using vendored dependencies.") + } + } + if modMode == ModVendor { log.Printf("Skipping dependency installation because a Go vendor directory was found.") } else { @@ -459,26 +480,21 @@ func main() { os.Chmod(script.Name(), 0700) install = exec.Command(script.Name()) log.Println("Installing dependencies using custom build command.") - } - if install != nil { - run(install) - } - - if modMode == ModVendor { - // test if running `go` with -mod=vendor works, and if it doesn't, try to fallback to -mod=mod - // or not set if the go version < 1.14. - vendorCheckCmd := exec.Command("go", "list", "-mod=vendor", "./...") - outp, err := vendorCheckCmd.CombinedOutput() - if err != nil { - badVendorRe := regexp.MustCompile(`(?m)^go: inconsistent vendoring in .*:$`) - if badVendorRe.Match(outp) { + if modMode == ModVendor { + // test if running `go` with -mod=vendor works, and if it doesn't, try to fallback to -mod=mod + // or not set if the go version < 1.14. + if !checkVendor() { modMode = modModIfSupported() log.Println("The vendor directory is not consistent with the go.mod; not using vendored dependencies.") } } } + if install != nil { + run(install) + } + // extract mypath, err := os.Executable() if err != nil { From 8f6b25e0acf85baea153276ff2bb55aa6a11b2dd Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 25 Aug 2020 03:51:13 -0700 Subject: [PATCH 13/18] autobuilder: Use -mod=mod for vendor directories wihtout modules.txt --- extractor/cli/go-autobuilder/go-autobuilder.go | 3 +++ extractor/util/util.go | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/extractor/cli/go-autobuilder/go-autobuilder.go b/extractor/cli/go-autobuilder/go-autobuilder.go index 75503aaeeec..99f9af145c9 100644 --- a/extractor/cli/go-autobuilder/go-autobuilder.go +++ b/extractor/cli/go-autobuilder/go-autobuilder.go @@ -244,7 +244,10 @@ func main() { // skip the dependency installation step and run the extractor with `-mod=vendor` if util.FileExists("vendor/modules.txt") { modMode = ModVendor + } else if util.DirExists("vendor") { + modMode = modModIfSupported() } + if modMode == ModVendor { // fix go vendor issues with go versions >= 1.14 when no go version is specified in the go.mod // if this is the case, and dependencies were vendored with an old go version (and therefore diff --git a/extractor/util/util.go b/extractor/util/util.go index c8eeb33f484..a592224672c 100644 --- a/extractor/util/util.go +++ b/extractor/util/util.go @@ -91,3 +91,12 @@ func FileExists(filename string) bool { } return err == nil && !info.IsDir() } + +// DirExists tests whether `filename` exists and is a directory. +func DirExists(filename string) bool { + info, err := os.Stat(filename) + if err != nil && !os.IsNotExist(err) { + log.Printf("Unable to stat %s: %s\n", filename, err.Error()) + } + return err == nil && info.IsDir() +} From 859b427881722bbd046db3aaca328e47773b59ea Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 26 Aug 2020 11:53:50 +0100 Subject: [PATCH 14/18] Check if the vendor/ directory is usable, even after a successful build --- extractor/cli/go-autobuilder/go-autobuilder.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/extractor/cli/go-autobuilder/go-autobuilder.go b/extractor/cli/go-autobuilder/go-autobuilder.go index 99f9af145c9..6fee70f8137 100644 --- a/extractor/cli/go-autobuilder/go-autobuilder.go +++ b/extractor/cli/go-autobuilder/go-autobuilder.go @@ -393,16 +393,17 @@ func main() { tryBuild("build", "./build") || tryBuild("build.sh", "./build.sh") - if !buildSucceeded { - if modMode == ModVendor { - // test if running `go` with -mod=vendor works, and if it doesn't, try to fallback to -mod=mod - // or not set if the go version < 1.14. - if !checkVendor() { - modMode = modModIfSupported() - log.Println("The vendor directory is not consistent with the go.mod; not using vendored dependencies.") - } + if modMode == ModVendor { + // test if running `go` with -mod=vendor works, and if it doesn't, try to fallback to -mod=mod + // or not set if the go version < 1.14. Note we check this post-build in case the build brings + // the vendor directory up to date. + if !checkVendor() { + modMode = modModIfSupported() + log.Println("The vendor directory is not consistent with the go.mod; not using vendored dependencies.") } + } + if !buildSucceeded { if modMode == ModVendor { log.Printf("Skipping dependency installation because a Go vendor directory was found.") } else { From 9ad2d6c1197a087ed07aa08ad4858439855c8196 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 26 Aug 2020 12:02:54 +0100 Subject: [PATCH 15/18] Factor default and custom install paths These now follow the same route: * Run a default or custom build script * If needed, check if vendor/ is usable * If it isn't, or if their build failed, install dependencies using go get etc This commit shouldn't cause any behavioural change. --- .../cli/go-autobuilder/go-autobuilder.go | 131 +++++++++--------- 1 file changed, 62 insertions(+), 69 deletions(-) diff --git a/extractor/cli/go-autobuilder/go-autobuilder.go b/extractor/cli/go-autobuilder/go-autobuilder.go index 6fee70f8137..fc0f98ab2a6 100644 --- a/extractor/cli/go-autobuilder/go-autobuilder.go +++ b/extractor/cli/go-autobuilder/go-autobuilder.go @@ -383,7 +383,7 @@ func main() { // check whether an explicit dependency installation command was provided inst := util.Getenv("CODEQL_EXTRACTOR_GO_BUILD_COMMAND", "LGTM_INDEX_BUILD_COMMAND") - var install *exec.Cmd + shouldInstallDependencies := false if inst == "" { // if there is a build file, run the corresponding build tool buildSucceeded := tryBuild("Makefile", "make") || @@ -393,65 +393,9 @@ func main() { tryBuild("build", "./build") || tryBuild("build.sh", "./build.sh") - if modMode == ModVendor { - // test if running `go` with -mod=vendor works, and if it doesn't, try to fallback to -mod=mod - // or not set if the go version < 1.14. Note we check this post-build in case the build brings - // the vendor directory up to date. - if !checkVendor() { - modMode = modModIfSupported() - log.Println("The vendor directory is not consistent with the go.mod; not using vendored dependencies.") - } - } - if !buildSucceeded { - if modMode == ModVendor { - log.Printf("Skipping dependency installation because a Go vendor directory was found.") - } else { - // automatically determine command to install dependencies - if depMode == Dep { - // set up the dep cache if SEMMLE_CACHE is set - cacheDir := os.Getenv("SEMMLE_CACHE") - if cacheDir != "" { - depCacheDir := filepath.Join(cacheDir, "go", "dep") - log.Printf("Attempting to create dep cache dir %s\n", depCacheDir) - err := os.MkdirAll(depCacheDir, 0755) - if err != nil { - log.Printf("Failed to create dep cache directory: %s\n", err.Error()) - } else { - log.Printf("Setting dep cache directory to %s\n", depCacheDir) - err = os.Setenv("DEPCACHEDIR", depCacheDir) - if err != nil { - log.Println("Failed to set dep cache directory") - } else { - err = os.Setenv("DEPCACHEAGE", "720h") // 30 days - if err != nil { - log.Println("Failed to set dep cache age") - } - } - } - } - - if util.FileExists("Gopkg.lock") { - // if Gopkg.lock exists, don't update it and only vendor dependencies - install = exec.Command("dep", "ensure", "-v", "-vendor-only") - } else { - install = exec.Command("dep", "ensure", "-v") - } - log.Println("Installing dependencies using `dep ensure`.") - } else if depMode == Glide { - install = exec.Command("glide", "install") - log.Println("Installing dependencies using `glide install`") - } else { - if depMode == GoGetWithModules { - // enable go modules if used - os.Setenv("GO111MODULE", "on") - } - - // get dependencies - install = exec.Command("go", "get", "-v", "./...") - log.Println("Installing dependencies using `go get -v ./...`.") - } - } + // Build failed; we'll try to install dependencies ourselves + shouldInstallDependencies = true } } else { // write custom build commands into a script, then run it @@ -482,21 +426,70 @@ func main() { log.Fatalf("Unable to close temporary script holding custom build commands: %s\n", err.Error()) } os.Chmod(script.Name(), 0700) - install = exec.Command(script.Name()) log.Println("Installing dependencies using custom build command.") + run(exec.Command(script.Name())) + } - if modMode == ModVendor { - // test if running `go` with -mod=vendor works, and if it doesn't, try to fallback to -mod=mod - // or not set if the go version < 1.14. - if !checkVendor() { - modMode = modModIfSupported() - log.Println("The vendor directory is not consistent with the go.mod; not using vendored dependencies.") - } + if modMode == ModVendor { + // test if running `go` with -mod=vendor works, and if it doesn't, try to fallback to -mod=mod + // or not set if the go version < 1.14. Note we check this post-build in case the build brings + // the vendor directory up to date. + if !checkVendor() { + modMode = modModIfSupported() + log.Println("The vendor directory is not consistent with the go.mod; not using vendored dependencies.") } } - if install != nil { - run(install) + if shouldInstallDependencies { + if modMode == ModVendor { + log.Printf("Skipping dependency installation because a Go vendor directory was found.") + } else { + // automatically determine command to install dependencies + var install *exec.Cmd + if depMode == Dep { + // set up the dep cache if SEMMLE_CACHE is set + cacheDir := os.Getenv("SEMMLE_CACHE") + if cacheDir != "" { + depCacheDir := filepath.Join(cacheDir, "go", "dep") + log.Printf("Attempting to create dep cache dir %s\n", depCacheDir) + err := os.MkdirAll(depCacheDir, 0755) + if err != nil { + log.Printf("Failed to create dep cache directory: %s\n", err.Error()) + } else { + log.Printf("Setting dep cache directory to %s\n", depCacheDir) + err = os.Setenv("DEPCACHEDIR", depCacheDir) + if err != nil { + log.Println("Failed to set dep cache directory") + } else { + err = os.Setenv("DEPCACHEAGE", "720h") // 30 days + if err != nil { + log.Println("Failed to set dep cache age") + } + } + } + } + + if util.FileExists("Gopkg.lock") { + // if Gopkg.lock exists, don't update it and only vendor dependencies + install = exec.Command("dep", "ensure", "-v", "-vendor-only") + } else { + install = exec.Command("dep", "ensure", "-v") + } + log.Println("Installing dependencies using `dep ensure`.") + } else if depMode == Glide { + install = exec.Command("glide", "install") + log.Println("Installing dependencies using `glide install`") + } else { + if depMode == GoGetWithModules { + // enable go modules if used + os.Setenv("GO111MODULE", "on") + } + // get dependencies + install = exec.Command("go", "get", "-v", "./...") + log.Println("Installing dependencies using `go get -v ./...`.") + } + run(install) + } } // extract From b13b54f7d72fe631b937b2e93ed01553992706cd Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 26 Aug 2020 12:21:03 +0100 Subject: [PATCH 16/18] Don't try to use -mod=... when go.mod doesn't exist Also don't pass a blank argument to `go` when using an old version. --- extractor/cli/go-autobuilder/go-autobuilder.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/extractor/cli/go-autobuilder/go-autobuilder.go b/extractor/cli/go-autobuilder/go-autobuilder.go index fc0f98ab2a6..d0908d94120 100644 --- a/extractor/cli/go-autobuilder/go-autobuilder.go +++ b/extractor/cli/go-autobuilder/go-autobuilder.go @@ -508,8 +508,13 @@ func main() { } var cmd *exec.Cmd - log.Printf("Running extractor command '%s %s ./...' from directory '%s'.\n", extractor, modMode, cwd) - cmd = exec.Command(extractor, modMode.String(), "./...") + if depMode == GoGetWithModules && modMode.String() != "" { + log.Printf("Running extractor command '%s %s ./...' from directory '%s'.\n", extractor, modMode, cwd) + cmd = exec.Command(extractor, modMode.String(), "./...") + } else { + log.Printf("Running extractor command '%s ./...' from directory '%s'.\n", extractor, cwd) + cmd = exec.Command(extractor, "./...") + } cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr err = cmd.Run() From c6dbb9fcb2b6d31dae82958523a6afb761953af7 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Thu, 27 Aug 2020 10:46:36 +0100 Subject: [PATCH 17/18] Tidy up -mod argument stringification --- .../cli/go-autobuilder/go-autobuilder.go | 48 +++++++++---------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/extractor/cli/go-autobuilder/go-autobuilder.go b/extractor/cli/go-autobuilder/go-autobuilder.go index d0908d94120..d4269b443f1 100644 --- a/extractor/cli/go-autobuilder/go-autobuilder.go +++ b/extractor/cli/go-autobuilder/go-autobuilder.go @@ -157,28 +157,24 @@ const ( ModVendor ) -func (m ModMode) String() string { +func (m ModMode) argsForGoVersion(version string) []string { switch m { case ModUnset: - return "" + return []string{} case ModReadonly: - return "-mod=readonly" + return []string{"-mod=readonly"} case ModMod: - return "-mod=mod" + if semver.Compare(getEnvGoVersion(), "1.14") < 0 { + log.Printf("%s < %s", getEnvGoVersion(), "1.14") + return []string{} // -mod=mod is the default behaviour for go <= 1.13, and is not accepted as an argument + } else { + log.Printf("%s >= %s", getEnvGoVersion(), "1.14") + return []string{"-mod=mod"} + } case ModVendor: - return "-mod=vendor" - } - return "" -} - -// modModIfSupported returns `ModMod` if that flag is supported, or `ModUnset` if it is not, in -// which case the behavior should be identical to `ModMod`. -func modModIfSupported() ModMode { - if semver.Compare(getEnvGoVersion(), "1.14") < 0 { - return ModUnset - } else { - return ModMod + return []string{"-mod=vendor"} } + return nil } // addVersionToMod add a go version directive, e.g. `go 1.14` to a `go.mod` file. @@ -245,7 +241,7 @@ func main() { if util.FileExists("vendor/modules.txt") { modMode = ModVendor } else if util.DirExists("vendor") { - modMode = modModIfSupported() + modMode = ModMod } if modMode == ModVendor { @@ -270,7 +266,7 @@ func main() { log.Println("Adding a version directive to the go.mod file as the modules.txt does not have explicit annotations") if !addVersionToMod(goMod, "1.13") { log.Println("Failed to add a version to the go.mod file to fix explicitly required package bug; not using vendored dependencies") - modMode = modModIfSupported() + modMode = ModMod } } } @@ -435,7 +431,7 @@ func main() { // or not set if the go version < 1.14. Note we check this post-build in case the build brings // the vendor directory up to date. if !checkVendor() { - modMode = modModIfSupported() + modMode = ModMod log.Println("The vendor directory is not consistent with the go.mod; not using vendored dependencies.") } } @@ -507,14 +503,14 @@ func main() { log.Fatalf("Unable to determine current directory: %s\n", err.Error()) } - var cmd *exec.Cmd - if depMode == GoGetWithModules && modMode.String() != "" { - log.Printf("Running extractor command '%s %s ./...' from directory '%s'.\n", extractor, modMode, cwd) - cmd = exec.Command(extractor, modMode.String(), "./...") - } else { - log.Printf("Running extractor command '%s ./...' from directory '%s'.\n", extractor, cwd) - cmd = exec.Command(extractor, "./...") + extractorArgs := []string{} + if depMode == GoGetWithModules { + extractorArgs = append(extractorArgs, modMode.argsForGoVersion(getEnvGoVersion())...) } + extractorArgs = append(extractorArgs, "./...") + + log.Printf("Running extractor command '%s %v' from directory '%s'.\n", extractor, extractorArgs, cwd) + cmd := exec.Command(extractor, extractorArgs...) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr err = cmd.Run() From 4d084372b519921f481b0ab28c8f930bbca16e90 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Thu, 27 Aug 2020 11:02:23 +0100 Subject: [PATCH 18/18] Fix autobuilder Go version comparison The semver package requires versions of the form v1.2.3, and unhelpfully evaluates any malformed versions as equal. --- .../cli/go-autobuilder/go-autobuilder.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/extractor/cli/go-autobuilder/go-autobuilder.go b/extractor/cli/go-autobuilder/go-autobuilder.go index d4269b443f1..63d8aa153d8 100644 --- a/extractor/cli/go-autobuilder/go-autobuilder.go +++ b/extractor/cli/go-autobuilder/go-autobuilder.go @@ -47,6 +47,7 @@ variable is 32. var goVersion = "" +// Returns the current Go version as returned by 'go version', e.g. go1.14.4 func getEnvGoVersion() string { if goVersion == "" { gover, err := exec.Command("go", "version").CombinedOutput() @@ -58,6 +59,15 @@ func getEnvGoVersion() string { return goVersion } +// Returns the current Go version in semver format, e.g. v1.14.4 +func getEnvGoSemVer() string { + goVersion := getEnvGoVersion() + if !strings.HasPrefix(goVersion, "go") { + log.Fatalf("Expected 'go version' output of the form 'go1.2.3'; got '%s'", goVersion) + } + return "v" + goVersion[2:] +} + func run(cmd *exec.Cmd) bool { cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr @@ -164,11 +174,12 @@ func (m ModMode) argsForGoVersion(version string) []string { case ModReadonly: return []string{"-mod=readonly"} case ModMod: - if semver.Compare(getEnvGoVersion(), "1.14") < 0 { - log.Printf("%s < %s", getEnvGoVersion(), "1.14") + if !semver.IsValid(version) { + log.Fatalf("Invalid Go semver: '%s'", version) + } + if semver.Compare(version, "v1.14") < 0 { return []string{} // -mod=mod is the default behaviour for go <= 1.13, and is not accepted as an argument } else { - log.Printf("%s >= %s", getEnvGoVersion(), "1.14") return []string{"-mod=mod"} } case ModVendor: @@ -505,7 +516,7 @@ func main() { extractorArgs := []string{} if depMode == GoGetWithModules { - extractorArgs = append(extractorArgs, modMode.argsForGoVersion(getEnvGoVersion())...) + extractorArgs = append(extractorArgs, modMode.argsForGoVersion(getEnvGoSemVer())...) } extractorArgs = append(extractorArgs, "./...")