From 96256ac63e4e5659f8bf4b5921f75bb2455ad33f Mon Sep 17 00:00:00 2001
From: Dane Everitt <dane@daneeveritt.com>
Date: Sat, 9 Jan 2021 17:52:27 -0800
Subject: [PATCH] [security] fix vulnerability when handling remote file
 redirects

Also adds the ability for an admin to just completely disable this service if it is not needed on the node.
---
 CHANGELOG.md                    |  7 +++++++
 config/config.go                |  7 ++++++-
 router/downloader/downloader.go | 17 ++++++++++++++++-
 router/middleware.go            | 15 +++++++++++++++
 router/router.go                |  6 +++---
 5 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7c4b4fc..a74ef1a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,12 @@
 # Changelog
 
+## v1.2.3
+### Fixed
+* **[Security]** Fixes a remaining security vulnerability in the code handling remote file downloads for servers relating to redirect validation.
+
+### Added
+* Adds a configuration key at `api.disable_remote_download` that can be set to `true` to completely download the remote download system.
+
 ## v1.2.2
 ### Fixed
 * Reverts changes to logic handling blocking until a server process is done running when polling stats. This change exposed a bug in the underlying Docker system causing servers to enter a state in which Wings was unable to terminate the process and Docker commands would hang if executed against the container.
diff --git a/config/config.go b/config/config.go
index 06c1157..0af323f 100644
--- a/config/config.go
+++ b/config/config.go
@@ -88,11 +88,16 @@ type ApiConfiguration struct {
 
 	// SSL configuration for the daemon.
 	Ssl struct {
-		Enabled         bool   `default:"false"`
+		Enabled         bool   `json:"enabled" yaml:"enabled"`
 		CertificateFile string `json:"cert" yaml:"cert"`
 		KeyFile         string `json:"key" yaml:"key"`
 	}
 
+	// Determines if functionality for allowing remote download of files into server directories
+	// is enabled on this instance. If set to "true" remote downloads will not be possible for
+	// servers.
+	DisableRemoteDownload bool `json:"disable_remote_download" yaml:"disable_remote_download"`
+
 	// The maximum size for files uploaded through the Panel in bytes.
 	UploadLimit int `default:"100" json:"upload_limit" yaml:"upload_limit"`
 }
diff --git a/router/downloader/downloader.go b/router/downloader/downloader.go
index a89cca1..20dcba7 100644
--- a/router/downloader/downloader.go
+++ b/router/downloader/downloader.go
@@ -18,7 +18,22 @@ import (
 	"time"
 )
 
-var client = &http.Client{Timeout: time.Hour * 12}
+var client = &http.Client{
+	Timeout: time.Hour * 12,
+	// Disallow any redirect on a HTTP call. This is a security requirement: do not modify
+	// this logic without first ensuring that the new target location IS NOT within the current
+	// instance's local network.
+	//
+	// This specific error response just causes the client to not follow the redirect and
+	// returns the actual redirect response to the caller. Not perfect, but simple and most
+	// people won't be using URLs that redirect anyways hopefully?
+	//
+	// We'll re-evaluate this down the road if needed.
+	CheckRedirect: func(req *http.Request, via []*http.Request) error {
+		return http.ErrUseLastResponse
+	},
+}
+
 var instance = &Downloader{
 	// Tracks all of the active downloads.
 	downloadCache: make(map[string]*Download),
diff --git a/router/middleware.go b/router/middleware.go
index c3f32fd..7fd52bf 100644
--- a/router/middleware.go
+++ b/router/middleware.go
@@ -120,6 +120,21 @@ func (m *Middleware) ServerExists() gin.HandlerFunc {
 	}
 }
 
+// Checks if remote file downloading is enabled on this instance before allowing access
+// to the given endpoint.
+func (m *Middleware) CheckRemoteDownloadEnabled() gin.HandlerFunc {
+	disabled := config.Get().Api.DisableRemoteDownload
+	return func(c *gin.Context) {
+		if disabled {
+			c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{
+				"error": "This functionality is not currently enabled on this instance.",
+			})
+			return
+		}
+		c.Next()
+	}
+}
+
 // Returns the server instance from the gin context. If there is no server set in the
 // context (e.g. calling from a controller not protected by ServerExists) this function
 // will panic.
diff --git a/router/router.go b/router/router.go
index 85a97a7..0eb4075 100644
--- a/router/router.go
+++ b/router/router.go
@@ -88,9 +88,9 @@ func Configure() *gin.Engine {
 			files.POST("/decompress", postServerDecompressFiles)
 			files.POST("/chmod", postServerChmodFile)
 
-			files.GET("/pull", getServerPullingFiles)
-			files.POST("/pull", postServerPullRemoteFile)
-			files.DELETE("/pull/:download", deleteServerPullRemoteFile)
+			files.GET("/pull", m.CheckRemoteDownloadEnabled(), getServerPullingFiles)
+			files.POST("/pull", m.CheckRemoteDownloadEnabled(), postServerPullRemoteFile)
+			files.DELETE("/pull/:download", m.CheckRemoteDownloadEnabled(), deleteServerPullRemoteFile)
 		}
 
 		backup := server.Group("/backup")