Backport bug 767623 to BMO: Use HMAC to generate tokens and sensitive graph filenames

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
7 months ago
a month ago

People

(Reporter: gaubugzilla, Assigned: dylan)

Tracking

({sec-low, wsec-csrf})

Bug Flags:
sec-bounty +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [reporter-external] [web-bounty-form] [verif?])

Attachments

(1 attachment)

(Reporter)

Description

7 months ago
A note up front: from what I can tell, exploiting this issue is currently very difficult. But even if that's the case, future code additions might open up vulnerabilities by relying on this code to work properly.

Bugzilla's CSRF tokens are currently generated as MD5 checksums over a number of values concatenated with asterisks (see Bugzilla::Token::issue_hash_token):

> MD5(<time>*<secret>*<userid>*<param1>*<param2>)

This is obviously the wrong way to do it. Not only is MD5 considered broken, HMAC is the proper way of hashing data along with a secret.

As to how to exploit this weakness, figuring this out takes a crypto expert which I'm not. My research indicated three potential scenarios however:

1. A length extension attack is easy to perform. However, it will only give you another valid CSRF token for the same user ID, nothing you couldn't get by other means. It will also allow manipulating token parameters. However, all relevant code will currently validate these parameters independently of the token itself, so there is nothing to gain from that either.

2. Inserting one more block at the beginning of the data, thus prepending the timestamp by 64 bytes. It would allow reusing expired tokens, e.g. if SSL traffic is intercepted and decoded later: the Bugzilla::Token::check_hash_token function doesn't validate the timestamp, the byte sequence merely needs to start with a large enough number and the token won't be considered expired. Executing this attack without changing the hash value requires a preimage attack however - while a preimage attack on MD5 has been published, it's still only a theoretical possibility at this point.

3. Generating a collision for the data block containing the user ID, by manipulating token generation parameters (param2 can be set to an arbitrary value in some cases). MD5 collisions differ only in a few bits, so you might get another valid CSRF token for somebody else's user ID. How feasible this attack is given that part of the data block (first 11 bytes) is the unknown site secret isn't something I can tell.

The obvious solution would be using HMAC-SHA256 for these tokens, this approach is already taken elsewhere in the same module.
Flags: sec-bounty?
Thanks for the report!

> The obvious solution would be using HMAC-SHA256 for these tokens

Yep.
Assignee: nobody → dylan
Group: websites-security
Status: NEW → ASSIGNED
Component: Other → WebService
Keywords: sec-low, wsec-csrf
Product: Websites → Bugzilla
QA Contact: default-qa
(Assignee)

Comment 2

7 months ago
This is a duplicate of bug 767623, and has been fixed in bugzilla upstream for a long time. I'm going to relabel this bug as a backport.
Component: WebService → General
Product: Bugzilla → bugzilla.mozilla.org
QA Contact: default-qa
Summary: Bugzilla uses broken crypto for CSRF tokens → Backport bug 767623 to BMO
Version: unspecified → Production
(Assignee)

Comment 3

7 months ago
Posted file GitHub Pull Request
(Assignee)

Comment 4

7 months ago
Note: pushing this will cause an interruption of service of sorts.
(Assignee)

Updated

7 months ago
Summary: Backport bug 767623 to BMO → Backport bug 767623 to BMO: Use HMAC to generate tokens and sensitive graph filenames
(Assignee)

Updated

6 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Although not directly exploitable as a CSRF against users (mainly mitigated by both timing and TLS mitigations) the panel felt this was a security vulnerability and worthy of a bounty.
Flags: sec-bounty? → sec-bounty+
You need to log in before you can comment on or make changes to this bug.