Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

security Issues with user-defined \p{} properties #11063

Closed
p5pRT opened this issue Jan 22, 2011 · 12 comments
Closed

security Issues with user-defined \p{} properties #11063

p5pRT opened this issue Jan 22, 2011 · 12 comments

Comments

@p5pRT
Copy link

p5pRT commented Jan 22, 2011

Migrated from rt.perl.org#82616 (status was 'open')

Searchable as RT82616$

@p5pRT
Copy link
Author

p5pRT commented Jan 22, 2011

From @iabyn

Created by @iabyn

This is a placeholder for the issues related to \p{UserDefined} being
too generous in calling functions.
For more details, see the p5p thread beginning at message-id​:

  <4CD4336F.9000801@​khwilliamson.com>

Perl Info

Flags:
    category=core
    severity=high

Site configuration information for perl 5.13.9:

Configured by davem at Fri Jan 21 13:46:15 GMT 2011.

Summary of my perl5 (revision 5 version 13 subversion 9) configuration:
  Local Commit: c43ae56ff9cdb8e0fb3d1724f564378b031a4d49
  Ancestor: e777a9142954feba63c48eced348e969729a0072
  Platform:
    osname=linux, osvers=2.6.34.7-66.fc13.x86_64, archname=x86_64-linux-thread-multi
    uname='linux pigeon 2.6.34.7-66.fc13.x86_64 #1 smp wed dec 15 07:04:30 utc 2010 x86_64 x86_64 x86_64 gnulinux '
    config_args='-des -Dusedevel -Dprefix=/home/davem/perl5/git/bleed.out -Uinstallusrbinperl -Duseithreads -Doptimize=-g -Accflags=-DDEBUG_LEAKING_SCALARS -ggdb'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBUG_LEAKING_SCALARS -ggdb -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-g',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBUG_LEAKING_SCALARS -ggdb -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.4.5 20101112 (Red Hat 4.4.5-2)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib /lib64 /usr/lib64 /usr/local/lib64
    libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=/lib/libc-2.12.2.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.12.2'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -g -L/usr/local/lib -fstack-protector'

Locally applied patches:
    


@INC for perl 5.13.9:
    lib
    /home/davem/perl5/git/bleed.out/lib/site_perl/5.13.9/x86_64-linux-thread-multi
    /home/davem/perl5/git/bleed.out/lib/site_perl/5.13.9
    /home/davem/perl5/git/bleed.out/lib/5.13.9/x86_64-linux-thread-multi
    /home/davem/perl5/git/bleed.out/lib/5.13.9
    /home/davem/perl5/git/bleed.out/lib/site_perl
    .


Environment for perl 5.13.9:
    HOME=/home/davem
    LANG=en_US.utf8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/usr/lib64/qt-3.3/bin:/usr/kerberos/sbin:/usr/kerberos/bin:/usr/lib64/ccache:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/home/davem/bin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2011

From @iabyn

On Sat, Jan 22, 2011 at 07​:12​:51AM -0800, Dave Mitchell wrote​:

This is a placeholder for the issues related to \p{UserDefined} being
too generous in calling functions.
For more details, see the p5p thread beginning at message-id​:

\<4CD4336F\.9000801@&#8203;khwilliamson\.com>

I have now added a taint check for the property function name with commit
0e9be77.

This means we now have the following new restrictions​:

* only call the function if its name begins with In or Is
* don't call the function if its name is tainted

There doesn't seem to be any consensus yet on what other measures, if any,
to take.

--
All wight. I will give you one more chance. This time, I want to hear
no Wubens. No Weginalds. No Wudolf the wed-nosed weindeers.
  -- Life of Brian

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2011

From @khwilliamson

On 02/22/2011 09​:37 AM, Dave Mitchell wrote​:

On Sat, Jan 22, 2011 at 07​:12​:51AM -0800, Dave Mitchell wrote​:

This is a placeholder for the issues related to \p{UserDefined} being
too generous in calling functions.
For more details, see the p5p thread beginning at message-id​:

 \<4CD4336F\.9000801@&#8203;khwilliamson\.com>

