767623 - Use HMAC to generate tokens and sensitive graph filenames

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Reed Loden [:reed]]

I've been doing some reading on length extension attacks that affect Merkle–Damgård hash algorithms, and I can't just help but wonder if issue_hash_token() is vulnerable to the same problem due to its use of md5_hex() with the shared secret towards the beginning of the concatenation (though, not the first part, which is a repeat of the timestamp).

Just filing this to get some more eyes on it and to help check if Bugzilla is vulnerable or not.

Comment 1

[Reed Loden [:reed]]

Attachment: patch - v1 (untested)

No matter if we're vulnerable or not, let's just go ahead and swap to using an HMAC.

I think it would be a good idea to delete old charts that were generated using the old md5_hex()-based method. I can add that if wanted.

Comment 2

[Frédéric Buclin]

IMO, we are not vulnerable to this specific attack, because the first argument is the timestamp, not the secret key, and all our tokens are time bombed. Moreover, we have full control on arguments passed to generate the token, contrary to the Flickr API which blindly accepts all user-defined arguments to generate the token. The main problem of the Flickr API is that they use no separator, which is not our case, and so Bugzilla cannot be abused this way. Said differently, we control the number of arguments and the order in which arguments are passed to issue_hash_token(), and these arguments are clearly separated by a separator, which prevents the injection of additional arguments.

About old charts, we have nothing to do. All existing charts are automatically deleted when running collectstats.pl, and as the PDF document mentioned in the URL field says, this attack doesn't give you access to the secret key, and so you cannot run any kind of attack against the old charting system. If we want to leave MD5 to generate the PNG image name, then this should be fixed in a separate bug, but this is not a security issue.

Comment 3

[Reed Loden [:reed]]

There's also http://rdist.root.org/2009/10/29/stop-using-unsafe-keyed-hashes-use-hmac/ that talks more about suffix-based attacks as well... I guess my concern is that the timestamp is a known piece of data (since it's prepended), so is it possible that an attacker would still be able to perform a similar attack somehow. Again, I don't claim to be a crypto expert, so a bit difficult for me to actually be sure about this.

(In reply to Frédéric Buclin from comment #2) 
> About old charts, we have nothing to do. All existing charts are
> automatically deleted when running collectstats.pl, and as the PDF document
> mentioned in the URL field says, this attack doesn't give you access to the
> secret key, and so you cannot run any kind of attack against the old
> charting system. If we want to leave MD5 to generate the PNG image name,
> then this should be fixed in a separate bug, but this is not a security
> issue.

Ah, I dug through some code in Bugzilla/Install/ and didn't see anything that removed old charts, but I forgot to check collectstats.pl, so that's cool. My comment was more that I wanted to make sure the old images were removed for disk space reasons (once the algorithm had been changed to an HMAC), not because I felt an attacker could get them.

Comment 4

[Reed Loden [:reed]]

Comment on attachment 635983 [details] [diff] [review]
patch - v1 (untested)

Review of attachment 635983 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Token.pm
@@ +172,3 @@
>  
>      my $token = join('*', @args);
> +    $token = hmac_sha512_base64($token, Bugzilla->localconfig->{'site_wide_secret'});

Er, I was hoping to cut down on the size of the token by using base64 rather than hex, but I just remembered that we use the hash tokens as URI parameters at times, so base64 is probably not the best choice here, sadly. So, pretend I'm using hmac_sha512_hex() instead.

@@ -173,5 @@
>      my $token = join('*', @args);
> -    # Wide characters cause md5_hex() to die.
> -    if (Bugzilla->params->{'utf8'}) {
> -        utf8::encode($token) if utf8::is_utf8($token);
> -    }

Just in case you're wondering, wide characters are fine with the hmac functions (I tested).

Comment 5

[Frédéric Buclin]

(In reply to Reed Loden [:reed] from comment #4)
> So, pretend I'm using hmac_sha512_hex() instead.

I think you should use hmac_sha256_hex() instead. From what I can read at http://perldoc.perl.org/Digest/SHA.html#EXPORTABLE-FUNCTIONS:

"Provided your C compiler supports a 64-bit type (e.g. the long long of C99, or __int64 used by Microsoft C/C++), all of these functions will be available for use. Otherwise, you won't be able to perform the SHA-384 and SHA-512 transforms, both of which require 64-bit operations."

So SHA-512 requires 64-bit operations, and I'm not sure such operations are available on all 32-bit machines. that's why we should use SHA-256 instead. And this way we would also be consistent with bz_crypt() which already uses SHA-256. Could you update your patch, please?

Comment 6

[Reed Loden [:reed]]

Attachment: patch - v2 (untested)

Ah, I thought for some reason that bz_crypt() was using SHA-512 as well.

Note that SHA-512 functions will work on 32-bit systems, but they will just be very slow. However, on 64-bit systems, SHA-512 is at least 2x+ faster than SHA-256, so performance is actually slower for the majority of users.

Comment 7

[Frédéric Buclin]

Comment on attachment 636170 [details] [diff] [review]
patch - v2 (untested)

This looks good, but now tokens are *much* longer than with md5_hex(). I suggest we use the "modified Base64 for URL" (aka RFC 4648) algorithm instead, see:

http://en.wikipedia.org/wiki/Base64#RFC_4648
http://tools.ietf.org/html/rfc4648#section-5

Basically, all you have to do is to replace:

  $token = hmac_sha256_hex(...);

by:

  $token = hmac_sha256_base64(...);
  $token =~ s/\+/-/g;
  $token =~ s/\//_/g;

So + and / are replaced by - and _ respectively, which do not need to be URL-encoded/decoded. This way, tokens are only slightly longer than what they currently are.

Could you please update your patch to use "Base64 for URL" instead? You can carry forward my r+.

Comment 8

[Frédéric Buclin]

Holding approval till the patch is updated according to comment 7.

Comment 9

[Reed Loden [:reed]]

Attachment: patch - v3 (untested)

Comment 10

[Frédéric Buclin]

You can remove the security flag once this patch is checked in.

Comment 11

[Reed Loden [:reed]]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified reports.cgi
modified Bugzilla/Token.pm
Committed revision 8311.

Comment 12

[Frédéric Buclin]

Added to relnotes for 4.4.