Bug 7614

Summary: REVIEW: Remove support from sa-update to check SHA-1 signatures
Product: Spamassassin Reporter: Kevin A. McGrail <kmcgrail>
Component: Building & PackagingAssignee: SpamAssassin Developer Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal CC: billcole, kmcgrail, sidney
Priority: P2    
Version: unspecified   
Target Milestone: Undefined   
Hardware: PC   
OS: Windows NT   
Whiteboard:
Attachments: Patch to remove SHA1 support from sa-update

Description Kevin A. McGrail 2018-09-04 19:04:53 UTC
Per https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7596, the policy now is designed to move away from MD5 and SHA-1 to use SHA-256 or SHA-512.

We are now producing binary releases and rule releases with SHA-256 & SHA-512 signatures.  For binary releases, we have stopped using SHA1 & MD5.

However, for rules, we would like approval to keep producing the SHA1 signature along with a SHA-256 & SHA-512 signature.

This will allow older installations back to Apache SpamAssassin 3.3.3 to continue to download and install our rule updates.

Please note that in addition to SHA-1, we also use a GPG signature fingerprint that much match cryptographically in addition to the hash signature.  We hope the combination of these two signatures is suitable for a variance.

Can we continue to produce rules with SHA-1, SHA-256 & SHA-512 signatures as long as it is also cryptographically signed?
Comment 1 Sidney Markowitz 2018-09-04 20:52:00 UTC
I'm +1 on doing this, however I think we should include plans for phasing out use of SHA-1, which implies some end of life date for the older versions of SpamAssassin that use it.

One thing that would make a difference in continuing with a variance - Does SpamAssassin 3.3.3 have any option to accept rules if the SHA-1 hash matches but another hash or digital signature does not match?
Comment 2 Sidney Markowitz 2018-09-08 03:14:18 UTC
This is late to bring this up, but I think I have a problem with this regarding what we did for bug #7596

It is one thing to keep publishing SHA-1 hash for rules to maintain backward compatibility with SpamAssassin 3.3.2 while we set an end of life for it. It is another to have SHA-1 checking in sa-update 3.4.2. If someone runs sa-update with the --no-gpg option, even if we publish rules with SHA-256 and SHA-512 hashes, an attacker who somehow introduces a fake rule update that matches our SHA-1 could block reception of the SHA-256 and SHA-512 hashes, and sa-update running with --no-gpg would accept the fake rules.

I think sa-update in 3.4.2 should not check the SHA-1 hash at all. We would still publish SHA-1 to allow 3.3.2 to accept the rules, only stopping that after reaching an announced end of life for SpamAssassin 3.3.2.

