Ticket #1589 (new bug)

Opened 12 years ago

Last modified 11 years ago

Move . to the end of the library search path

Reported by: sorear Owned by: soh_cah_toa
Priority: critical Milestone:
Component: core Version: 2.3.0
Severity: high Keywords:
Cc: cotto, dukeleto, soh_cah_toa Language:
Patch status: applied Platform: all

Description

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.

Attachments

tt1589.patch Download (2.2 KB) - added by soh_cah_toa 11 years ago.
Patch incudes both the changes and tests.

Change History

  Changed 11 years ago by 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

  Changed 11 years ago by jkeenan

Can someone open up a branch and prepare a patch?

follow-up: ↓ 4   Changed 11 years ago by NotFound

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

in reply to: ↑ 3   Changed 11 years ago by jkeenan

  • cc cotto added

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

  Changed 11 years ago by NotFound

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.

  Changed 11 years ago by dukeleto

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.

  Changed 11 years ago by cotto

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

  Changed 11 years ago by dukeleto

  • priority changed from normal to critical
  • platform set to all
  • component changed from none to core

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.

  Changed 11 years ago by 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.

follow-up: ↓ 11   Changed 11 years ago by soh_cah_toa

  • owner set to soh_cah_toa
  • status changed from new to assigned

in reply to: ↑ 10   Changed 11 years ago by 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

  Changed 11 years ago by 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.

Changed 11 years ago by soh_cah_toa

Patch incudes both the changes and tests.

follow-up: ↓ 14   Changed 11 years ago by dukeleto

  • status changed from assigned to new
  • patch set to applied

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.

in reply to: ↑ 13 ; follow-up: ↓ 15   Changed 11 years ago by 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

in reply to: ↑ 14   Changed 11 years ago by jkeenan

  • cc dukeleto, soh_cah_toa added

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.

Note: See TracTickets for help on using tickets.