Bad code from the trenches

This node is intended as a security-consciousness-raising exercise. I recently ran into the following code in a module:

   open FILE, "$dest";
   my @lines = <FILE>;
   close FILE;
[download]

I was really impressed. In three lines, I count 2 major security holes, one Worst Practice, and one very minor carelessness. (Can you see what they are? Would it help if I told you that $dest came straight out of a CGI parameter? UPDATE: It's only one major security hole, actually.)

Security hole: $dest is a CGI param and we are not running under taint mode. A piped open could be disastrous.
Security hole: $dest is in double quotes. If it contains something like @{[ arbitrary Perl code here]} then that Perl code will be executed. (Code interpolation in double quote context.) UPDATE: This is not correct. See the bottom.
Worst Practice: The author does not check to see that the open command succeeded. Neither is there any error handling in the case that @lines is empty (although, in fairness, that would probably come after the quoted snippet).
Very minor carelessness: my @lines = <FILE> reads all the lines in the file into an array. But since $/ (input record separator) is not explicitly set, you don't know for sure what constitutes a "line". This probably isn't worth worrying about but, since I was looking for failure modes in this code, I thought I'd point it out.

Further discussion in case the above doesn't ring bells:

A piped open allows you to open a UNIX pipe to another program and either send data to it or get data from it. (perldoc -f open for full details.) If the command in question were something like "mail the password file to hacker@address.com", that would be bad. The way to resolve this is to run in taint mode (use the -T switch; perldoc perltaint for full details), and to verify that you have good values in your parameters (note: verify that you have good parameters...do not try to weed out bad ones, or you will miss a few).

When you open a file, do it in either of the following ways:

    use IO::File;
    my $file = new IO::File($dest, '<')  
        or die "Oops, couldn't open $dest: $!";
[download]

    open FILE, '<', $dest   
        or die "Oops, couldn't open $dest: $!";
[download]

Either of these specifies precisely what mode (in the above examples, read) you want to open the file in), so any special characters in the filepath are not treated as commands.

Double quote interpolation allows you to stuff arbitrary code--not just scalars and arrays--into a double-quoted string and have it run. For more information, perldoc perlref. The way to make this safe is to not put $dest in quotes; they aren't needed.

UPDATE: I'm wrong about the double quote interpolation problem; as Corion pointed out to me in private message, code is only interpolated once. So these print different things:

    $a = "@{[print hello]}";
    $z = "$a";               # no output
    $z = "@{[print hello]}"; # output: hello
[download]

Comment on Bad code from the trenches Select or Download Code

Replies are listed 'Best First'.
Re: Bad code from the trenches by dragonchild (Archbishop) on Mar 14, 2005 at 14:00 UTC
Another few problems: How big is FILE? Why are you reading it completely into RAM before using it? Fix: Iterate over FILE in a while-loop. Are you sure you reading it completely before someone else modifies it? Why aren't you flock'ing the filehandle? You are running in an environment where the same script could be trying to open the same file more than once and /or another script potentially could be changing the file at the same time. Fix: Use flock. Are you checking for file existence first? What about if the path is a directory? Fix: Use -d and -e (and -f, potentially) Are your pathnames relative to server root? What happens if your user specifies '/etc/passwd' or '/proc/cpuinfo' or some other file they really shouldn't be touching? Fix: Don't allow your user to specify a filename. Make them choose from a list, then verify they actually chose from that list. Alternatives to your fixes above: You correctly specify the open() fixes for Perls greater than 5.005_03 (when 3-arg open was introduced). sysopen() is always a fix for security holes. perlopentut has the details. You're right in that you don't need double-quotes and that does skirt the immediate issue. However, the correct solution is to force your user to pick from a list of acceptable files and make sure they picked from it. (q.v. problems list above.) won't work as expected ... Being right, does not endow the right to be rude; politeness costs nothing. Being unknowing, is not the same as being stupid. Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence. Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.	[reply]
Re^2: Bad code from the trenches by Whitehawke (Pilgrim) on Mar 14, 2005 at 14:27 UTC
First off, I feel the need to be a little defensive and say "This ain't my code!" It was in a module written by the head developer of my current client. Ok, with that out of the way.... Your suggestion about picking from a list of acceptable files is a good one, but wouldn't work in this case (because of information that was not given in the original post). This code was used in a CGI environment and $dest was derived from a CGI param, but this wasn't code that was directly user-interactive, it was called from a PHP script. As to testing files with -e, -f, and -d before opening them...remember that doing this: `line 1) if ( -e $file && -f $file && -s $file < $ok ) { line 2) ...open file here.... }` [download] means that you have a race condition. If someone modifies or deletes $file in the moment between lines 1 and 2, the tests were all for nothing. A better solution is to do something like this (almost directly lifted from perlopentut): `use 5.004; use Fcntl qw(:DEFAULT :flock); open(FH, '<', $file) or die "can't open $file: $!"; unless (flock(FH, LOCK_SH \| LOCK_NB)) { local $\| = 1; print "Waiting for lock..."; flock(FH, LOCK_SH) or die "can't lock $file: $!"; print "got it.\n" } # now read from FH` [download]	[reply] [d/l] [select]
Re^3: Bad code from the trenches by dragonchild (Archbishop) on Mar 14, 2005 at 14:36 UTC
The problem is that not all OSes allow you to treat a directory as a normal file. Unix-like systems (including OS-X, but not OS-9) do, but Windows probably won't and VMS definitely won't (AFAIK). What happens then? As for the code being yours or not ... don't feel defensive. If it's not yours, you don't have anything to worry about. If it is yours, you've taken the right step(s) to fix your error(s) and learn better programming techniques. Either way, you have no fault. As for the entire pick-a-file thing ... there should still be a list of permissible files being presented within the PHP. At that point, both the Perl and the PHP need to agree on what this list of files is so that the PHP can present the list and the Perl can validate against it. Just because you have more than one code file doesn't mean the solution is wrong. Being right, does not endow the right to be rude; politeness costs nothing. Being unknowing, is not the same as being stupid. Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence. Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.	[reply]
Re^4: Bad code from the trenches by Whitehawke (Pilgrim) on Mar 14, 2005 at 20:23 UTC
Re^2: Bad code from the trenches by itub (Priest) on Mar 17, 2005 at 17:49 UTC
How big is FILE? Why are you reading it completely into RAM before using it? Fix: Iterate over FILE in a while-loop. And that only works if the file has reasonably long lines. You are still in trouble if they give you '/dev/zero' as the filename. Even if the files can not be picked from a list, at least the program should ensure that they are located in a "reasonable" directory.	[reply]
Re^3: Bad code from the trenches by dragonchild (Archbishop) on Mar 18, 2005 at 13:57 UTC
Define "reasonable" in a platform-independent way. Or, is this something that should be included in File::Spec? Who gets to make that list? In other words, /dev is perfectly reasonable on Win32 and /Windows is perfectly reasonable on Linux. Now what? Being right, does not endow the right to be rude; politeness costs nothing. Being unknowing, is not the same as being stupid. Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence. Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.	[reply]
Re^4: Bad code from the trenches by itub (Priest) on Mar 18, 2005 at 20:37 UTC
Re: Bad code from the trenches by ambrus (Abbot) on Mar 14, 2005 at 13:48 UTC
Security hole: $dest is in double quotes. If it contains something like `@{[ arbitrary Perl code here]}` then that Perl code will be executed. (Code interpolation in double quote context.) This one is clearly false. The other one (2-arg open) is indeed a security bug. (Update: formatting)	[reply] [d/l]
Re^2: Bad code from the trenches by dragonchild (Archbishop) on Mar 14, 2005 at 13:52 UTC
Oh? So, what does the following do? `my $y = "Boo!"; my $x = "@{[print $y, $/]}";` [download] Being right, does not endow the right to be rude; politeness costs nothing. Being unknowing, is not the same as being stupid. Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence. Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.	[reply] [d/l]
Re^3: Bad code from the trenches by Corion (Patriarch) on Mar 14, 2005 at 13:54 UTC
That's not the example. The example is: `my $y = 'Boo!'; my $x = '@{[print $y, $/]}'; # note the single quotes here my $z = "$x"; print $z;` [download]	[reply] [d/l]
Re^4: Bad code from the trenches by dragonchild (Archbishop) on Mar 14, 2005 at 14:03 UTC
Re^5: Bad code from the trenches by Joost (Canon) on Mar 14, 2005 at 14:08 UTC
Some notes below your chosen depth have not been shown here
Re: Bad code from the trenches by perrin (Chancellor) on Mar 14, 2005 at 18:45 UTC
If you're going to worry about the possiblity of someone leaving $/ in a bizarre state, you'll have to worry about dozens of other global perl settings too. Better to just make a rule: don't change global settings unless you have to, and even then do it with `local()`.	[reply] [d/l]
Re^2: Bad code from the trenches by Whitehawke (Pilgrim) on Mar 14, 2005 at 20:20 UTC
True. That's why I said it was a trivial thing and probably not worth worrying about.	[reply]
Re: Bad code from the trenches by demerphq (Chancellor) on Mar 14, 2005 at 18:12 UTC
Its much more fun to say `my @lines=do { local @ARGV=($dest); <>};` [download] ;-) Heh, ambrus caught my stupid, on something so simple too... Fixed. --- demerphq	[reply] [d/l]