ARM-software / tf-issues

Issue tracking for the ARM Trusted Firmware project
37 stars 16 forks source link

Add runtime support of trusted_board_boot #381

Closed sbranden closed 8 years ago

sbranden commented 8 years ago

Add runtime support for trusted_board_boot. Introduce plat_is_trusted_boot to determine if signature checks should be done on image or not. The plat_is_trusted_boot can be implemented on a per platform basis.

danh-arm commented 8 years ago

I can see how this would be useful but I don't think it's going to fly in its current form.

sbranden commented 8 years ago

This is quite urgent as we are making a ROM. Whether Trusted board boot is supported or not is determined at manufacturing time - not compile time.

What I really wanted is "SKIP_SiGNATURE_CHECK" but load trusted image otherwise. This allows the hash to still be used on a checksum of the image to validate the image is not corrupt. But, allow any ATF image to be loaded on the board.

It works perfectly for what we need actually. The "trusted" image is generated and the signature check can be skipped if the SOTP is not programmed with a key.

I don't need any other dynamic plat_ options added. Everything else can be done through certificate extensions for things such as whether watchdog is enabled/disable on jump to BL2, etc. We just need a generic certificate extension extraction routine added to ATF (or maybe it is there already).

alex-nemirovsky-ca commented 8 years ago

Dan, What kind of timeframe as you thinking? We have similar need. So +1 to what Scott said. ;-) Logic is similar. Check if ROTPK hash is programmed into SOTP, if not then skip authentication.

alex-nemirovsky-ca commented 8 years ago

Dan and Scott, How would you feel about using the return value of plat_get_rotpk_info() as an indictor to determine when authentication should be skipped? Platform specific code can determine when no PK or Hash of PK exists in the system and then return error from this function. In error is returned, authentication should be skipped, as we won't be able to authenticate without a valid PK or Hash of PK and boot will fail.
I believe that this type of logic using the existing platform call avoids creating yet another platform API call like plat_is_trusted_boot() to achieve the same result. I don't think this presents a security hole if no PK or Hash of PK exists in the system. Am I mistaken?

As an added bonus, this kind of approach may reduce the kinds of images that need to be supported and maintained. In short there may no longer be any need for non-TBB images anymore other than for when we need to scale down the size of the images to fit into ROM or SRAM. Which is another issue all together as we add more and more features. i.e. build scaling architecture.

Thoughts?

danh-arm commented 8 years ago

Hi Alex

That's an interesting idea. I don't think we'd do it exactly as you describe; for example I think we'd want an error from plat_is_trusted_boot() to continue being handled as an authentication failure. However, we could define a specific status code, which the generic code interprets as "manufacturing mode", and skips the ROTPK hash check. The generic code would continue verifying the signature but using the (unverified) public key in the certificate. The expectation would be that the CoT would be generated in the same way as in an in a deployed device, except using a development ROTPrivateK.

Would that meet both your needs?

Regarding the dynamic feature control in TF I mentioned before, that is not going to deliver until the end of the year at best; I can see we need an interim solution sooner than that.

Dan

sbranden commented 8 years ago

The purpose is more than just "manufacturing mode". Let's call it something like "untrusted mode". We also don't want to waste boot type verifying the signature. We just want to make sure the image is not corrupt.

The code I presented for dynamic feature is sufficient for what I need and not very intrusive in the code. I have no need for anything delivered from ARM at this time.

alex-nemirovsky-ca commented 8 years ago

Dan, the goal is to have the same ASIC support both customer who want TBB and Non-TBB. It's not cost effective to make two different ASIC with different BL1 images embedded into the cores. One as a TBB compile and another as a non-TBB compile. Does this make sense? I'm open to any way that you want to do it generically. Just thought we could reuse the return value of plat_get_rotpk_info() to determine when should attempt to do authentication. If you want read the public key from the cert and use that as the authentication key when plat_get_rotpk_info() returns error, then I'm fine with that. Sounds like it would be less code to modify and mostly follows the same flow.

Do you think we could get a test of this kind of modification soon?

