Fine-tune runc-related permissions #316
Conversation
runtime/runc_jailer.go
Outdated
| if err := copyFile(drivePath, newDrivePath, info.Mode()); err != nil { | ||
| // If the file will be mounted as read-only, drop write permissions | ||
| mode := info.Mode() | ||
| if *d.IsReadOnly { |
There was a problem hiding this comment.
This value could be nil.
if firecracker.BoolValue(d.IsReadOnly) {
}I'm pretty sure we set the drives to always be a value, but would prefer to be safe here and always nil check.
Signed-off-by: Kazuyoshi Kato <[email protected]>
Signed-off-by: Kazuyoshi Kato <[email protected]>
Signed-off-by: Kazuyoshi Kato <[email protected]>
It won't contain any sensitive values though. Signed-off-by: Kazuyoshi Kato <[email protected]>
Signed-off-by: Kazuyoshi Kato <[email protected]>
Signed-off-by: Kazuyoshi Kato <[email protected]>
| info, err := os.Stat(drivePath) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to stat drive %q", drivePath) | ||
| mode := 0600 |
There was a problem hiding this comment.
Shouldn't we use the original mode? This kind of couples this to the stub drive handler knowing that it chmods it to I'd rather use the original mode of the file that the user provided.0600.
There was a problem hiding this comment.
While the stub drive handler is using the same bits, the intention is picking the minimal permission bits we need to make this file both readable and writable from the Firecracker process which is running as the jailer user.
Even if we need to change the permission bits on the handler side (actually, we might be able to make the dummy file read-only, assuming that actual writes are happening on a different file...?), we don't have to change the bits here.
There was a problem hiding this comment.
This is for non-stub drives. These are the drives users have added themselves. So, we want them to be what users have specified.
There was a problem hiding this comment.
I'm not sure what reason we have for using the permissions of the original file. We are creating a new file inside the jail (even if its contents are a copy of that on the outside) and we can know that this new file inside the jail only needs 0600 or 0400. What advantage is there to making the permissions match what's present on the file outside the jail?
There was a problem hiding this comment.
As far as I understand, because of the permission of the parent directory, files which are copied to this directory are anyway not readable/writable from anyone else.
If we want to keep the original permissions, we probably re-think about the permissions on the parent directory to make a specific scenario possible.
There was a problem hiding this comment.
Users may expect them to be of a certain mode as they may want to do something with those drives themselves. Firecracker's jailer does the same thing and doesn't chown or chmod any of the drives that users provide. However, if we feel that there isn't a need to use the original mode of the file, I can be convinced otherwise. I was just following Firecracker's approach and it seemed reasonable to me.
There was a problem hiding this comment.
As far as I understand, because of the permission of the parent directory, files which are copied to this directory are anyway not readable/writable from anyone else.
That may be true but if we are going for full-out defense in-depth (which we may as well here since there's basically no cost to setting permissions on a file in terms of complexity IMO) then I still think it makes to set every individual permission as minimal as possible.
Users may expect them to be of a certain mode as they may want to do something with those drives themselves.
These files are not the original ones created by the users, they are created just for the jail we are creating (right?). I don't think we should operate under the assumption that users should feel free to directly interact arbitrarily with jailed files (they obviously are free to interact with the original files outside the jail) as the point of the jail is to create something isolated from the rest of the system with very fine grained permissions. I just don't see what use-cases we'd be enabling by trying to match the permissions, but if there are any I'm not thinking of let me know.
There was a problem hiding this comment.
The drives may be a copy right now, but I think in the future this will be changed to be a bind mount. So having them be able to do things seems feasible, the question here is should we allow it? Maybe reading the drive is something they may want to do? However, if we bind mount modifying anything will cause an error. I want to stay consistent with the level of jailing we do. If we suddenly stop allowing for drives to be read only and owned by the jailer, then that should be true for bind mounting. However, that would mean we can't bind mount with ro, and we'd be modifying the user's drive which is something we definitely do not want.
There was a problem hiding this comment.
Yes if we change to a bind-mount in the future then all of this and much more would change. But at the moment we're doing a copy and until we switch to a different approach, why wouldn't we just go with the least possible privilege we are aware there is a need for? There's no expectation for 100% backwards compatibility (changing to a bind-mount would be a much larger break anyways) and I can't imagine any scenario in which we'd ever want users to directly interact with files in the jail. If they really, really want to, they can be root and which point all of this is moot anyways.
Issue #, if available:
Description of changes:
While most of them are harmless to have, it may be better to keep files less readable/writable when we can.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.