Comment 39 for bug 1878234

On Mon, May 18, 2020 at 3:50 PM Christophe de Dinechin
<email address hidden> wrote:
>
> Peng Tao,
>
> > a single enable/disable flag is much easier to implement and maintain
> than a white-list.
>
> 1) It's not really simpler. It's a flag check instead of a function
> call. The three-state variant you suggested in #16 is clearly more
> complicated, which is when I noticed that regexps were addressing your
> concerns while keeping the patch simple.
>
> 2) It's not a long term solution, but a "quick fix" that does not
> address the criteria I outlined in #25. This implies that we would then
> need to replace it with something else. Do we want to offer an option to
> user, then walk it back and offer another one later? Why would you
> prefer that?
>
> 3) It's also not simpler in the sense that there is no patch for it.
> Again, I really invite you to focus on the existing patch, and check if
> it solves the problem and if it can be amended later (i.e. if it would
> not require a walk-back).
>
We do not have to take it back if the initial option is
`config_via_annotation = disabled`. We first accept `enabled` and
`disabled`. Later extend it with an config option whitelist, which
lists exactly which option can be changed via annotations.

> > The latter one is more complex than you think.
>
> Why do you think so? What is missing in my patch? Have you reviewed my
> patch?
>
>
> > All the options I listed in my previous reply are not
> > supposed to be altered by a random user.
>
> But they can be altered by random users today.
>
And that need to be fixed and that's why I am calling for a long term
solution. I do not want to stop at just fixing the binary paths. I am
supporting your solution because I think you would extend it to other
options as well.

> This is another issue with the simple flag: it only gives back
> functionality to those who need it by re-exposing the bug entirely. This
> is what I called a "risk" in my response to you in comment #25.
>
> > My fear is that you might end up with filtering just the binary paths.
>
> Have you reviewed my patch?
>
> It does filter only paths, because right now this is where the bug is.
> You cannot remote execute with other annotations. The dangerous bug
> today is remote execution.
>
> I am not saying there is no risk on other annotations. However, I have
> reviewed them, and so far, my evaluation of the risk for all other
> annotations is that it is negligible *relative to* the ability to
> execute arbitrary code.
>
They are less dangerous but they must be fixed as well. And we should
fix all of them in one solution. That is the whole point why I am
discussing the issue all the time.

> Specifically:
>
> - Annotations that configure the guest kernel, including boot options,
> kernel image, guest hook paths, etc, are normally contained by the VM
> layer. It may make administrative sense to provide similar filtering for
> them, but it's not a security issue for the host unless there is a flaw
> in the hypervisor.
>
> - Annotations that configure the guest, e.g. memory size, do require
> scrutiny. For example, memory could be use for denial-of-service
> attacks. We need to have a discussion on the mailing list on how to fix
> it. While there is a security risk, it is somewhat second-order relative
> to the ability to copy all data from the host to some remote machine or
> wipe its content clean.
>
> > I would like to ask what is the long term plan going your direction?
>
> I will split your question in three:
>
> a) Is there a long term plan for the path filtering options in the patch
> I submitted: I tried to design them to address your earlier comment #5
> and #8. I explained why a *global* option does not work, but I modified
> the *per-path* option to address your concern about making "free-for-
> all" possible (although the regexp approach has the additional benefit
> that you can restrict the free-for all, e.g. allow only /usr/bin/qemu.*
> and not any path).
>
> b) Is there a long term plan for other annotations: Yes, they need to be
> reviewed carefully. I have convinced myself that there are secondary
> security holes in other annotations that can be addressed later,
> starting with memory allocation. I can start working on this if you want
> me to.
>
> c) Is there a long-term plan for controlling the annotation feature
> itself: Re-reading your comments, I realize I may have missed one
> possible interpretation, which is that you believed my patch filtered
> annotations by name, instead of filtering the value of the annotations.
> In other words, my current patch filters what values of hypervisor.path
> are allowed, it does not enable/disable the hypervisor.path annotation.
> We could decide that it makes sense to also have an "annotations"
> configuration that filters which annotations names are whitelisted.
>
Yes, that is exactly the missing part. And I was hoping you could add
it maybe in later patches. There are options we do not want an end
user to change in any case. And I want to make sure we have an
agreement that it will be fixed. That is why I've been asking about a
long term solution that does not just stop at binary path filtering.

> > I would prefer a simple boolean flag (which is also a quick fix)
> > if there is no long term plan on how the other options are handled.
>
> I understand. But please review my patch, as I believe now that it may
> not be exactly what you thought it was.
>
>
> > To be specific, I was asking:
> > 1. will there be a classification of user-modifiable options and non-user-modifiable options?
> > 2. will sysadmin be able to modify the classification?
>
> This is my point c) above. I believe this is easily added, but it's
> somewhat orthogonal to the proposed fix. It would not be sufficient from
> a security point of view, because the security issue arises from being
> able to pass arbitrary values to hypervisor.path, not from the feature
> itself. In other words, if you give a mechanism to whitelist the
> "hypervisor.path" feature, as soon as you whitelist it, the security
> risk is back. So even if we implement annotation name whitelisting, we
> still need annotation value whitelisting.
>
The classification does not conflict with your whitelist filtering.
Instead, the workflow is like:
1. If an option is not in user-modifiable classification, do not
change it via annotation
2. If it is user-modifiable, and if the annotation value is within the
per-annotation whitelist, then change it

> > If I understand your messages correctly, the answers are two YES.
> > Then I agree with your solution. But please confirm if we are on the same page.
>
> I believe the answer is yes, but please check by reviewing the patch.
>
Yes, I did look through your patch. And I agree it can be used as a
quick fix. My intension was to confirm that we are on the same page
about how to extend it as a complete long term solution.

> > And w.r.t. filing new CVEs, anyone can file a CVE.
> > VMT will be responsible for triaging the security issue and coordinate
> > the progressive disclosure of a vulnerability. We've handled several
> > CVEs before and they all come from outside reporters.
>
> Let me make the questions more direct: do you want me to open the CVE
> or is is more polite to let someone from the kata VMT do it? I just
> want to make sure someone feels they "own" this.
>
Yes, please feel free to open a CVE for kata. You deserve the credit.

Cheers,
Tao
--
Into Sth. Rich & Strange