Hacker News new | past | comments | ask | show | jobs | submit login

There is actually work going into 2.4 that should further rectify the situation.

pfSenseHelpers.js (https://github.com/pfsense/pfsense/commit/f0136b178022cf3b04...) now contains code that intercepts clicks on anchor tags with the attribute "usepost" set. The target URL and the GET arguments are extracted from the event href attribute, and these are used to compose a new, temporary form with the previous arguments inserted as POST parameters.

Here is a simple example:

Before:

  <?php
  $id = $_GET['id'];
   
  if ($_POST) {
      if (!save_config($id)) {
          $input_errors[] = "Something broke";
      }
  }
  ?>
   
  <a type="button" href="system_something.php?id=<?=$id?>&user=<?=$pconfig['user']?> >
      <i class="fa fa-save></i>
      <?=gettext("Save")?>
  </a>

  After:
  <?php
  $id = $_POST['id'];    // $_GET was changed to $_POST
   
  if ($_POST['save']) {  // The generic if ($_POST) is now if ($_POST['save'] to detect when the form is being saved
      if (!save_config($id)) {
          $input_errors[] = "Something broke";
      }
  }
  ?>
   
  <!-- The "usepost" attribute is added to the anchor -->
  <a type="button" href="system_something.php?id=<?=$id?>&user=<?=$pconfig['user']?> usepost>
      <i class="fa fa-save></i>
      <?=gettext("Save")?>
  </a>
Most of the GUI is being converted to use these, and the result should be in pfSense software release 2.4.



Why isn't `$id` HTML-escaped before being echoed in an HTML attribute context? That's a potential XSS vector. Slightly more difficult to exploit when pulled from `$_POST`, but still a problem.


it's an example.


I don't mean to be combative, but when providing an example of how you're planning to mitigate vulnerabilities, it seems unwise to use code that contains an easily avoidable XSS vuln. I love pfSense and have used it for years, but this sort of thing doesn't inspire confidence.


Understood. It wasn't provided in the example, because it is not a user-entered item and is rarely displayed on the screen. It is typically an index. In other cases (perhaps) because people who originally wrote it knew no better.

(There is a lot if "isnumeric()" stuff used around $id. I inherited this codebase.)




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: