LovingTech

**carbonize** · Feb-04-2010, 05:57 PM

I was asked by someone to modify a basic forum script he used. Talk about some bad practises. They were using $_REQUEST instead of $_POST leaving it open to abuse. They had quotes around some HTML attributes but not around others. Oh and the best one.

eval(gzinflate(base64_decode('craphere')));

That is how they stopped people from removing the copyright footer and how they hid the anti spam function they had. But they didn't just use it once. No they would base64_encode the PHP then Gzip it then base64_encode the gzipped code and then gzip that. They would do this several times. I think they had done it ten times for the anti spam function. Can you imagine the overhead that would create for something that achieves nothing in reality.

It was from PHPJunkYard's forum script. They do the same in their guestbook script as well.

**cbuckley** · Feb-05-2010, 11:39 PM

WHAT!, are you actually kidding me?

The moral of the story really, don't use other peoples scripts and especially not scripts from people that don't know what they are doing.

**deathshadow** · Feb-06-2010, 12:54 AM

I'd call that surprising, except that going in and cleaning up other people's code was my specialty when I was doing this stuff for work. I've seen far, far, far worse than that.

Some of the worst PHP stems from people not removing old code, or making assumptions that apply to another language that don't work in PHP. Core amongst those issues:

Brute force writing functions that already exist in PHP - you'll see that one all the time. Because it's an interpreted language anything you can do as a native function will be faster as a native function; this is why out of the box PHP has more in-built functions than most any other language.

Leaving in 'dead' code or legacy bits - PHP is a bytecode interpreted language - the more code there is the slower it's gonna run, it's not like in a compiled language where unused bits don't get compiled into the final version.

A great example is a job I did two years ago, where a client had their contact form plugged into their goldmine application... The HTML form had a hidden input in it that had a 'encoded' copy of the destination e-mail address of the goldmine server. IT WAS PASSING THE REAL DESTINATION IN THE FORM!?!?!

Worse, was the encode/decode method used for it, the original looking something like this:

Code:

function Decode($strValueIn)

{

//Do not modify this function!

$intX = 0;

$intY = 0;

$Temp = "";

$Mod = "";

$intMod =0;

$intTemp = 0;

$ValueOut = "";

$intY = 1;

$strValueOut="";

for ($intX=1; $intX<=((strlen($strValueIn)/2)/2); $intX=$intX+1)

 {

  $strTemp="";

  $strMod="";

  if (($intX % 2)==0)

  {

   $strMod=substr(substr($strValueIn,$intY-1,4),0,2);

   $strTemp=substr(substr($strValueIn,$intY-1,4),strlen(substr($strValueIn,$intY-1,4))-(2));

  }

   else

  {

   $strMod=substr(substr($strValueIn,$intY-1,4),strlen(substr($strValueIn,$intY-1,4))-(2));

   $strTemp=substr(substr($strValueIn,$intY-1,4),0,2);

  } 

  $intMod=hexdec($strMod);

  $intTemp=hexdec($strTemp);

  $intTemp=$intTemp-$intMod;

  $strValueOut=$strValueOut.chr($intTemp);

  $intY=$intY+4;

 } 

 return $strValueOut;

}

What's wrong with that? Like five unused variables, redundant/uneccessary logic... ****ed up part was I rewrote it BEFORE I figured out it was to decode information that shouldn't be sent through the form in the first place... The following is functionally identical:

Code:

function Decode($inString) {

    $result='';

    $t=0;

    while ($t<strlen($inString)) {

        $top=hexdec(substr($inString,$t,2));

        $t+=2;

        $bottom=hexdec(substr($inString,$t,2));

        $t+=2;

        $result.=chr(($t%8)==0 ? $top-$bottom : $bottom-$top);

    }

    return $result;

}

Bad code is everywhere, most of the time it's spagetti mish-mash or programmers being too smart for their own good. That dual encode gzip/eval is the tip of the iceberg in terms of bad code, but a great example of overthinking/overcomplicating code for no good reason.

A LOT of developers are obsessed with things like code obfuscation when it comes to the internet - you want obfuscation, use a compiled language like C and make a native application - or use something like a server side java crapplet. One should not waste time trying to pull those types of stunts in a language intentionally designed NOT to hide anything from anyone.

**carbonize** · Feb-06-2010, 12:04 PM

