From 73b61af58d31d88763c865ef85377eccf1b9bb22 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 28 Mar 2018 12:04:35 -0600 Subject: [PATCH] tls: Prevent directory traversal via On-Demand TLS (fixes #2092) --- caddytls/client.go | 18 +++++++++++-- caddytls/filestorage.go | 43 +++++++++++++++++++++++------- caddytls/filestorage_test.go | 51 ++++++++++++++++++++++++++++++++++-- caddytls/filestoragesync.go | 13 +++++++++ caddytls/storage.go | 3 ++- 5 files changed, 113 insertions(+), 15 deletions(-) diff --git a/caddytls/client.go b/caddytls/client.go index c7094b86..b67a2928 100644 --- a/caddytls/client.go +++ b/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 - break + // 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 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 diff --git a/caddytls/filestorage.go b/caddytls/filestorage.go index 87ab842b..a43c62b8 100644 --- a/caddytls/filestorage.go +++ b/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 = strings.Replace(domain, "*", "wildcard_", -1) - domain = strings.ToLower(domain) + domain = fileSafe(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 = strings.ToLower(domain) + domain = fileSafe(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 = strings.ToLower(domain) + domain = fileSafe(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 = strings.ToLower(domain) + domain = fileSafe(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) +} diff --git a/caddytls/filestorage_test.go b/caddytls/filestorage_test.go index 1831f7b9..0986506d 100644 --- a/caddytls/filestorage_test.go +++ b/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 -// for file storage are done in the storagetest package. +// the fact that we want to use that harness to test file storage, most of +// 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) + } + } +} diff --git a/caddytls/filestoragesync.go b/caddytls/filestoragesync.go index a8e7b929..251f8861 100644 --- a/caddytls/filestoragesync.go +++ b/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 diff --git a/caddytls/storage.go b/caddytls/storage.go index 05606ed9..c0bc5bc8 100644 --- a/caddytls/storage.go +++ b/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 }