1497497 - Bugzilla's HMAC signatures ignore structure of signed data

Status: NEW

(bugzilla.mozilla.org :: General, defect)

Description

[Wladimir Palant]

A note up front: from what I can tell, this issue isn't currently exploitable. But even if that's the case, future code additions might open up vulnerabilities by relying on this code to work properly.

Bugzilla will currently generate HMAC-SHA256 signatures in two places. Bugzilla::Token::issue_hash_sig is used for relogins, Bugzilla::Token::check_auth_delegation_token is used for auth delegation. In both cases, multiple values are being signed: (random_salt, "prev_url:" + userid, url) for the former, (userid, callback_url) for the latter. These values are being passed to Digest::SHA::hmac_sha256_base64 as a list to generate the signature.

While the documentation on Digest::SHA::hmac_sha256_base64 doesn't make this obvious, specifying multiple values for its $data parameter will simply concatenate all the values. So in case of Bugzilla::Token::check_auth_delegation_token for example the value being signed is "<user_id><callback_url>" - no separators here. With the callback_url being passed in as query parameter, the following attack would be possible (fails merely due to scheme requirements on callback_url):

1. User with ID 1234 goes to /auth.cgi?callback=5http://malicious.com/.
2. Bugzilla signs the value "12345http://malicious.com/" and outputs the resulting token in the confirmation page.
3. User 1234 takes that token and tricks the user with ID 12345 into loading /auth.cgi?callback=http://malicious.com/&token=<token>.
4. Bugzilla verifies that the signature is valid for "12345http://malicious.com/" and sends the API token of user 12345 to http://malicious.com/ without requiring further confirmation.

As I said, this attack currently won't work because code validating the callback parameter will reject URLs where the scheme isn't http: or https:. While the parsing rules of the URI module relax this requirement somewhat and "<URL:http://example.com/>" will be accepted as well for example, making the URL start with a number doesn't seem possible.

When signing multiple values, concatenation without any separator is a very bad strategy. JSON encoding should be a better option, this module uses it already.

Comment 1

[Julien Vehent]

Thanks for the report. JSON encoding seems reasonable to me. We could also add a separate, like a vertical pipe "|", which isn't allowed in URLs between each parameter of the hash.

At any rate, since the current URL validation limits exploitation of this flaw, ranking as sec-low.

Comment 2

[Wladimir Palant]

Sure, a separator will do as well. However, with separators you have to make assumptions about the data. Using a generic serialization approach like JSON avoids this problem.

Comment 3

[Dylan Hardison [:dylan] (he/him)]

It looks to be unintentional that the userid of the tokens table isn't checked against the current user.
That should be fixed right away.

As for development plans:

1) All tokens should be https://metacpan.org/pod/Mojo::JWT
2) The custom CSRF tokens should instead be using the csrf tokens that are built into Mojolicious.

Comment 4

[Dylan Hardison [:dylan] (he/him)]

Attachment: Experimental WIP Pull Request

Comment 5

[April King [:April]]

Given that this is not exploitable given the particular contents of these fields, we're going to mark this as bug-bounty- (but hof+).