Project

General

Profile

Todo #7385

Sanitize PHP includes

Added by Kill Bill 4 months ago. Updated 4 months ago.

Status:
New
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
03/14/2017
Due date:
% Done:

0%


Description

Includes are massively wrong across the entire pfSense code.

Sort of a reminder. Please, review functions used in the code when touching a file. Pretty much every single piece of PHP seems to get this wrong, kinda surprised how few things break due to this. A particularly popular variant that must have been caused by some copy-paste is

require_once("filter.inc");
require_once("shaper.inc");

in files that have absolutely nothing to do with traffic shaping or filtering.

/usr/local/www/diag_backup.php:74:require_once("shaper.inc");
/usr/local/www/easyrule.php:65:require_once("shaper.inc");
/usr/local/www/firewall_aliases_edit.php:68:require_once("shaper.inc");
/usr/local/www/firewall_aliases_import.php:68:require_once("shaper.inc");
/usr/local/www/firewall_aliases.php:68:require_once("shaper.inc");
/usr/local/www/firewall_nat_1to1_edit.php:69:require_once("shaper.inc");
/usr/local/www/firewall_nat_1to1.php:68:require_once("shaper.inc");
/usr/local/www/firewall_nat_edit.php:69:require_once("shaper.inc");
/usr/local/www/firewall_nat_npt_edit.php:67:require_once("shaper.inc");
/usr/local/www/firewall_nat_npt.php:69:require_once("shaper.inc");
/usr/local/www/firewall_nat_out_edit.php:68:require_once("shaper.inc");
/usr/local/www/firewall_nat_out.php:68:require_once("shaper.inc");
/usr/local/www/firewall_nat.php:68:require_once("shaper.inc");
/usr/local/www/firewall_rules_edit.php:68:require_once("shaper.inc");
/usr/local/www/firewall_rules.php:69:require_once("shaper.inc");
/usr/local/www/firewall_schedule_edit.php:82:require_once("shaper.inc");
/usr/local/www/firewall_schedule.php:72:require_once("shaper.inc");
/usr/local/www/firewall_shaper.php:64:require_once("shaper.inc");
/usr/local/www/firewall_virtual_ip_edit.php:68:require_once("shaper.inc");
/usr/local/www/firewall_virtual_ip.php:69:require_once("shaper.inc");
/usr/local/www/interfaces_assign.php:72:require_once("shaper.inc");
/usr/local/www/interfaces.php:71:require_once("shaper.inc");
/usr/local/www/load_balancer_pool.php:65:require_once("shaper.inc");
/usr/local/www/load_balancer_setting.php:66:require_once("shaper.inc");
/usr/local/www/load_balancer_virtual_server.php:65:require_once("shaper.inc");
/usr/local/www/pkg_edit.php:66:require_once("shaper.inc");
/usr/local/www/pkg_edit.php.save:66:require_once("shaper.inc");
/usr/local/www/pkg_mgr_install.php:67:require_once("shaper.inc");
/usr/local/www/services_captiveportal_filemanager.php:80:require_once("shaper.inc");
/usr/local/www/services_captiveportal_hostname_edit.php:73:require_once("shaper.inc");
/usr/local/www/services_captiveportal_hostname.php:71:require_once("shaper.inc");
/usr/local/www/services_captiveportal_ip_edit.php:79:require_once("shaper.inc");
/usr/local/www/services_captiveportal_ip.php:71:require_once("shaper.inc");
/usr/local/www/services_captiveportal_mac_edit.php:79:require_once("shaper.inc");
/usr/local/www/services_captiveportal_mac.php:69:require_once("shaper.inc");
/usr/local/www/services_captiveportal.php:67:require_once("shaper.inc");
/usr/local/www/services_captiveportal_vouchers_edit.php:65:require_once("shaper.inc");
/usr/local/www/services_captiveportal_vouchers.php:69:require_once("shaper.inc");
/usr/local/www/services_captiveportal_zones_edit.php:64:require_once("shaper.inc");
/usr/local/www/services_captiveportal_zones.php:64:require_once("shaper.inc");
/usr/local/www/services_dhcp.php:68:require_once("shaper.inc");
/usr/local/www/services_dnsmasq.php:70:require_once("shaper.inc");
/usr/local/www/services_ntpd_acls.php:65:require_once("shaper.inc");
/usr/local/www/services_ntpd.php:65:require_once("shaper.inc");
/usr/local/www/status_captiveportal_expire.php:65:require_once("shaper.inc");
/usr/local/www/status_captiveportal.php:68:require_once("shaper.inc");
/usr/local/www/status_captiveportal_test.php:65:require_once("shaper.inc");
/usr/local/www/status_captiveportal_voucher_rolls.php:65:require_once("shaper.inc");
/usr/local/www/status_captiveportal_vouchers.php:65:require_once("shaper.inc");
/usr/local/www/status_interfaces.php:66:require_once("shaper.inc");
/usr/local/www/status_lb_pool.php:69:require_once("shaper.inc");
/usr/local/www/status_logs_settings.php:68:require_once("shaper.inc");
/usr/local/www/status_monitoring.php:61:require("shaper.inc");
/usr/local/www/system_advanced_admin.php:69:require_once("shaper.inc");
/usr/local/www/system_advanced_firewall.php:69:require_once("shaper.inc");
/usr/local/www/system_advanced_misc.php:69:require_once("shaper.inc");
/usr/local/www/system_advanced_network.php:69:require_once("shaper.inc");
/usr/local/www/system_gateway_groups.php:65:require_once("shaper.inc");
/usr/local/www/system_gateways.php:65:require_once("shaper.inc");
/usr/local/www/system.php:68:require_once("shaper.inc");
/usr/local/www/system_routes.php:68:require_once("shaper.inc");
/usr/local/www/vpn_ipsec.php:68:require_once("shaper.inc");
/usr/local/www/vpn_ipsec_settings.php:64:require_once("shaper.inc");
/usr/local/www/wizard.php:65:require_once("shaper.inc");
/usr/local/www/xmlrpc.php:67:require_once("shaper.inc");

