in reply to Upload Security (strip ../, etc.)

i've always used something like this for cheking cgi input. This is something that has mutated for a few years from code that my old coworker wrote: 
sub cgi_read_post

{
   local($buffer,$p,$n,$v);

   read(STDIN,$buffer,$ENV{'CONTENT_LENGTH'});
   foreach $p (split(/&/,$buffer))
   {
      ($n,$v)=split(/=/,$p);
      $v=~s/\+/ /g;
      $n=~s/\+/ /g;
      $v=~s/%([a-fA-F0-9][a-fA-F0-9])/pack("C",hex($1))/eg;
      $n=~s/%([a-fA-F0-9][a-fA-F0-9])/pack("C",hex($1))/eg;
      $v=~s/\cM+|\s+/ /g;
      $v=~s/^\s*|\s*$|\|//g;
      $in{$n}=($in{$n} eq ""?$v:"$in{$n}|$v");
   }
}

[download]
BlueLines

Replies are listed 'Best First'.
Yet Another Cargo Cult non-use of CGI.pm
by merlyn (Sage) on Jul 31, 2000 at 23:17 UTC
    Why, oh why, do people insist on cargo cult code?

    This code fails in the same ways that this code always fails, and then we get to repeat the same failure modes. For example:

    • You fail to verify that it was POST vs GET.
    • You fail to validate the existence or sanity of CONTENT_LENGTH.
    • You break multiple-select fields.
    Please stop with the cargo cult programming. use CGI.pm. It's there. It does the job.

    -- Randal L. Schwartz, Perl hacker

    UPDATE: OK, I don't understand these downvotes. I'm passing along information that is accurate, and designed to prevent security violations, and to make the code more maintainable.

    Are the anonymous cowards that are downvoting me doing it because it's technically incorrect, because security isn't that important, or something else entirely?

    Or would people prefer nicey-nice "blind leading the blind" like we get in alt.perl? Because that's what'll happen if you keep disrespecting some of us that have been around the block a few times.

    I'd gladly be willing to be called wrong on anything I post. If you think you have to protect someone else with a thin skin, you're damaging both yourself and them.

    {sigh} Why do I bother?

[reply]
      Are the anonymous cowards that are downvoting me doing it because it's technically incorrect, because security isn't that important, or something else entirely?

      I hesitated quite a while about voting this node, so I didn't. But this update... sigh. That almost pushes it to -- land.

      merlyn, you are a very, very smart guy. I like your books, I like your programming style. I admire your abilities with perl. But damnit, what is with the attitude? I don't think the votes were about the technical accuracy of your post - you were right, and most of us here know it. I feel pretty confident that if it were possible to check - something that SHOULD NOT be possible, by the way - you would find that they were all about attitude. Even those who may have voted it down because you're merlyn; well, why is that? Because you're often providing technically accurate answers? No, there are plenty who do that without getting hammered. So there must be some other reason.

      Or would people prefer nicey-nice "blind leading the blind" like we get in alt.perl? Because that's what'll happen if you keep disrespecting some of us that have been around the block a few times.

      To be blunt, when you start respecting others, you'll get some in return. BlueLines wrote code that you disagree with. Gee. I'm sorry. But instead of pointing to it and screaming "Cargo Cult! Cargo Cult!" why not give people the benefit of your superior experience and wisdom and say something like

      BlueLines, this code will work, generally speaking, but CGI.pm will do a much better job. It will do the same job in a simpler fashion, and it avoids the following problems:
      And then list the same points you did above? It's the same information. It's just as accurate; more so, since you didn't allow for the possibility that this code might work, if in a suboptimal way. And it doesn't take any longer to type; if anything, less, because you won't have to create pretty blue "UPDATE:" boxes to try to figure out where the problem is.

      I'd gladly be willing to be called wrong on anything I post. If you think you have to protect someone else with a thin skin, you're damaging both yourself and them.

      I'm not trying to protect BlueLines. I'm trying to protect the PerlMonks community specifically, and the Perl community in general. You can't code all the Perl in the world yourself. Which means others have to learn how. I prefer they learn how to code properly while also learning that their Elders in the community respect them.

      Or at least, that not ALL of their Elders disrespect them.

      (sigh) Why do I bother?

      I wonder the same thing. But then I remember; because if I don't, then poor manners and disrespect win. And I see too much of that now. If you don't want to put up with posts like this one, or with --, then you have two options. Start treating people with respect, or shut up and code. Your choice. I'm sorry if that seems disrespectful or harsh, but merlyn, that's how it is. We aren't going to "respect" you fully until you learn that other people matter, too. If that's disrespect in your book, then feel free to vote this node --.

      - email Ozymandias
