Improve folding signature algorithm #29

ludusrusso · 2020-05-22T19:42:14Z

Following #27, I've rewritten the folding algorithm in order to better fold headers and avoid introducing new lines inside headers.

However, it seems it breaks the skim sign.

In my understanding, this should have been call after the signature was generated, but it seems that the new algorithm changes the generates signature itself.

Any help?

ludusrusso · 2020-05-22T19:43:25Z

Example output:

        DKIM-Signature: a=ed25519-sha256;
         bh=2jUSOH9NhtVGCQWNr9BrIAPreKQjO6Sn7XIkfJVOzv8=; c=simple/simple;
         d=football.example.com; h=From:To:Subject:Date:Message-ID; s=brisbane;
         t=424242; v=1;
         b=CtvfpO96cCh7+PJLDna/bOJ1WdQ2qt7Nfrq6FzJqjYQv+TjOUc+rWk81TQJnmvdZsCDbmpr2
         hj7RDH4JDGa0CA==

foxcpp · 2020-05-24T12:46:41Z

In my understanding, this should have been call after the signature was generated, but it seems that the new algorithm changes the generates signature itself.

The likely issue is addition of non-empty b= that changes how folding behaves. I tried to integrate go-message/textproto algorithm into go-msgauth and hit the exactly same issue.
You can observe it by adding fmt.Println for sigField here:

go-msgauth/dkim/sign.go

Line 261 in e4c8736

hashed := hasher.Sum(nil)

and for formatSignature(s.sigParams) here:

go-msgauth/dkim/sign.go

Line 276 in e4c8736

s.sigParams = params

It is important to make sure both values are equal (with the exception of b= vs b=BASE64).

foxcpp · 2020-05-24T13:06:35Z

In your case, it shows:

DKIM-Signature: a=rsa-sha256;
 bh=2jUSOH9NhtVGCQWNr9BrIAPreKQjO6Sn7XIkfJVOzv8=; c=simple/simple;
 d=example.org; h=From:To:Subject:Date:Message-ID; s=brisbane; t=424242;
 v=1;

DKIM-Signature: a=rsa-sha256;
 bh=2jUSOH9NhtVGCQWNr9BrIAPreKQjO6Sn7XIkfJVOzv8=; c=simple/simple;
 d=example.org; h=From:To:Subject:Date:Message-ID; s=brisbane; t=424242;
 v=1;
 b=KXuOIdcHesiuc+CNVJC0ooTO9LvodnM01dstXNTA7elICw7r9LKD5zrPnGwtP5Ye+sOndhJO
 23pJIY3LP5oXzCnV3nkZiEwIb0rKIgYxgxx8JfCWbZgurhN35rQjnwCtmcGvN1aJ7ba1x1x63Vf
 xp0zwssLR2w8R+PkR4r1ROlM=

Uh, missing b=?

foxcpp

There is a problem with b= handling that causes generation of broken signatures. Additionally, I would appreciate if you add more test cases to ensure it generates valid signatures in edge cases related to folding.

foxcpp · 2020-05-24T13:21:46Z

dkim/header.go

+		}
+	}
+	sort.Strings(keys)
+	if found {


Never set to anything other than false.

foxcpp · 2020-05-24T13:29:44Z

dkim/header.go

 	}
+
+	if bvalue != "" {


Empty string is a valid value. The DKIM-Signature contents are signed with b=.

Oh thanks, I've got it now! It seems it works :D

codecov-commenter · 2020-05-24T14:04:44Z

Codecov Report

Merging #29 into master will increase coverage by 0.31%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   66.30%   66.62%   +0.31%     
==========================================
  Files           7        7              
  Lines         837      845       +8     
==========================================
+ Hits          555      563       +8     
  Misses        213      213              
  Partials       69       69

Impacted Files	Coverage Δ
dkim/header.go	`93.68% <100.00%> (+0.58%)`	⬆️
dkim/sign.go	`68.58% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4c8736...a231a2f. Read the comment docs.

ludusrusso · 2020-05-24T14:05:35Z

Additionally, I would appreciate if you add more test cases to ensure it generates valid signatures in edge cases related to folding.

Yea! I'll add some additional test cases! To be honest, I did not added more test before because they are covered by TestSign

ludusrusso · 2020-05-24T14:15:27Z

Now, if we have some part of the header longher than 75cahrs, the new algorithm will break the line length limit. This should not be an issues since this limit is only a recommendation! What do you think? #18 (comment)

Anyway this should fix #18

foxcpp · 2020-05-24T14:17:22Z

I agree that 75 chars limitation can be ignored. Following rationale in RFC, it is not really relevant today as a rare person looks at message header.

ludusrusso · 2020-05-24T14:23:23Z

@foxcpp thanks! I've also added a more test that checks only if the headers names in the "h" values are properly folded in the generated signature!

foxcpp · 2020-05-24T14:31:18Z

FWIW, That's the h= value my mail server used for a typical plaintext message:

Subject:Subject:Sender:To:To:Cc:From:From:Date:Date:MIME-Version:Content-Type:Content-Type:Content-Transfer-Encoding:Reply-To:In-Reply-To:Message-Id:Message-Id:References:Autocrypt:Openpgp

Interesting is that old folding algorithm broke it in the middle of a field name yet
OpenDKIM verified the signature successfully:
https://paste.dn42.us/paste/#/8T1GgumCllH0ZGDbwvOI8dvNYA8!TqXFCKZWbnYkBUP4_rBv1Fd3e-OVScQBZDav2mXSMw4

foxcpp · 2020-05-24T14:32:16Z

@emersion, what do you think about this PR?

ludusrusso · 2020-05-24T14:41:15Z

FWIW, That's the h= value my mail server used for a typical plaintext message:
Subject:Subject:Sender:To:To:Cc:From:From:Date:Date:MIME-Version:Content-Type:Content-Type:Content-Transfer-Encoding:Reply-To:In-Reply-To:Message-Id:Message-Id:References:Autocrypt:Openpgp
Interesting is that old folding algorithm broke it in the middle of a field name yet
OpenDKIM verified the signature successfully:
https://paste.dn42.us/paste/#/8T1GgumCllH0ZGDbwvOI8dvNYA8!TqXFCKZWbnYkBUP4_rBv1Fd3e-OVScQBZDav2mXSMw4

I also notice this! I've tried with different mail provider receivers and in some case (like hotmail/outlook) the dkim fails while other (like gmail) it passes.

dkim/header.go

emersion

Overall the meat of the patch looks good :)

ludusrusso · 2020-05-25T08:53:52Z

Hi @emersion, thanks for the feedback! I was wondering: if we do not want to limit the line lenght to 75 chars, we could make the folding algorithm simpler by just putting the "b" value at the end of the header without folding it! What do you think about this?

emersion · 2020-05-25T09:00:39Z

Would the b= value ever be longer than 1000 chars? (I guess that's an issue with other header fields too, like h=)

ludusrusso · 2020-05-25T09:08:08Z

Would the b= value ever be longer than 1000 chars? (I guess that's an issue with other header fields too, like h=)

Not super sure about b, but since it's an hash it should be fixed in length, I'm correct?

Regarding h, you're right, it could be more then 1000 chars if you include a lot of headers! I've to think about a way to fold it :D

ludusrusso · 2020-05-27T12:36:29Z

@emersion what do you think about b folding?

foxcpp · 2020-05-27T15:16:18Z

b= is a signature value. I do not expect it to hit 998 characters limitation even with RSA-4096 (though one might consider it to be dangerously close, ~682 characters in base64)

emersion

All right, let's merge this as-is since it already fixes a bug. I've opened #31 for future work.

Thanks!

up folding alg

174579d

ludusrusso changed the title ~~WIP: Improve format signature algorithm~~ WIP: Improve folding signature algorithm May 22, 2020

foxcpp mentioned this pull request May 23, 2020

sign_dkim may generate broken signatures due to header folding foxcpp/maddy#226

Closed

foxcpp suggested changes May 24, 2020

View reviewed changes

fix empty b value

98c017d

add test case for long header folding

5d1639f

add test case for long header folding

e9efb5d

ludusrusso changed the title ~~WIP: Improve folding signature algorithm~~ Improve folding signature algorithm May 24, 2020

emersion reviewed May 25, 2020

View reviewed changes

dkim/header.go Outdated Show resolved Hide resolved

emersion reviewed May 25, 2020

View reviewed changes

dkim/header.go Outdated Show resolved Hide resolved

emersion reviewed May 25, 2020

View reviewed changes

dkim/header.go Outdated Show resolved Hide resolved

emersion reviewed May 25, 2020

View reviewed changes

improve code style

a231a2f

emersion mentioned this pull request May 28, 2020

Improve line folding algorithm #31

Open

emersion approved these changes May 28, 2020

View reviewed changes

emersion merged commit fb07ec9 into emersion:master May 28, 2020

This was referenced May 28, 2020

Fix DKIM-Signature header flowing, it is not valid to arbitrarily sli… #27

Closed

Correctly wrap dkim header. #30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve folding signature algorithm #29

Improve folding signature algorithm #29

ludusrusso commented May 22, 2020 •

edited

Loading

ludusrusso commented May 22, 2020

foxcpp commented May 24, 2020 •

edited

Loading

foxcpp commented May 24, 2020 •

edited

Loading

foxcpp left a comment

foxcpp May 24, 2020

foxcpp May 24, 2020

ludusrusso May 24, 2020

codecov-commenter commented May 24, 2020 •

edited

Loading

ludusrusso commented May 24, 2020

ludusrusso commented May 24, 2020

foxcpp commented May 24, 2020

ludusrusso commented May 24, 2020

foxcpp commented May 24, 2020

foxcpp commented May 24, 2020

ludusrusso commented May 24, 2020

emersion left a comment

ludusrusso commented May 25, 2020

emersion commented May 25, 2020

ludusrusso commented May 25, 2020

ludusrusso commented May 27, 2020

foxcpp commented May 27, 2020

emersion left a comment

Improve folding signature algorithm #29

Improve folding signature algorithm #29

Conversation

ludusrusso commented May 22, 2020 • edited Loading

ludusrusso commented May 22, 2020

foxcpp commented May 24, 2020 • edited Loading

foxcpp commented May 24, 2020 • edited Loading

foxcpp left a comment

Choose a reason for hiding this comment

foxcpp May 24, 2020

Choose a reason for hiding this comment

foxcpp May 24, 2020

Choose a reason for hiding this comment

ludusrusso May 24, 2020

Choose a reason for hiding this comment

codecov-commenter commented May 24, 2020 • edited Loading

Codecov Report

ludusrusso commented May 24, 2020

ludusrusso commented May 24, 2020

foxcpp commented May 24, 2020

ludusrusso commented May 24, 2020

foxcpp commented May 24, 2020

foxcpp commented May 24, 2020

ludusrusso commented May 24, 2020

emersion left a comment

Choose a reason for hiding this comment

ludusrusso commented May 25, 2020

emersion commented May 25, 2020

ludusrusso commented May 25, 2020

ludusrusso commented May 27, 2020

foxcpp commented May 27, 2020

emersion left a comment

Choose a reason for hiding this comment

ludusrusso commented May 22, 2020 •

edited

Loading

foxcpp commented May 24, 2020 •

edited

Loading

foxcpp commented May 24, 2020 •

edited

Loading

codecov-commenter commented May 24, 2020 •

edited

Loading