(The above being on an uptodate 2.3.4 snapshot.)

Also, require_once("functions.inc"); should be replaced with proper includes everywhere.

/usr/local/www/crash_reporter.php:62:require_once("functions.inc");
/usr/local/www/diag_backup.php:72:require_once("functions.inc");
/usr/local/www/diag_command.php:312:            fwrite($phpfile, "require_once(\"/etc/inc/functions.inc\");\n\n");
/usr/local/www/diag_halt.php:69:require_once("functions.inc");
/usr/local/www/diag_reboot.php:69:require_once("functions.inc");
/usr/local/www/firewall_aliases_edit.php:66:require_once("functions.inc");
/usr/local/www/firewall_aliases.php:66:require_once("functions.inc");
/usr/local/www/firewall_nat_1to1.php:66:require_once("functions.inc");
/usr/local/www/firewall_nat_npt.php:67:require_once("functions.inc");
/usr/local/www/firewall_nat_out.php:66:require_once("functions.inc");
/usr/local/www/firewall_nat.php:66:require_once("functions.inc");
/usr/local/www/firewall_rules.php:66:require_once("functions.inc");
/usr/local/www/firewall_schedule_edit.php:80:require_once("functions.inc");
/usr/local/www/firewall_shaper.php:62:require_once("functions.inc");
/usr/local/www/firewall_shaper_queues.php:62:require_once("functions.inc");
/usr/local/www/firewall_shaper_vinterface.php:62:require_once("functions.inc");
/usr/local/www/firewall_shaper_wizards.php:62:require_once("functions.inc");
/usr/local/www/firewall_virtual_ip.php:67:require_once("functions.inc");
/usr/local/www/getstats.php:68:include_once("includes/functions.inc.php");
/usr/local/www/guiconfig.inc:82:require_once("functions.inc");
/usr/local/www/head.inc:55:require_once("functions.inc");
/usr/local/www/includes/functions.inc.php:3: * functions.inc.php
/usr/local/www/index.php:182:require_once('includes/functions.inc.php');
/usr/local/www/index.php:73:require_once('functions.inc');
/usr/local/www/interfaces_assign.php:70:require_once("functions.inc");
/usr/local/www/interfaces_gre_edit.php:62:require_once("functions.inc");
/usr/local/www/interfaces_gre.php:62:require_once("functions.inc");
/usr/local/www/interfaces_groups_edit.php:63:require_once("functions.inc");
/usr/local/www/interfaces_groups.php:62:require_once("functions.inc");
/usr/local/www/interfaces.php:68:require_once("functions.inc");
/usr/local/www/interfaces_ppps_edit.php:67:require_once("functions.inc");
/usr/local/www/interfaces_ppps.php:66:require_once("functions.inc");
/usr/local/www/interfaces_qinq.php:62:require_once("functions.inc");
/usr/local/www/load_balancer_pool.php:63:require_once("functions.inc");
/usr/local/www/load_balancer_setting.php:64:require_once("functions.inc");
/usr/local/www/load_balancer_virtual_server.php:63:require_once("functions.inc");
/usr/local/www/pfblockerng/pfblockerng.php:43:require_once('functions.inc');
/usr/local/www/pfblockerng/pfblockerng_update.php:49:require_once('functions.inc');
/usr/local/www/pkg_edit.php:64:require_once("functions.inc");
/usr/local/www/pkg_edit.php.save:64:require_once("functions.inc");
/usr/local/www/pkg_mgr_install.php:65:require_once("functions.inc");
/usr/local/www/services_captiveportal_filemanager.php:78:require_once("functions.inc");
/usr/local/www/services_captiveportal_hostname_edit.php:71:require_once("functions.inc");
/usr/local/www/services_captiveportal_hostname.php:69:require_once("functions.inc");
/usr/local/www/services_captiveportal_ip_edit.php:77:require_once("functions.inc");
/usr/local/www/services_captiveportal_ip.php:69:require_once("functions.inc");
/usr/local/www/services_captiveportal_mac_edit.php:77:require_once("functions.inc");
/usr/local/www/services_captiveportal_mac.php:67:require_once("functions.inc");
/usr/local/www/services_captiveportal.php:65:require_once("functions.inc");
/usr/local/www/services_captiveportal_vouchers_edit.php:63:require_once("functions.inc");
/usr/local/www/services_captiveportal_vouchers.php:67:require_once("functions.inc");
/usr/local/www/services_captiveportal_zones_edit.php:62:require_once("functions.inc");
/usr/local/www/services_captiveportal_zones.php:62:require_once("functions.inc");
/usr/local/www/services_dnsmasq.php:67:require_once("functions.inc");
/usr/local/www/services_servicewatchdog.php:42:require_once("functions.inc");
/usr/local/www/services_snmp.php:66:require_once("functions.inc");
/usr/local/www/squid_monitor.php:25:require_once("/etc/inc/functions.inc");
/usr/local/www/stats.php:55:require_once("includes/functions.inc.php");
/usr/local/www/status_captiveportal_expire.php:63:require_once("functions.inc");
/usr/local/www/status_captiveportal.php:66:require_once("functions.inc");
/usr/local/www/status_captiveportal_test.php:63:require_once("functions.inc");
/usr/local/www/status_captiveportal_voucher_rolls.php:63:require_once("functions.inc");
/usr/local/www/status_captiveportal_vouchers.php:63:require_once("functions.inc");
/usr/local/www/status_filter_reload.php:63:require_once("functions.inc");
/usr/local/www/status_lb_pool.php:67:require_once("functions.inc");
/usr/local/www/status_logs_settings.php:66:require_once("functions.inc");
/usr/local/www/status.php:72:require_once("functions.inc");
/usr/local/www/suricata/suricata_download_rules.php:27:require_once("functions.inc");
/usr/local/www/suricata/suricata_select_alias.php:27:require_once("functions.inc");
/usr/local/www/system_advanced_admin.php:67:require_once("functions.inc");
/usr/local/www/system_advanced_firewall.php:67:require_once("functions.inc");
/usr/local/www/system_advanced_misc.php:67:require_once("functions.inc");
/usr/local/www/system_advanced_network.php:67:require_once("functions.inc");
/usr/local/www/system_gateway_groups.php:63:require_once("functions.inc");
/usr/local/www/system_gateways.php:63:require_once("functions.inc");
/usr/local/www/system_patches.php:30:require_once("functions.inc");
/usr/local/www/system.php:66:require_once("functions.inc");
/usr/local/www/system_routes.php:66:require_once("functions.inc");
/usr/local/www/vpn_ipsec_keys_edit.php:65:require_once("functions.inc");
/usr/local/www/vpn_ipsec_keys.php:65:require_once("functions.inc");
/usr/local/www/vpn_ipsec_mobile.php:66:require_once("functions.inc");
/usr/local/www/vpn_ipsec_phase1.php:66:require_once("functions.inc");
/usr/local/www/vpn_ipsec_phase2.php:66:require_once("functions.inc");
/usr/local/www/vpn_ipsec.php:66:require_once("functions.inc");
/usr/local/www/vpn_ipsec_settings.php:61:require_once("functions.inc");
/usr/local/www/widgets/widgets/captive_portal_status.widget.php:64:require_once("functions.inc");
/usr/local/www/widgets/widgets/carp_status.widget.php:59:require_once("functions.inc");
/usr/local/www/widgets/widgets/dyn_dns_status.widget.php:60:require_once("functions.inc");
/usr/local/www/widgets/widgets/gateways.widget.php:63:require_once("functions.inc");
/usr/local/www/widgets/widgets/installed_packages.widget.php:65:require_once("functions.inc");
/usr/local/www/widgets/widgets/interface_statistics.widget.php:65:require_once("functions.inc");
/usr/local/www/widgets/widgets/interfaces.widget.php:27:require_once("functions.inc");
/usr/local/www/widgets/widgets/ipsec.widget.php:64:require_once("functions.inc");
/usr/local/www/widgets/widgets/load_balancer_status.widget.php:64:require_once("functions.inc");
/usr/local/www/widgets/widgets/log.widget.php:59:require_once("functions.inc");
/usr/local/www/widgets/widgets/ntp_status.widget.php:58:require_once("functions.inc");
/usr/local/www/widgets/widgets/picture.widget.php:58:require_once("functions.inc");
/usr/local/www/widgets/widgets/rss.widget.php:58:require_once("functions.inc");
/usr/local/www/widgets/widgets/smart_status.widget.php:61:require_once("functions.inc");
/usr/local/www/widgets/widgets/squid_antivirus_status.widget.php:23:require_once("functions.inc");
/usr/local/www/widgets/widgets/system_information.widget.php:59:require_once("functions.inc");
/usr/local/www/widgets/widgets/system_information.widget.php:63:include_once("includes/functions.inc.php");
/usr/local/www/widgets/widgets/traffic_graphs.widget.php:76:require_once("functions.inc");
/usr/local/www/wizard.php:63:require_once("functions.inc");
/usr/local/www/xmlrpc.php:63:require_once("functions.inc");

