Closed (fixed)
Project:
Project
Version:
4.7.x-1.x-dev
Component:
Releases
Priority:
Critical
Category:
Bug report
Assigned:
Reporter:
Created:
2 Nov 2006 at 20:44 UTC
Updated:
23 Nov 2006 at 10:31 UTC
Jump to comment: Most recent file
in the releases-are-nodes world (see http://drupal.org/node/83339) in the DRUPAL-4-7--2 branch of this code, there's a potential privilege escalation bug where a user with "administer projects" permission could XSS the site through unsafe printing of the version format string (which, as i was writing the code, i assumed we could trust). certainly, on drupal.org, the only people with "administer projects" permissions already have full html powers (if not php and then some), so it's not a security concern here. but, to be truly secure and happy, we should clean this up to make sure we don't consider the version format string trusted input, just to keep "administer projects" a fine-grained permission for other sites using this code, and not assume anyone who has that is basically a trusted-user already.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | package_release_nodes_validate_input.patch.txt | 8.71 KB | dww |
| #2 | package_release_nodes_no_trust.patch.txt | 1.77 KB | dww |
| #1 | project_release_extra_check_plain.patch.txt | 1.75 KB | dww |
Comments
Comment #1
dwwwow, after a more careful audit, not only is it clear i was already being paranoid enough about the version format string (it's only ever printed either via
theme('placeholder')or directly in form elements), i was in fact being over zealous with mycheck_plain()to the point of double-escaped output if someone *did* try to put a<a href>...</a>into a format string. ;)attached patch fixes this... though perhaps we should just be stripping *all* html and other things from the format string and version string in various places. check_plain() is all well and good, but it's probably just silly to allow html tags (even if harmless) in these strings (both formats and final versions).
Comment #2
dwwwhoops, that last comment wasn't what was supposed to be critical, this one is. ;)
unlike the project_release.module code, the packaging script itself was *not* treating the version string (and a few other things specified by project maintainers) as unsafe input, and that's bad, bad, bad... so, the patch attached here fixes these critical problems (by aggressive stripping of html and calling
escapeshellcmdon any user-supplied data that ends up in a shell command). in fact, since it occured to me that this exploit was available right now on s.d.o, and since drumm happened to be in IRC (to restart apache on api.d.o), i had him update the packaging script already. ;) so, this part has already been committed to DRUPAL-4-7--2 branch as revision 1.1.2.9.Comment #3
dwwafter further discussions with heine, we decided by initial attempt at plugging this hole weren't really enough. just stripping out all the html tags doesn't necessarily result in safe filenames. so, the new approach is to validate a few things on input so we're sure they're safe all along. instead of trying to exclude potentially bad characters, we just enforce that all of these values contain only alphanumeric, plus a few appropriate punctuation marks:
_ - /_ -_ - .plus the characters stripped out when used as variable identifiers:% # !Comment #4
dwwafter review from Kjartan, i committed both the patches from #3 and #1 to DRUPAL-4-7--2 branch (with 1 minor change: i realized the regexp to validate the version format string wasn't allowing
., even though the help text said it should). i also committed the cvs.module diff (to validate the "cvs directory") to DRUPAL-4-7 and HEAD, though it's not really a security problem there (the current d.o packaging scripts don't touch this data like package-release-nodes.php does). but, it seems like a good idea, to at least prevent stupidity (if not malice). ;)Kjartan updated s.d.o (and the minor diff to cvs.module on d.o) so this is hereby a solved problem. ;)
Comment #5
(not verified) commented