HEADS UP: Package maintainers – Check for call-time pass-by-reference usage!

jimp · May 8, 2014, 8:22 PM

pfSense 2.2 has moved to PHP 5.5, which has deprecated call-time pass-by-reference. Many packages use this for their input validation, and they will need to be fixed. Usually it's a simple fix, but there is a bit of a twist which I'll get to in a moment.

A quick example, the sudo package which I fixed last week. In the package's xml file, sudo.xml, it used code like this:


        <custom_php_validation_command>sudo_validate_commands(&$input_errors);
                ]]></custom_php_validation_command>

And in its accompanying sudo.inc, this existed:

function sudo_validate_commands($input_errors) {

This one was simple to fix because it was a custom function. All that was required was to move the & from the function call to the function definition, like so:

sudo.xml:


        <custom_php_validation_command>sudo_validate_commands($input_errors);
                ]]></custom_php_validation_command>

sudo.inc:

function sudo_validate_commands(&$input_errors) {

Other packages may have things specified slightly differently in their .xml, such as:

<custom_php_validation_command>validate_input_zabbix2($_POST, &$input_errors);</custom_php_validation_command>

– the fix is similar, remove "&" from the function call, edit the .inc and put a & before the corresponding variable in the function definition.

The twist comes from packages that call do_input_validation() which is built into pfSense. On 2.0.x the packages have to use call-time pass-by-reference, on 2.1 and later they should not. So a simple validation call like this:

do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);

Will need to be adjusted to have a version test:


	$pf_version=substr(trim(file_get_contents("/etc/version")),0,3);
	if ($pf_version < 2.1)
		$input_errors = eval('do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors); return $input_errors;');
	else
		do_input_validation($_POST, $reqdfields, $reqdfieldsn, $input_errors);

It's not perfect, but here is an egrep command to help locate such references in your package code (it does have some false positives, but better too much than too little):

egrep -Irn '(\&\$|\&\$)' * | egrep -v '(function |\=\ \&\$|\=>\ \&\$|\=\&\$|\=>\&\$|foreach|\@param)'

If you need help with your package, start a new thread with the details. When making the change, don't forget to bump/fix the version in all of the proper xml files – the package's own XML file, plus pkg_config.8.xml, pkg_config.8.xml.amd64, pkg_config.10.xml.

phil.davis · May 8, 2014, 1:13 AM

For ease of reference, here is a sample way of doing pfSense version testing, which is already in a few packages:

// Check pfSense version
$pfs_version = substr(trim(file_get_contents("/etc/version")),0,3);
switch ($pfs_version) {
  case "1.2":
  case "2.0":
    define('PKG_BANDWIDTHD_BASE', '/usr/local/bandwidthd');
    define('PKG_BANDWIDTHD_RUNTIME_LIBRARY_ENV', '');
    break;
  case "2.1":
    define('PKG_BANDWIDTHD_BASE', '/usr/pbi/bandwidthd-' . php_uname("m") . '/bandwidthd');
    define('PKG_BANDWIDTHD_RUNTIME_LIBRARY_ENV', '');
    break;
  default:
    define('PKG_BANDWIDTHD_BASE', '/usr/pbi/bandwidthd-' . php_uname("m") . '/local/bandwidthd');
    define('PKG_BANDWIDTHD_RUNTIME_LIBRARY_ENV', 'LD_LIBRARY_PATH=/usr/pbi/bandwidthd-' . php_uname("m") . '/local/lib');
 }
// End: Check pfSense version

For the call-by-reference changes, the "2.1" case won't be needed - "default:" will catch 2.1.* 2.2.* and on into the future. The different do_input_validation calls will end up in the appropriate "case" sections.

jimp · May 8, 2014, 8:01 PM

For this purpose, a simpler test is sufficient:


     $pf_version=substr(trim(file_get_contents("/etc/version")),0,3);
     if ($pf_version < 2.1)
          $input_errors = eval('do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors); return $input_errors;');
     else
          do_input_validation($_POST, $reqdfields, $reqdfieldsn, $input_errors);

jimp · May 8, 2014, 8:24 PM

Note that I updated the example test for pre-2.1 versions. PHP 5.5 will produce an error for call-time pass-by-reference usage even if the code would never be encountered. So it had to be wrapped up in an eval to make sure that the behavior worked as desired.

jimp · May 9, 2014, 7:02 PM

I have fixed most of them except for ones I knew were being maintained by others.