I have now added a taint check for the property function name with commit
0e9be77.

This means we now have the following new restrictions​:

* only call the function if its name begins with In or Is
* don't call the function if its name is tainted

There doesn't seem to be any consensus yet on what other measures, if any,
to take.

I'm very glad this got done. I suspect this needs to be applied as well
to the user-defined case changing functions, ToLower, ToUpper, ToTitle.
  Am I missing anything?

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2011

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2011

From @iabyn

On Tue, Feb 22, 2011 at 10​:23​:56AM -0700, Karl Williamson wrote​:

I'm very glad this got done. I suspect this needs to be applied as
well to the user-defined case changing functions, ToLower, ToUpper,
ToTitle. Am I missing anything?

I don't think the same rationale applies. The issue with \p{} was that
/\p{$tainted}/ could allow external data to force the calling of any named
subroutine it wanted. I don't see how that could occur with ToLower etc.

--
A walk of a thousand miles begins with a single step...
then continues for another 1,999,999 or so.

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2011

From marvin@rectangular.com

On Tue, Feb 22, 2011 at 10​:23​:56AM -0700, Karl Williamson wrote​:

I have now added a taint check for the property function name with commit
0e9be77.

This means we now have the following new restrictions​:

* only call the function if its name begins with In or Is
* don't call the function if its name is tainted

There doesn't seem to be any consensus yet on what other measures, if any,
to take.

I'm very glad this got done.

Seconded. Thanks for taking this on, Dave!

Marvin Humphrey

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2011

From tchrist@perl.com

Karl Williamson <public@​khwilliamson.com> wrote
on Tue, 22 Feb 2011 10​:23​:56 MST​:

On 02/22/2011 09​:37 AM, Dave Mitchell wrote​:

On Sat, Jan 22, 2011 at 07​:12​:51AM -0800, Dave Mitchell wrote​:

This is a placeholder for the issues related to \p{UserDefined} being
too generous in calling functions.
For more details, see the p5p thread beginning at message-id​:

 \<4CD4336F\.9000801@&#8203;khwilliamson\.com>

I have now added a taint check for the property function name with commit
0e9be77.

This means we now have the following new restrictions​:

* only call the function if its name begins with In or Is
* don't call the function if its name is tainted

There doesn't seem to be any consensus yet on what other measures, if any,
to take.

I'm very glad this got done. I suspect this needs to be applied as well
to the user-defined case changing functions, ToLower, ToUpper, ToTitle.

You mean for something that \U etc map to?

Am I missing anything?

I don't understand the need for the function to match /^I[ns]/.
If there *is* a security matter, I don't understand how it can
be addressed by choosing "careful" names.

I'd imagine a clearer registration system à la charnames might be
in order. I find myself using custom charnames quite a lot.
Yesterday I found myself writing​:

  # In case customizing charnames files are in bin not lib...
  use FindBin;
  use lib $FindBin​::Bin;

  use charnames (
  "​:full",
  "​:short",
  "​:alias" => "glyphs",
  "​:alias" => "html",
  "latin",
  "greek",
  "​:alias" => "uwords",
  );

I also expose those to patterns read in from the user,
which currently requires an eval { qr/$pattern }.

Why shouldn't we do custom properties in some similar fashion?

--tom

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2011

From @iabyn

On Tue, Feb 22, 2011 at 11​:02​:53AM -0800, tchrist1 via RT wrote​:

I don't understand the need for the function to match /^I[ns]/.
If there *is* a security matter, I don't understand how it can
be addressed by choosing "careful" names.

Because it was documented that it only called functions whose names
matched /^I[ns]/. That sop making it actually do that was a bug fix,
which happily as a side-effect, reduced vastly the proportion of random
functions that a malicious attacker could cause the code the call.

I'd imagine a clearer registration system à la charnames might be in
order.

But we need something that does doesn't break existing code (too much)
while making that code more secure.

--
In my day, we used to edit the inodes by hand. With magnets.

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2011

From tchrist@perl.com

On Tue, Feb 22, 2011 at 11​:02​:53AM -0800, tchrist1 via RT wrote​:

