tls: Prevent directory traversal via On-Demand TLS (fixes #2092)

caddytls/client.go

@@ -257,6 +257,13 @@ func (c *ACMEClient) Obtain(name string) error {
 			return errors.New(errMsg)
 		}
 
+		// double-check that we actually got a certificate; check a couple fields
+		// TODO: This is a temporary workaround for what I think is a bug in the acmev2 package (March 2018)
+		// but it might not hurt to keep this extra check in place
+		if certificate.Domain == "" || certificate.Certificate == nil {
+			return errors.New("returned certificate was empty; probably an unchecked error obtaining it")
+		}
+
 		// Success - immediately save the certificate resource
 		err = saveCertResource(c.storage, certificate)
 		if err != nil {
@@ -311,8 +318,15 @@ func (c *ACMEClient) Renew(name string) error {
 		acmeMu.Unlock()
 		namesObtaining.Remove([]string{name})
 		if err == nil {
-			success = true
+			// double-check that we actually got a certificate; check a couple fields
-			break
+			// TODO: This is a temporary workaround for what I think is a bug in the acmev2 package (March 2018)
+			// but it might not hurt to keep this extra check in place
+			if newCertMeta.Domain == "" || newCertMeta.Certificate == nil {
+				err = errors.New("returned certificate was empty; probably an unchecked error renewing it")
+			} else {
+				success = true
+				break
+			}
 		}
 
 		// wait a little bit and try again

caddytls/filestorage.go

@@ -58,30 +58,25 @@ func (s *FileStorage) sites() string {
 
 // site returns the path to the folder containing assets for domain.
 func (s *FileStorage) site(domain string) string {
-	// Windows doesn't allow * in filenames, sigh...
+	domain = fileSafe(domain)
-	domain = strings.Replace(domain, "*", "wildcard_", -1)
-	domain = strings.ToLower(domain)
 	return filepath.Join(s.sites(), domain)
 }
 
 // siteCertFile returns the path to the certificate file for domain.
 func (s *FileStorage) siteCertFile(domain string) string {
-	domain = strings.Replace(domain, "*", "wildcard_", -1)
+	domain = fileSafe(domain)
-	domain = strings.ToLower(domain)
 	return filepath.Join(s.site(domain), domain+".crt")
 }
 
 // siteKeyFile returns the path to domain's private key file.
 func (s *FileStorage) siteKeyFile(domain string) string {
-	domain = strings.Replace(domain, "*", "wildcard_", -1)
+	domain = fileSafe(domain)
-	domain = strings.ToLower(domain)
 	return filepath.Join(s.site(domain), domain+".key")
 }
 
 // siteMetaFile returns the path to the domain's asset metadata file.
 func (s *FileStorage) siteMetaFile(domain string) string {
-	domain = strings.Replace(domain, "*", "wildcard_", -1)
+	domain = fileSafe(domain)
-	domain = strings.ToLower(domain)
 	return filepath.Join(s.site(domain), domain+".json")
 }
 
@@ -95,7 +90,7 @@ func (s *FileStorage) user(email string) string {
 	if email == "" {
 		email = emptyEmail
 	}
-	email = strings.ToLower(email)
+	email = fileSafe(email)
 	return filepath.Join(s.users(), email)
 }
 
@@ -122,6 +117,7 @@ func (s *FileStorage) userRegFile(email string) string {
 	if fileName == "" {
 		fileName = "registration"
 	}
+	fileName = fileSafe(fileName)
 	return filepath.Join(s.user(email), fileName+".json")
 }
 
@@ -136,6 +132,7 @@ func (s *FileStorage) userKeyFile(email string) string {
 	if fileName == "" {
 		fileName = "private"
 	}
+	fileName = fileSafe(fileName)
 	return filepath.Join(s.user(email), fileName+".key")
 }
 
@@ -279,3 +276,29 @@ func (s *FileStorage) MostRecentUserEmail() string {
 	}
 	return ""
 }
+
+// fileSafe standardizes and sanitizes str for use in a file path.
+func fileSafe(str string) string {
+	str = strings.ToLower(str)
+	str = strings.TrimSpace(str)
+	repl := strings.NewReplacer("..", "",
+		"/", "",
+		"\\", "",
+		// TODO: Consider also replacing "@" with "_at_" (but migrate existing accounts...)
+		"+", "_plus_",
+		"%", "",
+		"$", "",
+		"`", "",
+		"~", "",
+		":", "",
+		";", "",
+		"=", "",
+		"!", "",
+		"#", "",
+		"&", "",
+		"|", "",
+		"\"", "",
+		"'", "",
+		"*", "wildcard_")
+	return repl.Replace(str)
+}

caddytls/filestorage_test.go

@@ -14,7 +14,54 @@
 
 package caddytls
 
+import "testing"
+
 // *********************************** NOTE ********************************
 // Due to circular package dependencies with the storagetest sub package and
-// the fact that we want to use that harness to test file storage, the tests
+// the fact that we want to use that harness to test file storage, most of
-// for file storage are done in the storagetest package.
+// the tests for file storage are done in the storagetest package.
+
+func TestPathBuilders(t *testing.T) {
+	fs := FileStorage{Path: "/test"}
+
+	for i, testcase := range []struct {
+		in, folder, certFile, keyFile, metaFile string
+	}{
+		{
+			in:       "example.com",
+			folder:   "/test/sites/example.com",
+			certFile: "/test/sites/example.com/example.com.crt",
+			keyFile:  "/test/sites/example.com/example.com.key",
+			metaFile: "/test/sites/example.com/example.com.json",
+		},
+		{
+			in:       "*.example.com",
+			folder:   "/test/sites/wildcard_.example.com",
+			certFile: "/test/sites/wildcard_.example.com/wildcard_.example.com.crt",
+			keyFile:  "/test/sites/wildcard_.example.com/wildcard_.example.com.key",
+			metaFile: "/test/sites/wildcard_.example.com/wildcard_.example.com.json",
+		},
+		{
+			// prevent directory traversal! very important, esp. with on-demand TLS
+			// see issue #2092
+			in:       "a/../../../foo",
+			folder:   "/test/sites/afoo",
+			certFile: "/test/sites/afoo/afoo.crt",
+			keyFile:  "/test/sites/afoo/afoo.key",
+			metaFile: "/test/sites/afoo/afoo.json",
+		},
+	} {
+		if actual := fs.site(testcase.in); actual != testcase.folder {
+			t.Errorf("Test %d: site folder: Expected '%s' but got '%s'", i, testcase.folder, actual)
+		}
+		if actual := fs.siteCertFile(testcase.in); actual != testcase.certFile {
+			t.Errorf("Test %d: site cert file: Expected '%s' but got '%s'", i, testcase.certFile, actual)
+		}
+		if actual := fs.siteKeyFile(testcase.in); actual != testcase.keyFile {
+			t.Errorf("Test %d: site key file: Expected '%s' but got '%s'", i, testcase.keyFile, actual)
+		}
+		if actual := fs.siteMetaFile(testcase.in); actual != testcase.metaFile {
+			t.Errorf("Test %d: site meta file: Expected '%s' but got '%s'", i, testcase.metaFile, actual)
+		}
+	}
+}

caddytls/filestoragesync.go

@@ -91,7 +91,20 @@ func (s *fileStorageLock) Unlock(name string) error {
 	if !ok {
 		return fmt.Errorf("FileStorage: no lock to release for %s", name)
 	}
+	// remove lock file
 	os.Remove(fw.filename)
+
+	// if parent folder is now empty, remove it too to keep it tidy
+	lockParentFolder := s.storage.site(name)
+	dir, err := os.Open(lockParentFolder)
+	if err == nil {
+		items, _ := dir.Readdirnames(3) // OK to ignore error here
+		if len(items) == 0 {
+			os.Remove(lockParentFolder)
+		}
+		dir.Close()
+	}
+
 	fw.wg.Done()
 	delete(fileStorageNameLocks, s.caURL+name)
 	return nil

caddytls/storage.go

@@ -58,7 +58,8 @@ type Locker interface {
 	// successfully obtained the lock (no Waiter value was returned)
 	// should call this method, and it should be called only after
 	// the obtain/renew and store are finished, even if there was
-	// an error (or a timeout).
+	// an error (or a timeout). Unlock should also clean up any
+	// unused resources allocated during TryLock.
 	Unlock(name string) error
 }