472790 - use sha256 or stronger encryption for hashes of mar files

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[Daniel Veditz [:dveditz]]

+++ This bug was initially created as a clone of Bug #421012 +++

In bug 421012 AUS hashes were upgraded from MD5 to SHA-1 which was an improvement, but still not best practice. bug 421012 comment 10 mentions some infrastructure obstacles that will have be overcome but we do need to move to a hash in the sha-2 family so we need to include that requirement as we plan toolchain upgrades.

Comment 1

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Triaging this to future for now, as it didnt make the cut with all our other higher priority Q1 goals. We'll revisit when looking at Q2 goals, or if our Q1 goals go well, we could even pick this up again sooner.

If there's any reason this is not ok, please add additional details to this bug and ask us to reconsider, ok?

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

Just for the record, the app supports these hash functions:
 http://mxr.mozilla.org/firefox/source/netwerk/base/public/nsICryptoHash.idl#49
That's Fx3.0, same for mozilla-1.9.1 and mozilla-central; also mozilla1.8 but in a different location. So the options are SHA256, SHA384, SHA512. 

Need to figure out how to update openssl on assorted linux machines, so that patcher can generate the hash values.

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

AFAICT AUS passes on the info it gets from the snippets, updating summary.

Comment 4

[Daniel Veditz [:dveditz]]

NSS contains a utility, bltest, that can hash files. It's not built by default but it could be built and then installed wherever needed if that's easier than upgrading the system-wide openssl.

Comment 5

[Reed Loden [:reed]]

Do the machines not have sha256sum? You don't need openssl to do this.

Comment 6

[Philippe M. Chiasson (:gozer)]

Another possible option is to use MD5 *and* SHA1 combined. It's not ideal, but does close the door to the collision attacks.

Just my 0.02$

Comment 7

[Reed Loden [:reed]]

