Thread: [Pas-dev] for better or worse, XSS filtering is here

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

Hopefully it'll be a welcome addition to our features. 

--
Mental (Me...@Ne...)

I took a baby shower.
--Steven Wright

GPG public key: http://www.neverlight.com/Mental.asc

> Hopefully it'll be a welcome addition to our features. 

Does it account for the possibility of multiple values for a single query 
parameter?

Kyle

-- 

------------------------------------------------------------------------------
Wisdom and Compassion are inseparable.
        -- Christmas Humphreys
mo...@vo...                            http://www.voicenet.com/~mortis
------------------------------------------------------------------------------

> > Hopefully it'll be a welcome addition to our features. 
> 
> Does it account for the possibility of multiple values for a single query 
> parameter?

Also, what about the possibility of malicious code in the query parameter 
names themselves?

Kyle

-- 

------------------------------------------------------------------------------
Wisdom and Compassion are inseparable.
        -- Christmas Humphreys
mo...@vo...                            http://www.voicenet.com/~mortis
------------------------------------------------------------------------------

> Also, what about the possibility of malicious code in the query parameter 
> names themselves?

Ok, I'm about to check in a changed version of the function.  The changes
include support for defanging the query parameter names, and it accounts
for multiple values for a query parameter.  It also uses the logging to 
log a warning that includes the remote addr (not completely reliable, but
better than nothing).

Kyle

==>

my $scriptingRegex =
  qr/(<\s*script[\s+?.*?>|>].*?\<\s*?\/script\s*?>)| 
     (<\s*embed[\s+?.*?>|>].*?\<\s*?\/embed\s*?>)|
     (<\s*applet[\s+?.*?>|>].*?\<\s*?\/applet\s*?>)|
     (<\s*object[\s+?.*?>|>].*?\<\s*?\/object\s*?>)/imx;

sub inspect_request
{
  my ($q)  = @_;

  my @badParameters = ();
  foreach my $p ($q->param()){
    if ($p =~ $scriptingRegex ) {
      $log->warn("possible XSS attack found while scrubbing ",
        "parameter names: '$p' REMOTE_ADDR: ",$ENV{'REMOTE_ADDR'}
      );
      push @badParameters, $p;
    }
  }

  foreach my $badParam (@badParameters) {
    my @values = $q->param($badParam);
    $q->delete($badParam);
    my $p = HTML::Entities::encode_entities($badParam);
    $q->param($p,@values);
  }

  foreach my $p ($q->param()){
    my @x = $q->param($p);
    foreach my $x ( @x ) {
      if ($x =~ $scriptingRegex ) {
        $log->warn("possible XSS attack found while scrubbing ",
          "parameter values: '$p' => '$x' REMOTE_ADDR: ",$ENV{'REMOTE_ADDR'}
        );
        $x = HTML::Entities::encode_entities($x);
        #$q->param($p,$x);
      }
    }
    $q->param($p,@x);
  }

  return $q;
}

-- 

------------------------------------------------------------------------------
Wisdom and Compassion are inseparable.
        -- Christmas Humphreys
mo...@vo...                            http://www.voicenet.com/~mortis
------------------------------------------------------------------------------

> > Ok, I'm about to check in a changed version of the function.  The changes
> > include support for defanging the query parameter names, and it accounts
> > for multiple values for a query parameter.  It also uses the logging to 
> > log a warning that includes the remote addr (not completely reliable, but
> > better than nothing).
> 
> Hey thanks. 

Dude, thank you for implementing it in the first place.  

From here on forward, I'd like to see us discuss new objects before we
add them -- do a little bit of group desing and concensus descision
making before we leap.  

I'm not proposing backing anything out at this point, just that we [all]
try to take a more democratic approach for enhancements that either change
the API, change behavior, or add new classes.  Silent or invisible changes
(like migration of _gss to makeGetSetMethods) are probably ok - just so
long as they actualy are invisible.

> I'll see what else I can dig up. Next up is me making the CVS code, my
> actual honest to god this is what runs the site code. 
> 
> I'll need to go back over Justins ideas about the db stuff before I start,
> but I may very well wind up doing 2 iterations of Postgres stuff. First
> make it work, then refactor how db stuff works in general.

Don't worry about how pretty it is, also don't worry about missing features.
As long as it doens't break the codebase, check it in -- you never know
what everyone else may be able to contribute to your ideas.  Working code
is alot easier to comment on than just ideas....

I just tested your code with my changes:

 http://localhost/pas/diagnostics/dump.psp?foo=%3Cscript%3Efoo%3C/script%3E
 http://localhost/pas/diagnostics/dump.psp?fo%3Cscript%3Ebar%3C/script%3E=%3Cscript%3Efoo%3C/script%3E

And it works like a charm -- the scripting is defanged.  Thanks!

We need to put this in our changes/news file, package up a new release,
and update the website, and tell the world that we can stop XSS.

Oh, one mroe thing -- how about we invert the logic for the config setting?
Make it on by default.  So we'd change:

 pas.enableRequestInquisitor=1

to:

 pas.disableRequestInquisitor=1

and invert the logic check (if => unless) in the code.  That way it's 
on by default, and an administrator has to ignore our warnings and 
best coding practices and turn it off explicitly if they want to.

?

Kyle

-- 

------------------------------------------------------------------------------
Wisdom and Compassion are inseparable.
        -- Christmas Humphreys
mo...@vo...                            http://www.voicenet.com/~mortis
------------------------------------------------------------------------------

Thread: [Pas-dev] for better or worse, XSS filtering is here

pas-dev