Michael Coates [:mcoates] (acct no longer active) Reporter
	Description • 14 years ago

Issue

Multiple methods within User.pm contains potential SQL injection vulnerabilities due to SQL statements that use string concatenation with the group names. There is a potential risk that a malicious user that has the ability to create groups could insert a SQL injection attack within the group name.  The malicious SQL would be executed each time vulnerable code is run with a user that is a member of the new group.

Vulnerable Methods:
can_edit_product
visible_bugs 
get_selectable_products
get_enterable_products

Recommendation Remediation:
Use parameterized queries to safely build the SQL statement. Ensure that all dynamic data, especially the group names, leverage the parameterized query creation process instead of the vulnerable string concatenation method.

Even if this is not currently exploitable, we want to modify this code to ensure that future modifications or enhancements do not insecurely reference this code and introduce a vulnerability.

Relevant Code:
http://mxr.mozilla.org/bugzilla/source/Bugzilla/User.pm#604
http://mxr.mozilla.org/bugzilla/source/Bugzilla/User.pm#655
http://mxr.mozilla.org/bugzilla/source/Bugzilla/User.pm#704
http://mxr.mozilla.org/bugzilla/source/Bugzilla/User.pm#801

	Frédéric Buclin
	Comment 1 • 14 years ago

groups_as_string() is a concatenation of group IDs, not names. That's not exploitable. There is no SQL injection here.

	Michael Coates [:mcoates] (acct no longer active) Reporter
	Comment 2 • 14 years ago

Thank you for the clarification. Assuming of course that the group ID is controlled by the app itself and cannot be influenced by the user (most likely case), then I agree.  No issue here.

	Dave Miller [:justdave]
	Comment 3 • 14 years ago

Confirmed invalid...  follow the links in MXR (it's good at that ;)

461 sub groups_as_string {
462     my $self = shift;
463     my @ids = map { $_->id } @{ $self->groups };
464     return scalar(@ids) ? join(',', @ids) : '-1';
465 }

$self->groups returns an array of group objects.
Note the map { $_->id }

	Dave Miller [:justdave]
	Comment 4 • 14 years ago

id is defined as an Integer in the SQL schema.

	Gervase Markham [:gerv]
	Comment 5 • 14 years ago

mcoates: are you doing a security review of the Bugzilla code? That would be very cool. If not, how did you notice this?

Gerv

	Michael Coates [:mcoates] (acct no longer active) Reporter
	Comment 6 • 14 years ago

Gerv,

Yes, we are performing a security review of the bugzilla running app and code during Q2.  Feel free to contact me if you want to discuss more or have some particular areas of concern.

-Michael