Project

General

Profile

Actions

Bug #14356

closed

URL scheme is not properly validated in some cases

Added by Jonathan Lee about 1 year ago. Updated 12 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
PHP Interpreter
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
Plus Target Version:
23.09
Release Notes:
Default
Affected Version:
All
Affected Architecture:
All

Description

Hello fellow pfSense Redmine community members can you please help?

If you generate an error inside of an Aliases specific when changing type "host" to "url(port)" the list of pink errors no longer displays, and this option now returns a PHP error.

Crash report begins. Anonymous machine information:

arm64
14.0-CURRENT
FreeBSD 14.0-CURRENT #0 plus-RELENG_23_01-n256037-6e914874a5e: Fri Feb 10 20:28:37 UTC 2023 root@freebsd:/var/jenkins/workspace/pfSense-Plus-snapshots-23_01-main/obj/aarch64/Z3hsU8Fs/var/jenkins/workspace/pfSense-Plus-snapshots-23_01-main/sources/Free

Crash report details:

PHP Errors:
[06-May-2023 15:43:48 US/Pacific] PHP Fatal error: Uncaught TypeError: implode(): Argument #1 ($pieces) must be of type array, string given in /usr/local/pfSense/include/www/alias-utils.inc:771
Stack trace:
#0 /usr/local/pfSense/include/www/alias-utils.inc(771): implode(' ', NULL)
#1 /usr/local/www/firewall_aliases_edit.php(157): saveAlias(Array, '19')
#2 {main}
thrown in /usr/local/pfSense/include/www/alias-utils.inc on line 771
[06-May-2023 15:43:51 US/Pacific] PHP Fatal error: Uncaught TypeError: implode(): Argument #1 ($pieces) must be of type array, string given in /usr/local/pfSense/include/www/alias-utils.inc:771
Stack trace:
#0 /usr/local/pfSense/include/www/alias-utils.inc(771): implode(' ', NULL)
#1 /usr/local/www/firewall_aliases_edit.php(157): saveAlias(Array, '19')
#2 {main}
thrown in /usr/local/pfSense/include/www/alias-utils.inc on line 771

No FreeBSD crash data found.

Please see attached


Files

Screenshot 2023-05-06 at 3.50.40 PM.png (430 KB) Screenshot 2023-05-06 at 3.50.40 PM.png change to url once saved error does not display pink errors however system crash occurs Jonathan Lee, 05/06/2023 10:58 PM
Screenshot 2023-05-06 at 3.51.32 PM.png (263 KB) Screenshot 2023-05-06 at 3.51.32 PM.png normally pink errors should have displayed like in past versions Jonathan Lee, 05/06/2023 10:58 PM
Screenshot 2023-05-06 at 3.52.28 PM.png (319 KB) Screenshot 2023-05-06 at 3.52.28 PM.png Error Jonathan Lee, 05/06/2023 10:59 PM
Actions #1

Updated by Kris Phillips about 1 year ago

Tested this in pfSense Plus 23.05 RC builds from May 6th. Created an Alias called "Test Alias" that was set to "Hosts" and added an entry for www.google.com. Saved and Applied.

Then re-edited the Alias, changed it to URL (Ports) with no other changes. The pink error box appeared without a crash. Seems this issue is either fixed in 23.05 or I'm missing a step in recreating this issue.

Actions #2

Updated by Jonathan Lee about 1 year ago

Exactly the steps I did to reproduce the issue. Thank you for checking.

Actions #3

Updated by Marcos M about 1 year ago

  • Subject changed from 23.01 pfSense crash inside of Firewall / Aliases /Edit Dashboard. Pink error in GUI are no longer displaying for change overs from hosts to URLs to URL scheme is not properly validated in some cases
  • Category changed from Aliases / Tables to Unknown
  • Status changed from New to Pull Request Review
  • Assignee set to Marcos M
  • Affected Architecture All added
  • Affected Architecture deleted (SG-2100)
In all uses of the function is_URL(), the URL scheme is required. If the subdomain www is specified, the function can return true even when the URL scheme is missing. This currently affects multiple areas:
  • cert URI SAN
  • alias with URL type
  • captive portal preauthurl/redirurl/blockedmacsurl
  • Check IP Service URL
  • dhcp TFTP server / network bootfile
  • OpenVPN ocspurl

Fix this by using native variable filters in PHP.
https://gitlab.netgate.com/pfSense/pfSense/-/merge_requests/1036

Actions #4

Updated by Jim Pingle about 1 year ago

Marcos M wrote in #note-3:

This currently affects multiple areas:

Affects how? They get a PHP error? Or they fail in some other way?

Actions #5

Updated by Marcos M about 1 year ago

Different areas behave differently, however they all expect a valid URL with a scheme. For example:
  • The OpenVPN input field is created with type="url" hence input validation is triggered before is_URL is called.
  • The cert manager simply accepts the invalid url for the URI SAN without triggering a PHP or input validation error.

The reason for the PHP error on the URL-type ports alias is because this part evaluates to true:
https://github.com/pfsense/pfsense/blob/89803e07a756de18beab184b88ede15e6ba617ff/src/usr/local/pfSense/include/www/alias-utils.inc#L376
... which then leads to this evaluating to true and triggering the PHP error:
https://github.com/pfsense/pfsense/blob/89803e07a756de18beab184b88ede15e6ba617ff/src/usr/local/pfSense/include/www/alias-utils.inc#L744

Example:

var_dump(is_URL('http://google.com'));     # true
var_dump(is_URL('http://www.google.com')); # true
var_dump(is_URL('google.com'));            # false
var_dump(is_URL('www.google.com'));        # true - should be false

Actions #6

Updated by Jim Pingle about 1 year ago

  • Target version set to 2.7.0
  • Plus Target Version set to 23.05

I just don't want to get caught in a situation where something big relies on the current behavior that would be a pain to adjust. If everything, including packages, should already be using full URLs with a scheme, then it should be OK to change it over.

We've had some issues in the past with that function and changes to it ended up having unintended fallout, but maybe that's all been addressed since then.

Actions #7

Updated by Jim Pingle about 1 year ago

  • Plus Target Version changed from 23.05 to 23.09
Actions #8

Updated by Jonathan Lee about 1 year ago

Thank you for looking at this, I thought I should share it as it could possibly be used with a zero day if it is not corrected. Thank you for reviewing this. Have a good day everyone.

Actions #9

Updated by Marcos M about 1 year ago

  • Status changed from Pull Request Review to Feedback
  • % Done changed from 0 to 100
Actions #10

Updated by Jonathan Lee about 1 year ago

Thank you for looking at this.

Actions #11

Updated by Marcos M about 1 year ago

  • Status changed from Feedback to Resolved
Actions #12

Updated by Jim Pingle 12 months ago

  • Category changed from Unknown to PHP Interpreter

Updating for release notes.

Actions

Also available in: Atom PDF