Closed Bug 555766 Opened 15 years ago Closed 15 years ago

Multiple methods contain SQL Injection via group name

Categories

(Bugzilla :: Administration, task)

task
Not set
normal

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
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
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.
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 }
id is defined as an Integer in the SQL schema.
mcoates: are you doing a security review of the Bugzilla code? That would be very cool. If not, how did you notice this? Gerv
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.