Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for secure upgrade #2698

Merged
merged 19 commits into from
Jun 26, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions scripts/verify_image_sign.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#!/bin/sh
image_file="${1}"
cms_sig_file="sig.cms"
lines_for_lookup=50
SECURE_UPGRADE_ENABLED=0
DIR="$(dirname "$0")"
if [ -d "/sys/firmware/efi/efivars" ]; then
ycoheNvidia marked this conversation as resolved.
Show resolved Hide resolved
if ! [ -n "$(ls -A /sys/firmware/efi/efivars 2>/dev/null)" ]; then
mount -t efivarfs none /sys/firmware/efi/efivars 2>/dev/null
fi
SECURE_UPGRADE_ENABLED=$(bootctl status 2>/dev/null | grep -c "Secure Boot: enabled")
else
echo "efi not supported - exiting without verification"
exit 0
fi

. /usr/local/bin/verify_image_sign_common.sh

if [ ${SECURE_UPGRADE_ENABLED} -eq 0 ]; then
echo "secure boot not enabled - exiting without image verification"
exit 0
fi

clean_up ()
{
if [ -d ${EFI_CERTS_DIR} ]; then rm -rf ${EFI_CERTS_DIR}; fi
if [ -d "${TMP_DIR}" ]; then rm -rf ${TMP_DIR}; fi
exit $1
}

TMP_DIR=$(mktemp -d)
DATA_FILE="${TMP_DIR}/data.bin"
CMS_SIG_FILE="${TMP_DIR}/${cms_sig_file}"
TAR_SIZE=$(head -n $lines_for_lookup $image_file | grep "payload_image_size=" | cut -d"=" -f2- )
SHARCH_SIZE=$(sed '/^exit_marker$/q' $image_file | wc -c)
SIG_PAYLOAD_SIZE=$(($TAR_SIZE + $SHARCH_SIZE ))
# Extract cms signature from signed file
# Add extra byte for payload
sed -e '1,/^exit_marker$/d' $image_file | tail -c +$(( $TAR_SIZE + 1 )) > $CMS_SIG_FILE
# Extract image from signed file
head -c $SIG_PAYLOAD_SIZE $image_file > $DATA_FILE
# verify signature with certificate fetched with efi tools
EFI_CERTS_DIR=/tmp/efi_certs
[ -d $EFI_CERTS_DIR ] && rm -rf $EFI_CERTS_DIR
mkdir $EFI_CERTS_DIR
efi-readvar -v db -o $EFI_CERTS_DIR/db_efi >/dev/null ||
{
echo "Error: unable to read certs from efi db: $?"
clean_up 1
}
# Convert one file to der certificates
sig-list-to-certs $EFI_CERTS_DIR/db_efi $EFI_CERTS_DIR/db >/dev/null||
{
echo "Error: convert sig list to certs: $?"
clean_up 1
}
for file in $(ls $EFI_CERTS_DIR | grep "db-"); do
LOG=$(openssl x509 -in $EFI_CERTS_DIR/$file -inform der -out $EFI_CERTS_DIR/cert.pem 2>&1)
if [ $? -ne 0 ]; then
logger "cms_validation: $LOG"
fi
# Verify detached signature
LOG=$(verify_image_sign_common $image_file $DATA_FILE $CMS_SIG_FILE)
VALIDATION_RES=$?
if [ $VALIDATION_RES -eq 0 ]; then
RESULT="CMS Verified OK using efi keys"
echo "verification ok:$RESULT"
# No need to continue.
# Exit without error if any success signature verification.
clean_up 0
fi
done
echo "Failure: CMS signature Verification Failed: $LOG"