[reply]
      I'm gonna take one shot at this, and that's it.

      Like I said in the chatterbox, "It's not what you say, it's how you say it". The first thing you do is make the person feel like an idiot: "Why, oh why, do people insist on cargo cult code?". Not everyone is a seasoned CGI/Perl programmer. There are a lot of issues involved with proper argument processing, security, compatibility, etc. It's not easy to recognize this when you're first starting out. There's so much to learn that you can't start out knowing it all. And worse yet, you can have something that will work, but be insecure. Once it starts working, you move on. You don't see the test case, or even *know* the test case that will break it.

      CGI programming is glorious, it's intruiging, it's attractive because lots of people may get to see your results. So unlike writing backends for converting dates from "2000-02-02" to "Feb 2nd, 2000", it has sex appeal. As a result, a lot of people take it on, not realizing the true underlying complexity. I can certainly say that about myself. I know there's plenty of things that I should be aware of in regards to ultimately great CGI programming. But I've also got to figure out to write HTML that's compliant across browers, DB interfaces, managing Apache, installing mod_perl, yada yada yada. So much "knowledge space", it's impractical to learn it all before you write your first lines of code.

      Sure, maybe you've been doing it for years. Sure, maybe it's all second nature to you. Sure, maybe you have a personal T3, so drudging through CPAN isn't painful. Sure, maybe you have an editic memory, and remember everything you read, instantly weeding out the implicitly wrong information from what's right. I don't have any of those. And I doubt most people do.

      You're a very smart person, Randal. No one denies that. People want to benefit from your knowledge. I'm convinced I could. But I REFUSE to listen to people who talk down to me. Both in Perlmonks, and in Real Life. I have no patience for that, whatsoever. Nor should anyone else. As an instructor and as a Perlmonks saint, you can't do that to people. Your answers are terse, and unsuitable for the inexperienced person. Which constitutes every person asking a question. If they didn't ask the question, then they must know the answer, and therefore be experienced. While a puppy can be trained by swatting him when he goes on the carpet, you CANNOT teach a programmer with the same technique. You'll insult their intelligence, and only cause them to ignore you (and, evidently, vote -- on any posts, regardless of merit).

      Instead, you have to point them (nicely) towards sources that answer their needs. Explain the *why* in the flaw. Your followup post below is MUCH more in line with how people should be educated. That's the type of response people are looking for. Not being smacked over the head with "Cargo Cult Programmer Alert!".

      I respect your abilities, and I don't want to see you leave PM on the basis of people mistreating you. But until you portray the role of the instructor, and not the role of "the man with the +7 FONT tag", things aren't going to get much better.

      I'll be more than happy to clarify on this if there's an issue. This post is NOT meant to be a "bash merlyn" post. Please understand that. Instead, it's meant to be a direct answer to the big green box.

      Any place with 2700+ registered users and 600+ active users is going to have politics. I understand that. But I don't want to see PM turn into a SlashDot type of environment. I like to think that I generally try to avoid that (not perfectly, mind you, but in general). And I'd like to see politics kept between the persons involved, privately and discreetly, rather than publically aired.

      --Chris

      e-mail jcwren