btw, to be honest with you, implementing plat_is_trusted_boot() would in our case just inspect the location that we store HASH of PK anyways to determine if a value key exists. Being asked for plat_get_rotpk_info() and returning OK when we know the contents is not a real HASH of PK, seems like an error to me. Do you see my reasoning here?

danh-arm commented 8 years ago

I'm hearing mixed requirements here. I understand the need to build ROMs with the ability to switch at least some aspects of TBB on or off at runtime, but as a said before TBB covers a number of features, e.g.

Another thought I've been having is that if we add a new plaform function, it could instead be called prior to any of the authentication methods (signature check, hash check, nv counter check). This would be more flexible in that the platform could then do whatever it likes before each authentication operation.

Scott - you say:

The code I presented for dynamic feature is sufficient for what I need and not very intrusive in the code. I have no need for anything delivered from ARM at this time.

Unfortunately it's a platform compability break and I don't think is future-proof enough for everyone's needs. If you don't need anything from us, does that mean you're going to carry that patch locally anyway? If so, should we just close your PR while we debate the upstream solution?

Alex - regarding the return code of plat_get_rotpk_info(): Fundamentally, we would need to distinguish 3 cases:

The last case could be determined either by a special error code or a status code. I'm not proposing we return success in this case.

alex-nemirovsky-ca commented 8 years ago

Dan, Special return code works for me if that logic makes more sense to you vs simply error. Thanks Alex

sbranden commented 8 years ago

Hi Dan, you summarized and understand the issue perfectly.

Yes, the one remaining thing I need to profile is the SHA256 time vs. a simpler SHA1 or CRC/checksum value. It would be better to add a simpler CRC/checksum to each image.

If a standard solution becomes available in the next month I would go with it. Otherwise I'll be going with my solution for the near term and possibly adding SHA1 or a simple checksum.

danh-arm commented 8 years ago

We had an internal discussion about this. The conclusion was that the correct long term solution is to provide:

  1. A special plat_get_rotpk_info() status/error code to allow platforms to use a development ROTPK in a production ROM, as described above.
  2. Dynamic configuration of the Chain of Trust, which is itself authenticated before use.
  3. Updates to the cert_create tool and authentication/image parser modules to allow more optimized certificate/hash methods.

We didn't like the idea of additional hooks in the authentication framework to bridge the gap until 2 becomes available, since these would provide an additional attack vector and probably need to be deprecated once that support did become available.

I've added a task to implement 1 in next month's sprint. 2 is still a longer term feature (currently aiming towards the end of this year). 3 is not planned yet.

Scott - this is not going to satisfy your requirements in the next month so I guess you'll have to go with your own solution for now.

sbranden commented 8 years ago

There seems little benefit in implementing #1. If you wish to use a development ROTPK in a production rom just program the development key into that SoC?

danh-arm commented 8 years ago

That's true if SoC vendors don't mind providng a development key (or an interface to one) in the SoC. I guess you're OK with this but does anyone else have a view?

In any case, this is a low risk change.

danh-arm commented 8 years ago

Internal ref: http://jira.arm.com/browse/GENFW-1448

alex-nemirovsky-ca commented 8 years ago

Dan, I think Scott's response is in reference to your commentary about why an error code would be used. i.e "A special plat_get_rotpk_info() status/error code to allow platforms to use a development ROTPK in a production ROM, as described above."

This isn't aligned with what was discussed in the thread prior. What was discussed is that plat_get_rotpk_info() would returned error code to signify that NO ROTPK is available in the system and to skip authentication. Or per your suggestion, instead of skipping authentication in the existing code, the ROTPK would actually be obtained out of the cert itself and checked against itself instead of provided by plat_get_rotpk_info () when this error code is return. This is to allow the same SW flow to be used instead of attempting to skip authentication in the code flow. As a bonus side effect, integrity check continues to be performed. Albeit it may be not as fast as Scott would like if boot time is an issue. This special error code provision MAY be a method that could be used during development, or it could be the method for production boards that will NEVER provide an ROTPK because they don't require TBB. Which is also fine. The main goal of this error code is to allow the same production ASIC to be used for customers who want TBB and those who don't want to turn on TBB yet. Either for development purposes or because they don't ever need or want TBB. That's really up to them. If and when the platform discovers or is told that an ROTPK is actually available, then plat_get_rotpk_info() will pass the KEY and return OK. Until that happens, it will return the error code and pass no KEY. KEY to be obtained from the cert and checked against itself in the later case. If that is because they are using board for development cycles or because they never intended to use TBB, that's really up to them.

