linux-kernel - Re: [PATCH v2] checkpatch: extend author Signed-off-by check for split From: header

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: <7958ded756c895ca614ba900aae7b830a992475e.camel@perches.com>
Date:   Sun, 20 Sep 2020 08:09:42 -0700
From:   Joe Perches <joe@...ches.com>
To:     Dwaipayan Ray <dwaipayanray1@...il.com>
Cc:     lukas.bulwahn@...il.com,
        linux-kernel-mentees@...ts.linuxfoundation.org, apw@...onical.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] checkpatch: extend author Signed-off-by check for
 split From: header

On Sun, 2020-09-20 at 14:47 +0530, Dwaipayan Ray wrote:
> Checkpatch did not handle cases where the author From: header
> was split into multiple lines. The author identity could not
> be resolved and checkpatch generated a false NO_AUTHOR_SIGN_OFF
> warning.

Hi Dwaipayan.

> A typical example is Commit e33bcbab16d1 ("tee: add support for
> session's client UUID generation"). When checkpatch was run on
> this commit, it displayed:
> 
> "WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal
> patch author ''"
> 
> This was due to split header lines not being handled properly and
> the author himself wrote in Commit cd2614967d8b ("checkpatch: warn
> if missing author Signed-off-by"):
> 
> "Split From: headers are not fully handled: only the first part
> is compared."
> 
> Support split From: headers by correctly parsing the header
> extension lines. RFC 2822, Section-2.2.3 stated that each extended
> line must start with a WSP character (a space or htab). The solution
> was therefore to concatenate the lines which start with a WSP to
> get the correct long header.

This is a good commit message, though I believe the
latest rfc is 5322.  I'm not sure there is any real
difference in the referenced section though.

While your patch seems to work for git format-email,
other emailers seem to set headers that have multiple
whitespace chars that should be collapsed into a
single space.

I think you'll find that the eliding all whitespace
after header folding causes mismatches for emails.

For instance:

From:   "=?UTF-8?q?Christian=20K=C3=B6nig?=" 
        <ckoenig.leichtzumerken@...il.com>

Always inserting a single space if there is any
whitespace after the folding WSP might be better
otherwise this is decoded as

From: "Christian König"<ckoenig.leichtzumerken@...il.com>

What I have does a bit more by saving any post-folding

"From: <name and email address>"

and comparing that to any "name and perhaps different
email address" in a Signed-off-by: line.

A new message is emitted if the name matches but the
email address is different.

Perhaps it's reasonable to apply your patch and then
update it with something like the below:
---
 scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3e474072aa90..1ecc179e938d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1240,6 +1240,15 @@ sub same_email_addresses {
 	       $email1_address eq $email2_address;
 }
 
+sub same_email_names {
+	my ($email1, $email2) = @_;
+
+	my ($email1_name, $name1_comment, $email1_address, $comment1) = parse_email($email1);
+	my ($email2_name, $name2_comment, $email2_address, $comment2) = parse_email($email2);
+
+	return $email1_name eq $email2_name;
+}
+
 sub which {
 	my ($bin) = @_;
 
@@ -2679,20 +2688,32 @@ sub process {
 		}
 
 # Check the patch for a From:
-		if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) {
+		if ($line =~ /^From:\s*(.*)/i) {
 			$author = $1;
-			$author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i);
+			my $curline = $linenr;
+			while (defined($rawlines[$curline]) && $rawlines[$curline++] =~ /^\s(\s+)?(.*)/) {
+				$author .= ' ' if (defined($1));
+				$author .= "$2";
+			}
+			if ($author =~ /=\?utf-8\?/i) {
+				$author = decode("MIME-Header", $author);
+				$author = encode("utf8", $author);
+			}
+
 			$author =~ s/"//g;
 			$author = reformat_email($author);
 		}
 
 # Check the patch for a signoff:
 		if ($line =~ /^\s*signed-off-by:\s*(.*)/i) {
+			my $sig = $1;
 			$signoff++;
 			$in_commit_log = 0;
 			if ($author ne '') {
-				if (same_email_addresses($1, $author)) {
-					$authorsignoff = 1;
+				if (same_email_addresses($sig, $author)) {
+					$authorsignoff = "1";
+				} elsif (same_email_names($sig, $author)) {
+					$authorsignoff = $sig;
 				}
 			}
 		}
@@ -6937,6 +6958,9 @@ sub process {
 		} elsif (!$authorsignoff) {
 			WARN("NO_AUTHOR_SIGN_OFF",
 			     "Missing Signed-off-by: line by nominal patch author '$author'\n");
+		} elsif ($authorsignoff ne "1") {
+			WARN("NO_AUTHOR_SIGN_OFF",
+			     "From:/SoB: email address mismatch: 'From: $author' != 'Signed-off-by: $authorsignoff'\n");
 		}
 	}