From 32eb55b808a34da773f562158fa7100440f0a9ee Mon Sep 17 00:00:00 2001 From: Matthew Garrett Date: Thu, 12 Jan 2017 15:49:13 -0800 Subject: [PATCH 1/5] coreos_sign_update: Use smartcards for signing Sign updates using private keys on smartcards. This involves changing the padding approach - rather than including the padding in the hash, ask the card to generate the padding itself, since the card will refuse to sign pre-padded material. Use + as a key separator rather than : as the PKCS#11 URI includes colons. --- core_sign_update | 22 +++++++++++----------- offline_signing/sign.sh | 4 ++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/core_sign_update b/core_sign_update index fd461c54fb..6363d7f2b2 100755 --- a/core_sign_update +++ b/core_sign_update @@ -43,10 +43,10 @@ delta_generator \ -new_kernel "$FLAGS_kernel" \ -out_file update -IFS=: read -a private_keys <<< "$FLAGS_private_keys" -IFS=: read -a public_keys <<< "$FLAGS_public_keys" +IFS=+ read -a private_keys <<< "$FLAGS_private_keys" +IFS=+ read -a public_keys <<< "$FLAGS_public_keys" -if [ ${#private_keys} -ne ${#public_keys} ]; then +if [ ${#private_keys[@]} -ne ${#public_keys[@]} ]; then echo "mismatch in count of private keys and public keys" exit 1 fi @@ -64,13 +64,13 @@ delta_generator \ --in_file update \ --out_hash_file update.hash -# The following is a standard PKCS1-v1_5 padding for SHA256 signatures, as -# defined in RFC3447. It is prepended to the actual signature (32 bytes) to -# form a sequence of 256 bytes (2048 bits) that is amenable to RSA signing. The -# padded hash will look as follows: +# The following is an ASN.1 header. It is prepended to the actual signature +# (32 bytes) to form a sequence of 51 bytes. OpenSSL will add additional +# PKCS#1 1.5 padding during the signing operation. The padded hash will look +# as follows: # -# 0x00 0x01 0xff ... 0xff 0x00 ASN1HEADER SHA256HASH -# |--------------205-----------||----19----||----32----| +# ASN1HEADER SHA256HASH +# |----19----||----32----| # # where ASN1HEADER is the ASN.1 description of the signed data. The complete 51 # bytes of actual data (i.e. the ASN.1 header complete with the hash) are @@ -83,13 +83,13 @@ delta_generator \ # } # OCTET STRING(2+32) # } -echo "AAH/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////ADAxMA0GCWCGSAFlAwQCAQUABCA=" | base64 -d > padding +echo "MDEwDQYJYIZIAWUDBAIBBQAEIA==" | base64 -d > padding cat padding update.hash > update.padhash i=1 signature_sizes="" for key in "${private_keys[@]}"; do - openssl rsautl -raw -sign -inkey ${key} -in update.padhash -out update.sig.${i} + openssl rsautl -engine pkcs11 -pkcs -sign -inkey ${key} -keyform engine -in update.padhash -out update.sig.${i} let "i += 1" done diff --git a/offline_signing/sign.sh b/offline_signing/sign.sh index 2fe4fb3e58..080bd566aa 100755 --- a/offline_signing/sign.sh +++ b/offline_signing/sign.sh @@ -17,5 +17,5 @@ cd "${DATA_DIR}" --image "${DATA_DIR}/coreos_production_update.bin" \ --kernel "${DATA_DIR}/coreos_production_image.vmlinuz" \ --output "${DATA_DIR}/coreos_production_update.gz" \ - --private_keys "${KEYS_DIR}/devel.key.pem:${KEYS_DIR}/prod-2.key.pem" \ - --public_keys "${KEYS_DIR}/devel.pub.pem:${KEYS_DIR}/prod-2.pub.pem" + --private_keys "${KEYS_DIR}/devel.key.pem+${KEYS_DIR}/prod-2.key.pem" \ + --public_keys "${KEYS_DIR}/devel.pub.pem+${KEYS_DIR}/prod-2.pub.pem" From 9d58bec73b0262e0f4f5d88120a1a2c2bc4360b2 Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Thu, 6 Jul 2017 13:46:32 -0700 Subject: [PATCH 2/5] coreos_sign_update: return 'legacy' signing support We currently sign with both a devel key and a prod key. The devel key is insecure and need not be included on a smartcard, so it makes sense to leave it be on disk. However, the previous commit's padding changes removed this legacy method of signing. For simplicity, simply re-introduce the old logic conditionally based on whether it's a smartcard or not. Alternate options could be using `-pkcs` instead of `-raw` for both keys, but that is a more intricate change I'd be less confident in making. --- core_sign_update | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/core_sign_update b/core_sign_update index 6363d7f2b2..6b55d1a0b9 100755 --- a/core_sign_update +++ b/core_sign_update @@ -29,9 +29,11 @@ set -e cleanup() { rm -f padding + rm -f padding-pkcs11 rm -f update rm -f update.hash rm -f update.padhash + rm -f update.pkcs11-padhash rm -f update.signed rm -f update.sig.* } @@ -64,6 +66,8 @@ delta_generator \ --in_file update \ --out_hash_file update.hash +# padding for openssl rsautl -pkcs (smartcard keys) +# # The following is an ASN.1 header. It is prepended to the actual signature # (32 bytes) to form a sequence of 51 bytes. OpenSSL will add additional # PKCS#1 1.5 padding during the signing operation. The padded hash will look @@ -83,13 +87,42 @@ delta_generator \ # } # OCTET STRING(2+32) # } -echo "MDEwDQYJYIZIAWUDBAIBBQAEIA==" | base64 -d > padding +echo "MDEwDQYJYIZIAWUDBAIBBQAEIA==" | base64 -d > padding-pkcs11 +cat padding-pkcs11 update.hash > update.pkcs11-padhash + +# Legacy padding for openssl -raw (non smartcard keys) +# +# The following is a standard PKCS1-v1_5 padding for SHA256 signatures, as +# defined in RFC3447. It is prepended to the actual signature (32 bytes) to +# form a sequence of 256 bytes (2048 bits) that is amenable to RSA signing. The +# padded hash will look as follows: +# +# 0x00 0x01 0xff ... 0xff 0x00 ASN1HEADER SHA256HASH +# |--------------205-----------||----19----||----32----| +# +# where ASN1HEADER is the ASN.1 description of the signed data. The complete 51 +# bytes of actual data (i.e. the ASN.1 header complete with the hash) are +# packed as follows: +# +# SEQUENCE(2+49) { +# SEQUENCE(2+13) { +# OBJECT(2+9) id-sha256 +# NULL(2+0) +# } +# OCTET STRING(2+32) +# } +echo "AAH/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////ADAxMA0GCWCGSAFlAwQCAQUABCA=" | base64 -d > padding cat padding update.hash > update.padhash + i=1 signature_sizes="" for key in "${private_keys[@]}"; do - openssl rsautl -engine pkcs11 -pkcs -sign -inkey ${key} -keyform engine -in update.padhash -out update.sig.${i} + if [[ "${key}" == pkcs11* ]]; then + openssl rsautl -engine pkcs11 -pkcs -sign -inkey ${key} -keyform engine -in update.pkcs11-padhash -out update.sig.${i} + else + openssl rsautl -raw -sign -inkey ${key} -in update.padhash -out update.sig.${i} + fi let "i += 1" done From e5e33866629422a62072d3da67d242ed0e89221c Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Thu, 6 Jul 2017 13:49:14 -0700 Subject: [PATCH 3/5] offline_signing: use a smartcard URI --- offline_signing/sign.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/offline_signing/sign.sh b/offline_signing/sign.sh index 080bd566aa..1f4a621909 100755 --- a/offline_signing/sign.sh +++ b/offline_signing/sign.sh @@ -17,5 +17,5 @@ cd "${DATA_DIR}" --image "${DATA_DIR}/coreos_production_update.bin" \ --kernel "${DATA_DIR}/coreos_production_image.vmlinuz" \ --output "${DATA_DIR}/coreos_production_update.gz" \ - --private_keys "${KEYS_DIR}/devel.key.pem+${KEYS_DIR}/prod-2.key.pem" \ + --private_keys "${KEYS_DIR}/devel.key.pem+pkcs11:object=CoreOS_Update_Signing_Key;type=private" \ --public_keys "${KEYS_DIR}/devel.pub.pem+${KEYS_DIR}/prod-2.pub.pem" From 3d975d7d37fb2b527d418599954f8343164ffe33 Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Tue, 11 Jul 2017 13:55:59 -0700 Subject: [PATCH 4/5] core_sign_update: remain compatible with older sign.sh The motivation behind retaining this backwards compatibility, at least now, is that it's actually non-trivial to revert these code changes for a given release. The `sign.sh` changes can easily be changed, but the `core_sign_update` code is included in the update-specific "au_zip" file. Replacing that is a little more fiddly. Since it's possible we'll still want to revert to the previous signing behavior, make it so the update payload (namely core_sign_update) should work both under the previous `sign.sh` script, and when using the new one. --- core_sign_update | 10 ++++++---- offline_signing/sign.sh | 3 ++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/core_sign_update b/core_sign_update index 6b55d1a0b9..20159cf255 100755 --- a/core_sign_update +++ b/core_sign_update @@ -18,8 +18,9 @@ export GCLIENT_ROOT=$(readlink -f "${SCRIPT_ROOT}/../../") DEFINE_string image "" "The filesystem image of /usr" DEFINE_string kernel "" "The kernel image" DEFINE_string output "" "Output file" -DEFINE_string private_keys "" "Path to private key in .pem format." -DEFINE_string public_keys "" "Path to public key in .pem format." +DEFINE_string private_keys "" "Path or pkcs11 URI to private keys." +DEFINE_string public_keys "" "Path to public keys in .pem format." +DEFINE string keys_separator ":" "Separator for the above keys" # Parse command line FLAGS "$@" || exit 1 @@ -45,8 +46,9 @@ delta_generator \ -new_kernel "$FLAGS_kernel" \ -out_file update -IFS=+ read -a private_keys <<< "$FLAGS_private_keys" -IFS=+ read -a public_keys <<< "$FLAGS_public_keys" +# The separator is configurable for backwards compatibility with old `sign.sh` scripts. +IFS="${keys_separator}" read -a private_keys <<< "$FLAGS_private_keys" +IFS="${keys_separator}" read -a public_keys <<< "$FLAGS_public_keys" if [ ${#private_keys[@]} -ne ${#public_keys[@]} ]; then echo "mismatch in count of private keys and public keys" diff --git a/offline_signing/sign.sh b/offline_signing/sign.sh index 1f4a621909..ecd690a951 100755 --- a/offline_signing/sign.sh +++ b/offline_signing/sign.sh @@ -18,4 +18,5 @@ cd "${DATA_DIR}" --kernel "${DATA_DIR}/coreos_production_image.vmlinuz" \ --output "${DATA_DIR}/coreos_production_update.gz" \ --private_keys "${KEYS_DIR}/devel.key.pem+pkcs11:object=CoreOS_Update_Signing_Key;type=private" \ - --public_keys "${KEYS_DIR}/devel.pub.pem+${KEYS_DIR}/prod-2.pub.pem" + --public_keys "${KEYS_DIR}/devel.pub.pem+${KEYS_DIR}/prod-2.pub.pem" \ + --keys_separator "+" From 42b2dd0ccdbe50e205960e8355a99290b2056f54 Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Tue, 18 Jul 2017 18:02:47 -0700 Subject: [PATCH 5/5] core_sign_update: fix flag parsing for keys_separator Introduced in #710, whoops. --- core_sign_update | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core_sign_update b/core_sign_update index 20159cf255..b2bb40350a 100755 --- a/core_sign_update +++ b/core_sign_update @@ -20,7 +20,7 @@ DEFINE_string kernel "" "The kernel image" DEFINE_string output "" "Output file" DEFINE_string private_keys "" "Path or pkcs11 URI to private keys." DEFINE_string public_keys "" "Path to public keys in .pem format." -DEFINE string keys_separator ":" "Separator for the above keys" +DEFINE_string keys_separator ":" "Separator for the above keys" # Parse command line FLAGS "$@" || exit 1 @@ -47,8 +47,8 @@ delta_generator \ -out_file update # The separator is configurable for backwards compatibility with old `sign.sh` scripts. -IFS="${keys_separator}" read -a private_keys <<< "$FLAGS_private_keys" -IFS="${keys_separator}" read -a public_keys <<< "$FLAGS_public_keys" +IFS="${FLAGS_keys_separator}" read -a private_keys <<< "$FLAGS_private_keys" +IFS="${FLAGS_keys_separator}" read -a public_keys <<< "$FLAGS_public_keys" if [ ${#private_keys[@]} -ne ${#public_keys[@]} ]; then echo "mismatch in count of private keys and public keys"