[vbox-dev] [PATCH] Removed chroot comment in redhat_postinstall.sh

[Timothy Tacker]

Looks like the redhat_postinstall.sh script automatically binds the cdrom
into jail when it needs to do so. The relevant section of the script is as
follows:

#
> # We want the ISO available inside the target jail.
> #
> if [ -d "${MY_TARGET}${MY_CHROOT_CDROM}" ]; then
>     MY_RMDIR_TARGET_CDROM=
> else
>     MY_RMDIR_TARGET_CDROM="yes"
>     log_command mkdir -p ${MY_TARGET}${MY_CHROOT_CDROM}
> fi
>
> if [ -f "${MY_TARGET}${MY_CHROOT_CDROM}/vboxpostinstall.sh" ]; then
>     MY_UNMOUNT_TARGET_CDROM=
>     echo "** binding cdrom into jail: already done" | tee -a
> "${MY_LOGFILE}"
> else
>     MY_UNMOUNT_TARGET_CDROM="yes"
>     log_command mount -o bind "${MY_CDROM_NOCHROOT}"
> "${MY_TARGET}${MY_CHROOT_CDROM}"
>     if [ -f "${MY_TARGET}${MY_CHROOT_CDROM}/vboxpostinstall.sh" ]; then
>         echo "** binding cdrom into jail: success"  | tee -a
> "${MY_LOGFILE}"
>     else
>         echo "** binding cdrom into jail: failed"   | tee -a
> "${MY_LOGFILE}"
>     fi
>     if [ "${MY_DEBUG}" = "yes" ]; then
>         log_command find "${MY_TARGET}${MY_CHROOT_CDROM}"
>     fi
> fi
>

As previously mentioned, the redhat_postinstall.sh script fails with the
Kickstart default to execute inside chroot; but it succeeds with
--nochroot on the %post section in the Kickstart script. I suggest this
demonstrates the comment provides incorrect information--the script does
NOT expect to be running chrooted; and in fact, it will fail under that
condition. No alternative language for the comment has been offered, so I
propose the change be incorporated in the patch as submitted. Still happy
to hear alternative views, if anyone has them. Thanks!

Timothy Tacker


On Mon, Jul 20, 2020 at 5:46 PM Timothy Tacker <
timothytacker+virtualbox at gmail.com> wrote:

> Valdis, thank you for your feedback.
>
> As you may know, --nochroot is used on %post sections in Kickstart files
> to tell Anaconda to run post install scripts without chroot. The default is
> to run them with chroot; but that's a default in Anaconda, not Virtual box.
>
> I used the redhat67_ks.cfg Kickstart file VirtualBox includes as a
> template to write a customized Kickstart file for use with CentOS 7, but I
> didn't include --nochroot after seeing the comment. The
> redhat_postinstall.sh script failed, but it worked with --nochroot. It took
> me a while to figure that out though, because I trusted the information the
> comment provided to be correct.
>
> I assume the Kickstart files for Red Hat distributions are all using
> --nochroot because the redhat_postinstall.sh script included with
> VirtualBox doesn't work with chroot, someone else already realized that,
> and they included --nochroot accordingly.
>
> I would have changed the comment to state the script expects NOT to be
> running chrooted, but I don't know if that's true in every case. I haven't
> tested other distributions, so I don't know if there's variation on this
> between older distributions, or even CentOS and Red Hat Enterprise Linux. I only
> have reason to believe it's true in some, or at least this one case; and if
> that's correct, then the comment provides incorrect information, which has
> potential to mislead other people, as it did me.
>
> I agree with you that an analysis of the code would be desirable; and if
> anyone has enough familiarity with the code in the redhat_postinstall.sh
> script, bandwidth to do an analysis, and would like to volunteer, that
> would be appreciated.
>
> Due to changes in Red Hat 8, I'm also writing a Kickstart file for CentOS
> 8. This may require me looking closer at redhat_postinstall.sh and becoming
> more familiar with it. I can't commit to it; but if this becomes the case,
> I may not only contribute the new Kickstart file but may also be able to
> answer some of your other questions. Depending on schedule, this may take a
> while; so in the interim, I thought it best to try to avoid others any
> problems that may be caused by the comment. It's not an ultimate, or ideal,
> solution; but it seemed a small, quick, and iterative improvement,
> nonetheless.
>
> With the above in mind, would you be amenable to suggesting different
> language for the comment, which at least bring the issue to the attention
> of the reader?
>
> Timothy Tacker
>
> On Sun, Jul 19, 2020, 11:30 PM Valdis Klētnieks <valdis.kletnieks at vt.edu>
> wrote:
>
>> On Sun, 19 Jul 2020 02:48:22 -0500, Timothy Tacker said:
>>
>> > All Kickstart files for Red Hat distributions in the UnattendedTemplates
>> > directory run the redhat_postinstall.sh script with --nochroot in the
>> %post
>> > section.
>>
>> > Nonetheless, the redhat_postinstall.sh script includes the following
>> > comment:
>> > # Note! This script expects to be running chrooted (inside new sytem).
>> >
>> > The patch below removes this comment. I'm licensing this patch under the
>> > MIT license. Please review and consider integrating. Feedback is
>> welcome.
>> > Thanks!
>>
>> I'd be leery of a patch that simply discards a comment - it obviously
>> *used*
>> to be important enough to make the note.  Probably what is *actually*
>> needed
>> here is an analysis of what the code used to do, what it does now, and
>> whether
>> it should be refactored to make --nochroot the default rather than every
>> single
>> user having to specify it.
>>
>> Homework problem for the student - if the script doesn't actually expect
>> to
>> be in a chroot by default, why are all the calls passing --nochroot? What
>> change
>> in behavior does that cause? And what can go wrong if somebody doesn't
>> know
>> it expects to be chrooted?
>>
>> (Sorry, over the last four decades I've seen entirely too many "let's
>> remove
>> the comment" patches that ended up sprouting a CVE because somebody didn't
>> know something important because it wasn't documented...)
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.virtualbox.org/pipermail/vbox-dev/attachments/20200807/b29e9233/attachment.html>