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

Sanitize CPUID flags used by cryptography libraries #1014

Closed
clauverjat opened this issue Oct 27, 2022 · 10 comments · Fixed by #1125
Closed

Sanitize CPUID flags used by cryptography libraries #1014

clauverjat opened this issue Oct 27, 2022 · 10 comments · Fixed by #1125
Assignees

Comments

@clauverjat
Copy link

clauverjat commented Oct 27, 2022

Description of the issue

In issue #8 Known security issues, you mention that Gramine applies some limited sanitization on CPUID results. You've closed the issue but the linked issue to the Graphene repository gramineproject/graphene#966 is still open. I looked at https://github.com/gramineproject/gramine/blob/master/pal/src/host/linux-sgx/pal_misc.c, there is indeed some sanitization, but it doesn't go very far.

Looking at the code, I don't see sanitization of flags that can impact the security of crypto libraries like the AES flag. If CPUID indicates that AES-NI is not supported, most crypto libraries will fall back to a software implementation of AES which opens the door to side-channel attacks. These side-channel attacks are a big threat in the Intel SGX threat model. This problem was already mentioned by @thomasknauth who opened the issue gramineproject/graphene#966 on the Graphene repository.

How Gramine could implement it ?

To my knowledge all Intel CPUs supporting SGX, also support AES-NI. So I would advise always setting the corresponding CPUID bit for sanitization. It could also be good to set the flags for RDRAND and RDSEED support. But the latter flags are less of a (security) concern because the usual fallback when RDRAND/RDSEED is unsupported, is to use /dev/random/ or /dev/urandom/ instead which are implemented by the Gramine runtime with the PalRandomBitsRead function which itself relies on RDRAND.

@clauverjat clauverjat changed the title CPUID sanitization of flags used by cryptography libraries Sanitize CPUID flags used by cryptography libraries Oct 27, 2022
@mkow
Copy link
Member

mkow commented Oct 28, 2022

the linked issue to the Graphene repository gramineproject/graphene#966 is still open

Yes, because the old repo was archived and is read-only, we can't close/edit issues there (without some hacks). So, it doesn't mean that it's still valid.

a software implementation of AES which opens the door to side-channel attacks

How exactly? Any concrete example? AFAIR crypto libraries should be secure against side channels of all kinds, they don't have naive, textbook implementations.

Anyways, if someone from Intel (@ScottR-Intel maybe?) confirms that all SGX-capable CPUs (including slow atoms and similar) have AES-NI then we can fail if they are not set (but definitely not silently report as available, as you suggested, will be super confusing in case such a CPU actually exists or some virtualizer configures it this way).

@dimakuv
Copy link

dimakuv commented Oct 28, 2022

Anyways, if someone from Intel (@ScottR-Intel maybe?) confirms that all SGX-capable CPUs (including slow atoms and similar) have AES-NI then we can fail if they are not set.

I asked some SGX experts internally. Let's see what they reply.

@dimakuv
Copy link

dimakuv commented Oct 31, 2022

Intel SGX experts told me that this feature is not implemented in Intel SGX SDK, instead SGX SDK relies on feature bits provided by untrusted runtime. If SGX SDK or Gramine would implement this enforcing of AES-NI and RDSEED/RDRAND, it would be problematic on platforms where these features are disabled (in particular, AES-NI).

Therefore, Gramine will not implement this feature.

@dimakuv dimakuv closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2022
@boryspoplawski
Copy link
Contributor

Well, we could have a manifest flag sgx.require_aesni, which would then exit Gramine if cpuid returns it's not supported

@mkow
Copy link
Member

mkow commented Nov 2, 2022

Intel SGX experts told me that this feature is not implemented in Intel SGX SDK, instead SGX SDK relies on feature bits provided by untrusted runtime. If SGX SDK or Gramine would implement this enforcing of AES-NI and RDSEED/RDRAND, it would be problematic on platforms where these features are disabled (in particular, AES-NI).

So, there are some CPU which have SGX but not AES-NI? Any example?

Therefore, Gramine will not implement this feature.

Waiit, not so fast. The fact that SGX SDK doesn't do something doesn't mean that it / we shouldn't. Let's finish the discussion first.

@clauverjat: ping about my question from the previous comment.

@mkow mkow reopened this Nov 2, 2022
@dimakuv
Copy link

dimakuv commented Nov 2, 2022

So, there are some CPU which have SGX but not AES-NI? Any example?

This can be disabled in the BIOS (if I read Intel SDM correctly). See Intel SDM, Volume 4, Table 2-6. There is an MSR MSR_FEATURE_CONFIG whose bits 1:0 may be set to 11b, meaning that AES-NI is disabled until next RESET.

