From 37d786c82aa0cb4e99ff01afb179f55c1e62188c Mon Sep 17 00:00:00 2001
From: Nick Craig-Wood <nick@craig-wood.com>
Date: Wed, 18 Oct 2023 11:25:32 +0100
Subject: [PATCH] selfupdate: fix "invalid hashsum signature" error

This was caused by a change to the upstream library
ProtonMail/go-crypto checking the flags on the keys more strictly.

However the signing key for rclone is very old and does not have those
flags. Adding those flags using `gpg --edit-key` and then the
`change-usage` subcommand to remove, save, quite then re-add, save
quit the signing capabilities caused the key to work.

This also adds tests for the verification and adds the selfupdate
tests into the integration test harness as they had been disabled on
CI because they rely on external sources and are sometimes unreliable.

Fixes #7373
---
 cmd/selfupdate/selfupdate_test.go          |   1 +
 cmd/selfupdate/testdata/verify/SHA256SUMS  |  10 +++
 cmd/selfupdate/testdata/verify/archive.zip | Bin 0 -> 195 bytes
 cmd/selfupdate/verify.go                   |  67 ++++++++++++++-------
 cmd/selfupdate/verify_test.go              |  40 ++++++++++++
 fstest/test_all/config.yaml                |   2 +
 6 files changed, 98 insertions(+), 22 deletions(-)
 create mode 100644 cmd/selfupdate/testdata/verify/SHA256SUMS
 create mode 100644 cmd/selfupdate/testdata/verify/archive.zip
 create mode 100644 cmd/selfupdate/verify_test.go

diff --git a/cmd/selfupdate/selfupdate_test.go b/cmd/selfupdate/selfupdate_test.go
index 9c02f4584..92d1d91c7 100644
--- a/cmd/selfupdate/selfupdate_test.go
+++ b/cmd/selfupdate/selfupdate_test.go
@@ -14,6 +14,7 @@ import (
 	"time"
 
 	"github.com/rclone/rclone/fs"
+	_ "github.com/rclone/rclone/fstest" // needed to run under integration tests
 	"github.com/rclone/rclone/fstest/testy"
 	"github.com/stretchr/testify/assert"
 )
diff --git a/cmd/selfupdate/testdata/verify/SHA256SUMS b/cmd/selfupdate/testdata/verify/SHA256SUMS
new file mode 100644
index 000000000..8a2df1e6a
--- /dev/null
+++ b/cmd/selfupdate/testdata/verify/SHA256SUMS
@@ -0,0 +1,10 @@
+-----BEGIN PGP SIGNED MESSAGE-----
+Hash: SHA1
+
+b20b47f579a2c790ca752fb5d8e5651fade7d5867cbac0a4f71e805fc5c468d0  archive.zip
+-----BEGIN PGP SIGNATURE-----
+
+iF0EARECAB0WIQT79zfs6firGGBL0qyTk14C/ztU+gUCZS+oVQAKCRCTk14C/ztU
++lNsAJ9XRiODlM4fIW9yqiltO3N+lLeucwCfRzD3cXk6BCB5wdz7pTgnItk9N74=
+=1GTr
+-----END PGP SIGNATURE-----
diff --git a/cmd/selfupdate/testdata/verify/archive.zip b/cmd/selfupdate/testdata/verify/archive.zip
new file mode 100644
index 0000000000000000000000000000000000000000..c2ecc6aecf550d7901026c729481315863256e31
GIT binary patch
literal 195
zcmWIWW@h1HU}9ik5DpCrZ?9JLmISgvn2SM%A;{Iy#n)A@q@pA=gp+|;Y=wR*2$xoH
zGcdBeU}j(d6CoLy#R@=_s8EtxT%w>?m06&WmYI{v72wUtB*%=)cnP2-3=E7wyrmJu
bLbi+*Vi}r60p6@^AeD?j7yzVWK^z7Ev1TKY

literal 0
HcmV?d00001

