in certify.admin.inc it looks like the user could put the path to any valid executable in the form and have it execute. This is hard to avoid with this setup, but the following could mininize potential security issues:

function certify_admin_settings_validate($form, $form_state) {

  $certificate_path = $form_state['values']['certify_certificate_path'];
  if (!is_dir($certificate_path)) {
    form_set_error('certify_certificate_path', t('Please select an existing directory.'));
  }
  if (!is_writable($certificate_path)) {
    form_set_error('certify_certificate_path', t('The directory needs to be writable for the webserver.'));
  }

  $pdftkpath = $form_state['values']['certify_pdftk_path'];
  if (!file_exists($pdftkpath) || !is_executable($pdftkpath)) {
    form_set_error('certify_pdftk_path', t('Please enter full path of pdftk executable.'));
  }

+  if (strrpos($pdftkpath, 'pdftk') !== FALSE)  {
+   form_set_error('certify_pdftk_path', t('Executable path must have pdftk in it.'));
+  }
}

This readme might also suggest something like:

To not allow the path to pdftk executable to be editable, add:
$conf['certify_pdftk_path'] = '/usr/bin/pdftk';
To your settings file.

Comments

fuzzy76’s picture

fuzzy76 commented
Status: Needs work » Needs review

When creating that setting I pondered briefly on the security aspects, but reckoned that users with the "administer site settings" could do all kinds of havoc anyway, so I didn't think it was a serious issue. That being said, I can see that we might need some kind of security measure in place for some server setups.

The problem with the suggested approach (using strrpos) is that malicious input like "/home/hackeduser/maliciousexec ; /usr/bin/pdftk" will pass the check, while still making it possible to run arbitrary executables on the server. Even PHP's basename() will happily return "pdftk" in this situation.

Further complicating this is that people might have to call the pdftk something else (in order to override the *nix distributions default executable or perhaps to differentiate different compiled versions). Ofcourse this could be remedied by requiring the name of the executable to contain the string "pdftk".

What seems like an alternative approach is doing a realpath() on the given path, as this will fail for the semi-colon example I provided above. I do not know if there might be more potential pitfalls to look out for, but I guess some security is still better than none. So perhaps:

if (strrpos(basename(realpath($pdftkpath)),'pdftk') !== FALSE) {

Using realpath() also has the advantage of resolving symlinks so a user can't create a symlink for pdftk to a completely different executable.

As for the $conf method, I think it is a very good idea to mention.

I am not sure if the severity of this is strong enough to update certify 1.x, but it will definitely be included in 2.x. I will do some research on security best practices, and let you know.

fuzzy76’s picture

fuzzy76 commented
Status: Needs review » Needs work
fuzzy76’s picture

fuzzy76 commented
Assigned: Unassigned » fuzzy76
Status: Needs review » Needs work