Stupidest thing in that script is not only does he eval(gzinflate(base64_decode it several times, and by that I mean it would look like eval(gzinflate(base64_decode(eval(gzinflate(base64_decode(eval(gzinflate(base64_decode ..., he actually uses it three times and two of the occurrences are to obfuscate the same code:

Code:

$link = true;

if(file_exists('mboard_license.php')) {

  include_once('mboard_license.php');

  if(@is_array($settings['mboard_license'])) {

    $link=false;

  }

}

if($link) {

  $content .= '<!--

  Changing the "Powered by" credit sentence without purchasing a licence is illegal!

  Please visit http://www.phpjunkyard.com/copyright-removal.php for more information.

  -->

  <p align="center"><font class="smaller">Powered by <a href="http://www.phpjunkyard.com/php-message-board.php" class="smaller" target="_blank">Free PHP message board</a> '.$settings['verzija'].' from <a href="http://www.phpjunkyard.com/" target="_blank" class="smaller">PHPJunkYard - Free PHP scripts</a></font></p>';

}

I mean if you are using the same code more than once it should be in a function anyway and given the amount of work required to get the final bit of code I would make it store the result to a global variable and check if the variable was empty or not before doing all that crap again.

And just to take the piss here is the second bit of code he obfuscates. It's just a spam post checker.

Code:

function JunkMark($email,$comments) {

  $spam_words_soft=array(

  '100%',

  'affordable',

  'ambien',

  'bargain',

  'buy',

  'chatroom',

  'cheap',

  'financing',

  'insurance',

  'investment',

  'loan',

  'poze',

  'pre-approved',

  'soma',

  'taboo',

  'teen',

  'wholesale'

  );

  $spam_words_hard=array(

  'adipex',

  'advicer',

  'baccarrat',

  'blackjack',

  'bllogspot',

  'booker',

  'byob',

  'carbohydrate',

  'car-rental-e-site',

  'car-rentals-e-site',

  'carisoprodol',

  'casino',

  'casinos',

  'cialis',

  'coolcoolhu',

  'coolhu',

  'credit-report-4u',

  'cwas',

  'cyclen',

  'cyclobenzaprine',

  'dating-e-site',

  'day-trading',

  'debt',

  'debt-consolidation-consultant',

  'drug',

  'discreetordering',

  'duty-free',

  'dutyfree',

  'equityloans',

  'financing',

  'fioricet',

  'flowers-leading-site',

  'freenet-shopping',

  'freenet',

  'gambling',

  'health-insurancedeals-4u',

  'homeequityloans',

  'homefinance',

  'holdem',

  'holdempoker',

  'holdemsoftware',

  'holdemtexasturbowilson',

  'hotel-dealse-site',

  'hotele-site',

  'hotelse-site',

  'incest',

  'insurance-quotesdeals-4u',

  'insurancedeals-4u',

  'jrcreations',

  'levitra',

  'macinstruct',

  'mortgage-4-u',

  'mortgagequotes',

  'online-gambling',

  'onlinegambling-4u',

  'ottawavalleyag',

  'ownsthis',

  'palm-texas-holdem-game',

  'paxil',

  'penis',

  'pharmacy',

  'phentermine',

  'poker',

  'poker-chip',

  'rental-car-e-site',

  'roulette',

  'shemale',

  'shoes',

  'slot-machine',

  'slot',

  'texas-holdem',

  'thorcarlson',

  'top-site',

  'top-e-site',

  'tramadol',

  'trim-spa',

  'ultram',

  'valeofglamorganconservatives',

  'viagra',

  'vioxx',

  'xanax',

  'zolus '

  );

  if (count($_POST)>20) {

    return 100;

  }

  $myscore = $email ? 10 : 0;

  foreach ($spam_words_hard as $sw) {

    if (strpos($comments,$sw)!==false) {

      $myscore+=70;

    }

  }

  $comments=strtolower($comments);

  foreach ($spam_words_soft as $sw) {

    if (strpos($comments,$sw)!==false) {

      $myscore+=30;

    }

  }

  if (strpos($comments,'http://')!==false || strpos($comments,'www.')!==false) {

    $myscore+=20;

  }

  if ( isset($_SERVER['HTTP_X_FORWARDED_FOR']) || isset($_SERVER['HTTP_VIA']) ||

  isset($_SERVER['HTTP_COOKIE2']) || isset($_SERVER['HTTP_X_FORWARDED_SERVER']) ||

  isset($_SERVER['HTTP_X_FORWARDED_HOST']) || isset($_SERVER['HTTP_MAX_FORWARDS']) ||

  isset($_SERVER['HTTP_PROXY_CONNECTION']) ) {

    $myscore += 50;

  }

  $myscore = ($myscore > 100) ? 100 : $myscore;

**deathshadow** · Feb-06-2010, 06:30 PM

Well lets be honest - this ALONE

<p align="center"><font class="smaller">

Screams "idiot"

**RT Cunningham** · Jun-23-2010, 01:48 PM

(Feb-06-2010 06:30 PM)deathshadow Wrote: Well lets be honest - this ALONE

<p align="center"><font class="smaller">

Screams "idiot"

Not by itself. If it was written before CSS took hold... I still see it on static sites.

**deathshadow** · Jun-23-2010, 02:52 PM

(Jun-23-2010 01:48 PM)RT Cunningham Wrote: Not by itself. If it was written before CSS took hold... I still see it on static sites.

Actually it still would; since the FONT tag is used for a class that means CSS, which means the class could go on the paragraph tag; at which point that class should be used to apply the align, not the markup...

Though you're right about the whole age of the codebase thing, which is why I get annoyed when I see people still doing that on what's supposed to be a 'new' website.