@mkow
Copy link
Member

mkow commented Nov 2, 2022

Ok, then I'm closing it unless someone shows an example where a respected or popular crypto library uses a side-channel-prone AES implementation when AES-NI is disabled (and a secure one if enabled). If we find such a case then we can implement a manifest switch, as Borys suggested.

@mkow mkow closed this as not planned Won't fix, can't repro, duplicate, stale Nov 2, 2022
@clauverjat
Copy link
Author

clauverjat commented Nov 2, 2022

Sorry for my late reply (it was public holiday).

but definitely not silently report as available, as you suggested, will be super confusing in case such a CPU actually exists or some virtualizer configures it this way

Indeed you are right silently enabling this bit would be a bad idea. I was suggesting a quick dirty fix. For a real implementation, one should follow @boryspoplawski idea of adding a manifest flag or something similar.

How exactly? Any concrete example? AFAIR crypto libraries should be secure against side channels of all kinds, they don't have naive, textbook implementations.

Of course (good) crypto libraries do not implement the naive textbook algorithm, but side-channel resistance against an active adversary is required for crypto inside SGX and it can be hard to achieve in software while keeping good performance. AES is notoriously hard to implement in a performant and secure manner in software (see [1]).

To be more concrete, I know at least one example of vulnerable implementation. The mbedtls AES fallback implementation is susceptible to active side channel attacks as shown in [2]. If the attacker can disable AES NI, the mbedtls AES crypto becomes vulnerable to a key recovery attack. Note that mbedtls is often used because it is quite easy to port. I even found it in one of your CI example https://github.com/gramineproject/gramine/tree/4dd793a853c6a2200dba568bb98225e136f5cf6b/CI-Examples/ra-tls-mbedtls
I expect the attack could be mounted against this example, so I think this shows that this is a concrete threat.
Also, I am not sure this is still applicable but [3] shows an attack against the SW implementation provided by OpenSSL.

[1] https://news.ycombinator.com/item?id=13364963
[2] S. Fei, Z. Yan, W. Ding, and H. Xie, “Security Vulnerabilities of SGX and Countermeasures: A Survey,” ACM Comput. Surv., vol. 54, no. 6, pp. 1–36, Jul. 2021, doi: 10.1145/3456631. Available: https://hal-cnrs.archives-ouvertes.fr/hal-02319488
[3] C. A. Venkatesh Bholanath Roy, M. Bhargav Sri and B. L. Menezes, “‘S-Box’ Implementation of AES is NOT side-channel resistant.” Accessed: Nov. 02, 2022. [Online]. Available: https://eprint.iacr.org/2018/1002.pdf

@clauverjat
Copy link
Author

clauverjat commented Nov 2, 2022

And here's more examples A. Moghimi and al. showed that OpenSSL 1.1.0f, WolfCrypt 3.11.0, Mozilla NSS 3.30.2, Libtomcrypt 1.17, Libgcrypt 1.7.7, MbedTLS 2.4.2 [1] all have a vulnerable implementation.

Edit : forgot to ping @mkow

[1] A. Moghimi, G. Irazoqui, and T. Eisenbarth, “CacheZoom: How SGX Amplifies the Power of Cache Attacks,” in Cryptographic Hardware and Embedded Systems – CHES 2017, vol. 10529, W. Fischer and N. Homma, Eds. Cham: Springer International Publishing, 2017, pp. 69–90. doi: 10.1007/978-3-319-66787-4_4. Available: https://moghimi.org/papers/chess2017-CacheZoom.pdf

@mkow
Copy link
Member

mkow commented Dec 16, 2022

Ok, I did some reading and this issue seems valid and quite serious, although it's hard to find any definite claims (e.g. mbedtls docs don't have any comments about their current software AES implementation, if it's not secure I'd expect some warnings there...).

Steps we need to take:

  • Compile out software implementation from our mbedtls build.
  • Force AES-NI in enclave configuration. I wouldn't even make this configurable, as it seems that one practically can't use SGX securely without it. Note: I'm not sure if this is possible, as AES-NI enable/disable switch is in MSR_FEATURE_CONFIG, not XCR0. So, I think we can't enforce it in enclave attributes?
  • Force AES-NI to be enabled in cpuid sanitization logic.
  • Probably ping SGX SDK team about this issue? (and maybe also other projects, if anyone has time)

I can implement all of this (actually, I already implemented a quick draft for experiments), but we need to first agree on the exact solution.

Useful links:

CC: @mrber - you may be interested in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants