[PATCH linux-next] kunit: tool: use absolute path for wget

[PATCH linux-next] kunit: tool: use absolute path for wget

[cgel.zte]

From: Xu Panda <xu.panda@zte.com.cn>

Not using absolute path when invoking wget can lead to serious
security issues.

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Xu Panda <xu.panda@zte.com.cn>
---
 tools/testing/kunit/qemu_configs/riscv.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/kunit/qemu_configs/riscv.py b/tools/testing/kunit/qemu_configs/riscv.py
index 6207be146d26..c3dcd654ca15 100644
--- a/tools/testing/kunit/qemu_configs/riscv.py
+++ b/tools/testing/kunit/qemu_configs/riscv.py
@@ -11,7 +11,7 @@ if not os.path.isfile(OPENSBI_FILE):
              'Would you like me to download it for you from:\n' + GITHUB_OPENSBI_URL + ' ?\n')
        response = input('yes/[no]: ')
        if response.strip() == 'yes':
-               os.system('wget ' + GITHUB_OPENSBI_URL)
+               os.system('/usr/bin/wget ' + GITHUB_OPENSBI_URL)
        else:
                sys.exit()

-- 
2.15.2

Re: [PATCH linux-next] kunit: tool: use absolute path for wget

[David Gow]

On Thu, Sep 22, 2022 at 4:36 PM <cgel.zte@gmail.com> wrote:
>
> From: Xu Panda <xu.panda@zte.com.cn>
>
> Not using absolute path when invoking wget can lead to serious
> security issues.
>
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: Xu Panda <xu.panda@zte.com.cn>
> ---

This seems mostly okay to me -- we'd be abandoning people who have
wget in an unusual location, but I don't think there are many people
who want to run KUnit under RISC-V, have wget in a non-standard
location, and can't acquire the bios file themselves.

So this is:
Reviewed-by: David Gow <davidgow@google.com>

However, would a patch like this make _more_ sense? It looks like (at
least on Debian and Arch), the OpenSBI bios is installed as part of
the appropriate qemu package anyway, into a standard location.
---
diff --git a/tools/testing/kunit/qemu_configs/riscv.py
b/tools/testing/kunit/qemu_configs/riscv.py
index 6207be146d26..12a1d525978a 100644
--- a/tools/testing/kunit/qemu_configs/riscv.py
+++ b/tools/testing/kunit/qemu_configs/riscv.py
@@ -3,17 +3,13 @@ import os
import os.path
import sys

-GITHUB_OPENSBI_URL =
'https://github.com/qemu/qemu/raw/master/pc-bios/opensbi-riscv64-generic-fw_dynamic.bin'
-OPENSBI_FILE = os.path.basename(GITHUB_OPENSBI_URL)
+OPENSBI_FILE = 'opensbi-riscv64-generic-fw_dynamic.bin'
+OPENSBI_PATH = '/usr/share/qemu/' + OPENSBI_FILE

-if not os.path.isfile(OPENSBI_FILE):
-       print('\n\nOpenSBI file is not in the current working directory.\n'
-             'Would you like me to download it for you from:\n' +
GITHUB_OPENSBI_URL + ' ?\n')
-       response = input('yes/[no]: ')
-       if response.strip() == 'yes':
-               os.system('wget ' + GITHUB_OPENSBI_URL)
-       else:
-               sys.exit()
+if not os.path.isfile(OPENSBI_PATH):
+       print('\n\nOpenSBI bios was not found in "' + OPENSBI_PATH + '".\n'
+             'Please ensure that qemu-system-riscv is installed, or
edit the path in "qemu_configs/riscv.py"\n')
+       sys.exit()

QEMU_ARCH = QemuArchParams(linux_arch='riscv',
                          kconfig='''
@@ -29,4 +25,4 @@ CONFIG_SERIAL_EARLYCON_RISCV_SBI=y''',
                          extra_qemu_params=[
                                          '-machine', 'virt',
                                          '-cpu', 'rv64',
-                                          '-bios',
'opensbi-riscv64-generic-fw_dynamic.bin'])
+                                          '-bios', OPENSBI_PATH])
---

That way, we could avoid using wget at all. (I did confirm that this
is the only use of it anywhere in kunit_tool.)

The other options would be to use some python library to download it?

Thoughts?
-- David

