Push extension configuration is vulnerable to CSRF

RESOLVED FIXED

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gaubugzilla, Assigned: dkl)

Tracking

({sec-moderate, wsec-csrf})

Production
sec-moderate, wsec-csrf
Bug Flags:
sec-bounty +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Push extension's configuration page lacks CSRF protection. This means that somebody can lure Bugzilla admins on a page containing the following code:

> <img src="https://bugzilla-dev.allizom.org/page.cgi?id=push_config.html&save=1&global.enabled=Disabled&global.log_purge=0&Aha.enabled=Disabled&Aha.account_domain=&Aha.account_realm=&Aha.username=&Aha.password=&File.enabled=Enabled&File.filename=../show_bug.cgi&ReviewBoard.enabled=Disabled&ReviewBoard.product=&ReviewBoard.component=&ReviewBoard.version=&ReviewBoard.group=&ReviewBoard.cc=&TCL.enabled=Disabled&TCL.tcl_user=&TCL.sftp_host=&TCL.sftp_port=&TCL.sftp_user=&TCL.sftp_pass=&TCL.sftp_remote_path=">

This will successfully change configuration parameters of the Push extension, and in this particular case disable almost everything. I didn't spend much time to investigate abuse potential here, in particular what the security group setting for ReviewBoard is doing. The link used above demonstrates one issue however: by setting File.filename parameter one can override files, even if they aren't located inside Bugzilla's data directory. I didn't bother figuring out whether one can somehow control the data being written to these files, but it allows adding data to a CGI file meaning that this could turn into a code execution vulnerability.
(Assignee)

Updated

3 years ago
Assignee: nobody → dkl
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8724947 [details] [diff] [review]
1252216_1.patch

- Added csrf token checking for configuration UI
- Added relative path check for File.pm filenames
Attachment #8724947 - Flags: review?(dylan)
(Assignee)

Comment 2

3 years ago
Created attachment 8724985 [details] [diff] [review]
1252216_2.patch

Forgot to call delete_token()
Attachment #8724947 - Attachment is obsolete: true
Attachment #8724947 - Flags: review?(dylan)
Attachment #8724985 - Flags: review?(dylan)
Flags: sec-bounty?
(In reply to Wladimir Palant from comment #0)
> ... but it allows adding data to a CGI file
> meaning that this could turn into a code execution vulnerability.

It is bad that paths weren't checked, but you'd have to find another location to abuse -- only data is writable by the httpd process. "Could turn into" would require code changes because the permissions are enforced by bugzilla code.

Amusingly, if anyone were to use this extension on Windows, the check would be completely meaningless because paths don't start with /.
Attachment #8724985 - Flags: review?(dylan) → review+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   4d95649..e5b9aa6  master -> master
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Summary: Push extension configuration is vulnerable to CSRF and potentially code execution → Push extension configuration is vulnerable to CSRF
(Reporter)

Comment 5

3 years ago
(In reply to Dylan William Hardison [:dylan] from comment #3)
> It is bad that paths weren't checked, but you'd have to find another
> location to abuse -- only data is writable by the httpd process. "Could turn
> into" would require code changes because the permissions are enforced by
> bugzilla code.

Sure, I could only judge my test installation where this location looked writable. The other obvious victim would be "params" file of course - it's located inside the data directory and yet being executed (running inside Safe but probably able to do damage regardless). Or the template cache - it's located outside the data directory but I suspect that it must be writable.
Group: bugzilla-security
dkl: could you take a stab at a security rating on this bug? What could be done to the Push extension, what damage would that cause to the installation?
Flags: needinfo?(dkl)
(Assignee)

Updated

3 years ago
Flags: needinfo?(dkl)
sec-moderate based on feedback from dkl.
Flags: sec-bounty? → sec-bounty+
Keywords: sec-moderate
Keywords: wsec-csrf
You need to log in before you can comment on or make changes to this bug.