Secureboot: Image signing verification enhancements

[bhouse-nexthop]

What I did

The current signature verification of sonic images assumes the DB Keys are all Root CAs. The secureboot standard says nothing about this, the DBKeys are explicitly trusted by signing them with the KEK, and that signing method does not follow the standard X.509 PKI architecture. Therefore the DB Key is not guaranteed to be a CA Root (aka not self-signed). It is possible the DB Key was created as an intermediate, but since it is explicitly trusted that is ok.

Fixes sonic-net/sonic-buildimage#23406

How I did it

This adds this explicit trust of the DB Key as a secondary signing verification if the original verification fails. It disables looking inside the pkcs7 container for any keys at all and assumes the key specified is the exact key for the signature.

How to verify it

Build secureboot image signed with a DB Key that is not self-signed, then run through the sonic-installer install with that image and see the verification succeeds.

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bhouse-nexthop]

@ycoheNvidia since you contributed the signing validation for sonic images, can you review this PR modification?

[ycoheNvidia]

Adding @davidpil2002 @DavidZagury @Yarden-Z as well

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tjchadaga]

@qiluo-msft - could you please help with the review?

[ycoheNvidia]

Looks fine to me

[bhouse-nexthop]

@Yarden-Z any comments?
@qiluo-msft any reason not to merge this?

[bhouse-nexthop]

@hdwhdw can you possibly take a look at this?

[lguohan]

Here’s a review of the changes made to verify_image_sign_common.sh in the referenced pull request:

Summary of Changes

• The image signature verification logic has been refactored to try two different verification methods using openssl cms -verify:

  • Method 1: Standard certificate chain verification (-CAfile), expecting the DB Key as a self-signed root and the image signed with an intermediate certificate.
  • Method 2: Direct verification assuming the DB key signed the image (-nointern -noverify -certfile), skipping root CA checks.

• The code now loops over both methods, returning success if either verification approach works.

Detailed Feedback

Positives:

• Robustness Improved: By verifying the image signature using two different approaches, the script is now more resilient to different signing setups, covering cases where the DB key is either a root CA or just a trusted signer.
• Clear Documentation: The added comments clearly explain the rationale for the two verification modes, making the logic easier to understand and maintain.
• Backward Compatibility: The fallback ensures that existing images signed with either method will be accepted, reducing risk of verification failures due to changing certificate hierarchies.

Suggestions:

• Variable Naming: The loop variable variant is slightly misleading since it sometimes contains multiple flags. Consider naming it verify_opts or similar for clarity.
• Error Handling: If both verification methods fail, the script proceeds to clean up and returns 1 as before, which is appropriate. However, you might want to log which verification method(s) failed for troubleshooting.
• Quoting Variables: For safety, it’s good practice to quote variables in shell scripts, e.g. "${CMS_SIG_FILE}", "${DATA_FILE}", and "${EFI_CERTS_DIR}/cert.pem" to avoid issues if paths contain spaces.
• Hardcoded Paths: The use of /tmp/efi_certs is unchanged. Ensure this path is appropriate for all environments where the script is used.

Example Logging Improvement:

LOG=$(openssl cms -verify $no_check_time -noout ${variant} "$EFI_CERTS_DIR/cert.pem" -binary -in "${CMS_SIG_FILE}" -content "${DATA_FILE}" -inform pem 2>&1 > /dev/null )
if [ $VALIDATION_RES -eq 0 ]; then
    RESULT="CMS Verified OK (${variant})"
    return 0
fi

Final Assessment

The change is well-justified and improves signature verification compatibility.
Minor improvements for safety and diagnostics could be considered, but the new logic is solid and clearly documented.

If you need a more formal approval or additional suggestions, let me know!