diff --git a/cmd/selfupdate/verify.go b/cmd/selfupdate/verify.go
index 503bbd7f6..7c3d76df6 100644
--- a/cmd/selfupdate/verify.go
+++ b/cmd/selfupdate/verify.go
@@ -26,24 +26,37 @@ QbogRGodbKhqY4v+cMNkKiemBuTQiWPkpKjifwNsD1fNjNKfDP3pJ64Yz7a4fuzV
 X1YwBACpKVuEen34lmcX6ziY4jq8rKibKBs4JjQCRO24kYoHDULVe+RS9krQWY5b
 e0foDhru4dsKccefK099G+WEzKVCKxupstWkTT/iJwajR8mIqd4AhD0wO9W3MCfV
 Ov8ykMDZ7qBWk1DHc87Ep3W1o8t8wq74ifV+HjhhWg8QAylXg7QlTmljayBDcmFp
-Zy1Xb29kIDxuaWNrQGNyYWlnLXdvb2QuY29tPohxBBMRCAAxBQsHCgMEAxUDAgMW
-AgECF4AWIQT79zfs6firGGBL0qyTk14C/ztU+gUCXjg2UgIZAQAKCRCTk14C/ztU
-+lmmAJ4jH5FyULzStjisuTvHLTVz6G44eQCfaR5QGZFPseenE5ic2WeQcBcmtoG5
-Ag0EO7LdgRAIAI6QdFBg3/xa1gFKPYy1ihV9eSdGqwWZGJvokWsfCvHy5180tj/v
-UNOLAJrdqglMSvevNTXe8bT65D6423AAsLhch9wq/aNqrHolTYABzxRigjcS1//T
-yln5naGUzlVQXDVfrDk3Md/NrkdOFj7r/YyMF0+iWwpFz2qAjL95i5wfVZ1kWGrT
-2AmivE1wD1sWT/Ja3FDI0NRkU0Nbz/a0TKe4ml8iLVtZXpTRbxxCCPdkHXXgSyu1
-eZ4NrF/wTJuvwGn12TJ1EF95aVkHxAUw0+KmLGdcyBG+IKuHamrsjWIAXGXV///K
-AxPgUthccQ03HMjltFsrdmen5Q034YM3eOsAAwUH/jAKiIAA8LpZmZPnt9GZ4+Ol
-Zp22VAfyfDOFl4Ol+cWjkLAgjAFsm5gnOKcRSE/9XPxnQqkhw7+ZygYuUMgTDJ99
-/5IM1UQL3ooS+oFrDaE99S8bLeOe17skcdXcA/K83VqD9m93rQRnbtD+75zqKkZn
-9WNFyKCXg5P6PFPdNYRtlQKOcwFR9mHRLUmapQSAM8Y2pCgALZ7GViKQca8/TT1T
-gZk9fJMZYGez+IlOPxTJxjn80+vywk4/wdIWSiQj+8u5RzT9sjmm77wbMVNGRqYd
-W/EemW9Zz9vi0CIvJGgbPMqcuxw8e/5lnuQ6Mi3uDR0P2RNIAhFrdZpVSME8xQaI
-RgQYEQIABgUCO7LdgQAKCRCTk14C/ztU+mLBAKC2cdFy7eLaQAvyzcE2VK6HVIjn
-JACguA00bxLQuJ4+RCJrLFZP8ZlN2sc=
-=TtR5
------END PGP PUBLIC KEY BLOCK-----`
+Zy1Xb29kIDxuaWNrQGNyYWlnLXdvb2QuY29tPoh0BBMRCAA0BQsHCgMEAxUDAgMW
+AgECF4ACGQEWIQT79zfs6firGGBL0qyTk14C/ztU+gUCZS/mXAIbIwAKCRCTk14C
+/ztU+tX+AJ9CUAnPvT4w5yRAPRfDiwWIPUqBOgCgiTelkzvUxvLWnYmpowwzKmsx
+qaSJAjMEEAEIAB0WIQTjs1jchY+zB/SBcLnLDb68XzLIHQUCZPRnNAAKCRDLDb68
+XzLIHZSAD/oCk9Z0xJfbpriphTBxFy7bWyPKF1lM1GZZaLKkktGfunf1i0Q7rhwp
+Nu+u1launlOTp6ZoY36Ce2Qa1eSxWAQdjVajw9kOHXCAewrTREOMY/mb7RVGjajo
+0Egl8T9iD3JRyaxu2iVtbpZYuqehtGG28CaCzmtqE+EJcx1cGqAGSuuaDWRYlVX8
+KDip44GQB5Lut30vwSIoZG1CPCR6VE82u4cl3mYZUfcJkCHsiLzoeadVzb+fOd+2
+ybzBn8Y77ifGgM+dSFSHe03mFfcHPdp0QImF9HQR7XI0UMZmEJsw7c2vDrRa+kRY
+2A4/amGn4Tahuazq8g2yqgGm3yAj49qGNarAau849lDr7R49j73ESnNVBGJ9ShzU
+4Ls+S1A5gohZVu2s1fkE3mbAmoTfU4JCrpRydOuL9xRJk5gbL44sKeuGODNshyTP
+JzG9DmRHpLsBn59v8mg5tqSfBIGqcqBxxnYHJnkK801MkaLW2m7wDmtz6P3TW86g
+GukzfIN3/OufLjnpN3Nx376JwWDDIyif7sn6/q+ZMwGz9uLKZkAeM5c3Dh4ygpgl
+iSLoV2bZzDz0iLxKWW7QOVVdWHmlEqbTldpQ7gUEPG7mxpzVo0xd6nHncSq0M91x
+29It4B3fATx/iJB2eardMzSsbzHiwTg0eswhYYGpSKZLgp4RShnVAbkCDQQ7st2B
+EAgAjpB0UGDf/FrWAUo9jLWKFX15J0arBZkYm+iRax8K8fLnXzS2P+9Q04sAmt2q
+CUxK9681Nd7xtPrkPrjbcACwuFyH3Cr9o2qseiVNgAHPFGKCNxLX/9PKWfmdoZTO
+VVBcNV+sOTcx382uR04WPuv9jIwXT6JbCkXPaoCMv3mLnB9VnWRYatPYCaK8TXAP
+WxZP8lrcUMjQ1GRTQ1vP9rRMp7iaXyItW1lelNFvHEII92QddeBLK7V5ng2sX/BM
+m6/AafXZMnUQX3lpWQfEBTDT4qYsZ1zIEb4gq4dqauyNYgBcZdX//8oDE+BS2Fxx
+DTccyOW0Wyt2Z6flDTfhgzd46wADBQf+MAqIgADwulmZk+e30Znj46VmnbZUB/J8
+M4WXg6X5xaOQsCCMAWybmCc4pxFIT/1c/GdCqSHDv5nKBi5QyBMMn33/kgzVRAve
+ihL6gWsNoT31Lxst457XuyRx1dwD8rzdWoP2b3etBGdu0P7vnOoqRmf1Y0XIoJeD
+k/o8U901hG2VAo5zAVH2YdEtSZqlBIAzxjakKAAtnsZWIpBxrz9NPVOBmT18kxlg
+Z7P4iU4/FMnGOfzT6/LCTj/B0hZKJCP7y7lHNP2yOabvvBsxU0ZGph1b8R6Zb1nP
+2+LQIi8kaBs8ypy7HDx7/mWe5DoyLe4NHQ/ZE0gCEWt1mlVIwTzFBohGBBgRAgAG
+BQI7st2BAAoJEJOTXgL/O1T6YsEAoLZx0XLt4tpAC/LNwTZUrodUiOckAKC4DTRv
+EtC4nj5EImssVk/xmU3axw==
+=VUqh
+-----END PGP PUBLIC KEY BLOCK-----
+`
 
 func verifyHashsum(ctx context.Context, siteURL, version, archive string, hash []byte) error {
 	sumsURL := fmt.Sprintf("%s/%s/SHA256SUMS", siteURL, version)
@@ -52,16 +65,26 @@ func verifyHashsum(ctx context.Context, siteURL, version, archive string, hash [
 		return err
 	}
 	fs.Debugf(nil, "downloaded hashsum list: %s", sumsURL)
+	return verifyHashsumDownloaded(ctx, sumsBuf, archive, hash)
+}
 
+func verifyHashsumDownloaded(ctx context.Context, sumsBuf []byte, archive string, hash []byte) error {
 	keyRing, err := openpgp.ReadArmoredKeyRing(strings.NewReader(ncwPublicKeyPGP))
 	if err != nil {
-		return errors.New("unsupported signing key")
+		return fmt.Errorf("unsupported signing key: %w", err)
 	}
+
 	block, rest := clearsign.Decode(sumsBuf)
-	// block.Bytes = block.Bytes[1:] // uncomment to test invalid signature
+	if block == nil {
+		return errors.New("invalid hashsum signature: couldn't find detached signature")
+	}
+	if len(rest) > 0 {
+		return fmt.Errorf("invalid hashsum signature: %d bytes of unsigned data", len(rest))
+	}
+
 	_, err = openpgp.CheckDetachedSignature(keyRing, bytes.NewReader(block.Bytes), block.ArmoredSignature.Body, nil)
-	if err != nil || len(rest) > 0 {
-		return errors.New("invalid hashsum signature")
+	if err != nil {
+		return fmt.Errorf("invalid hashsum signature: %w", err)
 	}
 
 	wantHash, err := findFileHash(sumsBuf, archive)
diff --git a/cmd/selfupdate/verify_test.go b/cmd/selfupdate/verify_test.go
new file mode 100644
index 000000000..d9b34cc2c
--- /dev/null
+++ b/cmd/selfupdate/verify_test.go
@@ -0,0 +1,40 @@
+package selfupdate
+
+import (
+	"context"
+	"encoding/hex"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestVerify(t *testing.T) {
+	ctx := context.Background()
+	sumsBuf, err := os.ReadFile("testdata/verify/SHA256SUMS")
+	require.NoError(t, err)
+	hash, err := hex.DecodeString("b20b47f579a2c790ca752fb5d8e5651fade7d5867cbac0a4f71e805fc5c468d0")
+	require.NoError(t, err)
+
+	t.Run("NoError", func(t *testing.T) {
+		err = verifyHashsumDownloaded(ctx, sumsBuf, "archive.zip", hash)
+		require.NoError(t, err)
+	})
+	t.Run("BadSig", func(t *testing.T) {
+		sumsBuf[0x60] ^= 1 // change the signature by one bit
+		err = verifyHashsumDownloaded(ctx, sumsBuf, "archive.zip", hash)
+		assert.ErrorContains(t, err, "invalid signature")
+		sumsBuf[0x60] ^= 1 // undo the change
+	})
+	t.Run("BadSum", func(t *testing.T) {
+		hash[0] ^= 1 // change the SHA256 by one bit
+		err = verifyHashsumDownloaded(ctx, sumsBuf, "archive.zip", hash)
+		assert.ErrorContains(t, err, "archive hash mismatch")
+		hash[0] ^= 1 // undo the change
+	})
+	t.Run("BadName", func(t *testing.T) {
+		err = verifyHashsumDownloaded(ctx, sumsBuf, "archive.zipX", hash)
+		assert.ErrorContains(t, err, "unable to find hash")
+	})
+}
diff --git a/fstest/test_all/config.yaml b/fstest/test_all/config.yaml
index 478695f70..38de401d7 100644
--- a/fstest/test_all/config.yaml
+++ b/fstest/test_all/config.yaml
@@ -12,6 +12,8 @@ tests:
    localonly: true
  - path: cmd/serve/docker
    localonly: true
+ - path: cmd/selfupdate
+   localonly: true
 backends:
  # - backend:  "amazonclouddrive"
  #   remote:   "TestAmazonCloudDrive:"