linux-kernel - Re: [RFC PATCH] checkpatch: add shebang check to EXECUTE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <63053585d41ff81cdaad6cb727eb83d81207041a.camel@perches.com>
Date:   Sun, 11 Oct 2020 23:17:23 -0700
From:   Joe Perches <joe@...ches.com>
To:     Ujjwal Kumar <ujjwalkumar0501@...il.com>,
        Lukas Bulwahn <lukas.bulwahn@...il.com>
Cc:     linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] checkpatch: add shebang check to EXECUTE_PERMISSIONS

On Mon, 2020-10-12 at 11:19 +0530, Ujjwal Kumar wrote:
> checkpatch.pl checks for invalid EXECUTE_PERMISSIONS on source
> files. The script leverages filename extensions and its path in
> the repository to decide whether to allow execute permissions on
> the file or not.
> 
> Based on current check conditions, a perl script file having
> execute permissions, without '.pl' extension in its filename
> and not belonging to 'scripts/' directory is reported as ERROR
> which is a false-positive.
> 
> Adding a shebang check along with current conditions will make
> the check more generalised and improve checkpatch reports.
> To do so, without breaking the core design decision of checkpatch,
> we can fetch the first line from the patch itself and match it for
> a shebang pattern.
> 
> There can be cases where the first line is not part of the patch.

For instance: a patch that only changes permissions
without changing any of the file content.

> 
> In that case there may be a false-positive report but in the end we
> will have less false-positives as we will be handling some of the
> unhandled cases.

> Signed-off-by: Ujjwal Kumar <ujjwalkumar0501@...il.com>
> ---
> Apologies, I forgot to include linux-kernel@...r.kernel.org so I'm
> now resending.
> 
>  scripts/checkpatch.pl | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -1795,6 +1795,23 @@ sub get_stat_here {
>  	return $herectx;
>  }

First some style trivia:

> +sub get_shebang {
> +	my ($linenr, $realfile) = @_;
> +	my $rawline = "";
> +	my $shebang = "";
> +
> +	$rawline = raw_line($linenr, 3);
> +	if (defined $rawline &&
> +		$rawline =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {

alignment to open parenthesis please

> +		if (defined $1 && $1 == 1) {
> +			$shebang = raw_line($linenr, 4);
> +			$shebang = substr $shebang, 1;

parentheses around substr please.

> +		}
> +	}
> +
> +	return $shebang;
> +}

And some real notes:

$realfile isn't used in this function so there doesn't
seem to be a reason to have it as an function argument.

> +
>  sub cat_vet {
>  	my ($vet) = @_;
>  	my ($res, $coded);
> @@ -2680,7 +2697,9 @@ sub process {
>  # Check for incorrect file permissions
>  		if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {

probably better here to use a capture group for the permissions

		if ($line =~ /^new (?:file )?mode (\d+)$/) {
			my $mode = substr($1, -3);

>  			my $permhere = $here . "FILE: $realfile\n";
> +			my $shebang = get_shebang($linenr, $realfile);
>  			if ($realfile !~ m@...ipts/@ &&

Maybe remove the $realfile directory test as
there are many source files that are not scripts
in this directory and its subdirectories.

> +			    $shebang !~ /^#!\s*(\/\w)+.*/ &&

unnecessary capture group

and add

			   $mode =~ /[1357]/ &&

>  			    $realfile !~ /\.(py|pl|awk|sh)$/) {

No need for a a capture group here either. (existing defect)

>  				ERROR("EXECUTE_PERMISSIONS",
>  				      "do not set execute permissions for source files\n" . $permhere);