net/freeradius: quote password values literally so % is not expanded

[dotCooCoo]

Important notices

Before you submit a pull request, we ask you kindly to acknowledge the following:

• I have read the contributing guidelines at https://github.com/opnsense/plugins/blob/master/CONTRIBUTING.md
• I opened an issue first for non-trivial changes and linked it below.

---

Describe the problem

User.xml allows % in the password field, and the users template writes the value in double quotes. Inside a double-quoted string, FreeRADIUS expands % followed by one of c d e l m n t v C D G H I M S T Y, and it expands %{...}. FreeRADIUS then compares the password the client sent against the expanded string, and PAP reports Cleartext password does not match "known good" password. The generated file still shows the text that was typed. #5678 lists the result for each character. An unterminated %{ fails to expand at all. rlm_files logs Failed parsing expanded value for check item, skipping entry, drops the check items for that entry, and the user is rejected.

The two Tunnel-Password values at lines 28 and 124 are written without quotes. An unquoted value stops at the first parser token, and the tunnel_password masks allow several of them: ( ) { } = # and the operators == := += -= ++ !=. A tunnel password containing one of these produces Parse error (reply) ... Expected end of line or comma. rlm_files then fails to load, and the server does not start.

The password has been written in double quotes since the plugin's first commit in 2017.

---

Describe the proposed solution

The template now writes all three values in single quotes. FreeRADIUS does not expand single-quoted strings, so % and %{...} are taken literally, and a parser token no longer ends the value. The only characters that need escaping inside single quotes are \ and ', and none of the three masks allows either, so the template does not escape anything.

The masks are unchanged. Widening them would require escaping \ and ' inside the single quotes, and that is not part of this change.

clients.conf and mods-enabled-sql write their secrets in double quotes, and proxy.conf writes its secret without quotes. Those three are covered by #5696 and are not part of this change. Their model fields have no mask, so an admin may already have escaped the secret by hand, as #1655 advised.

Verification

I loaded about 500 generated users entries into FreeRADIUS 3.2.10, read each value back through a reply attribute, and authenticated with PAP using a client that sends the exact password bytes:

• every character the current masks allow comes back unchanged from a single-quoted value, including % $ { } ( ) # = : & + !
• in double quotes, FreeRADIUS expands the 17 single-letter codes and %{...} listed above, and an unterminated %{ gets the user rejected
• without quotes, the file fails to load on a space and on any of ( ) { } = # , ; < > == := += -= ++ != =~ =*

I rendered the template from master and the patched template against the same stubbed configuration and compared the output. The number of lines is the same, and only the intended lines differ.

On a live OPNsense 26.7.3_8 install with os-freeradius 1.10.2 and FreeRADIUS 3.2.10, I applied the single-quoted template. A 62-character password containing %c and $$ went from Access-Reject to Access-Accept, and a second user whose password was not affected could still log in.

---

Related issue

Closes #5678. I opened #5694 for the same problem before finding #5678 and closed it as a duplicate.

[fichtner]

FWIW: feels spurious to mention all characters are supported

[dotCooCoo]

I agree. I have removed the help text changes, and the help texts and masks are now the same as on master.

[fichtner]

I'm not really looking forward to reviewing if this is safe. Depending on concerns this may miss the bar for inclusion to not introduce other issues. In these cases a simpler fix may be the better approach.

[dotCooCoo]

I have removed the macro. All three masks reject \ and ', which are the only characters that need escaping inside single quotes, so the password and both Tunnel-Password values are now written as '{{ value }}' without any escaping. The details are in my comment on the pull request.

[dotCooCoo]

I have cut the pull request down to the quoting change. It now changes three lines, all in service/templates/OPNsense/Freeradius/users. The macro, the mask changes and the help text changes have been removed.

I tested each way of quoting against FreeRADIUS 3.2.10, the version on my 26.7.3_8 install. I loaded about 500 generated users entries, read each value back through a reply attribute, and authenticated with PAP using a client that sends the exact password bytes:

• Inside double quotes, FreeRADIUS expands % followed by one of c d e l m n t v C D G H I M S T Y, and it expands %{...}. It then compares the password the client sent against the expanded string, and PAP rejects the user. This is what net/freeradius: Some Cleartext-Password values containing % fail PAP authentication #5678 reports. An unterminated %{ fails to expand at all. rlm_files then drops the check items for that entry, and the user is rejected. Every other character the current password mask allows comes back unchanged.
• Both Tunnel-Password attributes are currently written without quotes. An unquoted value stops at the first parser token. Any of ( ) { } = # , ; < >, the operators == := += -= ++ != =~ =*, or a space produces Parse error (reply) ... Expected end of line or comma. rlm_files then fails to load, and the server does not start. The tunnel_password masks allow several of them: ( ) { } = # and the operators == := += -= ++ !=.
• FreeRADIUS does not expand single-quoted values, and the only characters that need escaping inside them are \ and '. None of the three masks allows either character, so the values can be written as '{{ value }}' without any escaping.

If the masks are widened later, only \ and ' need escaping inside the single quotes. I verified the escaping with the same test. If you want the masks widened, I can open a separate pull request for that.

My comment on #5678 is wrong about %7. FreeRADIUS leaves a digit after % unchanged; what fails to expand is an unterminated %{.