Are we aligned?

danh-arm commented 8 years ago

Hi Alex

I think we're aligned on what my item 1 above will implement; I'm sorry if my description was misleading. I didn't mean to imply that the development ROTPK should exist in the production ROM; rather that a development ROTPK (that exists in the certificates) can be used while using a production ROM. The development certificate ROTPK is not verified against the platform ROTPK in this case.

I got the impression from Scott's comment that he doesn't need this since he thinks plat_get_rotpk_info () could just return a development ROTPK instead of a real ROTPK, until the time that the real ROTPK gets provisioned. The common code would not need changing in this case. However, I may have misunderstood - please correct me if I'm wrong.

Dan.

alex-nemirovsky-ca commented 8 years ago

Dan, great. sounds like you and I are aligned.

sbranden commented 8 years ago

I'll see what the final logic looks like to see if everything makes sense.

sbranden commented 8 years ago

To speed up boot time I wish to use a faster checksum. Do you see a problem if we extend the HASH extension to include both SHA256 and MD5 in the same certificate extension. At runtime, SHA256 would be used for trusted boot, MD5 would be used for untrusted boot to validate image integrity. Or, do you feel I need to add another certificate extension just for the faster hash? I've started on that path and it looks redundant nearly duplicating the current HASH certificate. Much cleaner would be to add it to the current hash certificate?

danh-arm commented 8 years ago

If you're asking for general advice, I'd say:

alex-nemirovsky-ca commented 8 years ago

Hi Dan, What the ETA on the code per our aligned understanding from 2 weeks ago (5 posts up)? I see you mention doing as part of May's sprint. Does this mean we should expect by end of May? Thanks Alex

danh-arm commented 8 years ago

Yes, our intention is to do this during May but this is not a commitment as we have not planned that sprint yet.

alex-nemirovsky-cortina commented 8 years ago

Hi Dan, still on target for end of this month? Thanks Alex

danh-arm commented 8 years ago

Yes, that's the plan

alex-nemirovsky-cortina commented 8 years ago

Hi Dan, Has this item been implemented? If so, where may I find it? Thanks Alex

danh-arm commented 8 years ago

It's still in internal review. Another (working) day or two I expect.

soby-mathew commented 8 years ago

@alex-nemirovsky-cortina, The PR#642 contains the changes.

danh-arm commented 8 years ago

Hi @alex-nemirovsky-cortina, @sbranden. I will merge https://github.com/ARM-software/arm-trusted-firmware/pull/642 this week unless you have any objections.

sbranden commented 8 years ago

Hi Dan,

This looks like a good start to me. A future enhancement is to add an MD5 checksum to the image certificates and allow that to be used for faster boot.

The comment: must not be used in a deployed production environment

could be change to something like: must not be used in a deployed production environment requiring complete TBBR support

On Tue, Jun 7, 2016 at 1:34 AM, danh-arm notifications@github.com wrote:

Hi @alex-nemirovsky-cortina https://github.com/alex-nemirovsky-cortina, @sbranden https://github.com/sbranden. I will merge ARM-software/arm-trusted-firmware#642 https://github.com/ARM-software/arm-trusted-firmware/pull/642 this week unless you have any objections.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ARM-software/tf-issues/issues/381#issuecomment-224215205, or mute the thread https://github.com/notifications/unsubscribe/AGSrtiHyUiXZskGPh4uNRKQUOqUWb-XMks5qJS0YgaJpZM4H3eoO .

alex-nemirovsky-cortina commented 8 years ago

Hi Dan, looks good to me. Thanks

danh-arm commented 8 years ago

Thanks Both.

Scott - we implicitly assume that when TBBR is enabled, this means complete compliance with the (mandatory) TBBR requirements. Platforms are free to not completely support these requirements but I don't think this needs spelling out in the comments.