Minisign verification failure == pass??
Needs reporter action, HighPublicBUG
Actions

Assigned To

Authored By

	b-
	Jan 5 2023, 2:30 PM

Description

I built my own image and set up signing in the toolchain. It gives me signed images, and that’s awesome!

What’s not awesome is what happened when I added an image that I signed on top of one that still had VyOS keys in it, which is that it ultimately said Digital signature is valid. and didn’t prompt me to confirm. Full install transcript below:

vyos@vyos:/config/user-data/vyos-config$ a s i https://github.com/b-/vyos-build-action/releases/download/v1.4-rolling_bri_add-ssh_config-202301051411/vyos-1.4-rolling_bri_add-ssh_config-latest-amd64.iso
Trying to fetch ISO file from https://github.com/b-/vyos-build-action/releases/download/v1.4-rolling_bri_add-ssh_config-202301051411/vyos-1.4-rolling_bri_add-ssh_config-latest-amd64.iso...
Downloading...
Redirecting to https://objects.githubusercontent.com/github-production-release-asset-2e65be/583816895/263dcd4d-1c17-4368-8761-60a4e9396438?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20230105%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20230105T142523Z&X-Amz-Expires=300&X-Amz-Signature=71a4149143f59fe8aae6e050b41947efc4e395741b614e56b4de5b9146147b61&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=583816895&response-content-disposition=attachment%3B%20filename%3Dvyos-1.4-rolling_bri_add-ssh_config-latest-amd64.iso&response-content-type=application%2Foctet-stream
The file is 460.000 MiB.
[##############################################] 100%
Download complete.
Done.
Checking for digital signature file...
Downloading...
Redirecting to https://objects.githubusercontent.com/github-production-release-asset-2e65be/583816895/a30e5933-50ec-4394-8a36-f0628001e8b5?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20230105%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20230105T142533Z&X-Amz-Expires=300&X-Amz-Signature=e08bd259ad9685f140efbad93df4f58701e4ab4becc2b652a79f6d33efb4d3dd&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=583816895&response-content-disposition=attachment%3B%20filename%3Dvyos-1.4-rolling_bri_add-ssh_config-latest-amd64.iso.minisig&response-content-type=application%2Foctet-stream
The file is 0.339 KiB.
[##############################################] 100%
Download complete.
Checking digital signature...
Signature key id in /var/tmp/install-image.3178/vyos-1.4-rolling_bri_add-ssh_config-latest-amd64.iso.minisig is 92C16282E3ED6C
but the key id in the public key is 9EA8ECDCBDDCD6D1
Signature check FAILED, trying BACKUP key...
Signature key id in /var/tmp/install-image.3178/vyos-1.4-rolling_bri_add-ssh_config-latest-amd64.iso.minisig is 92C16282E3ED6C
but the key id in the public key is 69C20BE1367AEBB0
Digital signature is valid.
Checking SHA256 checksums of files on the ISO image... OK.
Done!
What would you like to name this image? [1.4-rolling_bri_add-ssh_config-202301051411]:

Details

Difficulty level: Unknown (require assessment)
Version: 1.4
Why the issue appeared?: Implementation mistake
Is it a breaking change?: Stricter validation
Issue type: Security vulnerability

Revisions and Commits

	Restricted Diffusion Commit
	Restricted Diffusion Commit
	Restricted Diffusion Commit
	Restricted Diffusion Commit

Event Timeline

b- created this task.Jan 5 2023, 2:30 PM

The error handling on this line is basically nonexistent, but also the coding style is a little hard to follow.

Here's a hacked up version of just the relevant section of the script, replacing the commands with true and false:

#!/bin/sh
set -x
echo "Checking digital signature..."
if true; then
    false # 	minisign -V -q -p /usr/share/vyos/keys/vyos-release.minisign.pub -m ${filename} -x ${filename}.minisig
    if [ $? -ne 0 ]; then
	echo "Signature check FAILED, trying BACKUP key..."
	false # minisign -V -q -p /usr/share/vyos/keys/vyos-backup.minisign.pub -m ${filename} -x ${filename}.minisig
    fi
fi
if false; then
    true #gpg --verify ${filename}.asc ${filename} >/dev/null 2>&1
fi
if [ $? -ne 0 ]; then
    echo "Signature check FAILED."
    echo -n "Do you want to continue anyway? (yes/no) [no] "
    # response=$(get_response "No" "Yes No Y N")
    if [ "$response" == "no" ] || [ "$response" == "n" ]; then
	fail_exit 'OK. Installation will not be performed.'
    fi
    echo "OK. Proceeding with installation anyway."
else
    echo "Digital signature is valid."
fi

results:

$ bash test.sh
+ echo 'Checking digital signature...'
Checking digital signature...
+ true
+ false
+ '[' 1 -ne 0 ']'
+ echo 'Signature check FAILED, trying BACKUP key...'
Signature check FAILED, trying BACKUP key...
+ false
+ false
+ '[' 0 -ne 0 ']'
+ echo 'Digital signature is valid.'
Digital signature is valid.

I'm going to try to rewrite this fragment with case statements to make it more readable (and more functional).

I created a PR, but I'm not certain how to compile this part of VyOS to test this, and I'm hoping someone could help me do so -- a quick glance makes it look to me like this is compiled into a .deb that's then installed by https://github.com/vyos/vyos-build/blob/current/scripts/build-vyos-image ?

b- changed Why the issue appeared? from Will be filled on close to Implementation mistake.Jan 5 2023, 8:05 PM

b- changed Is it a breaking change? from Unspecified (possibly destroys the router) to Stricter validation.

b- changed Issue type from Unspecified (please specify) to Security vulnerability.

~~er wait hold up i made a mistake saving/pushing my changes~~
edit: fixed

I just edited the file /opt/vyatta/sbin/install-image in a running system to try testing this, and it works as expected at least for the primary minisign key. I didn't test GPG or 2nd minisign key, but I see no reason why there would be an issue there. I did touch those parts, though, so it's probably worth at least having another set of eyes look at it all.

But here's a transcript with the updated file. I did this on an image I built with my own minisign key as the primary key, so I replaced the primary public key with the backup VyOS public key (that I of course don't have the key to) in order to ensure that the verification fails.

vyos@vyos:~$ sudo cp /usr/share/vyos/keys/vyos-{backup,release}.minisign.pub
vyos@vyos:~$ a s i https://github.com/b-/vyos-build-action/releases/download/v1.4-rolling_bri_add-ssh_config-202301051411/vyos-1.4-rolling_bri_add-ssh_config-202301051411-amd64.iso
Trying to fetch ISO file from https://github.com/b-/vyos-build-action/releases/download/v1.4-rolling_bri_add-ssh_config-202301051411/vyos-1.4-rolling_bri_add-ssh_config-202301051411-amd64.iso...
Downloading...
Redirecting to https://objects.githubusercontent.com/github-production-release-asset-2e65be/583816895/ee8da0b1-ab07-4ac9-9427-5dcbbca32983?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20230105%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20230105T204913Z&X-Amz-Expires=300&X-Amz-Signature=39bacdfd2260730e394ae17cc6c47197a2fb9a1b2916fe84ff8636d35b3e11a0&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=583816895&response-content-disposition=attachment%3B%20filename%3Dvyos-1.4-rolling_bri_add-ssh_config-202301051411-amd64.iso&response-content-type=application%2Foctet-stream
The file is 460.000 MiB.
[#####################################################################################################] 100%
Download complete.
Done.
Checking for digital signature file...
Downloading...
Redirecting to https://objects.githubusercontent.com/github-production-release-asset-2e65be/583816895/75afc346-84ac-47fa-af76-dbaf28bf0520?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20230105%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20230105T204921Z&X-Amz-Expires=300&X-Amz-Signature=6d6fc180e84f493ec8a20cddcda9bea7f2da4947f0b4290364b668ed4b6423ff&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=583816895&response-content-disposition=attachment%3B%20filename%3Dvyos-1.4-rolling_bri_add-ssh_config-202301051411-amd64.iso.minisig&response-content-type=application%2Foctet-stream
The file is 0.345 KiB.
[#####################################################################################################] 100%
Download complete.
Downloaded minisign digital signature.
Checking digital signature...
Signature key id in /var/tmp/install-image.5158/vyos-1.4-rolling_bri_add-ssh_config-202301051411-amd64.iso.minisig is 92C16282E3ED6C
but the key id in the public key is 69C20BE1367AEBB0
Primary signature check FAILED, trying BACKUP key...
Signature key id in /var/tmp/install-image.5158/vyos-1.4-rolling_bri_add-ssh_config-202301051411-amd64.iso.minisig is 92C16282E3ED6C
but the key id in the public key is 69C20BE1367AEBB0
Backup signature check FAILED.
Signature check FAILED.
Do you want to continue anyway? (yes/no) [no]

b- added commits: Restricted Diffusion Commit, Restricted Diffusion Commit, Restricted Diffusion Commit, Restricted Diffusion Commit.Jan 5 2023, 9:08 PM

pasik added a subscriber: pasik.Jan 6 2023, 10:08 AM

I noticed this as well. The issue is expecting $? to refer to the exit code of minisign -V when it's actually referring to the exit code of the if [ -f ${filename}.asc ]; block which will always be 0.

An alternative to the above fix I tried before finding this issue would be to store $? in a variable immediately after execution and test that instead of $?.

Note: If the minisig is invalid but GPG is valid this will pass verification. I'm assuming this is expected behavior (similar to supporting the backup minisign key) but wanted to note that explicitly here.

From 246619f0116307ee80e5dfcce950027687883cf6 Mon Sep 17 00:00:00 2001
From: Ian Hattendorf <[email protected]>
Date: Wed, 15 Mar 2023 08:45:37 -0700
Subject: [PATCH] Don't lose minisign status

Checking $? needs to be done immediately after the command. In this case, if a gpg file doesn't exist we were using the result of the `if [ $signature_code -ne 0 ];` block as the signature status.
---
 scripts/install/install-image | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/scripts/install/install-image b/scripts/install/install-image
index b4b9cfba..970dd283 100755
--- a/scripts/install/install-image
+++ b/scripts/install/install-image
@@ -137,17 +137,21 @@ fetch_iso_by_url ()
         fi
     else
         echo "Checking digital signature..."
+        local signature_verify_code=0
         if [ -f ${filename}.minisig ]; then
             minisign -V -q -p /usr/share/vyos/keys/vyos-release.minisign.pub -m ${filename} -x ${filename}.minisig
-            if [ $? -ne 0 ]; then
+            signature_verify_code=$?
+            if [ $signature_verify_code -ne 0 ]; then
                 echo "Signature check FAILED, trying BACKUP key..."
                 minisign -V -q -p /usr/share/vyos/keys/vyos-backup.minisign.pub -m ${filename} -x ${filename}.minisig
+                signature_verify_code=$?
             fi
         fi
         if [ -f ${filename}.asc ]; then
             gpg --verify ${filename}.asc ${filename} >/dev/null 2>&1
+            signature_verify_code=$?
         fi
-        if [ $? -ne 0 ]; then
+        if [ $signature_verify_code -ne 0 ]; then
             echo "Signature check FAILED."
             echo -n "Do you want to continue anyway? (yes/no) [no] "
             response=$(get_response "No" "Yes No Y N")
-- 
2.39.2

I want to mention, the reason I wrote out the $? is because it can be confusing and fragile, as this issue demonstrates in the first place.

I don’t know if others necessarily feel as strongly about it as I do, but I personally feel that

# this is much easier to debug and harder to break by accident
if foo ; then
  bar
fi 

# as opposed to this, which is more fragile and less clear about what it’s doing
foo
foo_result=$?
if [ $foo_result -ne 0 ] ; then
  bar
fi

(The reason these are equivalent is that [ is just another command — see man test — that returns 0 if truthy or >0 otherwise)

Given that this is security-sensitive code, I personally think that the use of $? here should be given a lot of thought.

I don’t really want to say, “please don’t do it that way,” but only because I don’t exactly have a huge list of contributions under my name or anything. I do feel very strongly about this, though.

(Shell scripting sucks, maybe this should be implemented in Python or something, anyway? Is there a minisign module for Python?)

Agreed. I just posted my workaround as a minimal fix to highlight the issue: accessing $? after another command was ran (which can be easy to miss).

I won't advocate one way or another for how the issue is actually resolved as I'm not very familiar with this project and any reasoning for why something might have been done one way or the other.

I just noticed that this still is a problem. Excerpt below from downloading an upgrade:

starting serial terminal on interface serial0

vyos-home login: 
vyos-home login: vyos
Password: 
welcome to perchnet
vyos@vyos-home:~$ conf
[edit]
vyos@vyos-home# comp sav
No changes between working and saved configurations.

[edit]
vyos@vyos-home# ex
exit
nload/v1.4-rolling_bri/vyos-1.4-rolling_bri-202309260306-amd64.ison/releases/dow 
Trying to fetch ISO file from https://github.com/b-/vyos-build-action/releases/download/v1.4-rolling_bri/vyos-1.4-rolling_bri-202309260306-amd64.iso...
Downloading...
Redirecting to https://objects.githubusercontent.com/github-production-release-asset-2e65be/583816895/0637443b-af23-4c3c-9658-b8d4d3ed8ab0?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20230926%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20230926T173624Z&X-Amz-Expires=300&X-Amz-Signature=6cd3319975731d34494a9f385d690d6316a247f9141b09cbb6ca61e4dc5ba903&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=583816895&response-content-disposition=attachment%3B%20filename%3Dvyos-1.4-rolling_bri-202309260306-amd64.iso&response-content-type=application%2Foctet-stream
The file is 433.000 MiB.
[#[237434.860469] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:%
[237434.973323] rcu:    1-....: (1 GPs behind) idle=0de4/1/0x4000000000000000 softirq=16172334/16172335 fqs=4955
[##########[237511.022571] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
[#[237511.029271] rcu: #1-....: (1 GPs behind) idle=1094/0/0x3 softirq=16172569/16172569 fqs=5009
[[237513.623050] watchdog: BUG: soft lockup - CPU#1 stuck for 22s! [swapper/1:0]
[###########################################______________________]  66%
Message from syslogd@vyos-home at Sep 26 17:38:10 ...
 kernel:[237513.623050] watchdog: BUG: soft lockup - CPU#1 stuck for 22s! [swapper/1:0]
[#########################################[237552.643802] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
[################[237552.653817] rcu:   1-....: (24 ticks this GP) idle=109c/1/0x4000000000000000 softirq=16172681/16172681 fqs=4840
[#################################################################] 100%
Download complete.
Done.
Checking for digital signature file...
Downloading...
Redirecting to https://objects.githubusercontent.com/github-production-release-asset-2e65be/583816895/5d62d776-eaf7-41a6-8743-0c02235691d2?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20230926%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20230926T173907Z&X-Amz-Expires=300&X-Amz-Signature=1a5492d0522b63f3edc2b6a243f85f82d6d261f2d52eff5d01d6a42865624721&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=583816895&response-content-disposition=attachment%3B%20filename%3Dvyos-1.4-rolling_bri-202309260306-amd64.iso.minisig&response-content-type=application%2Foctet-stream
The file is 0.323 KiB.
[#################################################################] 100%
Download complete.
Checking digital signature...
Signature key id in /var/tmp/install-image.14988/vyos-1.4-rolling_bri-202309260306-amd64.iso.minisig is 92C16282E3ED6C
but the key id in the public key is 9EA8ECDCBDDCD6D1
Signature check FAILED, trying BACKUP key...
Signature key id in /var/tmp/install-image.14988/vyos-1.4-rolling_bri-202309260306-amd64.iso.minisig is 92C16282E3ED6C
but the key id in the public key is 69C20BE1367AEBB0
Digital signature is valid.
Checking SHA256 checksums of files on the ISO image... OK.
Done!
What would you like to name this image? [1.4-rolling_bri-202309260306]:

Just to be clear, the build I'm going from is just my own build of current to my own build of current -- it says 1.4 because I only changed the version string to 1.5 after this build went through since i'm the only one using my build :)

Can you please re-test with the latest 1.5 rolling image?

dmbaturin edited projects, added VyOS 1.4 Sagitta (1.4.0-epa3); removed VyOS 1.4 Sagitta.Fri, Apr 12, 1:45 PM

We'll close it if no response

Minisign verification failure == pass??Needs reporter action, HighPublicBUGActions

Description

Details

Revisions and Commits

Event Timeline

Minisign verification failure == pass??
Needs reporter action, HighPublicBUG
Actions