The following will need to be fixed properly before the packages will work on 2.2:

Snort:

config/snort/snort_interfaces_suppress_edit.php:93:     do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);
config/snort/snort_passlist_edit.php:115:       do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);

Suricata:

config/suricata/suricata_passlist_edit.php:117: do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);
config/suricata/suricata_suppress_edit.php:91:  do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);

Dansguardian:

config/dansguardian/dansguardian.xml:380:               dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_antivirus_acl.xml:234:         dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_blacklist.xml:166:             dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_config.xml:309:                dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_content_acl.xml:202:           dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_file_acl.xml:242:              dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_groups.xml:453:                dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_header_acl.xml:222:            dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_ldap.xml:167:          dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_limits.xml:176:                dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_log.xml:249:           dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_phrase_acl.xml:265:            dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_pics_acl.xml:199:              dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_search_acl.xml:259:            dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_site_acl.xml:295:              dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_sync.xml:161:          dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_url_acl.xml:346:               dansguardian_validate_input($_POST, &$input_errors);
config/dansguardian/dansguardian_users_footer.template:9:               dansguardian_validate_input($_POST, &$input_errors);

HAproxy-devel:

config/haproxy-devel/haproxy_global.php:67:             //do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);
config/haproxy-devel/haproxy_listeners_edit.php:147:    do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);
config/haproxy-devel/haproxy_pool_edit.php:131: do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);
config/haproxy-devel/haproxy_pool_edit.php:136:         do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);
config/haproxy-devel/haproxy_pool_edit.php:140:                 do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);

Mailscanner:

config/mailscanner/mailscanner.xml:350:         mailscanner_validate_input($_POST, &$input_errors);
config/mailscanner/mailscanner_alerts.xml:153:          mailscanner_validate_input($_POST, &$input_errors);
config/mailscanner/mailscanner_antispam.xml:448:                mailscanner_validate_input($_POST, &$input_errors);
config/mailscanner/mailscanner_antivirus.xml:184:               mailscanner_validate_input($_POST, &$input_errors);
config/mailscanner/mailscanner_attachments.xml:215:             mailscanner_validate_input($_POST, &$input_errors);
config/mailscanner/mailscanner_content.xml:237:         mailscanner_validate_input($_POST, &$input_errors);
config/mailscanner/mailscanner_report.xml:527:          mailscanner_validate_input($_POST, &$input_errors);
config/mailscanner/mailscanner_sync.xml:154:            mailscanner_validate_input($_POST, &$input_errors);

pfBlocker:

config/pf-blocker/pfblocker.xml:244:            pfblocker_validate_input($_POST, &$input_errors);
config/pf-blocker/pfblocker_lists.xml:249:              pfblocker_validate_input($_POST, &$input_errors);
config/pf-blocker/pfblocker_sync.xml:141:               pfblocker_validate_input($_POST, &$input_errors);
config/pf-blocker/pfblocker_topspammers.xml:161:                pfblocker_validate_input($_POST, &$input_errors);

Postfix:

config/postfix/postfix.xml:357:         postfix_validate_input($_POST, &$input_errors);
config/postfix/postfix_acl.xml:224:             postfix_validate_input($_POST, &$input_errors);
config/postfix/postfix_antispam.xml:282:                postfix_validate_input($_POST, &$input_errors);
config/postfix/postfix_domains.xml:140:         postfix_validate_input($_POST, &$input_errors);
config/postfix/postfix_recipients.xml:195:              postfix_validate_input($_POST, &$input_errors);
config/postfix/postfix_sync.xml:196:            postfix_validate_input($_POST, &$input_errors);

Sarg:

config/sarg/sarg.xml:366:               sarg_validate_input($_POST, &$input_errors);
config/sarg/sarg_schedule.xml:219:              sarg_validate_input($_POST, &$input_errors);
config/sarg/sarg_sync.xml:141:          sarg_validate_input($_POST, &$input_errors);
config/sarg/sarg_users.xml:214:         sarg_validate_input($_POST, &$input_errors);