(In reply to comment #6)
> Another possible option is to use MD5 *and* SHA1 combined. It's not ideal, but
> does close the door to the collision attacks.

We can only use what Gecko supports, which is what nthomas mentioned in comment #2.

Comment 8

[Philippe M. Chiasson (:gozer)]

(In reply to comment #7)
> (In reply to comment #6)
> > Another possible option is to use MD5 *and* SHA1 combined. It's not ideal, but
> > does close the door to the collision attacks.
> 
> We can only use what Gecko supports, which is what nthomas mentioned in comment
> #2.

And that's why I suggested that using SHA1+MD5 in combination, treating the result as it was a single hash value is also safe and ends up being quite a strong workaround if sha-2 and friends are not an option.

Comment 9

[Reed Loden [:reed]]

(In reply to comment #8)
> And that's why I suggested that using SHA1+MD5 in combination, treating the
> result as it was a single hash value is also safe and ends up being quite a
> strong workaround if sha-2 and friends are not an option.

Gecko doesn't support using them together. We can only use what Gecko supports as a valid hash algorithm for backwards compatibility, otherwise we'd break everybody with old Firefox/Thunderbird/whatever versions that don't support putting SHA1+MD5 together.

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

I mentioned openssl because other people use patcher, and openssl is a nice generic package that you can find across a bunch of platforms. Turns out we're in better shape than I expected because the openssl docs are out of date (more hashes are supported than they talk about than the man file).

We have three realms to consider, based on the age of the machines:
* Fx 3.1 and later releases: moz-linux-slave*
    openssl dgst supports sha256/284/512 without documenting it
* Fx 3.0 release updates : fx-linux-1.9-slave1/2
    Closer to 3.1 than I remembered, also based on CentOS 5
* Tb 2.0 releases, and nightly updates : *prometheus-vm*
    openssl supports up to sha1, RHEL AS release 3 (Taroon Update 8)

So it's just the oldest machines to worry about. Looks like it's pretty simple to compile a recent openssl to /usr/local/bin and not tangle with the system copy. 

With that we need to update patcher2.pl for releases, and patch-packager.pl for nightlies.

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

openssl 0.9.8j is installed to prometheus-vm:/usr/local/bin, but not yet used for anything.

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Generate nightly snippets using sha512 hashes

Turns out nightly updates are a bit more involved than I thought in comment #10, as the partial generator uses the same hash it gets from the nightly build's snippet. So that would means updating tinderbox and buildbot setups, not too bad until you find out that the hash function for buildbot is hardwired in a file on each of the 50 or so slaves.

Instead this patch sets the a hash function in the partial generator, and uses that for both the newly created partial.mar and recalculating the value for the complete mar. There is some code shuffling to pull down the complete mar in  create_complete_update, rather than create_partial_update. get_patch_info() is where it learns how to do the stronger hashes, borrowed from Mozbuild/Util::HashFile(). Also adds hash checks on the downloaded mar files before generating the partial.

Comment 13

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Generate release snippets using sha512 hashes

Requires updating production-prometheus-vm* with newer openssl (for Tb2.0), and testing that all is well in nightly-land before before moving forward.

Comment 14

[[e-mail] (:bhearsum)]

Comment on attachment 361216 [details] [diff] [review]
Generate nightly snippets using sha512 hashes

Looks fine to me. Glad to see we don't have to update SetMozillaBuildProperties.

Comment 15

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 361216 [details] [diff] [review]
Generate nightly snippets using sha512 hashes

rev 1.21

Comment 16

[Nick Thomas [:nthomas] (UTC+12)]

Tomorrows builds will use SHA512 hashes. I used Firefox 2.0 nightlies as guinea pigs and regenerated the partial to today's build, works fine for that & complete on all three platforms. Same for mac/mozilla-1.9.1/partial.

Comment 17

[[e-mail] (:bhearsum)]

Looks like this brought nightly updates. I'm pretty sure problem is here:
+    # Expects input like SHA512(mozconfig)= d7433cc4204b4f3c65d836fe483fa575...
+    # Removes everything up to and including the "= "
+    $hashvalue =~ s/^.+\s+(\w+)$/$1/;

which, in my local tests appears to empty out $hashValue. Helpfully, I didn't catch this in review.

I've got this local patch on prometheus-vm right now as a test:
-    $hashvalue =~ s/^.+\s+(\w+)$/$1/;
+    #$hashvalue =~ s/^.+\s+(\w+)$/$1/;
+    (undef, $hashValue) = split(/=/, $hashValue);
+    $hashValue =~ s/\s//;


In my tests this does indeed get us the hash value without any whitespace or other fluff. Standard8 kicked a tb mac nightly to test this out.

Comment 18

[[e-mail] (:bhearsum)]

Ok, my little patch broke it completely. I've backed out that and Nick's patch and I'll let him deal with it when he gets in. Nightly updates _should_ be fixed now, I'll be watching them.

Comment 19

[Nick Thomas [:nthomas] (UTC+12)]

Looks like the problem is that PATH is different when running under crontab than at the prompt, namely
 cron : /usr/bin:/bin
 shell: /usr/kerberos/bin:/usr/local/bin:/bin:/usr/bin:/usr/X11R6/bin:/home/cltbld/bin
The missing /usr/local/bin means the partial generator uses the old version of openssl, which returns 
  unknown option '-sha512'
and we end up with an empty hashvalue in the snippet. I'll fix that up and add some tests to get_patch_info to avoid this sort of thing.

Comment 20

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Generate nightly snippets using sha512 hashes w/ better checking

The fix for the bustage is a simple
 PATH=/usr/local/bin/:/usr/bin:/bin
at the top of the crontab (/usr/local/bin contains only openssl and c_rehash so this is very safe).

This patch is attachment 361216 [details] [diff] [review] plus additional sanity checking for the hashvalue calculation. It ensures that the partial generator bombs out before it pushes bogus hash information.

Comment 21

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 361706 [details] [diff] [review]
Generate nightly snippets using sha512 hashes w/ better checking

Checked in: new revision is 1.23

Deployed into use, with the cron path fix. Another Fx2.0.0.x linux nightly will run to verify all is well.

Comment 22

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #21)
> Deployed into use, with the cron path fix. Another Fx2.0.0.x linux nightly will
> run to verify all is well.

Verified working.

Comment 23

[[e-mail] (:bhearsum)]

Is there anything else to do here?

Comment 24

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 361217 [details] [diff] [review]
Generate release snippets using sha512 hashes

I think we're good to go with creating UPDATE_PACKAGING_R8 with this patch, enabling SHA512 hashes for releases. Can't find any bugs on nightly updates regressing since we turned on SHA512 hashes "some time ago", nor any other consumers of Mozbuild::Util::HashFile() that might be affected. NB the patch is against a working setup for patcher rather than CVS itself.

Here's a typical diff for snippet:
diff -ru original-snippets/aus2/Firefox/3.5b4/WINNT_x86-msvc/20090423204732/en-US/beta/partial.txt aus2/Firefox/3.5b4/WINNT_x86-msvc/20090423204732/en-US/beta/partial.txt
--- original-snippets/aus2/Firefox/3.5b4/WINNT_x86-msvc/20090423204732/en-US/beta/partial.txt   2009-05-21 04:49:55.000000000 -0700
+++ aus2/Firefox/3.5b4/WINNT_x86-msvc/20090423204732/en-US/beta/partial.txt     2009-05-21 21:37:37.000000000 -0700
@@ -1,8 +1,8 @@
 version=1
 type=partial
 url=http://download.mozilla.org/?product=firefox-3.5rc1-partial-3.5b4&os=win&lang=en-US
-hashFunction=SHA1
-hashValue=20c327807965f7b0893472b8d4a62156c593e02b
+hashFunction=SHA512
+hashValue=df661dbd8be61bab4c94f198b42d42381e8f6ba07cda9ec32fe6c06e79ac453c04b119099a4971f03aebce220d53f4e3213f691e245ecdc0d0baecfbd4c8ffb4
 size=2868990
 build=20090520233605
 appv=3.5 RC 1

I was thinking of using this for Fx 3.5rc1, and we'd pick it up for Tb 2.0.0.20 and Fx 3.0.12. What do you reckon ?

Comment 25

[Nick Thomas [:nthomas] (UTC+12)]

That should read Tb 2.0.0.22 rather than .20.

production-prometheus-vm has openssl 0.9.8j installed to /usr/local/bin

Comment 26

[[e-mail] (:bhearsum)]

Comment on attachment 361217 [details] [diff] [review]
Generate release snippets using sha512 hashes

Sounds good to me to turn this on for everything you listed.

Comment 27

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 361217 [details] [diff] [review]
Generate release snippets using sha512 hashes

Checking in patcher/patcher2.pl;
/cvsroot/mozilla/tools/patcher/patcher2.pl,v  <--  patcher2.pl
new revision: 1.38; previous revision: 1.37
done
Checking in release/MozBuild/Util.pm;
/cvsroot/mozilla/tools/release/MozBuild/Util.pm,v  <--  Util.pm
new revision: 1.23; previous revision: 1.22
done

Comment 28

[Nick Thomas [:nthomas] (UTC+12)]

UPDATE_PACKAGING_R8 created by adapting bug 420947 comment 17. Compared to R7 it also contains the Bootstrap changes from bug 483232, 481079, 451392, 467310, and Bootstrap config changes - none of these impact on update generation.

I'll change the release automation for Fx3.5rc1 to use the new _R8 tag, and have left notes on the build pages for Tb2.0.0.22 and Fx3.0.12 to do the same. Just need to decide what the most appropriate way to tag m-1.9.1 (and maybe m-c) is.

Comment 29

[Nick Thomas [:nthomas] (UTC+12)]

Created UPDATE_PACKAGING_R8 on
  http://hg.mozilla.org/mozilla-central/rev/fe9cc55b8db7
  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4e2a66949876
They're not identical over the whole code base obviously, but almost are over the bits we care about, namely modules/libbz2, modules/libmar, tools/update-packaging and other-licenses/bsdiff. There is this small difference:
--- mozilla-central/tools/update-packaging/unwrap_full_update.sh      
+++ releases/mozilla-1.9.1/tools/update-packaging/unwrap_full_update.sh
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh

There is just the one change since _R7 that we care about (sort of!)
  http://hg.mozilla.org/mozilla-central/rev/0720a4f0eeec
which is WinCE ifdef'd.

This is infrastructure complete. Individual releases will pick up SHA512 hashes as they use the R8 tag.

Comment 30

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: cvs rdiff between R7 and R8

Ben created this rdiff between update tags for easy inspection. Thanks!