pfSense-pkg-tftpd package improvements #262

doktornotor · 2017-01-20T23:25:31Z

Add option to bind to a single IP
Add option to force IPv4 only
Add option to specify the maximum permitted block size
Re-add backup/restore/upload/delete functionality
Remove useless custom_add_php_command
Remove unneeded includes
Use a PID file for the service
Remove useless filter_configure() call
Nuke the shipped startup script on install
Fix custom_php_{install,deinstall}_command

- Add option to bind to a single IP - Add option to force IPv4 only - Add option to specify the maximum permitted block size - Remove useless custom_add_php_command

- Add option to bind to a single IP - Add option to force IPv4 only - Add option to specify the maximum permitted block size - Remove unneeded includes - Use a PID file for the service - Remove useless filter_configure() call - Nuke the shipped startup scripts on install

jim-p

Looks good, but needs some security fixes. Should be simple to address though. Thanks!

jim-p · 2017-01-23T13:22:23Z

ftp/pfSense-pkg-tftpd/files/usr/local/pkg/tftpd.inc

 require_once("pfsense-utils.inc");
 require_once("service-utils.inc");
 require_once("util.inc");

+/* Helper function for files listing */
+function tftp_byte_convert($bytes) {


We have format_bytes() in util.inc, is this different in some way that's required by TFTP?

No idea, no clue that it existed.

jim-p · 2017-01-23T13:53:26Z

ftp/pfSense-pkg-tftpd/files/usr/local/pkg/tftpd.inc

+	global $backup_dir, $backup_path, $files_dir;
+
+	conf_mount_rw();
+	safe_mkdir("{$backup_dir}");


I'd prefer to see a test that $backup_dir contains something sane before using it in this way. Better as a function parameter than as a global. (Same with $files_dir)

backup_dir is hardcoded to /root/backup in tftp_files.php

jim-p · 2017-01-23T13:55:01Z

ftp/pfSense-pkg-tftpd/files/usr/local/pkg/tftpd.inc

+
+	conf_mount_rw();
+	safe_mkdir("{$backup_dir}");
+	if (mwexec("/usr/bin/tar -czC / -f {$backup_path} {$files_dir}") || !file_exists("{$backup_path}")) {


The variable insertions here need to be changed out so they use escapeshellarg() instead of using them directly. For example:
if (mwexec("/usr/bin/tar -czC / -f " . escapeshellarg($backup_path) . " " . escapeshellarg($files_dir)) || !file_exists("{$backup_path}")) {

jim-p · 2017-01-23T13:56:40Z

ftp/pfSense-pkg-tftpd/files/usr/local/pkg/tftpd.inc


 	write_rcfile(array(
 		"file" => "tftpd.sh",
-		"start" => "/usr/local/libexec/in.tftpd -l -s {$datadir}",
+		"start" => "/usr/local/libexec/in.tftpd -l -s {$datadir} {$address} -P {$pidfile} {$options}",


See above, re: escapeshellarg(), except for $options which should be OK already

jim-p · 2017-01-23T13:57:47Z

ftp/pfSense-pkg-tftpd/files/usr/local/pkg/tftpd.inc

+
+	// Max Block Size
+	if (!empty($tftpd_conf['tftpd_blocksize'])) {
+		$options .= " -B {$tftpd_conf['tftpd_blocksize']}";


See above, re: escapeshellarg()

jim-p · 2017-01-23T14:02:16Z

ftp/pfSense-pkg-tftpd/files/usr/local/www/tftp_files.php

+					<td><?=tftp_byte_convert(filesize("{$file}")); ?> </td>
+						<td>
+						<a name="tftpd_deleteX[]" id="tftpd_deleteX[]" type="button" title="<?=gettext('Delete this file');?>"
+							href='?type=tftp&amp;act=del&amp;filename=<?=$file;?>' style="cursor: pointer;" text="delete this">


Should use htmlspecialchars() on the filename in the URL

jim-p · 2017-01-23T14:02:23Z

ftp/pfSense-pkg-tftpd/files/usr/local/www/tftp_files.php

+							<i class="fa fa-trash" title="<?=gettext('Delete this file');?>"></i>
+						</a>
+						<a name="tftpd_dnloadX[]" id="tftpd_dnloadX[]" type="button" title="<?=gettext('Download this file');?>"
+							href='tftp_files.php?a=download&amp;filename=<?=$file;?>' style="cursor: pointer;">


Should use htmlspecialchars() on the filename in the URL

jim-p · 2017-01-23T14:03:05Z

ftp/pfSense-pkg-tftpd/files/usr/local/www/tftp_files.php

+			<i class="fa fa-download icon-embed-btn"></i>
+			<?=gettext('Download');?>
+		</a>
+		<a name="tftpd_backup" id="tftpd_backup" type="button" class="btn btn-success btn-sm" title="<?=sprintf(gettext('Backup all files to %s'), $backup_path);?>"


Should use htmlspecialchars() on the variable here.

jim-p · 2017-01-23T14:03:16Z

ftp/pfSense-pkg-tftpd/files/usr/local/www/tftp_files.php

+			<?=gettext('Backup');?>
+		</a>
+		<?php if (file_exists($backup_path)): ?>
+		<a name="tftpd_restore" id="tftpd_restore" type="button" class="btn btn-danger btn-sm" title="<?=sprintf(gettext('Restore all files from %s'), $backup_path);?>"


Should use htmlspecialchars() on the variable here.

jim-p · 2017-01-23T14:07:13Z

ftp/pfSense-pkg-tftpd/files/usr/local/pkg/tftpd.inc

+	}
+	$datadir = ($tftpd_conf['datadir'] ?: '/tftpboot');
+	// Will only get removed when empty
+	@rmdir("{$datadir}");


This definitely needs a sanity check to make sure the user didn't do something dumb like set it to /usr/local/www/ or /etc/. I see input validation prevents / but even so this may be too dangerous to rmdir. It's probably better to just leave the data in place.

As noted there, the only thing it removed are empty dirs. http://php.net/manual/en/function.rmdir.php

True but if it's empty, why bother?

Well to not leave lingering empty dir there (some people like to not leave cruft behind). Can remove it altogether of course, I have no preference here.

doktornotor · 2017-01-23T14:33:46Z

@jim-p - Most of the "variable" things you are requesting to escapeshellarg() are in fact hardcoded in the PHP. Most of the rest is treated by input validation. Whatever...

But before doing any other changes here - after looking at this again, I'd like to remove the datadir setting altogether and hardcode /tftpboot there. I cannot see any benefit here leaving it configurable, just adds a code bloat and potential issues with idiotic settings.

jim-p · 2017-01-23T14:42:50Z

That may be, but it's the best practice to avoid potential future changes causing a problem. I'd rather do it correctly now and make sure there is never a problem than have one creep in later. And it's really simple to do the extra encoding now so there's little reason to skip it.

doktornotor · 2017-01-23T14:44:40Z

OK. Any objections on hardcoding /tftpboot here?

jim-p · 2017-01-23T15:01:26Z

I can see how it's convenient to move it around, perhaps someone wants to put it under a specific user's home dir or whatnot, but those same issues could be solved via manual directory permissions and symlinks. I'd prefer to see it under /usr/local somewhere rather than in / but people are used to seeing it under /tftpboot so it may be too late to force that change if it can't be customized.

doktornotor · 2017-01-23T15:20:34Z

Well, this has been hardcoded to /tftpboot all the way up to 2.3 when the package went MIA - https://github.com/pfsense/pfsense-packages/blob/master/config/tftp2/tftp.inc#L60 - plus been the default with the only 2.3+ package version released so far.

Can do something like define('FILES_DIR', '/tftpboot'); and use that across the rest of code, so if someone really needs something else badly, then can change a single line in the package.

jim-p · 2017-01-23T15:31:12Z

That sounds good then.

- Hardcode /tftpboot for security reasons. If custom location is desired, it can be changed by patching the FILES_DIR here. Notify users on upgrade in case they need to relocate their TFTP server files. Remove no longer needed input validation. - Remove pointless tftp_byte_convert() and use format_bytes() from util.inc - Use define() and constants instead of variables for hardcoded locations - Move code for restoring a backup here. - Move code to check whether users are trying to download/delete files that are out of the TFTP directory here. - Add an escapeshellarg() for block size.

…security reasons.

- The hardcoded locations are now defined() in tftp.inc - Use format_bytes() from util.inc - Fix the logic for full backup download and rename the button accordingly - Code to code whether users are trying to download/delete files that are out of the TFTP directory here was moved to a function in tftp.inc - Added bunch of htmlspecialchars() protections as requested

doktornotor · 2017-01-23T18:10:53Z

@jim-p - Revamped and cleaned up the code quite a bit again, please check again. Thanks for feedback.

jim-p

Looks good to me now

Changelog [1]: New APIs * QWebSetting::ErrorPageEnabled - true by default, allows to disable built-in error pages if they are not desirable Improvements * CSS Compositing is now supported * Push is enabled by default for HTTP/2 requests * QtWebKit does not require Python 2 anymore for building and can use Python 3 instead * QtWebKit won't be eager to pick bitmap fonts as a suitable choice for standard font families * More appropriiate hinting option for web fonts is used Bug fixes * Fix memory leak in CustomEvent * SVG fragment identifier is not respected if it is a part of an HTTP URL (#818) * Context menu actions for images in Resources tab of inspector use blob: instead of original URL (#899) * Always enable JavaScript for QWebInspector (#566) * Inspector UI: Only selected item is painted in Styles combobox (#262) * Inspector should inspect itself in DEVELOPER_MODE only (#444) * fast/gradients/css3-color-stop-invalid.html - box is green instead of white (#230) * Work around QTBUG-77308 when using Qt < 5.14 * Fix compilation errors with ICU 65.1 and Qt 5.14 * Fix compilation when QPdf is disabled in Qt * Fix compilation with MinGW for x86_64 target [1] https://github.com/qtwebkit/qtwebkit/releases

Patch release with miscellaneous bug/doc/build fixes. Excerpt from release tag: [#269] fix memory leak in V3fArrayFromBuffer [#268] Add <cstdint> for int64_t [#263] Initialize x in testRoots.cpp:solve() to suppress compiler warning [#262] Fix gcc compiler warning in testFun.cpp [#261] Test return value of extractSHRT to avoid uninitialized reference [#260] Fix example code so it compiles as is [#259] Cuda safety in several headers [#256] Fix markdown and typos in README.md [#255] Do not warn if half.h has already being included [#248] Update sphinx version ChangeLog: https://github.com/AcademySoftwareFoundation/Imath/releases/tag/v3.1.6 MFH: 2022Q4

ChangeLog: https://www.nlnetlabs.nl/news/2023/Jun/07/nsd-4.7.0-released/ 4.7.0 ================ FEATURES: - Merge #263: Add bash autocompletion script for nsd-control. - Fix #267: Allow unencrypted local operation of nsd-control. - Merge #269 from Fale: Add systemd service unit. - Fix #271: DNSTAP over TCP, with dnstap-ip: "127.0.0.1@3333". - dnstap over TLS, default enabled. Configured with the options dnstap-tls, dnstap-tls-server-name, dnstap-tls-cert-bundle, dnstap-tls-client-key-file and dnstap-tls-client-cert-file. BUG FIXES: - Fix #239: -Wincompatible-pointer-types warning in remote.c. - Fix configure for -Wstrict-prototypes. - Fix #262: Zone(s) not synchronizing properly via TLS. - Fix for #262: More error logging for SSL read failures for zone transfers. - Merge #265: Fix C99 compatibility issue. - Fix #266: Fix build with --without-ssl. - Fix for #267: neater variable definitions. - Fix #270: reserved identifier violation. - Fix to clean more memory on exit of dnstap collector. - Fix dnstap to not check socket path when using IP address. - Fix to compile without ssl with dnstap-tls code. - Dnstap tls code fixes. - Fix include brackets for ssl.h include statements, instead of quotes. - Fix static analyzer warning about nsd_event_method initialization. - Fix #273: Large TXT record breaks AXFR. - Fix ixfr create from adding too many record types. - Fix cirrus script for submit to coverity scan to libtoolize the configure script components config.guess and config.sub. - Fix readme status badge links. - make depend. - Fix for build to run flex and bison before compiling code that needs the headers. - Fix to remove unused whitespace from acx_nlnetlabs.m4 and config.h. - For #279: Note that autoreconf -fi creates the configure script and also the needed auxiliary files, for autoconf 2.69 and 2.71. - Fix unused variable warning in unit test, from clang compile. - Fix #240: Prefix messages originating from verifier. - Fix #275: Drop unnecessary root server checks. PR: 272096 Reported by: jaap@NLnetLabs.nl (maintainer)

doktornotor added 3 commits January 21, 2017 00:25

pfSense-pkg-tftpd package improvements

12330f3

- Add option to bind to a single IP - Add option to force IPv4 only - Add option to specify the maximum permitted block size - Remove useless custom_add_php_command

Bump port version

9859ff4

netgate-git-updates added the CLA label Jan 20, 2017

doktornotor added 11 commits January 21, 2017 04:11

Change priv name to match the menu section

bc48698

Re-add backup/restore/upload/delete functionality, fix install/deinstall

57a8304

Re-add backup/restore/upload/delete functionality

589160c

Re-add backup/restore/upload/delete functionality

8ca4fd3

Re-add backup/restore/upload/delete functionality

97b210f

Re-add backup/restore/upload/delete functionality

d88217a

Re-add backup/restore/upload/delete functionality

74ae3e1

Rename the menu entry

92edca3

Whitespace fixes

8b98d75

HTML fixes

3c950c1

Do not allow "/" to be used as files directory

af4dac7

jim-p requested changes Jan 23, 2017

View reviewed changes

doktornotor added 3 commits January 23, 2017 19:00

Remove 'Directory for Files' setting; /tftpboot is now hardcoded for …

fe2af9c

…security reasons.

Remove commented out code leftovers; moved to tftp.inc

ea93c1e

jim-p approved these changes Jan 23, 2017

View reviewed changes

jim-p requested a review from rbgarga January 23, 2017 18:27

netgate-git-updates merged commit ea93c1e into pfsense:devel Jan 23, 2017

netgate-git-updates pushed a commit that referenced this pull request Jan 23, 2017

Merge pull request #262 from doktornotor/patch-1

496a854

doktornotor deleted the patch-1 branch January 23, 2017 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pfSense-pkg-tftpd package improvements #262

pfSense-pkg-tftpd package improvements #262

doktornotor commented Jan 20, 2017 •

edited

jim-p left a comment

jim-p Jan 23, 2017

doktornotor Jan 23, 2017

jim-p Jan 23, 2017

doktornotor Jan 23, 2017 •

edited

jim-p Jan 23, 2017

jim-p Jan 23, 2017

jim-p Jan 23, 2017

jim-p Jan 23, 2017

jim-p Jan 23, 2017

jim-p Jan 23, 2017

jim-p Jan 23, 2017

jim-p Jan 23, 2017

doktornotor Jan 23, 2017

jim-p Jan 23, 2017

doktornotor Jan 23, 2017

doktornotor commented Jan 23, 2017 •

edited

jim-p commented Jan 23, 2017

doktornotor commented Jan 23, 2017

jim-p commented Jan 23, 2017

doktornotor commented Jan 23, 2017

jim-p commented Jan 23, 2017

doktornotor commented Jan 23, 2017

jim-p left a comment

pfSense-pkg-tftpd package improvements #262

pfSense-pkg-tftpd package improvements #262

Conversation

doktornotor commented Jan 20, 2017 • edited

jim-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doktornotor Jan 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doktornotor commented Jan 23, 2017 • edited

jim-p commented Jan 23, 2017

doktornotor commented Jan 23, 2017

jim-p commented Jan 23, 2017

doktornotor commented Jan 23, 2017

jim-p commented Jan 23, 2017

doktornotor commented Jan 23, 2017

jim-p left a comment

Choose a reason for hiding this comment

doktornotor commented Jan 20, 2017 •

edited

doktornotor Jan 23, 2017 •

edited

doktornotor commented Jan 23, 2017 •

edited