[Docs] Add chapter on encrypted files implementation

[dimakuv]

Description of the changes

Add chapter on encrypted files implementation.

Also see #1841, which uses (almost) the same names of data structs and fields.

How to test this PR?

Temporarily enabled ReadTheDocs: https://gramine.readthedocs.io/en/dimakuv-docs-encrypted-files/devel/encfiles.html

---

This change is 

[dimakuv]

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

a discussion (no related file):
Blocking on #1841

---

[dimakuv]

Reviewable status: 0 of 11 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

---

Documentation/devel/encfiles.rst line 289 at r2 (raw file):

derivation, with input materials being the KDK and the salt (plus the hard-coded
label ``SGX-PROTECTED-FS-METADATA-KEY`` and the hard-coded integers ``1`` and
``128``).

I'm not sure these hard-coded label and integers are needed in this description. Moreover, I don't know why they are important at all. TODO: Ask crypto experts.

[g2flyer]

This is a very helpful PR for anybody who wants to understand how protected files work and how it's implemented! Find though a few suggestion for potential improvements.

Reviewed 10 of 11 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

---

Documentation/devel/encfiles.rst line 19 at r3 (raw file):

the application must be put under the "encrypted" FS mount in the Gramine
manifest. New files created in the "encrypted" FS mount are automatically
treated as encrypted. The encryption format used for encrypted files is borrowed

suggest s/encrypted/encrypted and integrity-protected/ and probably would drop the 'encryption' before format or call it file-format

---

Documentation/devel/encfiles.rst line 26 at r3 (raw file):

Each encrypted file is encrypted separately, i.e. Gramine employs file-level
encryption and not block-level encryption. Each "encrypted" FS mount can have a
separate encryption key. By putting each encrypted file in its own FS mount, it

we probably should call this key key derivation key as the actual file is not encrypted with that key but a key derived from that KDK. In fact, we later call it KDK so we best already name it here the same and maybe even explicitly that files are encrypted with derived key(s).
BTW: While strictly true, i think the subsequent "By putting each ..." is also a bit misleading and in any case personally i wouldn't explicitly put that in the text in such prominent form: this should be an uncommon (and undesirable) case and if somebody really needs it they will figure it out ...

---

Documentation/devel/encfiles.rst line 45 at r3 (raw file):

- **Confidentiality of user data**: all user data is encrypted and then written
  to untrusted host storage; this prevents user data leakage.
- **Integrity of user data**: all user data is read from disk and then decrypted

"decrypted with GMAC" is an oxymoron as GMAC wouldn't involve encryption and i think strictly speaking, the code also doesn't use GMAC (contrary to what some of the variables seem to indicate) nor even AAD from AES-GCM. So i would talk about AES-GCM as the overall scheme and when talking about the "MAC" use the standard AES-GCM term of "Authentication Tag" (i.e., s/GMAC/Authentication Tag/g)

---

Documentation/devel/encfiles.rst line 58 at r3 (raw file):

  Gramine does not guarantee the freshness of user data in the file after this
  file was closed. Note that while the file is opened, the rollback/replay
  attack is prevented (by comparing the root MHT hashes).

"(by comparing the root MHT hashes)" -> "(by keeping the root of a Merkle-tree over the file always in (trusted) memory and checking the consistency during access, see more details below)" or alike might be more clear? At least term "merkle-tree" would seem useful as crucially keeping the root always in memory?

---

Documentation/devel/encfiles.rst line 59 at r3 (raw file):

  file was closed. Note that while the file is opened, the rollback/replay
  attack is prevented (by comparing the root MHT hashes).
- **Side-channel attacks**. Some seemingly-insignificant information, such as

I would drop the "Some seemingly-insignificant.

---

Documentation/devel/encfiles.rst line 62 at r3 (raw file):

  file name, file size, access time, access patterns (e.g., which blocks are
  read/written), etc. is not protected. This information could be used by
  sophisticated attackers to gain sensitive information.

i would drop 'sophisticated'; this information is trivial to access and exploit if really sensitive?

BTW: this also reminds me that currently file meta data such as access time, ownership, permission bits, etc are also not integrity protected. Maybe also worth mentioning somewhere?

---

Documentation/devel/encfiles.rst line 98 at r3 (raw file):

- Reusability -- the encrypted-files code can be used as-is in stand-alone tools
  like :program:`gramine-sgx-pf-crypt`.
- Crypto reviews -- the encrypted-files code contains only the crypto

I guess you wanted to say that the encrypted files code is the only one (directly) using crypto? Currently it sounds like it does only crypto but clearly it does more (in fact, it doesn't even implement the crypto itself, just calling it?

---

Documentation/devel/encfiles.rst line 101 at r3 (raw file):

  algorithms, which facilitates crypto/security review efforts.

The application code is *not* aware of encrypted files. Applications treat

Maybe worth, though, describe somewhere what happens when we detect tampering/integrity-inconsistencies. Looking at code currently i think currently we set file status to PF_STATUS_CORRUPTED and return an "standard" error but otherwise continue as-is? Makes me wonder whether maybe there should be an option to abort in such cases? (not that this would be something for the PR, though :-))

---

Documentation/devel/encfiles.rst line 118 at r3 (raw file):

``libos_encrypted_file`` data structure is hosted in the glue code, and the
``pf_context`` data structure is hosted in the generic encrypted-files code. The
encryption key is installed through Gramine interfaces into the

s/encryption key/key derivation key/ (or just KDK?)

---

Documentation/devel/encfiles.rst line 225 at r3 (raw file):

``root_mht_node`` field. Also, the root MHT node's encryption key and GMAC are
stored directly in the encrypted header of the metadata node. The root MHT node
starts to be used when the plaintext file size exceeds 3KB.

maybe also worth mentioning that other nodes will be fetched on demand, cached and potentially evicted, the root MHT node will be always in memory for the lifetime of the file handle? (actually, notice that you mention that latter when discussing the LRU cache. So i guess it's fine to just mention it there ...)

---

Documentation/devel/encfiles.rst line 480 at r3 (raw file):

  into the cache.

- There is *no* multiprocess support for encrypted files. This means that if the

There is limited support for multi-process. The next sentence seems to allude to that though but arguably, contrary to the first sentence which generalizes on the safe side, seems a bit to to cover only a subset of problematic cases: the sentence can be read is if the issue is only if the open happens simultaneous but not if, e.g., opens are inherited from fork or alike? Maybe better to talk just about concurrent access of a file? (more precisely it would be concurrent access where one is writing but i guess that's a bit too much details and probably not the make or break of many use-case being supported or not.

---

Documentation/devel/encfiles.rst line 492 at r3 (raw file):

- There is no key rotation scheme. The application must perform key rotation of
  the KDK by itself (by overwriting the ``/dev/attestation/keys/``
  pseudo-files). Some support for key rotation may appear in future releases of

Not sure whether worth pointing out but given that mostly we have one-time use keys, the issue is primarily the KDF which produces much less ciphertext than if it would be used to directly encrypt data, so the usual NIST limits would be reached much less quickly. If there would be lots of files, what could help in making rotation less critical is to add another level of indirection to derive from the KDK first a file-specific KDK?

[dimakuv]

Reviewable status: 1 of 11 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @g2flyer)

---

Documentation/devel/encfiles.rst line 289 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'm not sure these hard-coded label and integers are needed in this description. Moreover, I don't know why they are important at all. TODO: Ask crypto experts.

@g2flyer Could I ask you with these labels and integers are important for deriving the keys?

---

Documentation/devel/encfiles.rst line 19 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

suggest s/encrypted/encrypted and integrity-protected/ and probably would drop the 'encryption' before format or call it file-format

Done.

---

Documentation/devel/encfiles.rst line 26 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

we probably should call this key key derivation key as the actual file is not encrypted with that key but a key derived from that KDK. In fact, we later call it KDK so we best already name it here the same and maybe even explicitly that files are encrypted with derived key(s).
BTW: While strictly true, i think the subsequent "By putting each ..." is also a bit misleading and in any case personally i wouldn't explicitly put that in the text in such prominent form: this should be an uncommon (and undesirable) case and if somebody really needs it they will figure it out ...

Done. 2x

---

Documentation/devel/encfiles.rst line 45 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

"decrypted with GMAC" is an oxymoron as GMAC wouldn't involve encryption and i think strictly speaking, the code also doesn't use GMAC (contrary to what some of the variables seem to indicate) nor even AAD from AES-GCM. So i would talk about AES-GCM as the overall scheme and when talking about the "MAC" use the standard AES-GCM term of "Authentication Tag" (i.e., s/GMAC/Authentication Tag/g)

Done. Using tag everywhere.

---

Documentation/devel/encfiles.rst line 58 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

"(by comparing the root MHT hashes)" -> "(by keeping the root of a Merkle-tree over the file always in (trusted) memory and checking the consistency during access, see more details below)" or alike might be more clear? At least term "merkle-tree" would seem useful as crucially keeping the root always in memory?

Done.

---

Documentation/devel/encfiles.rst line 59 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

I would drop the "Some seemingly-insignificant.

Done.

---

Documentation/devel/encfiles.rst line 62 at r3 (raw file):
Done.

this also reminds me that currently file meta data such as access time, ownership, permission bits, etc are also not integrity protected. Maybe also worth mentioning somewhere?

These file meta data are unused by Gramine. In particular:

• Access/modify/change timestamps are always zeroed-out in Gramine.
• Ownership is always the current user; ownership info from the host is ignored.
• Permission bits is always the current mask; permission info from the host is ignored.

We mention access/modify/change timestamps here: https://gramine.readthedocs.io/en/stable/devel/features.html#file-systems. But I agree that we don't explicitly mention about e.g. ownership (user/group) and permission peculiarities. Do you think I should add there the list that I have above?

---

Documentation/devel/encfiles.rst line 98 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

I guess you wanted to say that the encrypted files code is the only one (directly) using crypto? Currently it sounds like it does only crypto but clearly it does more (in fact, it doesn't even implement the crypto itself, just calling it?

Done.

---

Documentation/devel/encfiles.rst line 101 at r3 (raw file):
Done.

Makes me wonder whether maybe there should be an option to abort in such cases?

Do you think the application may be tricked into doing something else if the file was corrupted? I would think that it is the app's responsibility to decide what to do with a file that is not accessible. But if you have a use case, then yes, we could add such a knob to just fail fast the whole process.

---

Documentation/devel/encfiles.rst line 118 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

s/encryption key/key derivation key/ (or just KDK?)

Done.

---

Documentation/devel/encfiles.rst line 225 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

maybe also worth mentioning that other nodes will be fetched on demand, cached and potentially evicted, the root MHT node will be always in memory for the lifetime of the file handle? (actually, notice that you mention that latter when discussing the LRU cache. So i guess it's fine to just mention it there ...)

Done.

---

Documentation/devel/encfiles.rst line 480 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

There is limited support for multi-process. The next sentence seems to allude to that though but arguably, contrary to the first sentence which generalizes on the safe side, seems a bit to to cover only a subset of problematic cases: the sentence can be read is if the issue is only if the open happens simultaneous but not if, e.g., opens are inherited from fork or alike? Maybe better to talk just about concurrent access of a file? (more precisely it would be concurrent access where one is writing but i guess that's a bit too much details and probably not the make or break of many use-case being supported or not.

Done.

---

Documentation/devel/encfiles.rst line 492 at r3 (raw file):
Done

If there would be lots of files, what could help in making rotation less critical is to add another level of indirection to derive from the KDK first a file-specific KDK?

I am no crypto expert. Intuitively it feels like bumping up the overall security. I don't know who can run such crypto analysis, and if it's needed at all.

[g2flyer]

Looks good. See a few follow-up suggestion though.

Reviewed 9 of 10 files at r4, all commit messages.
Reviewable status: 10 of 11 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

Documentation/devel/encfiles.rst line 289 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@g2flyer Could I ask you with these labels and integers are important for deriving the keys?

i guess you refer to the buf.index=1 and buf.output_len=0x80 from ipf\_import\_metadata\_key. These are artifacts of the KDF construction of NIST-SP800-108: The index is used to handle the case where you need more key-material than a single invocation of a PRF (in this case aes-cmac) is required where each invocation increases the index. And the output length is to fix how many bits you really need (and so different lengths give completely different bitstreams for same label and you can't truncate the longer one to get the shorter one). As we are generating AES-128 keys, hence the 128.
So i would drop these constants altogether and probably rephrase the paragraph along the lines of
"Then in step 3, a NIST SP800-108 KDF (Key Derivation Function) is used to derive a sub-key, with input materials being the KDK, the hard-coded label SGX-PROTECTED-FS-METADATA-KEY and the salt as nonce." (Note in SP800-104 the place we put salt is called nonce, hence my reference to that.)

---

Documentation/devel/encfiles.rst line 62 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

this also reminds me that currently file meta data such as access time, ownership, permission bits, etc are also not integrity protected. Maybe also worth mentioning somewhere?

These file meta data are unused by Gramine. In particular:

• Access/modify/change timestamps are always zeroed-out in Gramine.
• Ownership is always the current user; ownership info from the host is ignored.
• Permission bits is always the current mask; permission info from the host is ignored.

We mention access/modify/change timestamps here: https://gramine.readthedocs.io/en/stable/devel/features.html#file-systems. But I agree that we don't explicitly mention about e.g. ownership (user/group) and permission peculiarities. Do you think I should add there the list that I have above?

Given that you mention this already in features (as it is more general than PF), why not just add here a link to that section along the lines of "See also [link] for additional discussion on file metadata" or alike?

---

Documentation/devel/encfiles.rst line 101 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

Makes me wonder whether maybe there should be an option to abort in such cases?

Do you think the application may be tricked into doing something else if the file was corrupted? I would think that it is the app's responsibility to decide what to do with a file that is not accessible. But if you have a use case, then yes, we could add such a knob to just fail fast the whole process.

Of course an application should always check return codes and alike. But in general applications are written under the assumption nothing malicious happen under the cover. E.g., an adversary could try to inject some faults and alike. That said, there probably more effective way to do such than causing random read or write failures so probably not worth. (And as far as i can tell, after an integrity failure, no read or write information would ever work again (i.e., there is no way to selectively suppress, say, some writes, or alike. Certainly looked like that in the code and i see now you also added a corresponding text below.)

---

Documentation/devel/encfiles.rst line 133 at r3 (raw file):

  size and all GMACs are 16B in size.

- AES-CMAC with AES-128-bit is used to derive keys from the user-supplied KDK.

Following up to comment elsewhere on constants 1 and 128, suggest changing here also to something like
"Sub-keys are derived from the user-supplied KDK using the NIST SP800-108 construction, with the required PRF instantiated by AES-128-CMAC.''

[g2flyer]

Reviewable status: 10 of 11 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

Documentation/devel/encfiles.rst line 289 at r2 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

i guess you refer to the buf.index=1 and buf.output_len=0x80 from ipf\_import\_metadata\_key. These are artifacts of the KDF construction of NIST-SP800-108: The index is used to handle the case where you need more key-material than a single invocation of a PRF (in this case aes-cmac) is required where each invocation increases the index. And the output length is to fix how many bits you really need (and so different lengths give completely different bitstreams for same label and you can't truncate the longer one to get the shorter one). As we are generating AES-128 keys, hence the 128.
So i would drop these constants altogether and probably rephrase the paragraph along the lines of
"Then in step 3, a NIST SP800-108 KDF (Key Derivation Function) is used to derive a sub-key, with input materials being the KDK, the hard-coded label SGX-PROTECTED-FS-METADATA-KEY and the salt as nonce." (Note in SP800-104 the place we put salt is called nonce, hence my reference to that.)

minor correction to my suggestion: s/to derive a sub-key/to derive a AES-128 sub-key/ (hence also implicitly fixing the 1 & 128 constants)

[dimakuv]

Reviewable status: 2 of 11 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @g2flyer)

---

Documentation/devel/encfiles.rst line 289 at r2 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

minor correction to my suggestion: s/to derive a sub-key/to derive a AES-128 sub-key/ (hence also implicitly fixing the 1 & 128 constants)

Done. Thanks for all the info, fixed according to your suggestion!

Btw, the standard is pretty easy to read, included a link to it in the text (https://csrc.nist.gov/pubs/sp/800/108/r1/upd1/final)

---

Documentation/devel/encfiles.rst line 62 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

Given that you mention this already in features (as it is more general than PF), why not just add here a link to that section along the lines of "See also [link] for additional discussion on file metadata" or alike?

Done. (I cannot insert a more specific link to the File Systems chapter because I don't understand how to do such cross-linking between RestructedText and Markdown files.)

---

Documentation/devel/encfiles.rst line 101 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

Of course an application should always check return codes and alike. But in general applications are written under the assumption nothing malicious happen under the cover. E.g., an adversary could try to inject some faults and alike. That said, there probably more effective way to do such than causing random read or write failures so probably not worth. (And as far as i can tell, after an integrity failure, no read or write information would ever work again (i.e., there is no way to selectively suppress, say, some writes, or alike. Certainly looked like that in the code and i see now you also added a corresponding text below.)

Hm, ok, I won't create a GitHub issue for this, since (1) this behavior is already described in this doc, and (2) we both agree that it's probably not worth it. If anyone else comes with a legitimate use case, then we'll resurrect the discussion.

---

Documentation/devel/encfiles.rst line 133 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

Following up to comment elsewhere on constants 1 and 128, suggest changing here also to something like
"Sub-keys are derived from the user-supplied KDK using the NIST SP800-108 construction, with the required PRF instantiated by AES-128-CMAC.''

Done.

[dimakuv]

Reviewable status: 2 of 11 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @g2flyer)

---

Documentation/devel/encfiles.rst line 8 at r5 (raw file):

   This is a living document. The last major update happened in **April 2024**
   and closely corresponds to Gramine v1.6.

During rebase, change this to v1.7 (released in the meantime).

Also, probably it won't be April 2024.

[dimakuv]

Reviewable status: 2 of 11 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @g2flyer)

---

Documentation/devel/encfiles.rst line 62 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. (I cannot insert a more specific link to the File Systems chapter because I don't understand how to do such cross-linking between RestructedText and Markdown files.)

Update: apparently I can't even insert an RST reference to the Markdown file... I circumvented this by specifying a normal HTTPS link.

[g2flyer]

I ran out of more suggestions to give, i.e., i approve (but just in this text as i don't seem to have the rights to in reviewable to actually hit the approve button)

Reviewed 8 of 9 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners