Closed
Bug 555766
Opened 15 years ago
Closed 15 years ago
Multiple methods contain SQL Injection via group name
Categories
(Bugzilla :: Administration, task)
Bugzilla
Administration
Tracking
()
RESOLVED
INVALID
People
(Reporter: mcoates, Unassigned)
References
Details
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
Reporter | ||
Updated•15 years ago
|
Blocks: q2-review-bmo
Comment 1•15 years ago
|
||
groups_as_string() is a concatenation of group IDs, not names. That's not exploitable. There is no SQL injection here.
Group: bugzilla-security
Severity: major → normal
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → INVALID
Reporter | ||
Comment 2•15 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.
Comment 3•15 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 }
Comment 4•15 years ago
|
||
id is defined as an Integer in the SQL schema.
Comment 5•15 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
Reporter | ||
Comment 6•15 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
You need to log in
before you can comment on or make changes to this bug.
Description
•