Hi,

On Fri, 2020-01-24 at 18:39 +0000, Raghupathy Krishnamurthy via TF-A wrote:

Here are my concerns:

1. The patch set claims to implement R060_TBBR_FUNCTION from the TBBR

spec. I don't agree with this. The patch is using authenticated encryption(which does provide confidentiality) but is different from implementing firmware encryption as described in the TBBR(and PSA) in my view. If we are implementing R060_TBBR_FUNCTION, in my view, this should be implemented such that the firmware is first encrypted(symmetric), and the encrypted firmware should be asymmetrically signed(as per PSA). Also, the order for verification should be asymmetric signature verification and then decryption(symmetric). In this patch, plain text firmware is asymmetrically signed, plain text firmware is encrypted and authenticated with a symmetric key using authenticated encryption. The order for verification in this patch is authenticate and decrypt using the symmetric key and THEN asymmetric signature verification on the decryted(and now plain text) firmware. The ordering of verification and decryption are subtly different from what the spec(TBBR and PSA) expects. Security does not *seem* to be broken as far as i can tell, but the patch is not strictly an implementation of the spec, as it claims. This can throw off people reading the spec and trying to match code to spec. This is not good when someone wants to audit and gain confidence in the security of the implementation. At a minimum, we need someone writing the spec to address this.

I do not have a strong opinion on this one. As I mentioned at the start of this email thread, I am quite new to the concept of authenticated decryption and naively thought this would not differ too much from using decryption + signature verification separately. However, if these are fundamentally different things like you say, I get your point and I too am worried about the confusion this could create.

2. The TBBR spec does not talk about authenticated encryption of

firmware and only talks about encryption of firmware. It also does not specify if the encrypted firmware needs to be signed or the plain text need to be signed. This is critical detail. It also does not specify the order in which encryption and signing, decryption and verification must be done. These are important details and need to be addressed explicitly(perhaps in both TBBR and PSA). Use of authenticated encryption needs to be explicitly discussed as well.

TBBR is quite an old specification and it is unlikely that it will ever be updated. PSA-TBFU is meant to supersede it so I think any update/clarification/addition would only apply to TBFU.

I agree that it would be good for PSA-TBFU to provide some guidance around encryption and signature together, in which order they should be done and what security properties they bring together. I can raise this to the PSA architects.

3. TBBR specifies that the TOC(table of contents) *may* be

authenticated. The TOC contains bits that dictate security policy, in this case, if the firmware is encrypted or not. I don't think it is good to consume security policy from unauthenticated data. In my view, this information must be in the signed image manifest(certificate in ATF) since an attacker can at-least cause simple DoS attacks by flipping bits in the FIP header, undetected. If the bit is flipped, a firmware image going out to a million devices may not boot, since an attacker decided to flip a bit, and signature verification fails, since the boot loader decided not decrypt the firmware based in an unsigned bit of information. PSA appears to have a better approach and makes no provision such data to be outside the image manifest, which is always signed.

I share this concern. I was actually surprised to see that the TBBR specification advocates putting this security policy bit in the unencrypted part of the FIP, I do not know the rationale for that.

Given that TBBR will be deprecated in the future, I wonder whether it would be better to adopt PSA's approach here and move this security policy bit in the signed part of the FIP. This will make this part of the implementation non-TBBR compliant but again, I don't see that as a big concern, as we should look at PSA-TBFU as the future and not worry too much about TBBR if we have good reasons to diverge.

4. This last one is a matter of opinion and can live with the current

design. The FIP layer is now aware of the crypto module. FIP has also become coupled with authenticated encryption(If we do this, why not have the FIP layer call auth_mod_verify_img() and why stop at cyrpto_mod_auth_decrypt()) . FIP is security aware. I'd like to keep things the way they are today, where IO and security are separate modules and glued together by another layer(load_auth_image() for example). This is cleaner in my view since security policy(should i decrypt? should i verify signatures? should i apply any security at all?) is separate from IO.

I agree with you. I too found it more elegant and cleaner to have the FIP layer and crypto layer as 2 independent modules. I too feel slightly uncomfortable about introducing such a dependency. But I could not think of a way to avoid it while keeping the security policy bit in the FIP ToC header. That said, if the latter is something we're considering to redesign then we might be able to remove this dependency and keep the design as it is.

Regards, Sandrine