Re: [Xen-devel] [PATCH 5/5] RFC: test/depriv: Add a tool to check process-level depriv
George Dunlap writes ("Re: [PATCH 5/5] RFC: test/depriv: Add a tool to check
process-level depriv"):
> Oh, actually, 65534 is "nogroup", which is the default when you don't
> add a specific group.
>
> Should we recommend creating a separate group for the Xen qemus in our
> feature doc? Or should we just mention the possibility, but leave the
> actual example to the default (which will normally end up with the
> `nogroup` group)?
`nogroup' isn't as big a problem in general as `nobody'. (No
processes may ever run as nobody because to avoid unintendedly
permitting access, such a non-id must either have no principals or no
objects, and a process running with a particular uid is both; whereas
running as a particular group does not turn a process into an object
accessible via that group.)
But it's still probably best avoided in case of mistakes. Also
assigning a group to all the qemus may make some kinds of
configuration applicable to all of them easier.
So I think we should recommend creating one group for this.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 5/5] RFC: test/depriv: Add a tool to check process-level depriv
On 10/08/2018 05:28 PM, Anthony PERARD wrote:
> On Fri, Oct 05, 2018 at 05:57:01PM +0100, George Dunlap wrote:
>> +# TEST: Process / group id
>> +#
>> +# Read /proc//status, checking Uid and Gid lines
>> +#
>> +# Uid should be xen-qemuuser-range-base+$domid
>> +# Gid should be 65534 ("nobody")
>
> That is wrong. Gid doesn't have to be nobody. gid can be chosen when
> creating the base user id. (And I'm pretty sure "nobody" should be
> avoided.)
Oh, actually, 65534 is "nogroup", which is the default when you don't
add a specific group.
Should we recommend creating a separate group for the Xen qemus in our
feature doc? Or should we just mention the possibility, but leave the
actual example to the default (which will normally end up with the
`nogroup` group)?
>
>> +# FIXME: deal with other UID configurations?
>> +echo -n "Process UID: "
>> +tgt_uid=$(id -u xen-qemuuser-range-base)
>> +tgt_uid=$(( $tgt_uid + $domid ))
>> +
>> +# Example input:
>> +# Uid: 1193119311931193
>> +input=$(grep ^Uid: /proc/$dmpid/status)
>> +if [[ "$input" =~
>> ^Uid:[[:space:]]+([0-9]+)[[:space:]]+([0-9]+)[[:space:]]+([0-9]+)[[:space:]]+([0-9]+)$
>> ]] ; then
>> +result="PASSED"
>> +for i in {1..4}; do
>> +if [[ "${BASH_REMATCH[$i]}" != "$tgt_uid" ]] ; then
>> +result="FAILED"
>> +failed="true"
>> +break
>> +fi
>> +done
>> +else
>> +result="FAILED"
>> +failed="true"
>> +fi
>> +echo $result
>> +
>> +# Example input:
>> +# Gid: 10020 10020 10020 10020
>> +echo -n "Process GID: "
>> +tgt_gid=$(id -g nobody)
>
> This should be `id -g xen-qemuuser-range-base`.
Got it
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 5/5] RFC: test/depriv: Add a tool to check process-level depriv
George Dunlap writes ("Re: [PATCH 5/5] RFC: test/depriv: Add a tool to check
process-level depriv"):
> FYI I do agree with all of those suggestions (both `set -e` and having
> functions to handle failure in a consistent way); but I didn't want to
> fix everything up in bash only to have to write it over again in perl
> (should I decide to do so). I wasn't expecting a review for this patch
> yet; I only included it for completeness. I guess I figured "I didn't
> make all the changes you wanted" would make that obvious, but next time
> I'll be more explicit when I don't expect a patch to be reviewed. :-)
Err, right, good. I'm glad I didn't try to review it anyway :-).
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 5/5] RFC: test/depriv: Add a tool to check process-level depriv
On 10/26/2018 03:06 PM, Ian Jackson wrote:
> George Dunlap writes ("[PATCH 5/5] RFC: test/depriv: Add a tool to check
> process-level depriv"):
>> Add a tool to check whether the various process-level deprivileging
>> operations have actually taken place on the process.
> ...
>> NB that a number of other requested changes (such as using `set -e`,
>> changing the output, ) have not been made, while I consider whether
>> to leave this as a stand-alone script, or whether to merge osstest's
>> fd checker functionality into it (perhaps changing the language to perl
>> at the same time).
>
> OK. But, unfortunately, it is very hard to review a shell script that
> is written without `set -e'. Generally a big focus of my usual review
> style is error handling.
>
> Also, I suggested some refactoring. Seeing the script as it is makes
> it more obvious that a systematic approach to printing FAILED
> etc. would be a good idea.
FYI I do agree with all of those suggestions (both `set -e` and having
functions to handle failure in a consistent way); but I didn't want to
fix everything up in bash only to have to write it over again in perl
(should I decide to do so). I wasn't expecting a review for this patch
yet; I only included it for completeness. I guess I figured "I didn't
make all the changes you wanted" would make that obvious, but next time
I'll be more explicit when I don't expect a patch to be reviewed. :-)
Thanks,
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 5/5] RFC: test/depriv: Add a tool to check process-level depriv
George Dunlap writes ("[PATCH 5/5] RFC: test/depriv: Add a tool to check
process-level depriv"):
> Add a tool to check whether the various process-level deprivileging
> operations have actually taken place on the process.
...
> NB that a number of other requested changes (such as using `set -e`,
> changing the output, ) have not been made, while I consider whether
> to leave this as a stand-alone script, or whether to merge osstest's
> fd checker functionality into it (perhaps changing the language to perl
> at the same time).
OK. But, unfortunately, it is very hard to review a shell script that
is written without `set -e'. Generally a big focus of my usual review
style is error handling.
Also, I suggested some refactoring. Seeing the script as it is makes
it more obvious that a systematic approach to printing FAILED
etc. would be a good idea.
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 5/5] RFC: test/depriv: Add a tool to check process-level depriv
Anthony PERARD writes ("Re: [PATCH 5/5] RFC: test/depriv: Add a tool to check
process-level depriv"):
> On Fri, Oct 05, 2018 at 05:57:01PM +0100, George Dunlap wrote:
> > +# TEST: Process / group id
> > +#
> > +# Read /proc//status, checking Uid and Gid lines
> > +#
> > +# Uid should be xen-qemuuser-range-base+$domid
> > +# Gid should be 65534 ("nobody")
>
> That is wrong. Gid doesn't have to be nobody. gid can be chosen when
> creating the base user id. (And I'm pretty sure "nobody" should be
> avoided.)
The gid is not really relevant but nobody is sometimes chosen as a gid
that no process has so is perhaps a poor choice. A single specific
gid for all of these would be fine.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 5/5] RFC: test/depriv: Add a tool to check process-level depriv
On Fri, Oct 05, 2018 at 05:57:01PM +0100, George Dunlap wrote:
> +# TEST: Process / group id
> +#
> +# Read /proc//status, checking Uid and Gid lines
> +#
> +# Uid should be xen-qemuuser-range-base+$domid
> +# Gid should be 65534 ("nobody")
That is wrong. Gid doesn't have to be nobody. gid can be chosen when
creating the base user id. (And I'm pretty sure "nobody" should be
avoided.)
> +# FIXME: deal with other UID configurations?
> +echo -n "Process UID: "
> +tgt_uid=$(id -u xen-qemuuser-range-base)
> +tgt_uid=$(( $tgt_uid + $domid ))
> +
> +# Example input:
> +# Uid: 1193119311931193
> +input=$(grep ^Uid: /proc/$dmpid/status)
> +if [[ "$input" =~
> ^Uid:[[:space:]]+([0-9]+)[[:space:]]+([0-9]+)[[:space:]]+([0-9]+)[[:space:]]+([0-9]+)$
> ]] ; then
> +result="PASSED"
> +for i in {1..4}; do
> + if [[ "${BASH_REMATCH[$i]}" != "$tgt_uid" ]] ; then
> + result="FAILED"
> + failed="true"
> + break
> + fi
> +done
> +else
> +result="FAILED"
> +failed="true"
> +fi
> +echo $result
> +
> +# Example input:
> +# Gid: 10020 10020 10020 10020
> +echo -n "Process GID: "
> +tgt_gid=$(id -g nobody)
This should be `id -g xen-qemuuser-range-base`.
--
Anthony PERARD
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
