From: Nick C. <ni...@cl...> - 2003-01-31 15:01:55
|
On Fri, Jan 31, 2003 at 08:23:52AM -0500, Wizard wrote: > > A few points: > > You can use get() like this: > > > > $_ =~ s/PARSE_ITEM_([a-zA-Z0-9_-]+)/ $cfg->get("MESSAGE_$1") /ge; > Yeah, I thought of that, but /e always makes me nervous parsing external > data, so I've tried to avoid it. If you think it would be better to do it > that way, then I'd be happy to change it. Yes, I think that's better. /e is fine - it just makes the code on the right hand side of the replace act as code, the same as code elsewhere in your script. You'd be right to be nervous of user input anywhere neat /ee though, because that's really a string eval. > > > Also, the patten should probably start with: > > /\bPARSE_ITEM_ > The tag name is a poor choice, and I plan on standardizing on NMS_* for all > tags, but the reason that I didn't do word boundaries was because I'm not > sure that there won't be a situation in which tags will need to be appended > to strings. For lack of a better example: > '<a href="/new_pathNMS_CFG_ITEM_wwwpost_url">Try the new list</a>' In that case, it might be better to have some sort of non-alpha delimiter, e.g. '<a href="/new_path[% NMS_CFG_ITEM %]_wwwpost_url">Try the new list</a>' since that'll make what's going on more obvious to someone reading the template. > > CGI::NMS::Data is badly named. This module contains code that looks > > pretty specific to nmsboard, so it shouldn't have such a general > > sounding name. How about something like > > CGI::NMS::Script::NMSBoard::Data ? > I'm not sure what the 'Script' is for (they're all scripts, right ;-). How > about CGI::NMS::NMSBoard::Data? There will also be a DataAdmin module, so > I'll likely rename it DataView, also. The scripts are all scripts, but not every module in CGI::NMS:: is associated with a particular script, e.g. CGI::NMS::Charset under /v2 in the CVS tree. I like the distinction of putting stuff specific to script foo under CGI::NMS::Script::Foo. > > You have at least one arbitrary command execution vulnerability in > > there. > I haven't done any escaping of CGI data yet (like .cfg filenames) but have > every intention of doing so, if that is what you mean. If I'm missing > something else, please let me know. > (Clue please, so I don't have to go hunting. ;-) You read the config filename from a CGI input, and call open() on it with no checking, so a 'cfg' value of "|`rm -rf /`" would cause the command 'rm -rf /' to run. IMO instead of: > # check if 'cfg' is set via the URL query_string, if so use that. > $def_cfg = $cgi_data->param('cfg') if( $cgi_data->param('cfg') ); ... you should be doing something more like: # check if 'cfg' is set via the URL query_string, if so use that. my $cfg_input = $cgi_data->param('cfg'); if ( defined $cfg_input ) { $cfg_input =~ /^([\w\-]{1,100})$/ or die "bad cfg value [$cfg_input]"; $def_cfg = "./conf/$1.cfg"; } ... which tightly limits the name of the config file. Also, in the open in CGI::NMS::Config you should add a '<' to the front of the filename so that the open can't act as a piped open and run commands even if an attacker gets something past the checks. -- Nick |