Squid-head (I'm not sure this is even active anywhere):

config/squid-head/squid.xml:201:                squid_before_form_general(&$pkg);
config/squid-head/squid.xml:204:                squid_validate_general($_POST, &$input_errors);
config/squid-head/squid_auth.xml:191:           squid_validate_auth($_POST, &$input_errors);
config/squid-head/squid_cache.xml:175:          squid_validate_cache($_POST, &$input_errors);
config/squid-head/squid_nac.xml:142:            squid_validate_nac($_POST, &$input_errors);
config/squid-head/squid_traffic.xml:174:                squid_validate_traffic($_POST, &$input_errors);
config/squid-head/squid_upstream.xml:128:               squid_validate_upstream($_POST, &$input_errors);

Squid 3.x and Squid 3-dev:

config/squid3/31/squid.xml:432:         squid_before_form_general(&$pkg);
config/squid3/31/squid.xml:438:         squid_validate_general($_POST, &$input_errors);
config/squid3/31/squid_auth.xml:247:            squid_validate_auth($_POST, &$input_errors);
config/squid3/31/squid_cache.xml:290:           squid_validate_cache($_POST, &$input_errors);
config/squid3/31/squid_nac.xml:181:             squid_validate_nac($_POST, &$input_errors);
config/squid3/31/squid_reverse.xml:349:         squid_before_form_general(&$pkg);
config/squid3/31/squid_reverse.xml:352:         squid_validate_reverse($_POST, &$input_errors);
config/squid3/31/squid_reverse_general.xml:241:         squid_before_form_general(&$pkg);
config/squid3/31/squid_reverse_general.xml:244:         squid_validate_reverse($_POST, &$input_errors);
config/squid3/31/squid_reverse_peer.xml:159:            squid_before_form_general(&$pkg);
config/squid3/31/squid_reverse_peer.xml:162:            squid_validate_reverse($_POST, &$input_errors);
config/squid3/31/squid_traffic.xml:198:         squid_validate_traffic($_POST, &$input_errors);
config/squid3/31/squid_upstream.xml:352:                squid_validate_upstream($_POST, &$input_errors);
config/squid3/33/squid.xml:558:         squid_before_form_general(&$pkg);
config/squid3/33/squid.xml:564:         squid_validate_general($_POST, &$input_errors);
config/squid3/33/squid_auth.xml:253:            squid_validate_auth($_POST, &$input_errors);
config/squid3/33/squid_cache.xml:315:           squid_validate_cache($_POST, &$input_errors);
config/squid3/33/squid_nac.xml:186:             squid_validate_nac($_POST, &$input_errors);
config/squid3/33/squid_reverse.xml:349:         squid_before_form_general(&$pkg);
config/squid3/33/squid_reverse.xml:352:         squid_validate_reverse($_POST, &$input_errors);
config/squid3/33/squid_reverse_general.xml:244:         squid_before_form_general(&$pkg);
config/squid3/33/squid_reverse_general.xml:247:         squid_validate_reverse($_POST, &$input_errors);
config/squid3/33/squid_reverse_peer.xml:159:            squid_before_form_general(&$pkg);
config/squid3/33/squid_reverse_peer.xml:162:            squid_validate_reverse($_POST, &$input_errors);
config/squid3/33/squid_traffic.xml:203:         squid_validate_traffic($_POST, &$input_errors);
config/squid3/33/squid_upstream.xml:356:                squid_validate_upstream($_POST, &$input_errors);
config/squid3/old/squid.xml:318:                squid_before_form_general(&$pkg);
config/squid3/old/squid.xml:324:                squid_validate_general($_POST, &$input_errors);
config/squid3/old/squid_auth.xml:223:           squid_validate_auth($_POST, &$input_errors);
config/squid3/old/squid_cache.xml:217:          squid_validate_cache($_POST, &$input_errors);
config/squid3/old/squid_nac.xml:138:            squid_validate_nac($_POST, &$input_errors);
config/squid3/old/squid_traffic.xml:172:                squid_validate_traffic($_POST, &$input_errors);
config/squid3/old/squid_upstream.xml:128:               squid_validate_upstream($_POST, &$input_errors);

Squidguard-devel:

config/squidGuard-devel/squidguard.inc:109:    if ($submit === APPLY_BTN)  sg_check_config_data(&$input_errors);
config/squidGuard-devel/squidguard.inc:117:    squidguard_validate_acl($post, &$input_errors);
config/squidGuard-devel/squidguard.inc:137:        check_name_format($name, &$input_errors);
config/squidGuard-devel/squidguard.inc:151:        sg_check_src($sgx, &$input_errors);
config/squidGuard-devel/squidguard.inc:195:    if (!sg_check_redirect($post[F_RMOD], $post[F_REDIRECT], &$errmsg)) {
config/squidGuard-devel/squidguard.inc:213:        check_name_format($name, &$input_errors);
config/squidGuard-devel/squidguard.inc:249:    sg_check_time($sgx, &$input_errors);
config/squidGuard-devel/squidguard.inc:260:        check_name_format($name, &$input_errors);
config/squidGuard-devel/squidguard.inc:280:    sg_check_dest($sgx, &$input_errors);
config/squidGuard-devel/squidguard.inc:291:            check_name_format($name, &$input_errors);
config/squidGuard-devel/squidguard.inc:1304:    squidguard_adt_safesrch_add(&$res[F_ITEM]);
config/squidGuard-devel/squidguard.inc:1377:    $cont = squidguard_logdump(SQUIDGUARD_LOGDIR . '/squidGuard.log', &$lnoffset, $lncount, $reverse);
config/squidGuard-devel/squidguard.inc:1391:    $cont = squidguard_logdump(SQUIDGUARD_LOGDIR . SQUIDGUARD_CONFLOGFILE, &$lnoffset, $lncount, $reverse);
config/squidGuard-devel/squidguard.inc:1405:    $cont = squidguard_logdump(SQUIDGUARD_LOGDIR . '/' . SQUIDGUARD_LOGFILE, &$lnoffset, $lncount, $reverse);
config/squidGuard-devel/squidguard.xml:242:                squidguard_validate(&$_POST, &$input_errors);
config/squidGuard-devel/squidguard.xml:245:                squidguard_before_form(&$pkg);
config/squidGuard-devel/squidguard_acl.xml:227:                         squidguard_validate_acl(&$_POST, &$input_errors);
config/squidGuard-devel/squidguard_acl.xml:230:                         squidguard_before_form_acl(&$pkg);
config/squidGuard-devel/squidguard_configurator.inc:849:    if (!sg_check_config_data(&$error_res)) {
config/squidGuard-devel/squidguard_configurator.inc:1074:                    acl_remove_blacklist_items(&$acl[F_DESTINATIONNAME]);
config/squidGuard-devel/squidguard_configurator.inc:1075:                    acl_remove_blacklist_items(&$acl[F_OVERDESTINATIONNAME]);
config/squidGuard-devel/squidguard_configurator.inc:1131:            acl_remove_blacklist_items(&$def[F_DESTINATIONNAME]);
config/squidGuard-devel/squidguard_configurator.inc:1257:    if (!sg_check_redirect($redirect_mode, $rdr_info, &$errmsg)) {
config/squidGuard-devel/squidguard_configurator.inc:1330:            if (!check_name_format($tm_name, &$err_s))
config/squidGuard-devel/squidguard_configurator.inc:1337:            sg_check_time($tm, &$elog);
config/squidGuard-devel/squidguard_configurator.inc:1348:            if (!check_name_format($src_name, &$err_s))
config/squidGuard-devel/squidguard_configurator.inc:1365:            if (!check_name_format($dst_name, &$err_s))
config/squidGuard-devel/squidguard_configurator.inc:1371:            sg_check_dest($dst, &$elog);
config/squidGuard-devel/squidguard_configurator.inc:1399:            if (!check_name_format($rw_name, &$err_s))
config/squidGuard-devel/squidguard_configurator.inc:1755:    array_packitems(&$dm);
config/squidGuard-devel/squidguard_configurator.inc:1756:    array_packitems(&$ur);
config/squidGuard-devel/squidguard_configurator.inc:1768:    sg_check_redirect($sgx[F_RMOD], $sgx[F_REDIRECT], &$elog);
config/squidGuard-devel/squidguard_default.xml:137:                             squidguard_validate_acl(&$_POST, &$input_errors);
config/squidGuard-devel/squidguard_default.xml:140:                             squidguard_before_form_acl(&$pkg, false);
config/squidGuard-devel/squidguard_dest.xml:175:                                squidguard_before_form_dest(&$pkg);
config/squidGuard-devel/squidguard_dest.xml:178:                                squidguard_validate_destination($_POST, &$input_errors);
config/squidGuard-devel/squidguard_log.php:80:              $res = squidguard_logrep(squidguard_guidump( &$offset, 50, true));
config/squidGuard-devel/squidguard_log.php:83:              $res = squidguard_logrep(squidguard_filterdump( &$offset, 50, true));
config/squidGuard-devel/squidguard_log.php:87:              $res = squidguard_logrep(squidguard_blockdump( &$offset, 50, true));
config/squidGuard-devel/squidguard_rewr.xml:139:                squidguard_validate_rewrite($_POST, &$input_errors);
config/squidGuard-devel/squidguard_time.xml:139:                squidguard_validate_times(&$_POST, &$input_errors);

If you are the maintainer of one of those packages and need assistance, look at my first post in this thread, and at the other recently committed fixes for call-time pass-by-reference. If you're still having trouble, reply here.

Thanks!

bmeeks · May 14, 2014, 2:52 PM

@jimp:

I have fixed most of them except for ones I knew were being maintained by others.

The following will need to be fixed properly before the packages will work on 2.2:

Snort:
config/snort/snort_interfaces_suppress_edit.php:93:     do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);
config/snort/snort_passlist_edit.php:115:       do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);
Suricata:
config/suricata/suricata_passlist_edit.php:117: do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);
config/suricata/suricata_suppress_edit.php:91:  do_input_validation($_POST, $reqdfields, $reqdfieldsn, &$input_errors);
…

If you are the maintainer of one of those packages and need assistance, look at my first post in this thread, and at the other recently committed fixes for call-time pass-by-reference. If you're still having trouble, reply here.

Thanks Jim for pinpointing the exact files and line numbers. I will fix this up in the Snort and Suricata packages as part of their next updates. Is the PBI environment on 2.2 static now? By that I mean are the conf file installation paths and the shared library issues resolved? The conf file paths seemed to be /usr/pbi/{package_name-arch}/local/etc in some of the early 2.2 tests I did. The 2.1 path omitted that extra local directory. If the extra local is going to remain, then packages like Snort and Suricata (and some others) that need to reference their configuration file paths directly will have to have three different locations depending on pfSense version (2.0.x, 2.1.x and 2.2).

Bill

jimp · May 14, 2014, 3:27 PM

@bmeeks:

Thanks Jim for pinpointing the exact files and line numbers. I will fix this up in the Snort and Suricata packages as part of their next updates. Is the PBI environment on 2.2 static now? By that I mean are the conf file installation paths and the shared library issues resolved? The conf file paths seemed to be /usr/pbi/{package_name-arch}/local/etc in some of the early 2.2 tests I did. The 2.1 path omitted that extra local directory. If the extra local is going to remain, then packages like Snort and Suricata (and some others) that need to reference their configuration file paths directly will have to have three different locations depending on pfSense version (2.0.x, 2.1.x and 2.2).

I'm not 100% sure on the config issue. The libraries and such should be OK now. I've tested several packages after Renato's last round of fixes and they've all worked when they were broken before. Granted they were simpler ones like client export, sudo, and nmap but they were all broken before as well, but work now.

bmeeks · May 14, 2014, 6:33 PM

@jimp:

I'm not 100% sure on the config issue. The libraries and such should be OK now. I've tested several packages after Renato's last round of fixes and they've all worked when they were broken before. Granted they were simpler ones like client export, sudo, and nmap but they were all broken before as well, but work now.

I just tested Suricata on a 2.2 install and it works now if I account for the configuration files being in /usr/pbi/{pkg_name-arch}/local/etc. So I can fix this by including an additional "if case" in the GUI code that defines the path to the conf files in Suricata and Snort.

So it looks like all the shared library issues are resolved. I am working now on fixing up Snort and Suricata to take care of the call-time pass-by-reference issue. I can accommodate this other 2.2 change in the same update.

Bill

salida · Jun 15, 2014, 7:39 PM

Can someone please confirm which packages are working , and which not ?

salida · Jun 16, 2014, 2:14 PM

Just tested squid 2.7.9_4 and squidguard 1.4_4 pkg v.1.9.6.

Squid is still not working displaying the call by reference error.
Squidguard seems ok but depends on squid to work.

Is squid fixed and i am the only one having still this error ?
or the problem persists ?

2.2-ALPHA (i386) built on Fri Jun 13 09:58:26 CDT 2014

athurdent · Jul 16, 2014, 11:28 AM

Sadly it seems that the maintainer of freeradius2 won't be able to do any more work on that package.

https://forum.pfsense.org/index.php?topic=43675.msg432223#msg432223

jimp · Jul 16, 2014, 6:29 PM

@athurdent:

Sadly it seems that the maintainer of freeradius2 won't be able to do any more work on that package.

https://forum.pfsense.org/index.php?topic=43675.msg432223#msg432223

That package does not appear to be affected by the problem for which this thread is intended. If it is broken in some other way on 2.2, post a new thread on the 2.2 board and someone may be able to look at the package if there enough detail on how to reproduce the issue.