Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl: the Markov chain saw
 
PerlMonks  

RFC: Perl-Critic policy: ProhibitInlineSystemArgs

by davidrw (Prior)
on Jul 01, 2006 at 00:39 UTC ( [id://558707]=perlmeditation: print w/replies, xml ) Need Help??

This was inspired by Re: Ideas Wanted for Perl::Critic Security Policies -- i decided to take a crack at making a Perl::Critic policy to
warn about system or exec calls that pass arguments inside the first parameter (i.e. system("$command $arg1 $arg2") instead of system($command, $arg1, $arg2)).

So here it is, in all it's glory .. I am a PPI novice, so comments/suggestions are very welcome. (Note: this was made by using ProhibitTwoArgOpen and ProhibitInterpolationOfLiterals as examples)
package Perl::Critic::Policy::DRW::ProhibitInlineSystemArgs; use strict; use warnings; use Perl::Critic::Utils; use Perl::Critic::Violation; use base 'Perl::Critic::Policy'; our $VERSION = '0.01'; $VERSION = eval $VERSION; ## no critic #--------------------------------------------------------------------- +----- my $desc = q{Inline-args in 'system' or 'exec' used}; my $expl = []; #--------------------------------------------------------------------- +----- sub default_severity { return $SEVERITY_HIGHEST } sub applies_to { return 'PPI::Token::Word' } #--------------------------------------------------------------------- +----- sub violates { my ($self, $elem, $doc) = @_; return if !($elem eq 'system' || $elem eq 'exec'); return if is_method_call($elem); return if is_hash_key($elem); my @args = parse_arg_list($elem); return unless @args && @{$args[0]}; my $firstArg = $args[0]->[0]; return unless defined $firstArg && ( $firstArg->isa('PPI::Token::Quote::Double') || $firstArg->isa('PPI::Token::Quote::Interpolate') ); if( $firstArg =~ m{ (?<!\\) [\ ] \S+ }mx ){ # has unescaped space my $sev = $self->get_severity(); return Perl::Critic::Violation->new( $desc, $expl, $elem, $sev + ); } return; #ok! } 1;
(trivial) code to test against (note it doesn't catch the join one):
system("c:\\program\ files\\foo.exe"); system( join " ", qw/asd wqe/ ); system("wc /etc/hosts"); system("whoami"); exec("wc /etc/hosts"); exec("whoami");
Another problem case (or at least not covered by the above attempt) would be $cmd = "$prog $arg1 $arg2"; system($cmd);

Replies are listed 'Best First'.
Re: RFC: Perl-Critic policy: ProhibitInlineSystemArgs
by Jenda (Abbot) on Jul 02, 2006 at 03:02 UTC

    This is probably not the right place for this comment, but I just scanned the policies of the perlcritic and I have to say I do violently disagree with quite a few.

    • ProhibitLvalueSubstr? Well, maybe.
    • ProhibitSleepViaSelect? Agreed.
    • ProhibitStringyEval? The summary is incorrect, but yeah, string eval is to be used with care.
    • ProhibitUniversalCan? Well well well ... the $foo->can(...) is easier to read, but you may have to check first that the foo is actually a blessed refererence. Otherwise instead of a false value you get an exception/error.
    • ProhibitUniversalIsa? Same thing.
    • RequireBlockGrep? Why???
    • RequireBlockMap? Double why???
    • RequireGlobFunction? Why not $FH->readline() and $res = regexp_replace( '\d+', 'number', 'g')? What about Assign( $x, Plus( 5, Times( $y, 7)))?
    • RequireSimpleSortBlock? Yeah, anything more complex is too hard for the reader. Use the scalar comma instead of the semicolon if you need to do a bit more.
    • ProhibitExplicitISA? If you insist ...
    • ProhibitOneArgBless? Agreed.
    • ProhibitHardTabs? Hey, it's my business what do I use to format my code, as long as I'm consistent. Why would I type (or have my editor generate) four (or whatever's your favourite number) spaces when one tab is enough? You don't mix'em!
    • ProhibitParensWithBuiltins? Severity 1? OK. I don't think it matters at all but ...
    • ProhibitQuotedWordLists? Yeah and confuse newbies and people that have Perl as their tenth and seldom used language just to save a few characters. Why would anyone care to check for this?
    • RequireTrailingCommas? OK, it's better to put it there in case you need to move things around, but ... is this worth testing?
    • ProhibitCStyleForLoops? I think you meant "use for my $i (1..20) instead of for($i=0; $i<=20; $i++)" right? Am I supposed to use for my $i (map {$_ * 2} (0..10)) instead of for($i=0; $i<=20; $i+=2) as well? I'd agree that this for(my $i=0; $i <= $#arr; $i++) is a red flag and most likely a for my $item (@arr) would be preferable, but to outright "ban" the C style for loop?
    • ProhibitCascadingIfElse? Well, sometimes there's no other way dude!
    • ProhibitPostfixControls? Why don't we program in VBScript then? It's simple and totally dumbed down. You might like it. Postfix controls, be it "if", "for" or "while" often lead to code that more clearly captures the thought flow of the author, that is easier to read and stresses the important part of the sentence ... I mean ... statement.
    • ProhibitUnlessBlocks? Kinda agreed.
    • ProhibitUnreachableCode? Well, agreed. Unreachable code is quite common while debugging, but in production code it's a red flag. You either forgot to delete something or remove some debug stuff or simply misunderstood something
    • ProhibitUntilBlocks? Perl has until()? I've never used it or seen it
    • RequirePodAtEnd? Other people say, put the documentation next to the things being documented. I think they are right. Doesn't seem important to me.
    • RequirePodSections? Agreed.
    • ProhibitBacktickOperators? Why use something simple if you can use for example IPC::Open3, right?
    • ProhibitBarewordFileHandles? Agreed.
    • ProhibitOneArgSelect? OK
    • ProhibitReadlineInForLoop? Agreed
    • ProhibitTwoArgOpen? Agreed. Though q{<} looks horrible to me. See bellow.
    • RequireBracedFileHandleWithPrint? Show me someone who does this!
    • ProhibitFormats? I never tried them. They probably do have their place though. And I don't think the people that do use them need their hands held, they tend to know what they are doing.
    • ProhibitTies? Ban the magic! She's a witch, she's a witch, burn her! (just came from Silent Hill) I don't see a reason to ban something that lets me use all the syntactic sugar Perl has for arrays and hashes with objects. If it helps me to write less and easier to read code. Would anyone want to have to rewrite the whole script once you find your hash doesn't fit in the memory?
    • RequireRcsKeywords? Some people may want that, maybe.
    • ProhibitAutomaticExportation? OK
    • ProhibitEvilModules? OK
    • ProhibitMultiplePackages? I wonder what proportion of well know and widely used CPAN modules would fail this. If I need a tiny little internal helper class I don't see why would I want to put it into a separate file.
    • RequireBarewordIncludes? OK
    • RequireEndWithOne? We can't have some fun, can we?
    • RequireExplicitPackage? Agreed
    • RequireVersionVar? Agreed
    • ProhibitAmbiguousNames? $thisIsTheLastValueIveSeenInThisFourLinesLongLoopUsingTheDefaultVariable
    • ProhibitMixedCaseSubs? I think there should be a ProhibitUnderscoredSubs available as well, I don't think we can all agree on this
    • ProhibitMixedCaseVars? Same as above
    • ProhibitDoubleSigils? I'd only prohibit double sigils if there is a [ or { following the variable name, I would not want to have to write push @{$arr_ref}, $foo. OTOH, @$foo[1,2,5] would left me wondering whether it really means what I think it does for a moment.
    • RequireExtendedFormatting? I find the /x-ed regexps harder to read most of the time. Yeah if it's something long and complex then /x migth help, but ...
    • RequireLineBoundaryMatching? Huh?
    • ProhibitAmpersandSigils? Fine with me
    • ProhibitBuiltinHomonyms? Agreed completely
    • ProhibitExcessComplexity? OK
    • ProhibitExplicitReturnUndef? Sure
    • ProhibitSubroutinePrototypes? Agreed. most of the time
    • ProtectPrivateSubs? OK
    • RequireFinalReturn? Well ... if the subroutine is longer than one or two lines.
    • ProhibitNoStrict and ProhibitNoWarnings? You might want to warn about this, but I'd think that if you do use strict/warnings and just turn them off for a moment then you probably know what you are doing.
    • RequireUseStrict and RequireUseWarnings? Sure
    • ProhibitConstantPragma? I know how to handle my constants thank you very much. Readonly scalars are not inlined so the optimizer has no way to make use of the fact they are constant. Might mean a lot for thing like print STDERR "..." if DEBUG; in a tight loop.
    • ProhibitEmptyQuotes? What? Beg your pardon?? Why the heck would I want to use q{} in place of ''??? And severity 2????
    • ProhibitEscapedCharacters? Well... does anyone know where do I find the table of those \N{whatever} things??? OTOH everyone knows what \x0D\x0A means. Besides I bet quite often there either is no name or you really want this specific byte, not whatever your OS thinks is \N{foo} at the moment.
    • ProhibitInterpolationOfLiterals? OK. Though the only speed difference is during compilation AFAICT.
    • ProhibitLeadingZeros? OK
    • ProhibitMixedBooleanOperators? Agreed. though I wonder whether the policy really prohibits just the mixed use.
    • ProhibitNoisyQuotes? What? Beg your pardon?? Use q{} to make them even noisier? q{,} instead of ','??? I mean, if I need to include quotes in the string, I'm all for switching to q{} or qq{} or even heredocs, but requiring the use of q{} for anything that's not purely alphanumeric?? What's noisy about comma?
    • ProhibitVersionStrings? OK
    • RequireInterpolationOfMetachars? I don't think I understand this one.
    • RequireNumberSeparators? Don't care.
    • RequireQuotedHeredocTerminator? Agreed
    • RequireUpperCaseHeredocTerminator? Why would I want to restrict the terminator if I need to be sure it will not be present in the literal?
    • ProhibitConditionalDeclarations? Right, this is not well defined so let's not do this.
    • ProhibitLocalVars? Agreed
    • ProhibitMatchVars? Agreed
    • ProhibitPackageVars? Sometimes package vars are the right thing
    • ProhibitPunctuationVars? And send most people searching the source code where did the $EVAL_ERROR variable came from. What's the "ENGLISH" for $!, $/ and $; ? Did you know what it was without consulting the docs?
    • ProtectPrivateVars? OK
    • RequireInitializationForLocalVars? my $data = do {local $/ = undef; <$FH>}? Well, if you insist.

    It looks like I do not feel so strong about most as I thought at first, looks like the first several I looked at were the (IMHO) craziest, but anyway ... there are some rules I would rather people not follow. :-(

      For most of the Perl::Critic policies, you can find a complete explanation in Conway's book "Perl Best Practices". If you're still not convinced, then you can always disable them. And if you stongly believe that a particular Policy should not be followed, then you're welcome to write a contra-policy (for example: ProhibitUnderscoreSubs). Perl::Critic is all about encouraging consistency. You can make whatever rules you like -- Perl::Critic is just there to help you and your team stick to those rules.

      -Jeff

      The real problem here is that it is very easy to create new laws, it is just very hard to create good ones, and almost impossible to repeal bad ones.

      Things like PerlCritic and PBP have a habit of becoming defacto-standards, and like voting for political parties, you either vote for or against. You cannot vote for tax breaks and against cuts in health care, you gotta take the package. And for every good suggestion or two or three they contain, there also lurks a bad one. Or one that should only be applied under specific circumstances, or one that should only be applied until the programmer can justify not applying it on the basis of his/her knowledge of the underlying reasoning; or the nature of their code; or the naturee of the usage of their code--but defacto-standards do not get mandated by programmers with such knowledge.

      Defacto-standards get applied as blanket requirements by PHBs, weak coders who want someone else to make their decisions and obsessive-compulsive paranoid pedants that once bitten through lack of knowledge or care, become twice shy about using their own faculties.


      Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
      Lingua non convalesco, consenesco et abolesco. -- Rule 1 has a caveat! -- Who broke the cabal?
      "Science is about questioning the status quo. Questioning authority".
      In the absence of evidence, opinion is indistinguishable from prejudice.
        Defacto-standards get applied as blanket requirements by PHBs, weak coders who want someone else to make their decisions and obsessive-compulsive paranoid pedants that once bitten through lack of knowledge or care, become twice shy about using their own faculties.

        All true.

        But what about folk like me who find tools like Perl::Critic useful aids in approaching lumps of bad legacy code, who are (hopefully) bright enough to realise that a code smell is just a smell and not necessarily evil in all cases. Who can read through PBP and agree with some things and disagree with others.

        Are we supposed to throw away useful tools like Perl::Critic because some idiots misuse them?

        In this particular instance it's been my experience - which obviously differs from yours - that people are always throwing unclean user input into single-string calls to system. I'd love a tool that helps me find these instances more quickly.

        The real problem here is that it is very easy to create new laws, it is just very hard to create good ones, and almost impossible to repeal bad ones.

        I think you are stretching this analogy to government farther than it will go. Many of the Perl::Critic rules are very useful and it's absolutely trivial to not use ones you don't like.

        You cannot vote for tax breaks and against cuts in health care, you gotta take the package.

        At YAPC::NA in Chicago last week, nearly every presenter who mentioned PBP said "except for a, b, and c, which I think are lame." The Perl::Critic presentation emphasized how to choose which tests to use and turn it off for sections where you need to. People are not having any trouble rejecting parts that they don't like.

      Jenda,

      Please don't hold back, tell us what you really think! :) OK, enough with giving you are hard time...

      What I have found for Perl::Critic is that it is a good developer testing module. It seems you have some strong opinions about numerous rules for Perl::Critic and that is OK. However, Perl::Critic addresses two major concerns that I consider more important than the actual rules

      1. Personal preference: It is easy enough to disable the rules you don't like.
      2. Consistency: If there are two programmers working together, you would like them to be able to have some sort of consistency in the work between them.

      Also since Perl::Critic is a developer testing module rather than a required distribution module, there is no reason that it should be an issue when writing your code.

      It is a tool that should make you happier and help, not be an opinion that is forced on you.

Re: RFC: Perl-Critic policy: ProhibitInlineSystemArgs
by BrowserUk (Patriarch) on Jul 01, 2006 at 01:02 UTC

    Why? Please explain your reasoning?


    Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
    Lingua non convalesco, consenesco et abolesco. -- Rule 1 has a caveat! -- Who broke the cabal?
    "Science is about questioning the status quo. Questioning authority".
    In the absence of evidence, opinion is indistinguishable from prejudice.
      Why? Please explain your reasoning?

      Are you asking why to avoid passing arguments in the single-arg form of system or exec? It can be dangerous if passed unclean user input. There is a particularly relevant paragraph in the exec documentation (emphasis mine):

      If there is more than one argument in LIST, or if LIST is an array with more than one value, calls execvp(3) with the arguments in LIST. If there is only one scalar argument or an array with one element in it, the argument is checked for shell metacharacters, and if there are any, the entire argument is passed to the system's command shell for parsing (this is /bin/sh -c on Unix platforms, but varies on other platforms). If there are no shell metacharacters in the argument, it is split into words and passed directly to execvp, which is more efficient.

      Notice that in the case of a single argument with meta-characters, the system shell is called. The system shell will then interpret those metacharacters in the standard way, which can cause dangerous side effects. Consider a malicious user that passes "> $0" as part of the input. If this were executed in a unix shell with enough permissions, it could cause some major damage.

      Of course it's always wise to validate user input, but the safest course of action is to not use the user input in a way that would be dangerous in the first place. In this case, the list forms of system and exec prevent the danger, because the shell is never invoked.

      Note: I apologize in advance if I'm not answering the question you asked, but perhaps someone else will find the information useful if that is the case.

      Update: apparently my efforts to avoid offending the ever-sensitive BrowserUk were in vain. I shall make a note to not even try in the future.

        Well. Ignoring that you are someone else, explaining someone elses reasoning, and indeed, the reasoning you explain is mostly not yours or the OP's; it will suffice to demonstrate that the OP's PerlCritic code does not even attempt to encapsulate the reasoning you've explained.

        The difference between the rule coded and the salient part of your explaination--that is somewhat diluted in your explanation through lack of emphasis and entirely omitted from your quoted reference--is your second sentance:

        It can be dangerous if passed unclean user input.

        Even in that simple prerequisite there are two conditional parts that are entirely omitted from the rule as coded:

        1. can be
        2. unclean user input

        Most Perl scripts are invoked by a user from a shell command line. The user has no need to deviously pass contaminated arguments into Perl scripts in order to have those scripts pass them on to the shell as they can do it directly.

        Most system & exec invokations within Perl scripts do not pass unclean user input as a part of their arguments.

        Omitting checks in the rule to allow scripts the benefits of reusing the facilities provided by system shells, where doing so does not conflict with security, is like passing a law that says 'noone is allowed to cross a road'.

        It can be dangerous to cross a road without looking; but if the road is empty, or if the road is a private road currently closed to vehicular traffic; or if you are of sound mind, with reasonable eyesight, and no mobility impediments and you look first; it's probably okay to cross roads.

        Imagine life if a law was imposed upon everyone that they could never cross roads because doing so can sometimes be dangerous if insufficient care is taken when doing so?

        Of course, it would be quite difficult to encapsulate the logic required to verify whether a particular use of the single argument system or exec was likely to fall foul of the particular set of circumstances that would render that usage unsafe--but flagging all uses as unsafe because it is easier than performing the checks is not an acceptable alternative.

        This is the metaphoric equivalent of imposing a blanket ban on eating out because some chefs are insufficiently aware of the rules for food preparation and might therefore give their client salmonella.


        Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
        Lingua non convalesco, consenesco et abolesco. -- Rule 1 has a caveat! -- Who broke the cabal?
        "Science is about questioning the status quo. Questioning authority".
        In the absence of evidence, opinion is indistinguishable from prejudice.

      I can tell you why I suggested that capacity in the referenced node: very simply, it's way too easy to mess up (usually by a failure to anticipate a problem) when using the single-argument form.

      When using a single-argument form of system or exec (i.e. system("$command $arg1 $arg2")), it's up to the programmer to escape those arguments properly for the shell. Which they might choose to do non-portably. The multiple-argument forms (i.e. exec($command, $arg1, $arg2, @other_args)) , though, don't have that issue: the shell escaping/quoting is handled under the covers.

      Fewer bugs creep into code, and fewer portability problems arise, with the multiple-argument form. There's also very few instances where the muliple-argument form is any kind of hinderance. That's reason enough, IMO, to consider that form a best practice (even if it's a "low-severity" one when broken).

      If that's not enough, though, there are some minor security issues. Tainted data being passed as arguments will be quoted for the shell (they won't be in the single-arg form), so there isn't a "Shell Code Injection" possibility. Maybe that doesn't come up most of the time, but considering the number of CGI and GUI applications I've seen that interpolate tainted (or very poorly untainted) user data into system calls, it's worth thinking about.

      Essentially, this is the same argument as using the 3-arg form of open, and using prototypes for SQL statements with DBI. There are times when it's perfectly safe to use 2-arg open and to interpolate variables in SQL statements (esp. with proper untainting). But, 3-arg open and prototypes are best practices because it's just as easy to do it the preferred way, and it helps prevent common mistakes and problems. Same thing here.

      I'll be the first to stand up and say that this isn't something Perl::Critic should gripe about by default, but it would be really nice to have it complain at lower severity levels.

      <radiant.matrix>
      A collection of thoughts and links from the minds of geeks
      The Code that can be seen is not the true Code
      I haven't found a problem yet that can't be solved by a well-placed trebuchet
        The multiple-argument forms (i.e. exec($command, $arg1, $arg2, @other_args)) , though, don't have that issue: the shell escaping/quoting is handled under the covers.

        Yes, the multiple-argument forms don't have that problem but it's not because shell quoting is handled in any way, but because there isn't any shell involved at all, so no quoting needed. Parameters to exec are forwarded directly to the underlying kernel, who uses them without further modification.

        --
        David Serrano

        ... the shell escaping/quoting is handled under the covers.

        Fewer bugs creep into code, and fewer portability problems arise, with the multiple-argument form.

        Complete red herring.

        There's also very few instances where the muliple-argument form is any kind of hinderance.

        Also not so


        Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
        Lingua non convalesco, consenesco et abolesco. -- Rule 1 has a caveat! -- Who broke the cabal?
        "Science is about questioning the status quo. Questioning authority".
        In the absence of evidence, opinion is indistinguishable from prejudice.

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlmeditation [id://558707]
Approved by planetscape
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others romping around the Monastery: (2)
As of 2024-03-19 05:57 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found