lib/upgrade: Auto upgrade signature should cover version & arch (fixes #3044)

New signature is the HMAC of archive name (which includes the release
version and architecture) plus the contents of the binary. This is
expected in a new file "release.sig" which may be present in a
subdirectory. The new release tools put this in [.]metadata/release.sig.

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3043

3 files changed, 40 insertions, 21 deletions

diff --git a/lib/upgrade/upgrade_common.go b/lib/upgrade/upgrade_common.go
index b224d1b6c5..fbc2639722 100644
--- a/lib/upgrade/upgrade_common.go
+++ b/lib/upgrade/upgrade_common.go
@@ -10,6 +10,7 @@ package upgrade
 import (
 	"errors"
 	"fmt"
+	"path"
 	"runtime"
 	"strconv"
 	"strings"
@@ -63,12 +64,12 @@ func To(rel Release) error {
 func ToURL(url string) error {
 	select {
 	case <-upgradeUnlocked:
-		path, err := osext.Executable()
+		binary, err := osext.Executable()
 		if err != nil {
 			upgradeUnlocked <- true
 			return err
 		}
-		err = upgradeToURL(path, url)
+		err = upgradeToURL(path.Base(url), binary, url)
 		// If we've failed to upgrade, unlock so that another attempt could be made
 		if err != nil {
 			upgradeUnlocked <- true
@@ -219,6 +220,10 @@ func versionParts(v string) ([]int, []interface{}) {
 }
 
 func releaseName(tag string) string {
+	// We must ensure that the release asset matches the expected naming
+	// standard, containing both the architecture/OS and the tag name we
+	// expect. This protects against malformed release data potentially
+	// tricking us into doing a downgrade.
 	switch runtime.GOOS {
 	case "darwin":
 		return fmt.Sprintf("syncthing-macosx-%s-%s.", runtime.GOARCH, tag)
diff --git a/lib/upgrade/upgrade_supported.go b/lib/upgrade/upgrade_supported.go
index b149e69944..a67821eb48 100644
--- a/lib/upgrade/upgrade_supported.go
+++ b/lib/upgrade/upgrade_supported.go
@@ -120,7 +120,7 @@ func upgradeTo(binary string, rel Release) error {
 		l.Debugln("considering release", assetName)
 
 		if strings.HasPrefix(assetName, expectedRelease) {
-			return upgradeToURL(binary, asset.URL)
+			return upgradeToURL(assetName, binary, asset.URL)
 		}
 	}
 
@@ -128,8 +128,8 @@ func upgradeTo(binary string, rel Release) error {
 }
 
 // Upgrade to the given release, saving the previous binary with a ".old" extension.
-func upgradeToURL(binary string, url string) error {
-	fname, err := readRelease(filepath.Dir(binary), url)
+func upgradeToURL(archiveName, binary string, url string) error {
+	fname, err := readRelease(archiveName, filepath.Dir(binary), url)
 	if err != nil {
 		return err
 	}
@@ -147,7 +147,7 @@ func upgradeToURL(binary string, url string) error {
 	return nil
 }
 
-func readRelease(dir, url string) (string, error) {
+func readRelease(archiveName, dir, url string) (string, error) {
 	l.Debugf("loading %q", url)
 
 	req, err := http.NewRequest("GET", url, nil)
@@ -164,13 +164,13 @@ func readRelease(dir, url string) (string, error) {
 
 	switch runtime.GOOS {
 	case "windows":
-		return readZip(dir, resp.Body)
+		return readZip(archiveName, dir, resp.Body)
 	default:
-		return readTarGz(dir, resp.Body)
+		return readTarGz(archiveName, dir, resp.Body)
 	}
 }
 
-func readTarGz(dir string, r io.Reader) (string, error) {
+func readTarGz(archiveName, dir string, r io.Reader) (string, error) {
 	gr, err := gzip.NewReader(r)
 	if err != nil {
 		return "", err
@@ -202,14 +202,14 @@ func readTarGz(dir string, r io.Reader) (string, error) {
 		}
 	}
 
-	if err := verifyUpgrade(tempName, sig); err != nil {
+	if err := verifyUpgrade(archiveName, tempName, sig); err != nil {
 		return "", err
 	}
 
 	return tempName, nil
 }
 
-func readZip(dir string, r io.Reader) (string, error) {
+func readZip(archiveName, dir string, r io.Reader) (string, error) {
 	body, err := ioutil.ReadAll(r)
 	if err != nil {
 		return "", err
@@ -241,7 +241,7 @@ func readZip(dir string, r io.Reader) (string, error) {
 		}
 	}
 
-	if err := verifyUpgrade(tempName, sig); err != nil {
+	if err := verifyUpgrade(archiveName, tempName, sig); err != nil {
 		return "", err
 	}
 
@@ -254,21 +254,22 @@ func archiveFileVisitor(dir string, tempFile *string, signature *[]byte, archive
 	var err error
 	filename := path.Base(archivePath)
 	archiveDir := path.Dir(archivePath)
-	archiveDirs := strings.Split(archiveDir, "/")
-	if len(archiveDirs) > 1 {
-		//don't consider files in subfolders
-		return nil
-	}
 	l.Debugf("considering file %s", archivePath)
 	switch filename {
 	case "syncthing", "syncthing.exe":
+		archiveDirs := strings.Split(archiveDir, "/")
+		if len(archiveDirs) > 1 {
+			// Don't consider "syncthing" files found too deeply, as they may be
+			// other things.
+			return nil
+		}
 		l.Debugf("found upgrade binary %s", archivePath)
 		*tempFile, err = writeBinary(dir, filedata)
 		if err != nil {
 			return err
 		}
 
-	case "syncthing.sig", "syncthing.exe.sig":
+	case "release.sig":
 		l.Debugf("found signature %s", archivePath)
 		*signature, err = ioutil.ReadAll(filedata)
 		if err != nil {
@@ -279,7 +280,7 @@ func archiveFileVisitor(dir string, tempFile *string, signature *[]byte, archive
 	return nil
 }
 
-func verifyUpgrade(tempName string, sig []byte) error {
+func verifyUpgrade(archiveName, tempName string, sig []byte) error {
 	if tempName == "" {
 		return fmt.Errorf("no upgrade found")
 	}
@@ -293,7 +294,20 @@ func verifyUpgrade(tempName string, sig []byte) error {
 	if err != nil {
 		return err
 	}
-	err = signature.Verify(SigningKey, sig, fd)
+
+	// Create a new reader that will serve reads from, in order:
+	//
+	// - the archive name ("syncthing-linux-amd64-v0.13.0-beta.4.tar.gz")
+	//   followed by a newline
+	//
+	// - the temp file contents
+	//
+	// We then verify the release signature against the contents of this
+	// multireader. This ensures that it is not only a bonafide syncthing
+	// binary, but it it also of exactly the platform and version we expect.
+
+	mr := io.MultiReader(bytes.NewBufferString(archiveName+"\n"), fd)
+	err = signature.Verify(SigningKey, sig, mr)
 	fd.Close()
 
 	if err != nil {
diff --git a/lib/upgrade/upgrade_unsupp.go b/lib/upgrade/upgrade_unsupp.go
index 9ea9384d25..99c0924e96 100644
--- a/lib/upgrade/upgrade_unsupp.go
+++ b/lib/upgrade/upgrade_unsupp.go
@@ -14,7 +14,7 @@ func upgradeTo(binary string, rel Release) error {
 	return ErrUpgradeUnsupported
 }
 
-func upgradeToURL(binary, url string) error {
+func upgradeToURL(archiveName, binary, url string) error {
 	return ErrUpgradeUnsupported
 }