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

Rsa for 2.5.0 #1610

Merged
merged 5 commits into from
Jun 19, 2017
Merged

Rsa for 2.5.0 #1610

merged 5 commits into from
Jun 19, 2017

Conversation

jbech-linaro
Copy link
Contributor

Fixes for the security issues that we discussed a while ago. Note that patches has already been reviewed once before this PR, so that is the reason why you already find that in the patches.

root@Vexpress:/ xtest -l 15
...
+-----------------------------------------------------
47112 subtests of which 0 failed
78 test cases of which 0 failed
0 test case was skipped
TEE test application done!
root@Vexpress:/ 

@jforissier
Copy link
Contributor

Thanks for creating the PR. I have 2 questions:

  • Do we have CVE numbers for the security fixes? If we do, the numbers should be in the commit description, but if we don't it shouldn't stop us from merging the patches of course.

  • The checkpatch warnings look easy enough to fix (long lines), can you address this please?

@jbech-linaro
Copy link
Contributor Author

Do we have CVE numbers for the security fixes? If we do, the numbers should be in the commit description, but if we don't it shouldn't stop us from merging the patches of course.

I've requested CVE's using this (it's actually quite a while ago), but I haven't got them yet. Compared to the first time, we got asked to accept a some legal stuff, which Rob Booth approved and I gave the information to them. But checking this page I still cannot find my own email address there. I can try to get hold of the Mitre people, but I don't think we will get any CVE's in time for the release. Instead I suggest that we update out Security Advisories page instead when we've actually got the CVE's.

The checkpatch warnings look easy enough to fix (long lines), can you address this please?

Of course, I will fix those.

@jforissier
Copy link
Contributor

@jbech-linaro sounds good to me, thanks!

@jforissier
Copy link
Contributor

All good, I think you can squash the fixup into the original patch and I'll merge everything.

LTC_LINARO_FIX_RSAWITHOUTCRT is used to handle the case where the CRT
optimized algorithm cannot be used because the optimized parameters are
missing. In the official LibTomCrypt tree, there is an official fix for
this.

Please see commits (official LibTomCrypt tree):
    01f184540232 ("harden RSA CRT by implementing the proposed
                   countermeasure from ch. 1.3 of [1]")
    a6e89d58d4fb ("RSA in CRT optimization parameters are empty")
    2bb3f0246f65 ("RSA in case CRT optimization parameters are not
                   populated")

Those patches were brought into OP-TEE with this patch
    a50cb36 ("ltc: sync from official develop branch")

And therefore there is no need to keep the LTC_LINARO_FIX_RSAWITHOUTCRT
any longer, hence this patch removes the flag and the code related to
that particular flag.

Signed-off-by: Joakim Bech <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
Reviewed-by: Jens Wiklander <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Tested-by: Jerome Forissier <[email protected]> (HiKey, GP)
Tested-by: Etienne Carriere <[email protected]> (b2260, GP)
Signed-off-by: Joakim Bech <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
Reviewed-by: Jens Wiklander <[email protected]>
Reviewed-by: Etienne Carriere <[email protected]>
Tested-by: Jerome Forissier <[email protected]> (HiKey, GP)
Tested-by: Etienne Carriere <[email protected]> (b2260, GP)
When enabling the flag LTC_RSA_BLINDING the code uses the mp_rand()
function, which isn't implemented for the mpa_desc descriptor. Implement
it as rand() in mpa_desc and mpa_get_random_digits() in libmpa.

Fixes: OP-TEE-2016-0003 which was reported by Applus+ Laboratories.

Signed-off-by: Joakim Bech <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
Reviewed-by: Jens Wiklander <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Tested-by: Jerome Forissier <[email protected]> (HiKey, GP)
Tested-by: Etienne Carriere <[email protected]> (b2260, GP)
Enable the hardening flags by default. This should make it robust to the
Bellcore attack when using RSA with CRT.

Fixes: OP-TEE-2016-0003 which was reported by Applus+ Laboratories.

Signed-off-by: Joakim Bech <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
Reviewed-by: Jens Wiklander <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Tested-by: Jerome Forissier <[email protected]> (HiKey, GP)
Tested-by: Etienne Carriere <[email protected]> (b2260, GP)
The mpa_exp_mod() function implements a LtoR algorithm. The LtoR
algorithm is sensitive to timing attacks since it leaks information
about the exponent since it's doing a different amount of work in each
loop when doing the modular exponentiation. It will always do a square
in each loop, but it will also do an additional multiply when the
exponent bit k=1.

This patch implements the Montgomery ladder (and thereby replaces the
old LtoR implementation), which always does the same amount of
operations in each loop and thereby make it more robust to timing
attacks.

Fixes: OP-TEE-2016-0002 which was reported by Applus+ Laboratories.

Signed-off-by: Joakim Bech <[email protected]>
Acked-by: Jerome Forissier <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Tested-by: Jerome Forissier <[email protected]> (HiKey, GP)
Tested-by: Etienne Carriere <[email protected]> (b2260, GP)
@jbech-linaro
Copy link
Contributor Author

Squashed, rebased and tag(s) applied (should be) ready for merge! Thanks!

(After rebasing to latest this morning I "Re-tested" with xtest -l 15 on fe685824 msg_param.h: add const qualifier for read-only functions, but when done I could see another commit has been merged from Etienne).

@jforissier jforissier merged commit 40b1b28 into OP-TEE:master Jun 19, 2017
@jforissier
Copy link
Contributor

Merged, thanks.

when done I could see another commit has been merged from Etienne

It's OK. Now I use the "rebase and merge" button to merge the PRs into master, so rebasing to the very latest is not required.

@jbech-linaro
Copy link
Contributor Author

It's OK. Now I use the "rebase and merge" button to merge the PRs into master, so rebasing to the very latest is not required.

Thanks Jerome! I did rebase it on Etienne's patch, but I didn't test on his patch! But ... I hardly suspect it will matter. Yeah, the "rebase and merge" is pretty nifty!

@jbech-linaro jbech-linaro deleted the rsa-for-2.5.0 branch June 19, 2017 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants