current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I'm looking for security feedback on the following fully functional code.

The code is trying to safely use the Unix 'file' command to give details about the file.

A hard link is used to create a safe filename that can be used in a command line.

The paramref is a hash read from config, and considered safe. file_base_dir is a directory with only alphanumerics in the name. The 'file' command is the standard one.

What ways could my code be exploited?

sub file_details {
    my ( $self, %arg ) = @_;
    my $filename = $arg{'filename'};
    my $paramref = $arg{'paramref'};
    my $file_cmd = '/usr/bin/file';

    my $safe_filename = $$paramref{'file_base_dir'} . "/link_to_unsafe_file_file_type_build";

    # Just in case the link exists.
    unlink($safe_filename);
    if ( not link( $filename, $safe_filename ) ) {
        confess "Failed to create link $safe_filename to $filename because $!";
    }

    my $file_type = `$file_cmd '$safe_filename'`;

    # remove the link
    unlink($safe_filename);
    $file_type =~ s/\A[^:]+://x;
    chomp $file_type;

    return $file_type;
} ## end sub file_details
share|improve this question

3 Answers 3

up vote 2 down vote accepted

Interesting approach with link. If the only vulnerable parameter is filename, this technique protects from malicious input, because the filename parameter will not be part of the `...` bit executing shell commands. I don't think it's possible to trick link into command execution.

On the other hand, although you say that "file_base_dir is a directory with only alphanumerics in the name", this function doesn't ensure that, simply trusts it to be true. It would be good to add a call in this function to another function that validates the directory parameter.

I tried to think of other ways to validate the filename parameter, without creating a link. You could forbid certain characters such as ; and $ to prevent command injection, but I'm not sure that's an exhaustive list. So while the link trick seems hackish, it does give comfort that vulnerable parameters never participate in a `...` shell expansion.

share|improve this answer
    
Also backticks might be able to be used to exploit the shell. Yes link is hackish, but it seemed to give good protection. –  nslntmnx Feb 21 at 15:04
up vote 0 down vote

I'd probably just ensure there are no .. in the path name and remove the whole symbolic link stuff. I'm not sure how that should improve security. The single quotes alone should make the parameter static.

share|improve this answer
1  
The hard link stuff ensures a clean filename on the command line. Imagine damage to the system if the insecure passed in filename was '; rm -fr / ' - including the single quotes. –  nslntmnx Feb 20 at 10:17
    
@nslntmnx That's the magic behind quotes. echo "$param" will expand the environment variable, while echo '$param' will just print $param without expanding anything - no matter what's inside the quotes. All you have to do is to ensure that your file name doesn't include any unescaped ' character, but that's something you'll have to check either way. In your case the unsafe filename might include .. to escape your base directory and the hard/symbolic link won't help you to avoid that. –  Mario Feb 20 at 15:25
    
Regarding escaping with .., I just ensure there are no directory separators / in the $filename, then no number of .. will allow escape. –  nslntmnx Feb 21 at 15:00
up vote 1 down vote

Safely quoting a filename is as simple as replacing all single quotes with '\''.

  • Close single quoted string.
  • Escaped single quote.
  • Open new single quoted string.

That prevents the shell from interpreting any characters from the filename itself and preserves the original filename.

An additional alternative would be to hand file /dev/stdin or - as argument and then manually pipe the file to the processes standard input.

share|improve this answer
    
Yes, piping standard in does work. I got it to work that way as well. I used pipes, fork and exec. –  nslntmnx Feb 23 at 22:50

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.