See SA-CONTRIB-2012-036 at http://drupal.org/node/1482126

Ubercart Bulk Stock Updater is an extension module for Ubercart 2.x running on Drupal 6.x which makes it easy to bulk-edit product stock levels. This module does not properly use the formAPI and this results in a CSRF attack vector.

CommentFileSizeAuthor
#7 uc_bsu-1515240-5.patch2.24 KBSeanA
#3 uc_bsu-1515240-3.patch1.51 KBSeanA
#1 1515240_fix_csrf.patch1.52 KBbenjaminbradley

Comments

benjaminbradley’s picture

benjaminbradley commented
Assigned: benjaminbradley » Unassigned
Status: Active » Needs review
StatusFileSize
new 1515240_fix_csrf.patch1.52 KB

Here is a simple fix which adds token validation to the AJAX request, thereby eliminating the CSVF vulnerability.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Version: » 6.x-1.x-dev
Status: Needs review » Needs work

Looks solid. drupal_get_token() should ideally be called with something specific to the action like a form ID and/or an order ID (or both concatenated together).

SeanA’s picture

SeanA commented
Status: Needs work » Needs review
StatusFileSize
new uc_bsu-1515240-3.patch1.51 KB

Not sure which version the first patch was based on, but function uc_bulk_stock_updater_stock_update_ajax() is slightly different in the git 'master' branch of this module. Here's a reworked patch. I used drupal_get_token('ucbsu') to generate the token.

Bryan Cordrey’s picture

Bryan Cordrey commented
Project: Ubercart Bulk Stock Updater » Security
Version: 6.x-1.x-dev » master
Status: Needs review » Reviewed & tested by the community

The patch in #3 seems to fix the issue. I have requested maintainership of this module however they want the patch to be approved prior to approval. I have moved this to the Security project, but if that is not correct please let me know how I could get this patch approved.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Project: Security » Ubercart Bulk Stock Updater
Version: master » 6.x-1.x-dev
Status: Reviewed & tested by the community » Needs work

Security is unrelated to the security team. http://drupal.org/node/251466 details what you should do and sreynen sent you that link 3 days ago, so it seems odd to say you are not sure if your action is the correct action.

Having reviewed this patch I suggest changing the token generation/validation so that it uses something like the node ID of the item being updated instead of just using ucbsu as the seed of the token.

zkrebs’s picture

zkrebs commented

Any update on this, client still would like to use this.

SeanA’s picture

SeanA commented
Status: Needs work » Needs review
StatusFileSize
new uc_bsu-1515240-5.patch2.24 KB

In this patch, a token based on the product sku is added to each row in the table and is used to validate editable fields in that row. The sku is used instead of the node id, since the sku is already available in the javascript, and it should be unique. (It only makes sense to have unique sku's on your products although I don't think Ubercart actually enforces it.)

This module seems too useful to let it languish, and I'm not aware of anything else that provides a similar feature. So if this patch passes review, I'll apply to be maintainer and work on updating it for D7.