>  tools/testing/kunit/qemu_configs/riscv.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/kunit/qemu_configs/riscv.py b/tools/testing/kunit/qemu_configs/riscv.py
> index 6207be146d26..c3dcd654ca15 100644
> --- a/tools/testing/kunit/qemu_configs/riscv.py
> +++ b/tools/testing/kunit/qemu_configs/riscv.py
> @@ -11,7 +11,7 @@ if not os.path.isfile(OPENSBI_FILE):
>               'Would you like me to download it for you from:\n' + GITHUB_OPENSBI_URL + ' ?\n')
>         response = input('yes/[no]: ')
>         if response.strip() == 'yes':
> -               os.system('wget ' + GITHUB_OPENSBI_URL)
> +               os.system('/usr/bin/wget ' + GITHUB_OPENSBI_URL)
>         else:
>                 sys.exit()
>
> --
> 2.15.2
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220922083610.235936-1-xu.panda%40zte.com.cn.

Re: [PATCH linux-next] kunit: tool: use absolute path for wget

[Greg KH]

On Thu, Sep 22, 2022 at 06:09:28PM +0800, David Gow wrote:
> On Thu, Sep 22, 2022 at 4:36 PM <cgel.zte@gmail.com> wrote:
> >
> > From: Xu Panda <xu.panda@zte.com.cn>
> >
> > Not using absolute path when invoking wget can lead to serious
> > security issues.
> >
> > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > Signed-off-by: Xu Panda <xu.panda@zte.com.cn>
> > ---
> 
> This seems mostly okay to me -- we'd be abandoning people who have
> wget in an unusual location, but I don't think there are many people
> who want to run KUnit under RISC-V, have wget in a non-standard
> location, and can't acquire the bios file themselves.
> 
> So this is:
> Reviewed-by: David Gow <davidgow@google.com>

Please no, at this point in time, submissions from this gmail "alias"
are going to have to be rejected from the kernel.

greg k-h

Re: [PATCH linux-next] kunit: tool: use absolute path for wget

[David Gow]

On Thu, Sep 22, 2022 at 6:20 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 22, 2022 at 06:09:28PM +0800, David Gow wrote:
> > On Thu, Sep 22, 2022 at 4:36 PM <cgel.zte@gmail.com> wrote:
> > >
> > > From: Xu Panda <xu.panda@zte.com.cn>
> > >
> > > Not using absolute path when invoking wget can lead to serious
> > > security issues.
> > >
> > > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > > Signed-off-by: Xu Panda <xu.panda@zte.com.cn>
> > > ---
> >
> > This seems mostly okay to me -- we'd be abandoning people who have
> > wget in an unusual location, but I don't think there are many people
> > who want to run KUnit under RISC-V, have wget in a non-standard
> > location, and can't acquire the bios file themselves.
> >
> > So this is:
> > Reviewed-by: David Gow <davidgow@google.com>
>
> Please no, at this point in time, submissions from this gmail "alias"
> are going to have to be rejected from the kernel.
>

Good to know, thanks.

This isn't queued anyway, as I think that getting rid of the code to
download the BIOS (and instead relying on the user's distro to provide
it) is probably a better solution.

Cheers,
-- David

Re: [PATCH linux-next] kunit: tool: use absolute path for wget

[Greg KH]

On Thu, Sep 22, 2022 at 06:50:59PM +0800, David Gow wrote:
> On Thu, Sep 22, 2022 at 6:20 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Sep 22, 2022 at 06:09:28PM +0800, David Gow wrote:
> > > On Thu, Sep 22, 2022 at 4:36 PM <cgel.zte@gmail.com> wrote:
> > > >
> > > > From: Xu Panda <xu.panda@zte.com.cn>
> > > >
> > > > Not using absolute path when invoking wget can lead to serious
> > > > security issues.
> > > >
> > > > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > > > Signed-off-by: Xu Panda <xu.panda@zte.com.cn>
> > > > ---
> > >
> > > This seems mostly okay to me -- we'd be abandoning people who have
> > > wget in an unusual location, but I don't think there are many people
> > > who want to run KUnit under RISC-V, have wget in a non-standard
> > > location, and can't acquire the bios file themselves.
> > >
> > > So this is:
> > > Reviewed-by: David Gow <davidgow@google.com>
> >
> > Please no, at this point in time, submissions from this gmail "alias"
> > are going to have to be rejected from the kernel.
> >
> 
> Good to know, thanks.
> 
> This isn't queued anyway, as I think that getting rid of the code to
> download the BIOS (and instead relying on the user's distro to provide
> it) is probably a better solution.s

That's a much better solution, we have authenticated firmware download
paths for BIOS images on Linux now integrated into distros.  Let's use
that infrastructure that is set up for that for this type of thing.

thanks,

greg k-h