Project

General

Profile

Actions
Copy link

Todo #7385

open

Sanitize PHP includes

Added by Kill Bill almost 8 years ago. Updated over 5 years ago.

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

0%

Estimated time:
Plus Target Version:
Release Notes:

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");
Actions
Copy link
 #2

Updated by Jim Thompson almost 8 years ago

  • Assignee set to Anonymous

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

Actions
Copy link
 #3

Updated by Jim Pingle almost 8 years 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.

Actions
Copy link
 #4

Updated by Kill Bill almost 8 years 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.

Actions
Copy link
 #5

Updated by Anonymous almost 8 years 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.

Actions
Copy link
 #6

Updated by Phillip Davis almost 8 years 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.

Actions
Copy link
 #7

Updated by Kill Bill almost 8 years 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.

Actions
Copy link
 #8

Updated by Kill Bill almost 8 years 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.

Actions
Copy link
 #9

Updated by Jim Pingle over 5 years ago

  • Category set to Web Interface
Actions
Copy link

Also available in: Atom PDF