clean_up 1
34 changes: 34 additions & 0 deletions scripts/verify_image_sign_common.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#!/bin/bash
verify_image_sign_common() {
image_file="${1}"
cms_sig_file="sig.cms"
TMP_DIR=$(mktemp -d)
DATA_FILE="${2}"
CMS_SIG_FILE="${3}"

openssl version | awk '$2 ~ /(^0\.)|(^1\.(0\.|1\.0))/ { exit 1 }'
if [ $? -eq 0 ]; then
# for version 1.1.1 and later
no_check_time="-no_check_time"
else
# for version older than 1.1.1 use noattr
no_check_time="-noattr"
fi

# making sure image verification is supported
EFI_CERTS_DIR=/tmp/efi_certs
RESULT="CMS Verification Failure"
LOG=$(openssl cms -verify $no_check_time -noout -CAfile $EFI_CERTS_DIR/cert.pem -binary -in ${CMS_SIG_FILE} -content ${DATA_FILE} -inform pem 2>&1 > /dev/null )
VALIDATION_RES=$?
if [ $VALIDATION_RES -eq 0 ]; then
RESULT="CMS Verified OK"
if [ -d "${TMP_DIR}" ]; then rm -rf ${TMP_DIR}; fi
echo "verification ok:$RESULT"
# No need to continue.
# Exit without error if any success signature verification.
return 0
fi

if [ -d "${TMP_DIR}" ]; then rm -rf ${TMP_DIR}; fi
return 1
}
2 changes: 2 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,8 @@
'scripts/memory_threshold_check_handler.py',
'scripts/techsupport_cleanup.py',
'scripts/storm_control.py',
'scripts/verify_image_sign.sh',
'scripts/verify_image_sign_common.sh',
'scripts/check_db_integrity.py',
'scripts/sysreadyshow'
],
Expand Down
4 changes: 4 additions & 0 deletions sonic_installer/bootloader/bootloader.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ def supports_package_migration(self, image):
"""tells if the image supports package migration"""
return True

def verify_image_sign(self, image_path):
Copy link
Contributor

@qiluo-msft qiluo-msft May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify_image_sign

A unsecure normal image is considered not signed image, and should return False. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to verify image signature only of secure boot flag is enabled. This flow was already merged in previous PR #2337 (that was reverted even though it had a fix ready in a day) and described and approved in the HLD as well (sonic-net/SONiC#1024)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qiluo-msft please add your comments/approval

Copy link
Contributor

@qiluo-msft qiluo-msft Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After checking HLD, my comment is still applicable.
I understand "We want to verify image signature only of secure boot flag is enabled." This is the base class implementation, if subclass does not override, it will confuse user that an unsecure normal image is considered "verify_image_sign"-ed.

Copy link
Contributor Author

@ycoheNvidia ycoheNvidia Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you suggest to do? @qiluo-msft Does the user read the python code? If we move the prints around verify_image_sign call in main.py inside grub bootloader will it be enough? I Can also add print inside base bootloader ("image verification not implemented, continue without verification"). Returning False here will make other vendor's CI fail, as the previous PR did, and I don't think implementing this method for each bootloader will be positive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean "image verification not implemented", you should either throw NotImplmented exception or return a reasonable value to indicate the error. Silently "continue without verification" does not make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified the code as suggested, please review @qiluo-msft

"""verify image signature is valid"""
raise NotImplementedError

@classmethod
def detect(cls):
"""returns True if the bootloader is in use"""
Expand Down
11 changes: 11 additions & 0 deletions sonic_installer/bootloader/grub.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,17 @@ def verify_image_platform(self, image_path):
# Check if platform is inside image's target platforms
return self.platform_in_platforms_asic(platform, image_path)

def verify_image_sign(self, image_path):
click.echo('Verifying image signature')
verification_script_name = 'verify_image_sign.sh'
script_path = os.path.join('/usr', 'local', 'bin', verification_script_name)
if not os.path.exists(script_path):
click.echo("Unable to find verification script in path " + script_path)
return False
verification_result = subprocess.run([script_path, image_path], capture_output=True)
click.echo(str(verification_result.stdout) + " " + str(verification_result.stderr))
return verification_result.returncode == 0

