| Message ID | 20220217183124.303291-2-adrian.freihofer@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [meta-swupdate,1/3] swupdate-image: remove invalid bbclass in inherit | expand |
Hi Adrian, Walter, On 17.02.22 19:31, Adrian Freihofer wrote: > From: Walter Schweizer <walter.schweizer@siemens.com> > > This feature allows to verify a root file system before mounting > it readonly with a signature that is in a environment variable in > U-Boot. The verification is done in an initramfs phase. > The signature is base64 encoded. > > The mechanism to add the signature is similar to adding a sha256 > hash. Instead of "@" the "?" is used in sw-description. > > bootenv: ( > { > name = "rootfs_sig_b"; > value = "?@@ROOTFS_IMAGE@@"; > } > ); > I think I understand the reason and I want to check with you if this is a general and secure solution to be mainlined. The use case should be secure boot, and rootfs should not be mounted if hash does not match. However, it looks to me that using a U-Boot variable is a weak solution. The variable can easy be set, and there is no cryptographic check around U-Boot environment. If an attacker dumps the environment from flash and replaces with hiw own, he could easy get rid of this. Because I think the use case here is Secure Boot, I just explain my way to do I used in previous projects. With secure boot, the BootROM should already have verified the bootloader and we can trust it. Instead of putting the hash into the environment, the hash can be added to the fitImage with the kernel. The big advantage is the fitImage is verified by U-Boot (how this is done, it depends on the SOC, in worst case U-Boot verifies in SW with a public key) and we can trust the fitImage, but we cannot trust the environment. Even setting CONFIG_ENV_FLAGS_LIST_STATIC does not help a lot, because we are not able anymore to update. What do you think ? Best regards, Stefano Babic > Signed-off-by: Walter Schweizer <walter.schweizer@siemens.com> > --- > classes/swupdate-common.bbclass | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/classes/swupdate-common.bbclass b/classes/swupdate-common.bbclass > index d77d9d4..9bcaf01 100644 > --- a/classes/swupdate-common.bbclass > +++ b/classes/swupdate-common.bbclass > @@ -51,6 +51,17 @@ def swupdate_get_sha256(s, filename): > m.update(data) > return m.hexdigest() > > +def swupdate_get_sig(d, s, filename): > + import subprocess > + > + keyfile = d.getVar('SWUPDATE_PRIVATE_KEY') > + passphrase = d.getVar('SWUPDATE_PASSWORD_FILE') > + signcmd = "zcat %s | openssl dgst -sha256 -keyform PEM -sign %s -passin file:%s | base64 -w 0" % \ > + (os.path.join(s, filename), keyfile, passphrase) > + bb.note("running command %s" % (signcmd)) > + signature = subprocess.run(signcmd, shell=True, check=True, capture_output=True) > + return signature.stdout.decode('ascii') > + > def swupdate_extract_keys(keyfile_path): > try: > with open(keyfile_path, 'r') as f: > @@ -93,6 +104,23 @@ def swupdate_write_sha256(s): > for line in write_lines: > f.write(line) > > +def swupdate_write_sig(d, s): > + import re > + write_lines = [] > + with open(os.path.join(s, "sw-description"), 'r') as f: > + for line in f: > + m = re.match(r"^\s+value\s+[=:]\s*(?P<quote>[\'\"])[?](?P<filename>.*)(?P=quote)", line) > + if m: > + filename = m.group('filename') > + hash = swupdate_get_sig(d, s, filename) > + write_lines.append(line.replace("?%s" % (filename), hash)) > + else: > + write_lines.append(line) > + > + with open(os.path.join(s, "sw-description"), 'w+') as f: > + for line in write_lines: > + f.write(line) > + > def swupdate_expand_bitbake_variables(d, s): > write_lines = [] > > @@ -236,6 +264,7 @@ def prepare_sw_description(d): > swupdate_expand_auto_versions(d, s) > > swupdate_write_sha256(s) > + swupdate_write_sig(d, s) > > encrypt = d.getVar('SWUPDATE_ENCRYPT_SWDESC', True) > if encrypt:
Hi Stefano, Yes, the mechanism is for secure-boot. The boot-container including U-Boot is verified by a signature. We use i.MX8 AHAB for this. Then we load the FIT image which *is* part of the rootfs. U-Boot verifies the configuration and starts the kernel with an initramfs. So both are verified by U-Boot. The initramfs then uses the signature in the variable to verify the rootfs. So if the variable is not correctly set, the verification will fail and initramfs will print an error and stop. We do not need to trust the variable, we check. Because the FIT image resides within the rootfs, we cannot store the rootfs signature in it. That is why we selected the solution with the variable. The variable is not just a hash - it is a signature and the initramfs has the public-key. Thanks Walter On Fri, 2022-02-18 at 11:12 +0100, Stefano Babic wrote: > Hi Adrian, Walter, > > On 17.02.22 19:31, Adrian Freihofer wrote: > > From: Walter Schweizer <walter.schweizer@siemens.com> > > > > This feature allows to verify a root file system before mounting > > it readonly with a signature that is in a environment variable in > > U-Boot. The verification is done in an initramfs phase. > > The signature is base64 encoded. > > > > The mechanism to add the signature is similar to adding a sha256 > > hash. Instead of "@" the "?" is used in sw-description. > > > > bootenv: ( > > { > > name = "rootfs_sig_b"; > > value = "?@@ROOTFS_IMAGE@@"; > > } > > ); > > > > I think I understand the reason and I want to check with you if this > is > a general and secure solution to be mainlined. The use case should be > secure boot, and rootfs should not be mounted if hash does not match. > > However, it looks to me that using a U-Boot variable is a weak > solution. > The variable can easy be set, and there is no cryptographic check > around > U-Boot environment. If an attacker dumps the environment from flash > and > replaces with hiw own, he could easy get rid of this. > > Because I think the use case here is Secure Boot, I just explain my > way > to do I used in previous projects. With secure boot, the BootROM > should > already have verified the bootloader and we can trust it. Instead of > putting the hash into the environment, the hash can be added to the > fitImage with the kernel. The big advantage is the fitImage is > verified > by U-Boot (how this is done, it depends on the SOC, in worst case U- > Boot > verifies in SW with a public key) and we can trust the fitImage, but > we > cannot trust the environment. > > Even setting CONFIG_ENV_FLAGS_LIST_STATIC does not help a lot, > because > we are not able anymore to update. > > What do you think ? > > Best regards, > Stefano Babic > > > Signed-off-by: Walter Schweizer <walter.schweizer@siemens.com> > > --- > > classes/swupdate-common.bbclass | 29 > > +++++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/classes/swupdate-common.bbclass b/classes/swupdate- > > common.bbclass > > index d77d9d4..9bcaf01 100644 > > --- a/classes/swupdate-common.bbclass > > +++ b/classes/swupdate-common.bbclass > > @@ -51,6 +51,17 @@ def swupdate_get_sha256(s, filename): > > m.update(data) > > return m.hexdigest() > > > > +def swupdate_get_sig(d, s, filename): > > + import subprocess > > + > > + keyfile = d.getVar('SWUPDATE_PRIVATE_KEY') > > + passphrase = d.getVar('SWUPDATE_PASSWORD_FILE') > > + signcmd = "zcat %s | openssl dgst -sha256 -keyform PEM -sign > > %s -passin file:%s | base64 -w 0" % \ > > + (os.path.join(s, filename), keyfile, passphrase) > > + bb.note("running command %s" % (signcmd)) > > + signature = subprocess.run(signcmd, shell=True, check=True, > > capture_output=True) > > + return signature.stdout.decode('ascii') > > + > > def swupdate_extract_keys(keyfile_path): > > try: > > with open(keyfile_path, 'r') as f: > > @@ -93,6 +104,23 @@ def swupdate_write_sha256(s): > > for line in write_lines: > > f.write(line) > > > > +def swupdate_write_sig(d, s): > > + import re > > + write_lines = [] > > + with open(os.path.join(s, "sw-description"), 'r') as f: > > + for line in f: > > + m = > > re.match(r"^\s+value\s+[=:]\s*(?P<quote>[\'\"])[?](?P<filename>.*)( > > ?P=quote)", line) > > + if m: > > + filename = m.group('filename') > > + hash = swupdate_get_sig(d, s, filename) > > + write_lines.append(line.replace("?%s" % (filename), > > hash)) > > + else: > > + write_lines.append(line) > > + > > + with open(os.path.join(s, "sw-description"), 'w+') as f: > > + for line in write_lines: > > + f.write(line) > > + > > def swupdate_expand_bitbake_variables(d, s): > > write_lines = [] > > > > @@ -236,6 +264,7 @@ def prepare_sw_description(d): > > swupdate_expand_auto_versions(d, s) > > > > swupdate_write_sha256(s) > > + swupdate_write_sig(d, s) > > > > encrypt = d.getVar('SWUPDATE_ENCRYPT_SWDESC', True) > > if encrypt: > >
Hi Walter, On 18.02.22 14:28, Schweizer, Walter wrote: > Hi Stefano, > > Yes, the mechanism is for secure-boot. > > The boot-container including U-Boot is verified by a signature. We use > i.MX8 AHAB for this. > Ok, fine. > Then we load the FIT image which *is* part of the rootfs. U-Boot > verifies the configuration and starts the kernel with an initramfs. Got it so U-Boot is verified by AHAB. rootfs is at the moment untrusted, you load a fitImage ==> this is still ok IMHO. But why is fitImage not verified by U-Boot, independently if this is done by AHAB or by U-Boot in code ? Or is it verified by AHAB ? > So > both are verified by U-Boot. Ok - so you mean that fitImage is verified by U-Boot, right ? > The initramfs then uses the signature in > the variable to verify the rootfs. So you are verifying rootfs with a signature (urrg...then we have a 256bytes long variable in U-Boot) via openSSL. > So if the variable is not correctly > set, the verification will fail and initramfs will print an error and > stop. We do not need to trust the variable, we check. Ok - my concern is if an attacker can work around ths, but this is possible only if the fitImage is not authenticated. > > Because the FIT image resides within the rootfs, we cannot store the > rootfs signature in it. Ok, this is the point (chicken-egg problem). > That is why we selected the solution with the > variable. The variable is not just a hash - it is a signature and the > initramfs has the public-key. Ok, got it Regards, Stefano > > Thanks > Walter > > On Fri, 2022-02-18 at 11:12 +0100, Stefano Babic wrote: >> Hi Adrian, Walter, >> >> On 17.02.22 19:31, Adrian Freihofer wrote: >>> From: Walter Schweizer <walter.schweizer@siemens.com> >>> >>> This feature allows to verify a root file system before mounting >>> it readonly with a signature that is in a environment variable in >>> U-Boot. The verification is done in an initramfs phase. >>> The signature is base64 encoded. >>> >>> The mechanism to add the signature is similar to adding a sha256 >>> hash. Instead of "@" the "?" is used in sw-description. >>> >>> bootenv: ( >>> { >>> name = "rootfs_sig_b"; >>> value = "?@@ROOTFS_IMAGE@@"; >>> } >>> ); >>> >> >> I think I understand the reason and I want to check with you if this >> is >> a general and secure solution to be mainlined. The use case should be >> secure boot, and rootfs should not be mounted if hash does not match. >> >> However, it looks to me that using a U-Boot variable is a weak >> solution. >> The variable can easy be set, and there is no cryptographic check >> around >> U-Boot environment. If an attacker dumps the environment from flash >> and >> replaces with hiw own, he could easy get rid of this. >> >> Because I think the use case here is Secure Boot, I just explain my >> way >> to do I used in previous projects. With secure boot, the BootROM >> should >> already have verified the bootloader and we can trust it. Instead of >> putting the hash into the environment, the hash can be added to the >> fitImage with the kernel. The big advantage is the fitImage is >> verified >> by U-Boot (how this is done, it depends on the SOC, in worst case U- >> Boot >> verifies in SW with a public key) and we can trust the fitImage, but >> we >> cannot trust the environment. >> >> Even setting CONFIG_ENV_FLAGS_LIST_STATIC does not help a lot, >> because >> we are not able anymore to update. >> >> What do you think ? >> >> Best regards, >> Stefano Babic >> >>> Signed-off-by: Walter Schweizer <walter.schweizer@siemens.com> >>> --- >>> classes/swupdate-common.bbclass | 29 >>> +++++++++++++++++++++++++++++ >>> 1 file changed, 29 insertions(+) >>> >>> diff --git a/classes/swupdate-common.bbclass b/classes/swupdate- >>> common.bbclass >>> index d77d9d4..9bcaf01 100644 >>> --- a/classes/swupdate-common.bbclass >>> +++ b/classes/swupdate-common.bbclass >>> @@ -51,6 +51,17 @@ def swupdate_get_sha256(s, filename): >>> m.update(data) >>> return m.hexdigest() >>> >>> +def swupdate_get_sig(d, s, filename): >>> + import subprocess >>> + >>> + keyfile = d.getVar('SWUPDATE_PRIVATE_KEY') >>> + passphrase = d.getVar('SWUPDATE_PASSWORD_FILE') >>> + signcmd = "zcat %s | openssl dgst -sha256 -keyform PEM -sign >>> %s -passin file:%s | base64 -w 0" % \ >>> + (os.path.join(s, filename), keyfile, passphrase) >>> + bb.note("running command %s" % (signcmd)) >>> + signature = subprocess.run(signcmd, shell=True, check=True, >>> capture_output=True) >>> + return signature.stdout.decode('ascii') >>> + >>> def swupdate_extract_keys(keyfile_path): >>> try: >>> with open(keyfile_path, 'r') as f: >>> @@ -93,6 +104,23 @@ def swupdate_write_sha256(s): >>> for line in write_lines: >>> f.write(line) >>> >>> +def swupdate_write_sig(d, s): >>> + import re >>> + write_lines = [] >>> + with open(os.path.join(s, "sw-description"), 'r') as f: >>> + for line in f: >>> + m = >>> re.match(r"^\s+value\s+[=:]\s*(?P<quote>[\'\"])[?](?P<filename>.*)( >>> ?P=quote)", line) >>> + if m: >>> + filename = m.group('filename') >>> + hash = swupdate_get_sig(d, s, filename) >>> + write_lines.append(line.replace("?%s" % (filename), >>> hash)) >>> + else: >>> + write_lines.append(line) >>> + >>> + with open(os.path.join(s, "sw-description"), 'w+') as f: >>> + for line in write_lines: >>> + f.write(line) >>> + >>> def swupdate_expand_bitbake_variables(d, s): >>> write_lines = [] >>> >>> @@ -236,6 +264,7 @@ def prepare_sw_description(d): >>> swupdate_expand_auto_versions(d, s) >>> >>> swupdate_write_sha256(s) >>> + swupdate_write_sig(d, s) >>> >>> encrypt = d.getVar('SWUPDATE_ENCRYPT_SWDESC', True) >>> if encrypt: >> >> >
Hi Stefano Thanks for your reply. See below. Best Regards Walter On Fri, 2022-02-18 at 15:51 +0100, Stefano Babic wrote: > Hi Walter, > > On 18.02.22 14:28, Schweizer, Walter wrote: > > Hi Stefano, > > > > Yes, the mechanism is for secure-boot. > > > > The boot-container including U-Boot is verified by a signature. We > > use > > i.MX8 AHAB for this. > > > > Ok, fine. > > > Then we load the FIT image which *is* part of the rootfs. U-Boot > > verifies the configuration and starts the kernel with an initramfs. > > Got it so U-Boot is verified by AHAB. rootfs is at the moment > untrusted, you load a fitImage ==> this is still ok IMHO. But why is > fitImage not verified by U-Boot, independently if this is done by > AHAB > or by U-Boot in code ? Or is it verified by AHAB ? > > > So > > both are verified by U-Boot. > > Ok - so you mean that fitImage is verified by U-Boot, right ? Yes, the fitImage is verified by U-Boot. Therefore the kernel and the bundled initramfs are verified. > > > The initramfs then uses the signature in > > the variable to verify the rootfs. > > So you are verifying rootfs with a signature (urrg...then we have a > 256bytes long variable in U-Boot) via openSSL. > > Yes. Works so far well. > > So if the variable is not correctly > > set, the verification will fail and initramfs will print an error > > and > > stop. We do not need to trust the variable, we check. > > Ok - my concern is if an attacker can work around ths, but this is > possible only if the fitImage is not authenticated. > I turned off legacy image support. > > > > Because the FIT image resides within the rootfs, we cannot store > > the > > rootfs signature in it. > > Ok, this is the point (chicken-egg problem). Yes, exactly. > > > That is why we selected the solution with the > > variable. The variable is not just a hash - it is a signature and > > the > > initramfs has the public-key. > > Ok, got it > > Regards, > Stefano > > > > > Thanks > > Walter > > > > On Fri, 2022-02-18 at 11:12 +0100, Stefano Babic wrote: > > > Hi Adrian, Walter, > > > > > > On 17.02.22 19:31, Adrian Freihofer wrote: > > > > From: Walter Schweizer <walter.schweizer@siemens.com> > > > > > > > > This feature allows to verify a root file system before > > > > mounting > > > > it readonly with a signature that is in a environment variable > > > > in > > > > U-Boot. The verification is done in an initramfs phase. > > > > The signature is base64 encoded. > > > > > > > > The mechanism to add the signature is similar to adding a > > > > sha256 > > > > hash. Instead of "@" the "?" is used in sw-description. > > > > > > > > bootenv: ( > > > > { > > > > name = "rootfs_sig_b"; > > > > value = "?@@ROOTFS_IMAGE@@"; > > > > } > > > > ); > > > > > > > > > > I think I understand the reason and I want to check with you if > > > this > > > is > > > a general and secure solution to be mainlined. The use case > > > should be > > > secure boot, and rootfs should not be mounted if hash does not > > > match. > > > > > > However, it looks to me that using a U-Boot variable is a weak > > > solution. > > > The variable can easy be set, and there is no cryptographic check > > > around > > > U-Boot environment. If an attacker dumps the environment from > > > flash > > > and > > > replaces with hiw own, he could easy get rid of this. > > > > > > Because I think the use case here is Secure Boot, I just explain > > > my > > > way > > > to do I used in previous projects. With secure boot, the BootROM > > > should > > > already have verified the bootloader and we can trust it. Instead > > > of > > > putting the hash into the environment, the hash can be added to > > > the > > > fitImage with the kernel. The big advantage is the fitImage is > > > verified > > > by U-Boot (how this is done, it depends on the SOC, in worst case > > > U- > > > Boot > > > verifies in SW with a public key) and we can trust the fitImage, > > > but > > > we > > > cannot trust the environment. > > > > > > Even setting CONFIG_ENV_FLAGS_LIST_STATIC does not help a lot, > > > because > > > we are not able anymore to update. > > > > > > What do you think ? > > > > > > Best regards, > > > Stefano Babic > > > > > > > Signed-off-by: Walter Schweizer <walter.schweizer@siemens.com> > > > > --- > > > > classes/swupdate-common.bbclass | 29 > > > > +++++++++++++++++++++++++++++ > > > > 1 file changed, 29 insertions(+) > > > > > > > > diff --git a/classes/swupdate-common.bbclass > > > > b/classes/swupdate- > > > > common.bbclass > > > > index d77d9d4..9bcaf01 100644 > > > > --- a/classes/swupdate-common.bbclass > > > > +++ b/classes/swupdate-common.bbclass > > > > @@ -51,6 +51,17 @@ def swupdate_get_sha256(s, filename): > > > > m.update(data) > > > > return m.hexdigest() > > > > > > > > +def swupdate_get_sig(d, s, filename): > > > > + import subprocess > > > > + > > > > + keyfile = d.getVar('SWUPDATE_PRIVATE_KEY') > > > > + passphrase = d.getVar('SWUPDATE_PASSWORD_FILE') > > > > + signcmd = "zcat %s | openssl dgst -sha256 -keyform PEM - > > > > sign > > > > %s -passin file:%s | base64 -w 0" % \ > > > > + (os.path.join(s, filename), keyfile, passphrase) > > > > + bb.note("running command %s" % (signcmd)) > > > > + signature = subprocess.run(signcmd, shell=True, > > > > check=True, > > > > capture_output=True) > > > > + return signature.stdout.decode('ascii') > > > > + > > > > def swupdate_extract_keys(keyfile_path): > > > > try: > > > > with open(keyfile_path, 'r') as f: > > > > @@ -93,6 +104,23 @@ def swupdate_write_sha256(s): > > > > for line in write_lines: > > > > f.write(line) > > > > > > > > +def swupdate_write_sig(d, s): > > > > + import re > > > > + write_lines = [] > > > > + with open(os.path.join(s, "sw-description"), 'r') as f: > > > > + for line in f: > > > > + m = > > > > re.match(r"^\s+value\s+[=:]\s*(?P<quote>[\'\"])[?](?P<filename> > > > > .*)( > > > > ?P=quote)", line) > > > > + if m: > > > > + filename = m.group('filename') > > > > + hash = swupdate_get_sig(d, s, filename) > > > > + write_lines.append(line.replace("?%s" % > > > > (filename), > > > > hash)) > > > > + else: > > > > + write_lines.append(line) > > > > + > > > > + with open(os.path.join(s, "sw-description"), 'w+') as f: > > > > + for line in write_lines: > > > > + f.write(line) > > > > + > > > > def swupdate_expand_bitbake_variables(d, s): > > > > write_lines = [] > > > > > > > > @@ -236,6 +264,7 @@ def prepare_sw_description(d): > > > > swupdate_expand_auto_versions(d, s) > > > > > > > > swupdate_write_sha256(s) > > > > + swupdate_write_sig(d, s) > > > > > > > > encrypt = d.getVar('SWUPDATE_ENCRYPT_SWDESC', True) > > > > if encrypt: > > > > > > > > > >
Hi Walter, On 17.02.22 19:31, Adrian Freihofer wrote: > From: Walter Schweizer <walter.schweizer@siemens.com> > > This feature allows to verify a root file system before mounting > it readonly with a signature that is in a environment variable in > U-Boot. The verification is done in an initramfs phase. > The signature is base64 encoded. > > The mechanism to add the signature is similar to adding a sha256 > hash. Instead of "@" the "?" is used in sw-description. > > bootenv: ( > { > name = "rootfs_sig_b"; > value = "?@@ROOTFS_IMAGE@@"; > } > ); > This just increase the chaos in swupdate-common, adding characters that should hide a special meaning. It is not far away for a previous post, see this thread, too: https://groups.google.com/g/swupdate/c/cJtvPJUsIxU/m/P8ywiyn0FQAJ Or the mainlined Auto version. Even in that case, a special pattern was proposed. I am sure that this design does not scale at all, and more and more functions will be added in future, some with general purpose and some to solve a project specific issue. And we have already some special case like sha256, or @SWUPDATE_AUTO_VERSION. It is clear to me thaat something else should be done. My approach is to have a wayto call functions defined in sw-description, so that the change above could be written as: bootenv: ( { name = "rootfs_sig_b"; value = "$swupdate_sign(@@ROOTFS_IMAGE@@)"; } ); And we have just to check for "$" to exec a function, like we have in recipe for example with "$bb.utils...". The parsing won't be change in future and it is enough to add the functions, that can be collected together ($swupdate_het_sha256, $swupdate_size, $swupdate_ivt...). > Signed-off-by: Walter Schweizer <walter.schweizer@siemens.com> > --- > classes/swupdate-common.bbclass | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/classes/swupdate-common.bbclass b/classes/swupdate-common.bbclass > index d77d9d4..9bcaf01 100644 > --- a/classes/swupdate-common.bbclass > +++ b/classes/swupdate-common.bbclass > @@ -51,6 +51,17 @@ def swupdate_get_sha256(s, filename): > m.update(data) > return m.hexdigest() > > +def swupdate_get_sig(d, s, filename): > + import subprocess > + > + keyfile = d.getVar('SWUPDATE_PRIVATE_KEY') > + passphrase = d.getVar('SWUPDATE_PASSWORD_FILE') > + signcmd = "zcat %s | openssl dgst -sha256 -keyform PEM -sign %s -passin file:%s | base64 -w 0" % \ > + (os.path.join(s, filename), keyfile, passphrase) This does not work - we can have uncompressed artifact or zstd as compressor. > + bb.note("running command %s" % (signcmd)) > + signature = subprocess.run(signcmd, shell=True, check=True, capture_output=True) > + return signature.stdout.decode('ascii') > + > def swupdate_extract_keys(keyfile_path): > try: > with open(keyfile_path, 'r') as f: > @@ -93,6 +104,23 @@ def swupdate_write_sha256(s): > for line in write_lines: > f.write(line) > > +def swupdate_write_sig(d, s): > + import re > + write_lines = [] > + with open(os.path.join(s, "sw-description"), 'r') as f: > + for line in f: > + m = re.match(r"^\s+value\s+[=:]\s*(?P<quote>[\'\"])[?](?P<filename>.*)(?P=quote)", line) > + if m: > + filename = m.group('filename') > + hash = swupdate_get_sig(d, s, filename) > + write_lines.append(line.replace("?%s" % (filename), hash)) > + else: > + write_lines.append(line) > + > + with open(os.path.join(s, "sw-description"), 'w+') as f: > + for line in write_lines: > + f.write(line) > + This is also a copy of already copied code in swupdate-common, and this tells us we need to refactor. sw-description is read / rewritten many times, and it is sure we can do better. > def swupdate_expand_bitbake_variables(d, s): > write_lines = [] > > @@ -236,6 +264,7 @@ def prepare_sw_description(d): > swupdate_expand_auto_versions(d, s) > > swupdate_write_sha256(s) > + swupdate_write_sig(d, s) > > encrypt = d.getVar('SWUPDATE_ENCRYPT_SWDESC', True) > if encrypt: Best regards, Stefano
Hi Stefano, On Sun, 2022-02-20 at 12:28 +0100, Stefano Babic wrote: > Hi Walter, > > On 17.02.22 19:31, Adrian Freihofer wrote: > > From: Walter Schweizer <walter.schweizer@siemens.com> > > > > This feature allows to verify a root file system before mounting > > it readonly with a signature that is in a environment variable in > > U-Boot. The verification is done in an initramfs phase. > > The signature is base64 encoded. > > > > The mechanism to add the signature is similar to adding a sha256 > > hash. Instead of "@" the "?" is used in sw-description. > > > > bootenv: ( > > { > > name = "rootfs_sig_b"; > > value = "?@@ROOTFS_IMAGE@@"; > > } > > ); > > > > This just increase the chaos in swupdate-common, adding characters > that > should hide a special meaning. It is not far away for a previous > post, > see this thread, too: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fg%2Fswupdate%2Fc%2FcJtvPJUsIxU%2Fm%2FP8ywiyn0FQAJ&data=04%7C01%7Cwalter.schweizer%40siemens.com%7C885aab6f44a5452c59cc08d9f4642137%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637809533159236598%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=NbcxzOvkS6aB573GQnuZbxIHiKGoeBXdrqNhSE5K9YM%3D&reserved=0 > > Or the mainlined Auto version. > > Even in that case, a special pattern was proposed. I am sure that > this > design does not scale at all, and more and more functions will be > added > in future, some with general purpose and some to solve a project > specific issue. And we have already some special case like sha256, or > @SWUPDATE_AUTO_VERSION. It is clear to me thaat something else should > be > done. > > My approach is to have a wayto call functions defined in sw- > description, > so that the change above could be written as: > > bootenv: ( > { > name = "rootfs_sig_b"; > value = "$swupdate_sign(@@ROOTFS_IMAGE@@)"; > } > ); > > And we have just to check for "$" to exec a function, like we have in > recipe for example with "$bb.utils...". The parsing won't be change > in > future and it is enough to add the functions, that can be collected > together ($swupdate_het_sha256, $swupdate_size, $swupdate_ivt...). I see your point. > > > > Signed-off-by: Walter Schweizer <walter.schweizer@siemens.com> > > --- > > classes/swupdate-common.bbclass | 29 > > +++++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/classes/swupdate-common.bbclass b/classes/swupdate- > > common.bbclass > > index d77d9d4..9bcaf01 100644 > > --- a/classes/swupdate-common.bbclass > > +++ b/classes/swupdate-common.bbclass > > @@ -51,6 +51,17 @@ def swupdate_get_sha256(s, filename): > > m.update(data) > > return m.hexdigest() > > > > +def swupdate_get_sig(d, s, filename): > > + import subprocess > > + > > + keyfile = d.getVar('SWUPDATE_PRIVATE_KEY') > > + passphrase = d.getVar('SWUPDATE_PASSWORD_FILE') > > + signcmd = "zcat %s | openssl dgst -sha256 -keyform PEM -sign > > %s -passin file:%s | base64 -w 0" % \ > > + (os.path.join(s, filename), keyfile, passphrase) > > This does not work - we can have uncompressed artifact or zstd as > compressor. Yes, I could distinguish between functions: $swupdate_sign() $swupdate_sign_zlib() $swupdate_sign_zstd() The other possibility would be to remember the last compressed string and deal in swupdate_sign(). I would not support for the boolean compressed variant. > > > + bb.note("running command %s" % (signcmd)) > > + signature = subprocess.run(signcmd, shell=True, check=True, > > capture_output=True) > > + return signature.stdout.decode('ascii') > > + > > def swupdate_extract_keys(keyfile_path): > > try: > > with open(keyfile_path, 'r') as f: > > @@ -93,6 +104,23 @@ def swupdate_write_sha256(s): > > for line in write_lines: > > f.write(line) > > > > +def swupdate_write_sig(d, s): > > + import re > > + write_lines = [] > > + with open(os.path.join(s, "sw-description"), 'r') as f: > > + for line in f: > > + m = > > re.match(r"^\s+value\s+[=:]\s*(?P<quote>[\'\"])[?](?P<filename>.*)( > > ?P=quote)", line) > > + if m: > > + filename = m.group('filename') > > + hash = swupdate_get_sig(d, s, filename) > > + write_lines.append(line.replace("?%s" % (filename), > > hash)) > > + else: > > + write_lines.append(line) > > + > > + with open(os.path.join(s, "sw-description"), 'w+') as f: > > + for line in write_lines: > > + f.write(line) > > + > > This is also a copy of already copied code in swupdate-common, and > this > tells us we need to refactor. sw-description is read / rewritten many > times, and it is sure we can do better. Agreed. > > > def swupdate_expand_bitbake_variables(d, s): > > write_lines = [] > > > > @@ -236,6 +264,7 @@ def prepare_sw_description(d): > > swupdate_expand_auto_versions(d, s) > > > > swupdate_write_sha256(s) > > + swupdate_write_sig(d, s) > > > > encrypt = d.getVar('SWUPDATE_ENCRYPT_SWDESC', True) > > if encrypt: > > Best regards, > Stefano > Best Regards Walter
Hi Walter, On 25.02.22 11:58, Schweizer, Walter wrote: > Hi Stefano, > > On Sun, 2022-02-20 at 12:28 +0100, Stefano Babic wrote: >> Hi Walter, >> >> On 17.02.22 19:31, Adrian Freihofer wrote: >>> From: Walter Schweizer <walter.schweizer@siemens.com> >>> >>> This feature allows to verify a root file system before mounting >>> it readonly with a signature that is in a environment variable in >>> U-Boot. The verification is done in an initramfs phase. >>> The signature is base64 encoded. >>> >>> The mechanism to add the signature is similar to adding a sha256 >>> hash. Instead of "@" the "?" is used in sw-description. >>> >>> bootenv: ( >>> { >>> name = "rootfs_sig_b"; >>> value = "?@@ROOTFS_IMAGE@@"; >>> } >>> ); >>> >> >> This just increase the chaos in swupdate-common, adding characters >> that >> should hide a special meaning. It is not far away for a previous >> post, >> see this thread, too: >> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fg%2Fswupdate%2Fc%2FcJtvPJUsIxU%2Fm%2FP8ywiyn0FQAJ&data=04%7C01%7Cwalter.schweizer%40siemens.com%7C885aab6f44a5452c59cc08d9f4642137%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637809533159236598%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=NbcxzOvkS6aB573GQnuZbxIHiKGoeBXdrqNhSE5K9YM%3D&reserved=0 >> >> Or the mainlined Auto version. >> >> Even in that case, a special pattern was proposed. I am sure that >> this >> design does not scale at all, and more and more functions will be >> added >> in future, some with general purpose and some to solve a project >> specific issue. And we have already some special case like sha256, or >> @SWUPDATE_AUTO_VERSION. It is clear to me thaat something else should >> be >> done. >> >> My approach is to have a wayto call functions defined in sw- >> description, >> so that the change above could be written as: >> >> bootenv: ( >> { >> name = "rootfs_sig_b"; >> value = "$swupdate_sign(@@ROOTFS_IMAGE@@)"; >> } >> ); >> >> And we have just to check for "$" to exec a function, like we have in >> recipe for example with "$bb.utils...". The parsing won't be change >> in >> future and it is enough to add the functions, that can be collected >> together ($swupdate_het_sha256, $swupdate_size, $swupdate_ivt...). > > I see your point. > >> >> >>> Signed-off-by: Walter Schweizer <walter.schweizer@siemens.com> >>> --- >>> classes/swupdate-common.bbclass | 29 >>> +++++++++++++++++++++++++++++ >>> 1 file changed, 29 insertions(+) >>> >>> diff --git a/classes/swupdate-common.bbclass b/classes/swupdate- >>> common.bbclass >>> index d77d9d4..9bcaf01 100644 >>> --- a/classes/swupdate-common.bbclass >>> +++ b/classes/swupdate-common.bbclass >>> @@ -51,6 +51,17 @@ def swupdate_get_sha256(s, filename): >>> m.update(data) >>> return m.hexdigest() >>> >>> +def swupdate_get_sig(d, s, filename): >>> + import subprocess >>> + >>> + keyfile = d.getVar('SWUPDATE_PRIVATE_KEY') >>> + passphrase = d.getVar('SWUPDATE_PASSWORD_FILE') >>> + signcmd = "zcat %s | openssl dgst -sha256 -keyform PEM -sign >>> %s -passin file:%s | base64 -w 0" % \ >>> + (os.path.join(s, filename), keyfile, passphrase) >> >> This does not work - we can have uncompressed artifact or zstd as >> compressor. > > Yes, I could distinguish between functions: > $swupdate_sign() > $swupdate_sign_zlib() > $swupdate_sign_zstd() > > The other possibility would be to remember the last compressed string > and deal in swupdate_sign(). But this does not work if the artifact was already compressed by the image recipe, let's say with IMAGE_FSTYPES:append = " zst". Another way is to detect the type of compression, like running "file -L " on the artifact and detect which is the compression type (zst,zlib, none). > > I would not support for the boolean compressed variant. > >> >>> + bb.note("running command %s" % (signcmd)) >>> + signature = subprocess.run(signcmd, shell=True, check=True, >>> capture_output=True) >>> + return signature.stdout.decode('ascii') >>> + >>> def swupdate_extract_keys(keyfile_path): >>> try: >>> with open(keyfile_path, 'r') as f: >>> @@ -93,6 +104,23 @@ def swupdate_write_sha256(s): >>> for line in write_lines: >>> f.write(line) >>> >>> +def swupdate_write_sig(d, s): >>> + import re >>> + write_lines = [] >>> + with open(os.path.join(s, "sw-description"), 'r') as f: >>> + for line in f: >>> + m = >>> re.match(r"^\s+value\s+[=:]\s*(?P<quote>[\'\"])[?](?P<filename>.*)( >>> ?P=quote)", line) >>> + if m: >>> + filename = m.group('filename') >>> + hash = swupdate_get_sig(d, s, filename) >>> + write_lines.append(line.replace("?%s" % (filename), >>> hash)) >>> + else: >>> + write_lines.append(line) >>> + >>> + with open(os.path.join(s, "sw-description"), 'w+') as f: >>> + for line in write_lines: >>> + f.write(line) >>> + >> >> This is also a copy of already copied code in swupdate-common, and >> this >> tells us we need to refactor. sw-description is read / rewritten many >> times, and it is sure we can do better. > > Agreed. > >> >>> def swupdate_expand_bitbake_variables(d, s): >>> write_lines = [] >>> >>> @@ -236,6 +264,7 @@ def prepare_sw_description(d): >>> swupdate_expand_auto_versions(d, s) >>> >>> swupdate_write_sha256(s) >>> + swupdate_write_sig(d, s) >>> >>> encrypt = d.getVar('SWUPDATE_ENCRYPT_SWDESC', True) >>> if encrypt: >> >> Best regards, >> Stefano >> > Best Regards > Walter > Best regards, Stefano
diff --git a/classes/swupdate-common.bbclass b/classes/swupdate-common.bbclass index d77d9d4..9bcaf01 100644 --- a/classes/swupdate-common.bbclass +++ b/classes/swupdate-common.bbclass @@ -51,6 +51,17 @@ def swupdate_get_sha256(s, filename): m.update(data) return m.hexdigest() +def swupdate_get_sig(d, s, filename): + import subprocess + + keyfile = d.getVar('SWUPDATE_PRIVATE_KEY') + passphrase = d.getVar('SWUPDATE_PASSWORD_FILE') + signcmd = "zcat %s | openssl dgst -sha256 -keyform PEM -sign %s -passin file:%s | base64 -w 0" % \ + (os.path.join(s, filename), keyfile, passphrase) + bb.note("running command %s" % (signcmd)) + signature = subprocess.run(signcmd, shell=True, check=True, capture_output=True) + return signature.stdout.decode('ascii') + def swupdate_extract_keys(keyfile_path): try: with open(keyfile_path, 'r') as f: @@ -93,6 +104,23 @@ def swupdate_write_sha256(s): for line in write_lines: f.write(line) +def swupdate_write_sig(d, s): + import re + write_lines = [] + with open(os.path.join(s, "sw-description"), 'r') as f: + for line in f: + m = re.match(r"^\s+value\s+[=:]\s*(?P<quote>[\'\"])[?](?P<filename>.*)(?P=quote)", line) + if m: + filename = m.group('filename') + hash = swupdate_get_sig(d, s, filename) + write_lines.append(line.replace("?%s" % (filename), hash)) + else: + write_lines.append(line) + + with open(os.path.join(s, "sw-description"), 'w+') as f: + for line in write_lines: + f.write(line) + def swupdate_expand_bitbake_variables(d, s): write_lines = [] @@ -236,6 +264,7 @@ def prepare_sw_description(d): swupdate_expand_auto_versions(d, s) swupdate_write_sha256(s) + swupdate_write_sig(d, s) encrypt = d.getVar('SWUPDATE_ENCRYPT_SWDESC', True) if encrypt: