Bug #13105
closed
DNS Forwarder custom options may fail after save/restore when options are only separated by newline
0%
Description
Sometimes when saving DNS Forwarder (dnsmasq) config, the custom options data gets mangled (a newline is lost, so 2 config options are munged together). This causes dnsmasq to fail to start, and basically everything is broken until you figure out what's wrong and manually fix it. Rebooting the firewall does not fix it, because the data is actually saved incorrectly in the config.xml.
This seems to happen occasionally during config backup/restore, or sometimes when just making normal config changes. It has happened to me (rarely) for years, so definitely not a new bug. But it's a doozy to track down. I looked at the code in /etc/inc/services.inc as well as /usr/local/pfSense/include/www/services_dnsmasq.inc and it appears fine. I can't find a reliable way to reproduce this yet, but I wanted to log this here in case others are experiencing it (which I imagine they are) so more info can be collected. Not even ruling out a possible browser bug (maybe the formdata is getting mangled somehow during submit or a plugin is interfering?)
an example:
config as entered:
localise-queries clear-on-reload no-negcache local-ttl=1800 dhcp-ttl=0
after save / corruption:
localise-queries clear-on-reloadno-negcache local-ttl=1800 dhcp-ttl=0
another report from r/PFSENSE:
https://www.reddit.com/r/PFSENSE/comments/jeyxxn/very_occasional_config_reload_bug_in_dnsforwarder/
The fix is just to examine the config and find the invalid line and re-insert the newline, re-save and restart dnsmasq.
Updated by → luckman212 almost 2 years ago
(I recently experienced this on 22.05 snaps, btw)
Updated by Jim Pingle almost 2 years ago
The inconsistent handling of newlines in text boxes in browsers is one of the reasons the OpenVPN advanced options insists on using semicolons as a line separator. Historically this has been inconsistent at best, varying by OS newline style (e.g. UNIX vs DOS/Windows line endings) and browser vendor.
This may be browser-dependent and not something we can easily solve, but either way we need a reliable way to reproduce the problem before anything can be done.
Alternately, consider separating the options by spaces and not newlines. Both should work in that box.
Updated by Jim Pingle almost 2 years ago
- Subject changed from dnsmasq custom_options gets corrupt sometimes during save/config restore (newlines are lost resulting in 2 lines getting melded together) to DNS Forwarder custom options may fail after save/restore when options are only separated by line
Updated by → luckman212 almost 2 years ago
2 other possible workarounds:
- have each custom option in its own row, with an "add row" button UI similar to defining aliases
- before starting dnsmasq, validate the config and if it fails to validate, start it without the custom options and dispatch an alert. At least this way when the bug happens, you don't completely take down the network due to lack of DNS, and will have an easier time remediating
If you like either of these ideas I can try to work up a PR
Updated by Jim Pingle almost 2 years ago
→ luckman212 wrote in #note-4:
2 other possible workarounds:
- have each custom option in its own row, with an "add row" button UI similar to defining aliases
That would add too much complexity for this, IMO.
- before starting dnsmasq, validate the config and if it fails to validate, start it without the custom options and dispatch an alert. At least this way when the bug happens, you don't completely take down the network due to lack of DNS, and will have an easier time remediating
It already does this: https://github.com/pfsense/pfsense/blob/master/src/usr/local/pfSense/include/www/services_dnsmasq.inc#L208
Updated by → luckman212 almost 2 years ago
Jim Pingle As far as I can tell from looking at the code (and my experience as well) it only validates on SAVE, but not when trying to start or restart the service. So, if the data gets corrupted in the config.xml via whatever other mechanism that's causing this bug (e.g. after a reboot or config restore), then dnsmasq will silently fail and you just lose DNS completely.
Updated by Jim Pingle almost 2 years ago
A reboot or restore couldn't "corrupt" this. A reboot doesn't alter the configuration. It could only change on save.
Only way a restore could do it is if the user edited the config.xml and maybe messed up the line endings that way.
The way this option stores the text it does so plainly in the config.xml, so it has newlines in place where they are unreliable.
A better fix for all this may be to wrap it in base64 encoding before save and then decode before use, as it done with lots of other similar fields. It would prevent the newlines bare in the config from being a problem.
Updated by → luckman212 almost 2 years ago
Oh great idea! Only downside is losing the ability to see the data when directly viewing the XML, but that's a very minor tradeoff. To preserve backward compatibility there would need to be some upgrade functions and the data would need to be tested to see if it was valid base64 and fall back to plaintext interpretation if not. I haven't looked yet but it sounds like this method is already in use elsewhere on other pages. Do you want me to try at a PR?
Updated by Jim Pingle almost 2 years ago
→ luckman212 wrote in #note-8:
Oh great idea! Only downside is losing the ability to see the data when directly viewing the XML, but that's a very minor tradeoff.
Lots of editors have built-in Base64 encode/decode functions or plugins which makes that less of an issue. A couple extra clicks if it's needed.
To preserve backward compatibility there would need to be some upgrade functions and the data would need to be tested to see if it was valid base64 and fall back to plaintext interpretation if not. I haven't looked yet but it sounds like this method is already in use elsewhere on other pages. Do you want me to try at a PR?
It would need an upgrade code function to change it over to base64 during upgrade, no need to do any kind of conditional test after since it's safe to assume it's base64 in the config at that point. You can try a PR if you'd like. See 86584ded30c27b9ad1b017fb743399dc01180f02 for a similar change in the past.
Updated by → luckman212 almost 2 years ago
Jim Pingle I submitted a PR: https://github.com/pfsense/pfsense/pull/4579
Updated by Jim Pingle almost 2 years ago
- Status changed from New to Pull Request Review
- Target version set to 2.7.0
- Plus Target Version set to 22.05
Updated by Viktor Gurov almost 2 years ago
- Status changed from Pull Request Review to Feedback
PR merged, thanks!
Updated by Jim Pingle almost 2 years ago
- Subject changed from DNS Forwarder custom options may fail after save/restore when options are only separated by line to DNS Forwarder custom options may fail after save/restore when options are only separated by newline
Updated by Danilo Zrenjanin almost 2 years ago
- Status changed from Feedback to Resolved
Tested backing up a config file with (from the system running 2.7.0.a.20220520.0600) the following custom options in dnsmasq:
local=/int.jpmvrealtime.com/ selfmx log-queries=extra
in the backup.xml the custom options were encoded in base64 format.
<dnsmasq>
<enable></enable>
<interface>wan,lan</interface>
<custom_options>bG9jYWw9L2ludC5qcG12cmVhbHRpbWUuY29tLyAKc2VsZm14IApsb2ctcXVlcmllcz1leHRyYQ==</custom_options>
<strict_order></strict_order>
</dnsmasq>
Then converted config file for restoring to SG-1100 running 22.05-BETA version. After the restore process, the custom_options were correctly formatted. This was not the case before the encoding in base64 was implemented.
I am marking this ticket resolved.