Move . to the end of the library search path

[sorear]

Here's a snippet of strace output after I accidentally ran parrot-nqp in a directory with a Regex.pbc file:

stat64("./Regex.pbc", {st_mode=S_IFREG|0644, st_size=100432, ...}) = 0
open("./Regex.pbc", O_RDONLY|O_LARGEFILE) = 3
stat64("./P6object.pbc", 0xbf9ee9bc)    = -1 ENOENT (No such file or directory)
stat64("./P6object.pir", 0xbf9ee9bc)    = -1 ENOENT (No such file or directory)
stat64("./P6object.pasm", 0xbf9ee9bc)   = -1 ENOENT (No such file or directory)
stat64("./P6object.pbc", 0xbf9ee9bc)    = -1 ENOENT (No such file or directory)
stat64("/usr/local/./P6object.pbc", 0xbf9ee9bc) = -1 ENOENT (No such file or directory)
stat64("/usr/local/./P6object.pir", 0xbf9ee9bc) = -1 ENOENT (No such file or directory)
stat64("/usr/local/./P6object.pasm", 0xbf9ee9bc) = -1 ENOENT (No such file or directory)
stat64("/usr/local/./P6object.pbc", 0xbf9ee9bc) = -1 ENOENT (No such file or directory)
stat64("/usr/local/lib/parrot/2.3.0-devel/library/P6object.pbc", {st_mode=S_IFREG|0644, st_size=18448, ...}) = 0
stat64("/usr/local/lib/parrot/2.3.0-devel/library/P6object.pbc", {st_mode=S_IFREG|0644, st_size=18448, ...}) = 0
stat64("/usr/local/lib/parrot/2.3.0-devel/library/P6object.pbc", {st_mode=S_IFREG|0644, st_size=18448, ...}) = 0
stat64("/usr/local/lib/parrot/2.3.0-devel/library/P6object.pbc", {st_mode=S_IFREG|0644, st_size=18448, ...}) = 0
open("/usr/local/lib/parrot/2.3.0-devel/library/P6object.pbc", O_RDONLY|O_LARGEFILE) = 3

Parrot has taken Regex.pbc in the current directory before even checking for it in the standard libraries. The same behavior occurs with all other Parrot-based programs which use installed libraries. This provides an attack vector against Parrot users:

1. Wait for Perl6-on-Parrot to hit the big time.
2. Distribute a shady tarball containing a malicious P6Regex.pbc inside it.
3. The victim unpacks the tarball and attempts to analyze the contents.
4. The user runs his Perl 6 based editor.
5. Rakudo loads Perl6.pbc from the current directory. My code is now running.

It's probably best to follow Perl 5's example here:

$ perl -V
...
  @INC:
    /usr/local/lib/perl5/site_perl/5.12.0/i686-linux-thread-multi
    /usr/local/lib/perl5/site_perl/5.12.0
    /usr/local/lib/perl5/5.12.0/i686-linux-thread-multi
    /usr/local/lib/perl5/5.12.0
    .

With the current directory at the end, installed programs which use only installed libraries will never be tricked into running code in the current directory. Hopefully it is not too common for installed programs to reference nonexistant libraries.

Originally http://trac.parrot.org/parrot/ticket/1589

[nwellnhof]

The current directory should be completely removed from the search path. Putting the current directory at the end of the path doesn't help if an application tries to load a library that might not exist on every system. Windows made the same mistake:

 http://www.google.com/search?q=windows+dll+hijack+vulnerability

[jkeenan]

Can someone open up a branch and prepare a patch?

[parrot]

There is a design decision about that? And a deprecation notice? The coding job is the minor part of the issue, IMO.

[jkeenan]

Replying to NotFound:

There is a design decision about that? And a deprecation notice? The coding job is the minor part of the issue, IMO.

cotto,

Could you evaluate the design decision (if any) to be made here?

Thanks.

kid51

[parrot]

Design decision: pick one of the three:

1 - Keep the current way: look in current directory before library search paths. 2 - Look in curent directory only if no match found in library search paths. 3 - Look only in the library search paths.

[leto]

I am +1 for #3, i.e. remove . from the default search path. This removes an entire class of security vulnerabilities. As long as we give a code snippet of how HLL authors can add . to the default search path, this shouldn't be a big issue.

[cotto]

Looking in the cwd as a last resort sounds like a sane default. +1.

[leto]

Can we resolve this and make it part of 3.3 ? I think it is important. I am fine with putting . at the end of our search path.

[soh-cah-toa]

I would like to take this ticket but, obviously, there is still no general consensus.

My opinion is that it should be removed all together from the search path. Placing the current working directory at the end of the search path does help protect against Trojans somewhat, but not completely.

[jkeenan]

Replying to soh_cah_toa:

soh_cah_toa,

Thanks for the submission.

But I have to ask: Are there any tests we should write to demonstrate what this patch does and that it does what we want it to?

I would be reluctant to apply it otherwise.

kid51

[soh-cah-toa]

Yeah, cotto mentioned the same thing a few days ago.

Unfortunately, I'm still new to the whole unit testing thing and would need some guidance on that part.

I'll make it a point to talk to you guys about it tomorrow afternoon.

[soh-cah-toa]

2245 byte attachment from soh-cah-toa
at http://trac.parrot.org/parrot/raw-attachment/ticket/1589/tt1589.patch

index 2bb9e13..b3758b9 100644
--- a/src/library.c
+++ b/src/library.c
@@ -155,8 +155,6 @@ parrot_init_library_paths(PARROT_INTERP)
         if (!STRING_IS_NULL(envvar) && !STRING_IS_EMPTY(envvar))
             VTABLE_push_string(interp, paths, envvar);
     }
-    entry = CONST_STRING(interp, "./");
-    VTABLE_push_string(interp, paths, entry);
  
   /\* define library paths */
   paths = Parrot_pmc_new(interp, enum_class_ResizableStringArray);
  @@ -168,15 +166,11 @@ parrot_init_library_paths(PARROT_INTERP)
       if (!STRING_IS_NULL(envvar) && !STRING_IS_EMPTY(envvar))
           VTABLE_push_string(interp, paths, envvar);
   }
-    entry = CONST_STRING(interp, "./");
-    VTABLE_push_string(interp, paths, entry);
  
   /\* define languages paths */
   paths = Parrot_pmc_new(interp, enum_class_ResizableStringArray);
   VTABLE_set_pmc_keyed_int(interp, lib_paths,
           PARROT_LIB_PATH_LANG, paths);
-    entry = CONST_STRING(interp, "./");
-    VTABLE_push_string(interp, paths, entry);
  
   /\* define dynext paths */
   paths = Parrot_pmc_new(interp, enum_class_ResizableStringArray);
  diff --git a/t/library/lib_search_path.t b/t/library/lib_search_path.t
  new file mode 100644
  index 0000000..62c3cca
  --- /dev/null
  +++ b/t/library/lib_search_path.t
  @@ -0,0 +1,43 @@
  +#!./parrot
  +# Copyright (C) 2011 Parrot Foundation.
  +
  +=head1 NAME
  +
  +t/library/lib_search_path.t - testing for proper search path precedence
  +
  +=head1 SYNOPSIS
  +
  +This test program verifies that Parrot searches for libraries in the
  +proper order.
  +
  +=head1 AUTHOR
  +
  +Kevin Polulak (a.k.a. soh_cah_toa) kpolulak@gmail.com
  +
  +=cut
  +
  +.const string TESTS = 2
  +
  +.sub 'main' :main
-    .include 'test_more.pir'
  +
-    .local pmc lib
-    .local pmc lib_cwd
  +
-    plan(TESTS)
  +
-    # Verify current working directory isn't searched
-    lib_cwd = loadlib 'lib_search_path.t'
-    is(lib_cwd, '', 'Verify current working directory not searched')
  +
-    # Verify runtime/parrot/dynext is searched
-   lib = loadlib 'osutils.pir'
-    isnt(lib, '', 'Verify runtime/parrot/dynext is searched')
  +.end
  +
  +# Local Variables:
  +#   mode: pir
  +#   fill-column: 100
  +# End:
  +# vim: expandtab shiftwidth=4 ft=pir:
  +

[soh-cah-toa]

Patch incudes both the changes and tests.

[leto]

Patch applied in  https://github.com/parrot/parrot/tree/tt1589_library_path

Deprecation data, wiki pages and possibly some docs still need to get modified.

[jkeenan]

Replying to dukeleto:

Patch applied in  https://github.com/parrot/parrot/tree/tt1589_library_path Deprecation data, wiki pages and possibly some docs still need to get modified.

dukeleto, soh_cah_toa:

Which of you can take care of those items?

Thank you very much.

kid51

[jkeenan]

Replying to jkeenan:

Replying to dukeleto:

Patch applied in  https://github.com/parrot/parrot/tree/tt1589_library_path Deprecation data, wiki pages and possibly some docs still need to get modified.

dukeleto, soh_cah_toa: Which of you can take care of those items?

Repeat question.