Is there any reason to keep support for checking SHA-1 in 3.4.2? Will there be any rule updates released in the future that do not include the SHA-256 and SHA-512 hashes?
Comment 3 Bill Cole 2018-09-08 05:21:21 UTC
(In reply to Sidney Markowitz from comment #2)
> This is late to bring this up, but I think I have a problem with this
> regarding what we did for bug #7596

On 2nd thought, so do I. 

> It is one thing to keep publishing SHA-1 hash for rules to maintain backward
> compatibility with SpamAssassin 3.3.2 while we set an end of life for it. It
> is another to have SHA-1 checking in sa-update 3.4.2. If someone runs
> sa-update with the --no-gpg option, even if we publish rules with SHA-256
> and SHA-512 hashes, an attacker who somehow introduces a fake rule update
> that matches our SHA-1 could block reception of the SHA-256 and SHA-512
> hashes, and sa-update running with --no-gpg would accept the fake rules.
> 
> I think sa-update in 3.4.2 should not check the SHA-1 hash at all. 

I am not sure that it should not check at all, but it should not consider a SHA1 match adequate for trusting a rule package. 

> We would
> still publish SHA-1 to allow 3.3.2 to accept the rules, only stopping that
> after reaching an announced end of life for SpamAssassin 3.3.2.
> 
> Is there any reason to keep support for checking SHA-1 in 3.4.2?

Marginally, yes.
 
> Will there
> be any rule updates released in the future that do not include the SHA-256
> and SHA-512 hashes?

In theory, no.

However, weird stuff happens. It would be good to limit the scope of weird stuff that could break SA. The logic I suggest is:

1. Check all 3 hashes and signature (unless --no-gpg is used)
2. Require that ALL available hashes and signature (unless --no-gpg is used) verify. Reject update if any available hash or the signature fails to verify.
3. Require that there is at least one of the SHA256 and SHA512 hashes available and (per (2)) each available hash verifies. 
4. (implicit in (2)) If a SHA1 hash is available, require it to verify. 

Rationale:

It is plausible that a usable collision attack on SHA256 and/or SHA512 will become possible in the future. Hash collision attacks exist, so new ones are reasonable to expect. It is NOT plausible that such an attack will provide simultaneous collision with the SHA1 hash of the target file. We will be generating SHA1 hashes anyway, so checking them (NOT trusting them) is a cheap backstop. 

I will try to get a fix to sa-update ready tomorrow.
Comment 4 Sidney Markowitz 2018-09-08 05:52:47 UTC
(In reply to Bill Cole from comment #3)

I agree with everything you said. I'm least strongly agreed on continuing support for SHA1. On the one hand it is considered flawed and including it opens the remote possibility of a vulnerability based on tricking sa-update into accepting it even when there is no SHA256 or SHA512 hash. On the other hand, your proposal explicitly blocks that, and it does protect against another remote possibility of a future second-preimage attack against SHA256 and SHA512.

I would choose to drop SHA-1 in sa-update 3.4.2 completely. If a second-preimage attack against SHA256 and SHA512 is found, it will happen in stages like it did with SHA-1. The security world would be turned upside down if it happened suddenly. There will be plenty of time to phase in a new hash.

But I would not vote against sa-update verifying the SHA-1 hash that we will continue to provide, as long as sa-update requires SHA256 or SHA512 to be available too, i.e., I'll go along with your proposal if nobody else objects.
Comment 5 Sidney Markowitz 2018-09-08 07:44:13 UTC
(In reply to Bill Cole from comment #3)

It just occurred to me regarding the rationale you listed: If it is not plausible that a hypothetical attack will provide simultaneous collisions against two hash functions, then there still is no reason to check SHA-1, since there will be both SHA256 and SHA512 hashes supplied with the updates.

The argument against checking SHA-1 is that any unneeded code provides more places that a bug or an unexpected vulnerability could hide. Complexity is always the enemy of security.
Comment 6 Bill Cole 2018-09-08 15:52:53 UTC
(In reply to Sidney Markowitz from comment #5)
> (In reply to Bill Cole from comment #3)
> 
> It just occurred to me regarding the rationale you listed: If it is not
> plausible that a hypothetical attack will provide simultaneous collisions
> against two hash functions, then there still is no reason to check SHA-1,
> since there will be both SHA256 and SHA512 hashes supplied with the updates.
> 
> The argument against checking SHA-1 is that any unneeded code provides more
> places that a bug or an unexpected vulnerability could hide. Complexity is
> always the enemy of security.

An excellent point. 

Simplicity is better. Quicker to code too.
Comment 7 Bill Cole 2018-09-08 17:23:19 UTC
Created attachment 5598 [details]
Patch to remove SHA1 support from sa-update

Removes SHA1 code and fixes documentation of the verification mechanics.
Comment 8 Kevin A. McGrail 2018-09-08 20:43:42 UTC
Thanks for all the comments on this.  A bit of bug hijacking here so I've create bug 7618 and vote +1 on this patch.

With the John Hardin 1/2 vote, Sidney's clear intent to vote +1, my +1 and Bill's implicit +1 with his patch, I'm patching it.

3.4
Committed revision 1840377.

Trunk
Committed revision 1840378.
Comment 9 Sidney Markowitz 2018-09-08 23:38:57 UTC
And here is my explicit +1 after reviewing the patch, just to have it on the record.