@classmethod
def detect(cls):
return os.path.isfile(os.path.join(HOST_PATH, 'grub/grub.cfg'))
13 changes: 12 additions & 1 deletion sonic_installer/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,8 @@ def sonic_installer():
@click.option('-y', '--yes', is_flag=True, callback=abort_if_false,
expose_value=False, prompt='New image will be installed, continue?')
@click.option('-f', '--force', '--skip-secure-check', is_flag=True,
help="Force installation of an image of a non-secure type than secure running image")
help="Force installation of an image of a non-secure type than secure running " +
" image, this flag does not affect secure upgrade image verification")
@click.option('--skip-platform-check', is_flag=True,
help="Force installation of an image of a type which is not of the same platform")
@click.option('--skip_migration', is_flag=True,
Expand Down Expand Up @@ -576,6 +577,16 @@ def install(url, force, skip_platform_check=False, skip_migration=False, skip_pa
"Aborting...", LOG_ERR)
raise click.Abort()

# Calling verification script by default - signature will be checked if enabled in bios
ycoheNvidia marked this conversation as resolved.
Show resolved Hide resolved
echo_and_log("Verifing image {} signature...".format(binary_image_version))
try:
if not bootloader.verify_image_sign(image_path):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the force option is enabled, can we skip the verify_image_sign, as descirption in line 514? The check logic should be the same behavior as line 567 (secure bot), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xumia we don't want to allow force option for this feature, it will reduce its security properties. This remark was addressed in the previous merged and reverted PR #2337 (at least as I can remember)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @xumia.
The security is protected by the secure boot process, not by the installer. If the user intention is to install it anyway, no matter it will boot or not, we should respect the intetion, right?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is not what was defined in the HLD for this feature.
If we would like to make a feature change for this item - it should go as a separate request.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should protect it and not allow it. we do protect on other flows as well.

echo_and_log('Error: Failed verify image signature', LOG_ERR)
raise click.Abort()
else:
echo_and_log('Verification successful')
except NotImplementedError:
echo_and_log('Image verification not impelmented, continue image install without it')
echo_and_log("Installing image {} and setting it as default...".format(binary_image_version))
with SWAPAllocator(not skip_setup_swap, swap_mem_size, total_mem_threshold, available_mem_threshold):
bootloader.install_image(image_path)
Expand Down
10 changes: 10 additions & 0 deletions tests/installer_bootloader_aboot_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,13 @@ def test_set_fips_aboot():

# Cleanup
shutil.rmtree(dirpath)

def test_verify_image_sign():
bootloader = aboot.AbootBootloader()
return_value = None
try:
return_value = bootloader.verify_image_sign(exp_image)
except NotImplementedError:
pass
else:
assert False, "Wrong return value from verify_image_sign, returned" + str(return_value)
8 changes: 8 additions & 0 deletions tests/installer_bootloader_grub_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,11 @@ def test_set_fips_grub():

# Cleanup the _tmp_host folder
shutil.rmtree(tmp_host_path)

def test_verify_image():

bootloader = grub.GrubBootloader()
image = f'{grub.IMAGE_PREFIX}expeliarmus-{grub.IMAGE_PREFIX}abcde'

# command should fail
assert not bootloader.verify_image_sign(image)
10 changes: 10 additions & 0 deletions tests/installer_bootloader_onie_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,13 @@ def test_get_current_image(re_search):
# Test image dir conversion
onie.re.search().group = Mock(return_value=image)
assert bootloader.get_current_image() == exp_image

def test_verify_image_sign():
bootloader = onie.OnieInstallerBootloader()
return_value = None
try:
return_value = bootloader.verify_image_sign('some_path.path')
except NotImplementedError:
pass
else:
assert False, "Wrong return value from verify_image_sign, returned" + str(return_value)
11 changes: 11 additions & 0 deletions tests/installer_bootloader_uboot_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,3 +141,14 @@ def mock_run_command(cmd):
# Test fips disabled
bootloader.set_fips(image, False)
assert not bootloader.get_fips(image)

def test_verify_image_sign():
bootloader = uboot.UbootBootloader()
image = 'test-image'
return_value = None
try:
return_value = bootloader.verify_image_sign(image)
except NotImplementedError:
pass
else:
assert False, "Wrong return value from verify_image_sign, returned" + str(return_value)
40 changes: 40 additions & 0 deletions tests/scripts/create_mock_image.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
repo_dir=$1
input_image=$2
output_file=$3
cert_file=$4
key_file=$5
tmp_dir=
clean_up()
{
sudo rm -rf $tmp_dir
sudo rm -rf $output_file
exit $1
}

DIR="$(dirname "$0")"

tmp_dir=$(mktemp -d)
sha1=$(cat $input_image | sha1sum | awk '{print $1}')
echo -n "."
cp $repo_dir/installer/sharch_body.sh $output_file || {
echo "Error: Problems copying sharch_body.sh"
clean_up 1
}
# Replace variables in the sharch template
sed -i -e "s/%%IMAGE_SHA1%%/$sha1/" $output_file
echo -n "."
tar_size="$(wc -c < "${input_image}")"
cat $input_image >> $output_file
sed -i -e "s|%%PAYLOAD_IMAGE_SIZE%%|${tar_size}|" ${output_file}
CMS_SIG="${tmp_dir}/signature.sig"

echo "$0 CMS signing ${input_image} with ${key_file}. Output file ${output_file}"
. $repo_dir/scripts/sign_image_dev.sh
sign_image_dev ${cert_file} ${key_file} $output_file ${CMS_SIG} || clean_up 1

cat ${CMS_SIG} >> ${output_file}
echo "Signature done."
# append signature to binary
sudo rm -rf ${CMS_SIG}
sudo rm -rf $tmp_dir
exit 0
91 changes: 91 additions & 0 deletions tests/scripts/create_sign_and_verify_test_files.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
repo_dir=$1
out_dir=$2
mock_image="mock_img.bin"
output_file=$out_dir/output_file.bin
cert_file=$3
other_cert_file=$4
tmp_dir=
clean_up()
{
sudo rm -rf $tmp_dir
sudo rm -rf $mock_image
exit $1
}
DIR="$(dirname "$0")"
[ -d $out_dir ] || rm -rf $out_dir
mkdir $out_dir
tmp_dir=$(mktemp -d)
#generate self signed keys and certificate
key_file=$tmp_dir/private-key.pem
pub_key_file=$tmp_dir/public-key.pem
openssl ecparam -name secp256r1 -genkey -noout -out $key_file
openssl ec -in $key_file -pubout -out $pub_key_file
openssl req -new -x509 -key $key_file -out $cert_file -days 360 -subj "/C=US/ST=Test/L=Test/O=Test/CN=Test"
alt_key_file=$tmp_dir/alt-private-key.pem
alt_pub_key_file=$tmp_dir/alt-public-key.pem
openssl ecparam -name secp256r1 -genkey -noout -out $alt_key_file
openssl ec -in $alt_key_file -pubout -out $alt_pub_key_file
openssl req -new -x509 -key $alt_key_file -out $other_cert_file -days 360 -subj "/C=US/ST=Test/L=Test/O=Test/CN=Test"

echo "this is a mock image\nThis is another line !2#4%6\n" > $mock_image
echo "Created a mock image with following text:"
cat $mock_image
# create signed mock image

sh $DIR/create_mock_image.sh $repo_dir $mock_image $output_file $cert_file $key_file || {
echo "Error: unable to create mock image"
clean_up 1
}

[ -f "$output_file" ] || {
echo "signed mock image not created - exiting without testing"
clean_up 1
}

test_image_1=$out_dir/test_image_1.bin
cp -v $output_file $test_image_1 || {
echo "Error: Problems copying image"
clean_up 1
}

# test_image_1 = modified image size to something else - should fail on signature verification
image_size=$(sed -n 's/^payload_image_size=\(.*\)/\1/p' < $test_image_1)
sed -i "/payload_image_size=/c\payload_image_size=$(($image_size - 5))" $test_image_1

test_image_2=$out_dir/test_image_2.bin
cp -v $output_file $test_image_2 || {
echo "Error: Problems copying image"
clean_up 1
}

# test_image_2 = modified image sha1 to other sha1 value - should fail on signature verification
im_sha=$(sed -n 's/^payload_sha1=\(.*\)/\1/p' < $test_image_2)
sed -i "/payload_sha1=/c\payload_sha1=2f1bbd5a0d411253103e688e4e66c00c94bedd40" $test_image_2

tmp_image=$tmp_dir/"tmp_image.bin"
echo "this is a different image now" >> $mock_image
sh $DIR/create_mock_image.sh $repo_dir $mock_image $tmp_image $cert_file $key_file || {
echo "Error: unable to create mock image"
clean_up 1
}
# test_image_3 = original mock image with wrong signature
# Extract cms signature from signed file
test_image_3=$out_dir/"test_image_3.bin"
tmp_sig="${tmp_dir}/tmp_sig.sig"
TMP_TAR_SIZE=$(head -n 50 $tmp_image | grep "payload_image_size=" | cut -d"=" -f2- )
sed -e '1,/^exit_marker$/d' $tmp_image | tail -c +$(( $TMP_TAR_SIZE + 1 )) > $tmp_sig

TAR_SIZE=$(head -n 50 $output_file | grep "payload_image_size=" | cut -d"=" -f2- )
SHARCH_SIZE=$(sed '/^exit_marker$/q' $output_file | wc -c)
SIG_PAYLOAD_SIZE=$(($TAR_SIZE + $SHARCH_SIZE ))
head -c $SIG_PAYLOAD_SIZE $output_file > $test_image_3
sudo rm -rf $tmp_image

cat ${tmp_sig} >> ${test_image_3}

# test_image_4 = modified image with original mock image signature
test_image_4=$out_dir/"test_image_4.bin"
head -c $SIG_PAYLOAD_SIZE $output_file > $test_image_4
echo "this is additional line" >> $test_image_4
cat ${tmp_sig} >> ${test_image_4}
clean_up 0
29 changes: 29 additions & 0 deletions tests/scripts/verify_image_sign_test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#!/bin/bash
image_file="${1}"
cert_path="${2}"
cms_sig_file="sig.cms"
TMP_DIR=$(mktemp -d)
DATA_FILE="${TMP_DIR}/data.bin"
CMS_SIG_FILE="${TMP_DIR}/${cms_sig_file}"
lines_for_lookup=50

TAR_SIZE=$(head -n $lines_for_lookup $image_file | grep "payload_image_size=" | cut -d"=" -f2- )
SHARCH_SIZE=$(sed '/^exit_marker$/q' $image_file | wc -c)
SIG_PAYLOAD_SIZE=$(($TAR_SIZE + $SHARCH_SIZE ))
# Extract cms signature from signed file - exit marker marks last sharch prefix + number of image lines + 1 for next linel
# Add extra byte for payload - extracting image signature from line after data file
sed -e '1,/^exit_marker$/d' $image_file | tail -c +$(( $TAR_SIZE + 1 )) > $CMS_SIG_FILE
# Extract image from signed file
head -c $SIG_PAYLOAD_SIZE $image_file > $DATA_FILE
EFI_CERTS_DIR=/tmp/efi_certs
[ -d $EFI_CERTS_DIR ] && rm -rf $EFI_CERTS_DIR
mkdir $EFI_CERTS_DIR
cp $cert_path $EFI_CERTS_DIR/cert.pem

DIR="$(dirname "$0")"
. $DIR/verify_image_sign_common.sh
verify_image_sign_common $image_file $DATA_FILE $CMS_SIG_FILE
VERIFICATION_RES=$?
if [ -d "${TMP_DIR}" ]; then rm -rf ${TMP_DIR}; fi
[ -d $EFI_CERTS_DIR ] && rm -rf $EFI_CERTS_DIR
exit $VERIFICATION_RES
Loading