I don't understand the need for the function to match /^I[ns]/.
If there *is* a security matter, I don't understand how it can
be addressed by choosing "careful" names.

Because it was documented that it only called functions whose names
matched /^I[ns]/.

Ok.

That sop making it actually do that was a bug fix, which happily as a
side-effect, reduced vastly the proportion of random functions that a
malicious attacker could cause the code the call.

I'd imagine a clearer registration system à la charnames
might be in order.

But we need something that does doesn't break existing code (too much)
while making that code more secure.

I see you recognize how those two goals can easily conflict. :)

I believe Abigail has used custom properties a great deal more than
I have. Mine have been limited to things like \p{vowel} and \p{consonant}.

I never use the /^I[sn]/. I guess I've come to dislike those particular
prefixes because of how wildly differently various regex systems have
implemented them. I always say \p{Block=Whatever} these days, although
I still use \p{Scriptname} for \p{Scriptname}, and seldom (but not never)
use \p{GC=Category}.

I rather like \p{OUR​::Customized} for a customization function in
the current package. I'm still toying with the idea of clearer
namespaces for properties.

--tom

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2011

From tchrist@perl.com

I miswrote​:

I never use the /^I[sn]/. I guess I've come to dislike those particular
prefixes because of how wildly differently various regex systems have
implemented them. I always say \p{Block=Whatever} these days, although

I still use \p{Scriptname} for \p{Scriptname}, and seldom (but not never)
use \p{GC=Category}.

I of course meant​:

  I still use \p{Scriptname} for \p{Script=Scriptname},
  and seldom (but not never) use \p{GC=Category}.

--tom

@p5pRT
Copy link
Author

p5pRT commented Feb 22, 2011

From @Abigail

On Tue, Feb 22, 2011 at 12​:18​:54PM -0700, Tom Christiansen wrote​:

On Tue, Feb 22, 2011 at 11​:02​:53AM -0800, tchrist1 via RT wrote​:

I don't understand the need for the function to match /^I[ns]/.
If there *is* a security matter, I don't understand how it can
be addressed by choosing "careful" names.

Because it was documented that it only called functions whose names
matched /^I[ns]/.

Ok.

That sop making it actually do that was a bug fix, which happily as a
side-effect, reduced vastly the proportion of random functions that a
malicious attacker could cause the code the call.

Yeah. OTOH, I kind of like the idea that \p{Whatever} is possible. The
prefix "In/Is" seems like an arbitrary and pointless restriction. Specially
since there's an awful lot of code that doesn't execute patterns fed by
potentially malicious attackers - so they have nothing to worry about.

I wonder if it's worthwhile to allow \p{Whatever} if 'use re "eval";' is
in effect.

I'd imagine a clearer registration system à la charnames
might be in order.

But we need something that does doesn't break existing code (too much)
while making that code more secure.

I see you recognize how those two goals can easily conflict. :)

I believe Abigail has used custom properties a great deal more than
I have. Mine have been limited to things like \p{vowel} and \p{consonant}.

Outside of the realm "what happens if I pull this lever", I've only used
\p{Is...} (not even \p{In...}). But I haven't been quiet about the fact
that \p{Whatever} is possible. As I said, I like the idea one can (could)
use \p{Whatever} - I never did because it was documented not to.

Of course, most of the time, I use /[$whatever]/, or /$thisaswell/, because

  $whatever = "acegikmoqsuwy"; # or
  $thisaswell = "[bdfhjlnprtvxz]";

is a lot easier to write, and a lot clearer than subs for \p{IsWhatever}.

I never use the /^I[sn]/. I guess I've come to dislike those particular
prefixes because of how wildly differently various regex systems have
implemented them. I always say \p{Block=Whatever} these days, although
I still use \p{Scriptname} for \p{Scriptname}, and seldom (but not never)
use \p{GC=Category}.

I rather like \p{OUR​::Customized} for a customization function in
the current package. I'm still toying with the idea of clearer
namespaces for properties.

Abigail

@khwilliamson
Copy link
Contributor

The case changing functions have been removed, and the restriction to In... Is... name, and the taintedness checking should be sufficient

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants