cp-40190 Steps towards preventing SWTPM from filling dom0 root partition #4841
Conversation
… swtpm-wrapper. Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>
xennifer
force-pushed
the
private/jenniferhe/cp-40190
branch
from
ddb681b
to
baaeff5
Compare
ocaml/xenopsd/scripts/swtpm-wrapper
Outdated
| n += 1 | ||
|
|
||
| uri_sep = tpm_state.find("://") |
There was a problem hiding this comment.
urlparse is in the standard library and should be used here:
urlparse('dir://tpm2-00.permall')
ParseResult(scheme='dir', netloc='tpm2-00.permall', path='', params='', query='', fragment='')
ocaml/xenopsd/scripts/swtpm-wrapper
Outdated
|
|
||
| if (tpm_state_prefix == "dir"): | ||
| tpm_uri = tpm_dir | ||
| tpm_file = os.path.join(tpm_dir, tpm_state_file) |
There was a problem hiding this comment.
tpm_file has the same definiton in both branches, should be defined before to avoid repetition
ocaml/xenopsd/scripts/swtpm-wrapper
Outdated
| return False | ||
|
|
||
| # Check if block device has non-zero header | ||
| file = open(fname, "r") |
There was a problem hiding this comment.
I like with open as file as you will never miss to close a file.
ocaml/xenopsd/scripts/swtpm-wrapper
Outdated
| tpm_path = tpm_dir | ||
| depriv = True | ||
|
|
||
| n= 3 | ||
| n = 4 |
There was a problem hiding this comment.
Put this higher and use if len(argv) < n above.
ocaml/xenopsd/scripts/swtpm-wrapper
Outdated
|
|
||
| except OSError as error: | ||
| print(error) | ||
| return | ||
|
|
||
| tpm_path = '/' | ||
| tpm_uri = tpm_state | ||
|
|
||
| if (tpm_state_prefix == "file"): |
There was a problem hiding this comment.
Should this be in the if depriv section?
There was a problem hiding this comment.
It all about restricting priveladge, so yes?
ocaml/xenopsd/scripts/swtpm-wrapper
Outdated
| @@ -132,21 +176,32 @@ def main(argv): | |||
| os.chown(tpm_dir, uid, uid) | |||
There was a problem hiding this comment.
I don't think this is right. If the tpm_dir is owned by the same uid as the swtpm process, then it can change the permissions back to 07xx. If the tpm_dir is owned by root, that can't happen since only the owner can change permissions.
xennifer
force-pushed
the
private/jenniferhe/cp-40190
branch
2 times, most recently
from
d9453b9
to
cd989d8
Compare
ocaml/xenopsd/scripts/swtpm-wrapper
Outdated
| with open(fname, "r") as file: | ||
| hdr = file.read(8) | ||
|
|
||
| if len(hdr) == 8 and hdr != "\0\0\0\0\0\0\0\0": |
There was a problem hiding this comment.
It would be better to see if there is a magic number you can use here instead.
The source code defines SWTPM_NVSTORE_LINEAR_MAGIC - does that work?
There was a problem hiding this comment.
I based this code on what it does to 'delete' the tpm. It always zeros it.
I only wanted the equivelant of 'if the file exists' which we had before.
I suppose the magic number would work too, I didnt try.
| if (tpm_state_scheme != "file"): | ||
| os.chown(tpm_dir, uid, uid) | ||
| else: | ||
| os.chown(tpm_dir, 0 , uid) |
There was a problem hiding this comment.
Just noting here that this works because xenopsd creates tpm_dir as 0750. After swtpm switches uid/gid, the process gid matches the dir gid and allows swtpm to read files in tpm_dir but not create new ones. It can't change the directory permissions either because that is reserved for the directory owner.
We need to ensure that the xenospd side doesn't ever change to create tpm_dir with more lax permissions at some point in the future.
There was a problem hiding this comment.
Maybe it should chmod 0750 here as well to ensure that?
Signed-off-by: Jennifer Herbert <jennifer.herbert@citrix.com>
xennifer
force-pushed
the
private/jenniferhe/cp-40190
branch
from
cd989d8
to
0373161
Compare
Steps are made so the main swtpm software dose not need to create any files on the filesytem.
This incluse:
Note that the switching to linear file is to be controlled by xenopsd, using a parameter. Prevously this was incorrectly set as
'file' type when it should be 'dir' type. This has been changed. When xenopd is ready to use linear state file, it should pass the 'file' type. Until this is done, swtpm is allowed to create files, and is hence, still a small risk.