linux-kernel - Re: [RFC PATCH v2] 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: <da3053be-f27e-f99c-0a8a-447adb9733d2@gmail.com>
Date:   Tue, 13 Oct 2020 17:43:59 +0530
From:   Ujjwal Kumar <ujjwalkumar0501@...il.com>
To:     Joe Perches <joe@...ches.com>,
        Lukas Bulwahn <lukas.bulwahn@...il.com>
Cc:     linux-kernel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [RFC PATCH v2] checkpatch: add shebang check to
 EXECUTE_PERMISSIONS

On 13/10/20 5:31 pm, 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>
> ---
> Changes in v2:
>   - Spelling correction and add example to commit
>     message
>   - Code style changes
>   - Remove unncessary function argument
>   - Use non-capturing group in regexp
> 
>  scripts/checkpatch.pl | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index fab38b493cef..7ebbee9c3672 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1795,6 +1795,23 @@ sub get_stat_here {
>  	return $herectx;
>  }
> 
> +sub get_shebang {
> +	my ($linenr) = @_;
> +	my $rawline = "";
> +	my $shebang = "";
> +
> +	$rawline = raw_line($linenr, 3);

I'm wondering if the range information can be at a
different offset from the 'new mode line'.

> +	if (defined($rawline) &&
> +	    $rawline =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
> +		if (defined($1) && $1 == 1) {
> +			$shebang = raw_line($linenr, 4);
> +			$shebang = substr($shebang, 1);
> +		}
> +	}
> +
> +	return $shebang;
> +}
> +
>  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}$/) {
>  			my $permhere = $here . "FILE: $realfile\n";
> +			my $shebang = get_shebang($linenr);
>  			if ($realfile !~ m@...ipts/@ &&
> +			    $shebang !~ /^#!\s*(?:\/\w)+.*/ &&
>  			    $realfile !~ /\.(py|pl|awk|sh)$/) {

Consider the following case:
a python script file with '.py' filename extension but without
a shebang line. Would it be meaningful to allow execute permission
on such a file?

>  				ERROR("EXECUTE_PERMISSIONS",
>  				      "do not set execute permissions for source files\n" . $permhere);
> 
> base-commit: 148fdf990dee4efd23c1114811b205de9c966680
> --
> 2.26.2
> 

Thanks
Ujjwal Kumar