Re: [PATCH] docs: kernel_feat.py: fix command injection

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 10 Jan 2024, Vegard Nossum <vegard.nossum@oracle.com> wrote:
> The kernel-feat directive passes its argument straight to the shell.
> This is unfortunate and unnecessary.
>
> Let's always use paths relative to $srctree/Documentation/ and use
> subprocess.check_call() instead of subprocess.Popen(shell=True).
>
> This also makes the code shorter.
>
> This is analogous to commit 3231dd586277 ("docs: kernel_abi.py: fix
> command injection") where we did exactly the same thing for
> kernel_abi.py, somehow I completely missed this one.
>
> Link: https://fosstodon.org/@jani/111676532203641247
> Reported-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

Good stuff, thanks for doing this.

> diff --git a/Documentation/sphinx/kernel_feat.py b/Documentation/sphinx/kernel_feat.py
> index b5fa2f0542a5..b9df61eb4501 100644
> --- a/Documentation/sphinx/kernel_feat.py
> +++ b/Documentation/sphinx/kernel_feat.py
> @@ -37,8 +37,6 @@ import re
>  import subprocess
>  import sys
>  
> -from os import path
> -
>  from docutils import nodes, statemachine
>  from docutils.statemachine import ViewList
>  from docutils.parsers.rst import directives, Directive
> @@ -76,33 +74,26 @@ class KernelFeat(Directive):
>          self.state.document.settings.env.app.warn(message, prefix="")
>  
>      def run(self):
> -
>          doc = self.state.document
>          if not doc.settings.file_insertion_enabled:
>              raise self.warning("docutils: file insertion disabled")
>  
>          env = doc.settings.env
> -        cwd = path.dirname(doc.current_source)
> -        cmd = "get_feat.pl rest --enable-fname --dir "
> -        cmd += self.arguments[0]
> -
> -        if len(self.arguments) > 1:
> -            cmd += " --arch " + self.arguments[1]
>  
> -        srctree = path.abspath(os.environ["srctree"])
> +        srctree = os.path.abspath(os.environ["srctree"])

As a follow-up, please replace the srctree environment variable with

        srctree = os.path.join(env.srcdir, '..')

or something like that. env.srcdir points at the Documentation
directory.

Ditto for the kernel_abi.py extension.

> -        fname = cmd
> +        args = [
> +            os.path.join(srctree, 'scripts/get_feat.pl'),
> +            'rest',
> +            '--enable-fname',
> +            '--dir',
> +            os.path.join(srctree, 'Documentation', self.arguments[0]),
> +        ]
>  
> -        # extend PATH with $(srctree)/scripts
> -        path_env = os.pathsep.join([
> -            srctree + os.sep + "scripts",
> -            os.environ["PATH"]
> -        ])
> -        shell_env = os.environ.copy()
> -        shell_env["PATH"]    = path_env
> -        shell_env["srctree"] = srctree
> +        if len(self.arguments) > 1:
> +            args.extend(['--arch', self.arguments[1]])
>  
> -        lines = self.runCmd(cmd, shell=True, cwd=cwd, env=shell_env)
> +        lines = subprocess.check_output(args, cwd=os.path.dirname(doc.current_source)).decode('utf-8')

Another nice cleanup might be to always run the tools in the top level
directory instead of changing the cwd. Like, we don't change he working
directory for the compiler either.

BR,
Jani.

>  
>          line_regex = re.compile(r"^\.\. FILE (\S+)$")
>  
> @@ -121,30 +112,6 @@ class KernelFeat(Directive):
>          nodeList = self.nestedParse(out_lines, fname)
>          return nodeList
>  
> -    def runCmd(self, cmd, **kwargs):
> -        u"""Run command ``cmd`` and return its stdout as unicode."""
> -
> -        try:
> -            proc = subprocess.Popen(
> -                cmd
> -                , stdout = subprocess.PIPE
> -                , stderr = subprocess.PIPE
> -                , **kwargs
> -            )
> -            out, err = proc.communicate()
> -
> -            out, err = codecs.decode(out, 'utf-8'), codecs.decode(err, 'utf-8')
> -
> -            if proc.returncode != 0:
> -                raise self.severe(
> -                    u"command '%s' failed with return code %d"
> -                    % (cmd, proc.returncode)
> -                )
> -        except OSError as exc:
> -            raise self.severe(u"problems with '%s' directive: %s."
> -                              % (self.name, ErrorString(exc)))
> -        return out
> -
>      def nestedParse(self, lines, fname):
>          content = ViewList()
>          node    = nodes.section()

-- 
Jani Nikula, Intel