History

#2 Updated by Jim Thompson 4 months ago

  • Assignee set to Steve Beaver

Assigned to Steve Beaver, but this looks like it could be a hairball.

#3 Updated by Jim Pingle 4 months ago

I seem to recall shaper.inc being in unexpected places because running a filter reload action fails without it. But the reason for doing it that way and not fixing the includes so it was handled by filter.inc are probably lost to time.

#4 Updated by Kill Bill 4 months ago

Hmmm well, I obviously left the filter.inc in places where stuff like filter_configure() is being used (talking about the PRs linked above). If it doesn't work without shaper.inc being included, then yeah, adding the include to filter.inc with some note would obviously be better than scattering things across files that have nothing to do with shaping and use none of the functions there.

#5 Updated by Steve Beaver 4 months ago

Thanks for this. Given that JimP and others have a recollection that there is a reason for the seemingly odd includes and that it touches so many files, this change needs a complete QA matrix run.

Unfortunately I don't have the bandwidth for that now, so I am going to postpone merging this until just before the next release when a QA run will be scheduled.

#6 Updated by Phillip Davis 4 months ago

@Kill Bill - there are write_config(gettext()) things mixed together with include_once() changes in these PRs.
Wouldn't it be better to separate the 2 things?
Then the write_config() stuff can be committed easily.

#7 Updated by Kill Bill 4 months ago

Up to pfSense devs. If you prefer, I can do a single PR for all the write_config() stuff, however that shouldn't rot there because that's going to inevitably result in conflicts sooner or later.

As for the includes, leaving the shaper.inc mystery aside, most of the PRs are actually adding (quite a bunch of) missing includes.

#8 Updated by Kill Bill 4 months ago

I redid the write_config() stuff as a separate PR. If someone provides some ETA for "next QA run", I'll redo the includes stuff shortly before that to avoid conflicts.

Also available in: Atom PDF