Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[kmac] Entropy error code #15088

Open
cindychip opened this issue Sep 22, 2022 · 15 comments
Open

[kmac] Entropy error code #15088

cindychip opened this issue Sep 22, 2022 · 15 comments
Labels
Component:Darjeeling Component:Doc Documentation issue Component:RTL IP:kmac Priority:P2 Priority: medium SW:cryptolib Crypto library Type:Enhancement Feature requests, enhancements Type:FutureRelease Not relevant to currently planned releases/milestones

Comments

@cindychip
Copy link
Contributor

When reviewing KMAC entropy related error code, here is one concern we want to flag:
There are two KMAC entropy error code: WaitTimerExpired and IncorrectEntropyMode.
Currently both of them will flag an KMAC error.

However, the concern here is: the current KMAC error does not block any incoming operations.
So with these error, we believe KMAC will continue to process all the incoming operations with the existing entropy.
We think this might have some potential issue and at least we should flag an error on keymgr app interface and stop the process there.

@eunchan please feel free to elaborate more.
@jadephilipoom @johannheyszl @cdgori please let us know if you have any other thoughts.

Thanks,
Cindy

@eunchan
Copy link
Contributor

eunchan commented Sep 22, 2022

In Top Earlgrey, we have a few assumptions that may reduce this risk.

  1. App interface (KeyMgr) is initiated by SW always. So, any entropy events SW receives could be assessed and processed prior to the next App request.
  2. Incorrect Entropy Mode case, we can assume it is due to FI attacks. If SW is compromised (kernel SW), then there's no way KMAC avoid manipulations. So, in that case, SW is still able to receive the error event prior to initiate KDFs.
  3. If an userspace SW has been compromised, the error is being received by the kernel SW. so no problem so far.

With assumptions above, still the risk is quite low in my opinion. However, the design as itself has weakness here. So it is better for KMAC to report errors via App interface in case of entropy errors occur. There's no direct error report from kmac_entropy to kmac_app module yet.

@jadephilipoom
Copy link
Contributor

Just to check that I've understood the comments above properly:

  • If entropy refresh fails, the KMAC block will raise a kmac_error interrupt and write the corresponding error code to the ERR_CODE register, but will not block a new incoming operation, meaning that the next KMAC operation would reuse the existing entropy.
  • The above is not considered an issue for software-initiated KMAC operations, since software will see the interrupt and stop requesting KMAC operations. However, it could be an issue for the hardware application interfaces, which do not see this error and would not block new operations.
  • Currently, for top_earlgrey, this is not likely to cause a problem since, of the 3 application interfaces (keymgr, lifecycle control, and ROM control), only keymgr performs a KMAC operation -- the others are cSHAKE -- and all keymgr operations are initiated by software. So if software sees a KMAC error, it will stop initiating keymgr operations.
  • Although it's not a problem for the top_earlgrey design, it would be nice to report errors through application interfaces so that security doesn't rely on the assumtion that all KMAC operations are software-initiated and -monitored.

@cindychip @eunchan Is that understanding accurate?

@eunchan
Copy link
Contributor

eunchan commented Sep 26, 2022

@jadephilipoom Exactly!

@cindychip cindychip added the Priority:P2 Priority: medium label Sep 28, 2022
@moidx moidx added the Hotlist:Security Security Opinion Needed label Sep 29, 2022
@cindychip cindychip modified the milestones: Project: M2, Project: M3 Sep 29, 2022
@vogelpi vogelpi added the IP:kmac label Dec 8, 2022
@johannheyszl johannheyszl added SW:cryptolib Crypto library Type:Enhancement Feature requests, enhancements and removed Hotlist:Security Security Opinion Needed labels Dec 12, 2022
@johannheyszl
Copy link
Contributor

Results from the discussion in Sec WG 12/08:

(1) SW needs to check the error in KMAC before using it through key manager; document in SW guidelines.

(2) Mentioned errors are currently not reported on app interface to prevent any unwanted lock-ups; the error reporting on app interface needs to be extended for future versions where no/less SW interaction is available.

@cindychip
Copy link
Contributor Author

Thank you Johann for summarizing the conclusion.
@timothytrippel could you help us update the SW guidelines?
@vogelpi could you help update the kmac doc?

@cindychip cindychip added Priority:P3 Priority: low Priority:P2 Priority: medium and removed Priority:P2 Priority: medium Priority:P3 Priority: low labels Feb 28, 2023
@vogelpi
Copy link
Contributor

vogelpi commented Feb 28, 2023

Hi @cindychip ,
yes, I can help with the doc but let's wait with documentation content changes until the doc refactor has landed. There will be a large amount of rebasing needed and if we introduce documentation changes in the meantime, there is a non-negligible risk to loose such changes.

@cindychip
Copy link
Contributor Author

Totally makes sense, thanks Pirmin.

@ballifatih
Copy link
Contributor

estimate 16

@johngt
Copy link

johngt commented Mar 30, 2023

@cindychip / @vogelpi - I see this moved up from P3 to P2? Please confirm as it's not easy to see with the labelling system here.

@cindychip
Copy link
Contributor Author

@johngt @ballifatih for M2.5, this task only needs documentation change. (I think might take 2 hours max?) --> P2 for M2.5
For future release, we can consider adding a error reporting mechanism for interface. ---> future release.

If it is easier for tracking, we can split into two issues?

Reference: #15088 (comment)

@ballifatih
Copy link
Contributor

Sounds reasonable @cindychip, I forked a new issue to track SW/doc changes which are relevant for M2.5.

We can keep this issue around for the RTL changes for FutureRelease. I will label it accordingly.

@ballifatih ballifatih added the Type:FutureRelease Not relevant to currently planned releases/milestones label Apr 3, 2023
@msfschaffner msfschaffner added the Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones label Oct 7, 2023
@msfschaffner
Copy link
Contributor

@vogelpi can you take a look at this and make an assessment as to whether we should fix this for PROD? Thanks!

@vogelpi
Copy link
Contributor

vogelpi commented Jan 27, 2024

I had a look at his earlier this week. My view is that the key thing is to document how software should check those error codes. Reporting such errors on the app interfaces is desirable whenever hardware can trigger such operations but it may lead to unwanted side effects including blocking behavior. For Earlgrey, it might be safer to not do it in HW. We should definitely review this in the Integrated context.

@johannheyszl
Copy link
Contributor

Agree w/ @vogelpi (and others before) to not change RTL for Earl Grey.
Future Release / Integrated and close as not planned are all fine for me.

SW requirement is documented in #17785

@vogelpi
Copy link
Contributor

vogelpi commented Jan 29, 2024

Thanks for your feedback @johannheyszl , let's keep this open but in the Integrated context.

@vogelpi vogelpi removed this from the Earlgrey-PROD.M4 milestone Jan 29, 2024
@vogelpi vogelpi added Component:Darjeeling and removed Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones labels Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:Darjeeling Component:Doc Documentation issue Component:RTL IP:kmac Priority:P2 Priority: medium SW:cryptolib Crypto library Type:Enhancement Feature requests, enhancements Type:FutureRelease Not relevant to currently planned releases/milestones
Projects
None yet
Development

No branches or pull requests

9 participants