705474 - CSRF vulnerability in createaccount.cgi allows possible unauthorized account creation e-mail request

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Mario Gomes]

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/535.8 (KHTML, like Gecko) Chrome/17.0.942.0 Safari/535.8

Steps to reproduce:

Hello,

There is a CSRF vulnerability in Bugzilla that allows sending several e-mails from registering accounts in Bugzilla.

Reproduce:
1. Logout of Bugzilla
2. Go to "javascript:url='https://landfill.bugzilla.org/bugzilla-tip/createaccount.cgi?login=DEMOCSRF'+parseInt(Math.random()*10000)+'@gmail.com'; location.href=url;"
3. See the email sent.


Actual results:

sent the email.


Expected results:

Bugzilla does not make use of Tonkes to inhibit this type of attack.

Comment 1

[Frédéric Buclin]

There is no CSRF vulnerability at all here. You don't need any form of validation to request an account.

Comment 2

[Max Kanat-Alexander]

Ah, no, this is not invalid. It's just not very critical. We should generate a hash token and check it when the form is submitted.

Comment 3

[Reed Loden [:reed]]

Attachment: patch - v1 (untested)

Something like this? Since the user isn't logged-in, I made issue_hash_token() use remote_ip() as the "user ID" part of the token. This will also allow me to fix bug 471801.

Comment 4

[Max Kanat-Alexander]

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

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

::: Bugzilla/Token.pm
@@ +178,5 @@
>  
> +    # For the user ID, use the actual ID if the user is logged in.
> +    # Otherwise, use the remote IP, in case this is for something
> +    # such as creating an account or logging-in.
> +    my $user_id = Bugzilla->user ? Bugzilla->user->id : remote_ip();

Actually, you should just do Bugzilla->user->id || remote_ip();

Bugzilla->user is always defined.

This does theoretically allow anybody with the same IP address to CSRF this, but I wouldn't say this is a high-security issue.

Comment 5

[Reed Loden [:reed]]

Attachment: patch - v2

Comment 6

[Frédéric Buclin]

(In reply to Reed Loden [:reed] (very busy) from comment #3)
> Something like this? Since the user isn't logged-in, I made
> issue_hash_token() use remote_ip() as the "user ID" part of the token.

Did you make sure that all places which call issue_hash_token() aren't affected by this change?


(In reply to Max Kanat-Alexander from comment #2)
> Ah, no, this is not invalid. It's just not very critical. We should generate
> a hash token and check it when the form is submitted.

How is that a security bug? No account is created. Only the user getting the email can confirm the account creation.

Comment 7

[Reed Loden [:reed]]

(In reply to Frédéric Buclin from comment #6)
> (In reply to Reed Loden [:reed] (very busy) from comment #3)
> > Something like this? Since the user isn't logged-in, I made
> > issue_hash_token() use remote_ip() as the "user ID" part of the token.
> 
> Did you make sure that all places which call issue_hash_token() aren't
> affected by this change?

Unless they already rely on Bugzilla->User->id being undef or null or whatever happens when there's no actual Bugzilla user, then I don't think so... I just did a quick grep through the codebase, and I didn't see anything that worried me or gave cause for concern.

> (In reply to Max Kanat-Alexander from comment #2)
> > Ah, no, this is not invalid. It's just not very critical. We should generate
> > a hash token and check it when the form is submitted.
> 
> How is that a security bug? No account is created. Only the user getting the
> email can confirm the account creation.

Yes, but somebody could still use it to send a ton of account creation e-mails to various addresses (without the consent of the user actually submitting the form). It's a super-minor "security" issue, but there's no reason why it shouldn't be fixed, especially when the fix is so easy (and useful in other cases as well).

Comment 8

[Frédéric Buclin]

(In reply to Reed Loden [:reed] (very busy) from comment #7)
> Yes, but somebody could still use it to send a ton of account creation
> e-mails to various addresses

You know that the attacker could still use e.g. Selenium to achieve the same goal, even with your patch applied? The attacker really doesn't need someone else to run his "evil" code as you don't need to be logged in.

Also, FYI, when you are logged out, Bugzilla->user->id returns 0. If I see any regression due to this patch, I will back it out immediately.

Comment 9

[Max Kanat-Alexander]

I could create a web page that made you send account creation emails to people. That would be pretty obnoxious. The CSRF could also be a sort of DDoS for free, while Selenium would require you to actually control the machines you're attacking from.

I agree that its effect is pretty minor, but I don't want people to create some annoying script that exploits this, before we have a checked-in fix.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 577118 [details] [diff] [review]
patch - v2

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

::: Bugzilla/Token.pm
@@ +177,5 @@
>      $time ||= time();
>  
> +    # For the user ID, use the actual ID if the user is logged in.
> +    # Otherwise, use the remote IP, in case this is for something
> +    # such as creating an account or logging-in.

Nit: No hyphen.

Comment 11

[Max Kanat-Alexander]

Approved for immediate checkin.

Comment 12

[Reed Loden [:reed]]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified createaccount.cgi
modified Bugzilla/Token.pm
modified template/en/default/account/create.html.tmpl
Committed revision 8042.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.2/
modified createaccount.cgi
modified Bugzilla/Token.pm
modified template/en/default/account/create.html.tmpl
Committed revision 7981.

Comment 13

[Frédéric Buclin]

This breaks test_create_user_accounts.t and webservice_user_offer_account_by_email.t. I will check if that's a bug in the QA scripts or in this patch.

Comment 14

[Frédéric Buclin]

This is not a bug in the QA script, but a real regression:

Undefined subroutine &main::check_hash_token called at /var/www/html/bugzilla_qa42/createaccount.cgi line 68.

Comment 15

[Frédéric Buclin]

createaccount.cgi had no |use Bugzilla::Token| and so couldn't find check_hash_token(). This was trivial to catch with minimal testing. Next time, please *test* your patches, especially when committing to branches. This attitude is unacceptable.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified createaccount.cgi
Committed revision 8046.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified createaccount.cgi
Committed revision 7984.