Multiple fixes for go-getter v2 (#361)

* Fix command injection in go-getter when passing params to hg clone

The fix for this is to add -- to the arguments of each hg command,
before any user-input. This indicates the end of optional arguments,
only positional arguments are allowed.

* Remove upwards path traversal in subdirectories, filenames

* Prevent arbitrary file read, path traversal via subdirectory extraction
Not opt-in or opt-out, just never allowed. Upwards path traversal is not a subdirectory.

*Prevent arbitrary file write via `filename`
Not opt-in or opt-out, just never allowed. Upwards path traversal is not a filename in a subdirectory.

* Add Timeout option to HgGetter and GitGetter enforced with os/exec.CommandContext

* Add DisableSymlinks option to getter request

The fix for this is a new client request option, DisableSymlinks. When set to true, symlinks are disabled.
This prevents the client, likely in combination with the GitGetter, from following a symlink when the subdirectory
selection from the checked out repo is a symlink.

* Add custom symlink copy error

* Add DisableSymlinks as client option

Setting DisableSymlinks per request works but must be set on all request
made by a client. Adding it as a top-level client config option allows
for setting DisableSymlinks for all client.Get requests.

* Update get_http to address various get concerns

* Add XTerraformGetLimit and XTerraformGetDisabled
* Add Multiple new options to limit resource consumption:
  DoNotCheckHeadFirst, HeadFirstTimeout, ReadTimeout, MaxBytes
* Add getter client to context for reuse
* Add setters/getters for storing configured getter.Client in a context
* Update HttpGetter to use ClientFromContext when available; otherwise
  use a limited client for supporting X-Terraform-Get request
* Refactor HttpGetter function to make it clear when a configured
  getter.Client is required
* Add security section to README

* Port changes from hashicorp/eastebry/timeout-for-getters

Adding timeout to s3Getter

* Port changes from from hashicorp/add-missing-timeouts

Add missing timeouts to `S3Getter` and `GCSGetter`

* Remove windows test for FileGetter

* Change to next-get image

Co-authored-by: Kent 'picat' Gruber <[email protected]>
Co-authored-by: Sylvia Moss <[email protected]>

Author: 3 people

.circleci/config.yml

@@ -49,11 +49,11 @@ commands:
 jobs:
   linux-tests:
     docker:
-      - image: circleci/golang:<< parameters.go-version >>
+      - image: cimg/go:<< parameters.go-version >>
     parameters:
       go-version:
         type: string
-    environment: 
+    environment:
       <<: *ENVIRONMENT
     parallelism: 4
     steps:
@@ -104,7 +104,7 @@ jobs:
           path: *TEST_RESULTS_PATH
 
   windows-tests:
-    executor: 
+    executor:
       name: win/default
       shell: bash --login -eo pipefail
     environment:
@@ -115,12 +115,12 @@ jobs:
         type: string
       gotestsum-version:
         type: string
-    steps: 
+    steps:
       - run: git config --global core.autocrlf false
       - checkout
       - attach_workspace:
           at: .
-      - run: 
+      - run:
           name: Setup (remove pre-installed go)
           command: |
             rm -rf "c:\Go"
@@ -131,16 +131,16 @@ jobs:
             - win-golang-<< parameters.go-version >>-cache-v1
             - win-gomod-cache-{{ checksum "go.mod" }}-v1
 
-      - run: 
+      - run:
           name: Install go version << parameters.go-version >>
-          command: | 
+          command: |
             if [ ! -d "c:\go" ]; then
               echo "Cache not found, installing new version of go"
               curl --fail --location https://dl.google.com/go/go<< parameters.go-version >>.windows-amd64.zip --output go.zip
               unzip go.zip -d "/c"
             fi
 
-      - run: 
+      - run:
           command: go mod download
 
       - save_cache:
@@ -176,7 +176,7 @@ jobs:
 
   go-smb-test:
     docker:
-      - image: circleci/golang:<< parameters.go-version >>
+      - image: cimg/go:<< parameters.go-version >>
     parameters:
       go-version:
         type: string

README.md

@@ -21,8 +21,8 @@ URLs. For example: "github.com/hashicorp/go-getter" would turn into a
 Git URL. Or "./foo" would turn into a file URL. These are extensible.
 
 This library is used by [Terraform](https://terraform.io) for
-downloading modules and [Nomad](https://nomadproject.io) for downloading
-binaries.
+downloading modules, [Packer](https://packer.io) for downloading binaries, and
+[Nomad](https://nomadproject.io) for downloading binaries.
 
 ## Installation and Usage
 
@@ -47,6 +47,16 @@ $ go-getter github.com/foo/bar ./foo
 
 The command is useful for verifying URL structures.
 
+## Security
+Fetching resources from user-supplied URLs is an inherently dangerous operation and may
+leave your application vulnerable to [server side request forgery](https://owasp.org/www-community/attacks/Server_Side_Request_Forgery),
+[path traversal](https://owasp.org/www-community/attacks/Path_Traversal), [denial of service](https://owasp.org/www-community/attacks/Denial_of_Service)
+or other security flaws.
+
+go-getter contains mitigations for some of these security issues, but should still be used with
+caution in security-critical contexts. See the available [security options](#Security-Options) that
+can be configured to mitigate some of these risks.
+
 ## URL Format
 
 go-getter uses a single string URL as input to download from a variety of
@@ -83,7 +93,7 @@ is built-in by default:
     file URLs.
   * GitHub URLs, such as "github.com/mitchellh/vagrant" are automatically
     changed to Git protocol over HTTP.
-  * GitLab URLs, such as "gitlab.com/inkscape/inkscape" are automatically 
+  * GitLab URLs, such as "gitlab.com/inkscape/inkscape" are automatically
     changed to Git protocol over HTTP.
   * BitBucket URLs, such as "bitbucket.org/mitchellh/vagrant" are automatically
     changed to a Git or mercurial protocol using the BitBucket API.
@@ -178,7 +188,7 @@ checksum string. Examples:
 ```
 ./foo.txt?checksum=file:./foo.txt.sha256sum
 ```
- 
+
 When checksumming from a file - ex: with `checksum=file:url` - go-getter will
 get the file linked in the URL after `file:` using the same configuration. For
 example, in `file:http://releases.ubuntu.com/cosmic/MD5SUMS` go-getter will
@@ -279,7 +289,7 @@ None
     from a private key file on disk, you would run `base64 -w0 <file>`.
 
     **Note**: Git 2.3+ is required to use this feature.
-  
+
   * `depth` - The Git clone depth. The provided number specifies the last `n`
     revisions to clone from the repository.
 
@@ -374,35 +384,107 @@ files from a smb shared folder whenever the url is prefixed with `smb://`.
 
 ⚠️ The [`smbclient`](https://www.samba.org/samba/docs/current/man-html/smbclient.1.html) command is available only for Linux.
 This is the ONLY option for a Linux user and therefore the client must be installed.
-    
+
 The `smbclient` cli is not available for Windows and MacOS. The go-getter
 will try to get files using the file system, when this happens the getter uses the FileGetter implementation.
 
-When connecting to a smb server, the OS creates a local mount in a system specific volume folder, and go-getter will 
+When connecting to a smb server, the OS creates a local mount in a system specific volume folder, and go-getter will
 try to access the following folders when looking for local mounts.
 
 - MacOS: /Volumes/<shared_path>
 - Windows: \\\\\<host>\\\<shared_path>
 
-The following examples work for all the OSes:  
+The following examples work for all the OSes:
 - smb://host/shared/dir (downloads directory content)
-- smb://host/shared/dir/file (downloads file) 
+- smb://host/shared/dir/file (downloads file)
 
-The following examples work for Linux:  
+The following examples work for Linux:
 - smb://username:password@host/shared/dir (downloads directory content)
 - smb://username@host/shared/dir
 - smb://username:password@host/shared/dir/file (downloads file)
 - smb://username@host/shared/dir/file
 
 ⚠️ The above examples also work on the other OSes but the authentication is not used to access the file system.
 
-   
-        
+
+
 #### SMB Testing
 The test for `get_smb.go` requires a smb server running which can be started inside a docker container by
-running `make start-smb`. Once the container is up the shared folder can be accessed via `smb://<ip|name>/public/<dir|file>` or 
-`smb://user:password@<ip|name>/private/<dir|file>` by another container or machine in the same network. 
+running `make start-smb`. Once the container is up the shared folder can be accessed via `smb://<ip|name>/public/<dir|file>` or
+`smb://user:password@<ip|name>/private/<dir|file>` by another container or machine in the same network.
 
-To run the tests inside `get_smb_test.go` and `client_test.go`, prepare the environment with `make smbtests-prepare`. On prepare some 
+To run the tests inside `get_smb_test.go` and `client_test.go`, prepare the environment with `make smbtests-prepare`. On prepare some
 mock files and directories will be added to the shared folder and a go-getter container will start together with the samba server.
-Once the environment for testing is prepared, run `make smbtests` to run the tests. 
+Once the environment for testing is prepared, run `make smbtests` to run the tests.
+
+### Security Options
+
+**Disable Symlinks**
+
+In your getter client config, we recommend using the `DisableSymlinks` option,
+which prevents writing through or copying from symlinks (which may point outside the directory).
+
+```go
+client := getter.Client{
+    // This will prevent copying or writing files through symlinks
+    DisableSymlinks: true,
+}
+```
+
+**Disable or Limit `X-Terraform-Get`**
+
+Go-Getter supports arbitrary redirects via the `X-Terraform-Get` header. This functionality
+exists to support [Terraform use cases](https://www.terraform.io/language/modules/sources#http-urls),
+but is likely not needed in most applications.
+
+For code that uses the `HttpGetter`, add the following configuration options:
+
+```go
+var httpGetter = &getter.HttpGetter{
+    // Most clients should disable X-Terraform-Get
+    // See the note below
+    XTerraformGetDisabled: true,
+    // Your software probably doesn’t rely on X-Terraform-Get, but
+    // if it does, you should set the above field to false, plus
+    // set XTerraformGet Limit to prevent endless redirects
+    // XTerraformGetLimit: 10,
+}
+```
+
+**Enforce Timeouts**
+
+The `HttpGetter` supports timeouts and other resource-constraining configuration options. The `GitGetter` and `HgGetter`
+only support timeouts.
+
+Configuration for the `HttpGetter`:
+
+```go
+var httpGetter = &getter.HttpGetter{
+    // Disable pre-fetch HEAD requests
+    DoNotCheckHeadFirst: true,
+
+    // As an alternative to the above setting, you can
+    // set a reasonable timeout for HEAD requests
+    // HeadFirstTimeout: 10 * time.Second,
+    // Read timeout for HTTP operations
+    ReadTimeout: 30 * time.Second,
+    // Set the maximum number of bytes
+    // that can be read by the getter
+    MaxBytes: 500000000, // 500 MB
+}
+```
+
+For code that uses the `GitGetter` or `HgGetter`, set the `Timeout` option:
+```go
+var gitGetter = &getter.GitGetter{
+    // Set a reasonable timeout for git operations
+    Timeout: 5 * time.Minute,
+}
+```
+
+```go
+var hgGetter = &getter.HgGetter{
+    // Set a reasonable timeout for hg operations
+    Timeout: 5 * time.Minute,
+}
+```

client.go

@@ -2,6 +2,7 @@ package getter
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -14,6 +15,9 @@ import (
 	safetemp "github.com/hashicorp/go-safetemp"
 )
 
+// ErrSymlinkCopy means that a copy of a symlink was encountered on a request with DisableSymlinks enabled.
+var ErrSymlinkCopy = errors.New("copying of symlinks has been disabled")
+
 // Client is a client for downloading things.
 //
 // Top-level functions such as Get are shortcuts for interacting with a client.
@@ -27,6 +31,10 @@ type Client struct {
 	// Getters is the list of protocols supported by this client. If this
 	// is nil, then the default Getters variable will be used.
 	Getters []Getter
+
+	// Disable symlinks is used to prevent copying or writing files through symlinks for Get requests.
+	// When set to true any copying or writing through symlinks will result in a ErrSymlinkCopy error.
+	DisableSymlinks bool
 }
 
 // GetResult is the result of a Client.Get
@@ -41,15 +49,36 @@ func (c *Client) Get(ctx context.Context, req *Request) (*GetResult, error) {
 		return nil, err
 	}
 
+	// Pass along the configured Getter client in the context for usage with the X-Terraform-Get feature.
+	ctx = NewContextWithClient(ctx, c)
+
 	// Store this locally since there are cases we swap this
 	if req.GetMode == ModeInvalid {
 		req.GetMode = ModeAny
 	}
 
+	// Client setting takes precedence for all requests
+	if c.DisableSymlinks {
+		req.DisableSymlinks = true
+	}
+
 	// If there is a subdir component, then we download the root separately
 	// and then copy over the proper subdir.
 	req.Src, req.subDir = SourceDirSubdir(req.Src)
+
 	if req.subDir != "" {
+		// Check if the subdirectory is attempting to traverse upwards, outside of
+		// the cloned repository path.
+		req.subDir = filepath.Clean(req.subDir)
+		if containsDotDot(req.subDir) {
+			return nil, fmt.Errorf("subdirectory component contain path traversal out of the repository")
+		}
+
+		// Prevent absolute paths, remove a leading path separator from the subdirectory
+		if req.subDir[0] == os.PathSeparator {
+			req.subDir = req.subDir[1:]
+		}
+
 		td, tdcloser, err := safetemp.Dir("", "getter")
 		if err != nil {
 			return nil, err
@@ -123,7 +152,7 @@ func (c *Client) get(ctx context.Context, req *Request, g Getter) (*GetResult, *
 	// Determine if we have an archive type
 	archiveV := q.Get("archive")
 	if archiveV != "" {
-		// Delete the paramter since it is a magic parameter we don't
+		// Delete the parameter since it is a magic parameter we don't
 		// want to pass on to the Getter
 		q.Del("archive")
 		req.u.RawQuery = q.Encode()
@@ -199,6 +228,10 @@ func (c *Client) get(ctx context.Context, req *Request, g Getter) (*GetResult, *
 				filename = v
 			}
 
+			if containsDotDot(filename) {
+				return nil, &getError{true, fmt.Errorf("filename query parameter contain path traversal")}
+			}
+
 			req.Dst = filepath.Join(req.Dst, filename)
 		}
 	}
@@ -284,7 +317,7 @@ func (c *Client) get(ctx context.Context, req *Request, g Getter) (*GetResult, *
 			return nil, &getError{true, err}
 		}
 
-		err = copyDir(ctx, req.realDst, subDir, false, req.umask())
+		err = copyDir(ctx, req.realDst, subDir, false, req.DisableSymlinks, req.umask())
 		if err != nil {
 			return nil, &getError{false, err}
 		}

client_option.go

@@ -1,5 +1,26 @@
 package getter
 
+import (
+	"context"
+)
+
+type clientContextKey int
+
+const clientContextValue clientContextKey = 0
+
+func NewContextWithClient(ctx context.Context, client *Client) context.Context {
+	return context.WithValue(ctx, clientContextValue, client)
+}
+
+func ClientFromContext(ctx context.Context) *Client {
+	// ctx.Value returns nil if ctx has no value for the key;
+	client, ok := ctx.Value(clientContextValue).(*Client)
+	if !ok {
+		return nil
+	}
+	return client
+}
+
 // configure configures a client with options.
 func (c *Client) configure() error {
 	// Default decompressor values

cmd/go-getter/main.go

@@ -16,6 +16,7 @@ import (
 func main() {
 	modeRaw := flag.String("mode", "any", "get mode (any, file, dir)")
 	progress := flag.Bool("progress", false, "display terminal progress")
+	noSymlinks := flag.Bool("disable-symlinks", false, "prevent copying or writing files through symlinks")
 	flag.Parse()
 	args := flag.Args()
 	if len(args) < 2 {
@@ -54,12 +55,16 @@ func main() {
 	if *progress {
 		req.ProgressListener = defaultProgressBar
 	}
-
 	wg := sync.WaitGroup{}
 	wg.Add(1)
 
 	client := getter.DefaultClient
 
+	// Disable symlinks for all client requests
+	if *noSymlinks {
+		client.DisableSymlinks = true
+	}
+
 	getters := getter.Getters
 	getters = append(getters, new(gcs.Getter))
 	getters = append(getters, new(s3.Getter))