[reply]

      Actually, this in one of the few hand-rolled CGI parameter parsers I've seen which doesn't break multi-select fields (well, I suppose it might do if the values contained '|' characters - but it's still better than most!)

      Your other points are spot on tho.

      --
      <http://www.dave.org.uk>

      European Perl Conference - Sept 22/24 2000, ICA, London
      <http://www.yapc.org/Europe/>
[reply]
        Yeah, it breaks it with respect to vertical bars. My worry is that someone will copy that code without understanding, and either remove that line, or not understand the meaning of the vertical bar. (And splitting on vertical bars seems to be the one thing that beginners seem to constantly get wrong on the first few tries.)

        CGI::param gets it right. In a scalar context, you get back the first one seen. In a list context, you get them all. No confusion for either kind of user.

        -- Randal L. Schwartz, Perl hacker

[reply]
      whoops. i don't know what to say. i hereby swear to tatoo CGI.pm on my forehead and to look in the mirror every time i'm tempted to roll my own. although i guess i don't understand the problem with using code like this if it does the job correctly for me. Not that i doubt the inherent goodness of CGI.pm, but would rewriting old scripts that i use once every 6 months be worth it just for stylistic correctness? I mean, if it's not broke and not mission-critical, is it worth the time?

      BlueLines
[reply]
[d/l]
[select]
        Don't worry about it, BlueLines. merlyn is correct that CGI.pm is better than doing it the way that you did, but that doesn't mean you HAVE to do it that way. Nor does it mean that it is always wrong to roll your own. How do you learn what not to do without trying it?

        In this case, merlyn is right; you SHOULD use CGI.pm. It won't break, most likely, if you upgrade to a newer version of CGI.pm later, although that is possible. And it is more secure than hand-rolling, for many reasons, including that CGI.pm doesn't make typo's, while humans do.

        Finally and most importantly, you and your team members are the ONLY people who can decide what is and is not appropriate to your situation. merlyn is no more a god than any of us here. He has good advice, usually; that doesn't mean you must always do as he demands.

        - email Ozymandias
[reply]
        Is it worth the time? Depends on how much time it takes. If you feel like you are playing Phil Hartman's The Anal Retentive Coder ("I like to use a space, then a tab - space tab space tab - makes the characters happy and playful, don't you think?"), then no - it's not worth the time.

        If you are going to re-invent the wheel, take a look at the source for the wheel (i.e. CGI.pm) - there's a lot of good stuff in there.

[reply]
      I think people should understand that merlyn's pique is not entirely selfish. Programming sloppily in Perl hurts Perl.

      On the other hand, merlyn, your tone is counterproductive. As someone who usually agrees with you on technical matters, I'm frustrated when your lack of diplomacy hinders your advocacy of issues that I believe in.

      The Perl community doesn't march in lockstep for anyone, not even for Larry Wall. Whether you're right or wrong, you can't take other people's code personally.

[reply]
      ok, i don't understand the downvotes on your post either. I also don't like being called "thin skinned". What I posted is exactly what i posted. It works fine for me . i never claimed that this bit of code would cure cancer or prove Fermat's theorem. All it does is grab some cgi input for me.

      note the new sig



      BlueLines

      Disclaimer: This post may contain inaccurate information, be habit forming, cause atomic warfare between peaceful countries, speed up male pattern baldness, or interfere with your cable reception. No batteries included, no strings attached, your mileage may vary.
[reply]
      I'm out of votes. But I was going to -- your post. Not because of what you wrote, but because I can't read grey on pastel green. :) However since I'm out of votes, don't worry! :)

      Ciao,
      Gryn

[reply]
        Heh.. my text is black on green. If I could specify foreground color in an HTML 4.0 way without CSS, I'd have done that. :)

        UPDATE: I tried to edit the table, adding a color style. Probably doesn't work very well. I don't completely understand CSS yet.

        -- Randal L. Schwartz, Perl hacker

[reply]

In Section Seekers of Perl Wisdom