opnsense / opnsense/plugins

freeradius: shared secrets and the SQL password are rendered without safe quoting, and proxy.conf leaves the secret unquoted entirely

Open
#5,696 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
PHP
Stars
1.2k
Forks
863
Avg merge
2d 6h
Merged PRs (30d)
10

Description

Important notices
Before you add a new report, we ask you kindly to acknowledge the following:

Describe the bug

Depending on the file, a secret containing a space, a comma, #, ", \, ${ or $ENV{ either stops FreeRADIUS from starting or is stored truncated or altered without any error. The GUI accepts the value either way. Clicking Apply restarts the service. A secret the parser rejects therefore stops the running server until the value is corrected, and a secret that is stored truncated or altered leaves the server running with a secret different from the one entered.

Three templates write a secret in double quotes or with no quotes at all, and none of the three model fields has a Mask, so the stored value can contain any character:

Template Line Rendering Model field
clients.conf 7 secret = "{{ client_list.secret }}" Client.xml secret, no mask
proxy.conf 24 secret = {{ homeserver_list.secret }} Proxy.xml secret, no mask
mods-enabled-sql 46 password = "{{ ...mysqlpassword }}" General.xml mysqlpassword, no mask

I checked what FreeRADIUS 3.2.10 does with each rendering by writing test secrets into clients.conf and proxy.conf and running the server's configuration check. For the clients.conf secrets I then authenticated with the raw secret; the proxy.conf values were checked with the configuration check alone. These files are read by the configuration parser, so the rules differ from those for the users file in #5678, and % is not expanded in any of the three.

In a double-quoted value, which is how clients.conf and mods-enabled-sql write theirs:

  • The server replaces ${name} when it reads the file. A name that exists, such as ${confdir}, is substituted without any error. A name that does not exist fails with Reference "${name}" not found, and a ${ with no closing brace fails with Variable expansion missing }. In both cases the server does not start. \$ does not suppress the expansion, but the octal escape \044 for $ does, because the parser processes escapes after the expansion.
  • The server replaces $ENV{NAME} with the environment variable, or with an empty string when it is unset.
  • Backslash sequences are rewritten: \" becomes ", \\ becomes \, \n becomes a newline, \NNN is read as octal, and the backslash is dropped from any other \x. A value ending in a backslash fails with Parse error: Unterminated string, and the server does not start.
  • A " inside the value ends it. If the rest of the line, after any spaces, starts with #, it is a comment: with "AA"#AA" the server starts with AA as the secret. A comma there starts another item and also leaves the secret truncated. Anything else after the closing quote is a syntax error.

In an unquoted value, which is how proxy.conf writes its home server secret:

  • The value ends at the first space. With a secret of two words the server does not start, and the log shows proxy.conf[N]: Syntax error: Expected comma after 'two': words. The message includes the parsed value and the rest of the line, so the whole secret appears in the log.
  • A space followed by # starts a comment, so abc #def is stored as abc without any error. A # as the first character leaves the secret unset, and the server does not start: No shared secret defined for home server. A # inside the word is kept.
  • A comma ends the value, and the parser reads the rest as a further configuration item. abc,def is stored as abc without any error, because def parses as a bare name. The rest is a syntax error when it reaches a parser token such as ( or ;, which is the #1655 failure quoted below.
  • ${name} and $ENV{NAME} are replaced exactly as in a double-quoted value. Octal escapes are not processed in an unquoted value.
  • Backslashes are kept as written. A backslash at the end of the value joins the next line to it, and the joined line is a syntax error.

mods-enabled-ldap line 15 writes the LDAP bind password in single quotes without escaping, and Ldap.xml password has no mask either, so a ' in that password also stops the server from starting. It is outside this report, but the same fix applies to it.

clients.conf wrote the secret unquoted until #2511 added the double quotes in 2021, fixing #2493. The proxy.conf and mods-enabled-sql renderings shown in the table are unchanged since those fields were added.

To Reproduce

  1. Go to Services > FreeRADIUS > Proxy, open the Home Servers tab, and add a home server.
  2. Set its secret to a value containing a space, for example two words.
  3. Apply, then inspect the generated file: grep secret /usr/local/etc/raddb/proxy.conf
  4. The line reads secret = two words. FreeRADIUS 3.2.10 rejects that line with Syntax error: Expected comma after 'two': words, so the service does not start.

For the ${ case, set a client secret under Services > FreeRADIUS > Clients to a value that contains ${ followed by a name that is not defined, or with no closing brace, and apply. The service does not start, and the log shows Reference "${...}" not found or Variable expansion missing }. With a name that is defined, such as ${confdir}, the service starts with the substituted value instead.

Expected behavior

The generated configuration contains the secret exactly as entered, or the field rejects any value the file cannot represent.

Relevant log files

#1655 records this fault in clients.conf from the time when that template still wrote the secret unquoted. The value ended at a comma in the secret, and the parser read the characters after it as the next configuration item:

/usr/local/etc/raddb/clients.conf[3]: Parse error after "~": unexpected token "("
Errors reading or parsing /usr/local/etc/raddb/radiusd.conf

The parser checks above ran against a stock FreeRADIUS 3.2.10 install outside OPNsense. 3.2.10 is also the FreeRADIUS version on my OPNsense 26.7.3_8 box, but the paths and line numbers below come from the stock install. Each line comes from a separate run. Every line except No shared secret defined for home server localhost was followed by Errors reading or parsing /etc/freeradius/radiusd.conf:

/etc/freeradius/proxy.conf[256]: Syntax error: Expected comma after 'two': words
/etc/freeradius/proxy.conf[163]: No shared secret defined for home server localhost
/etc/freeradius/clients.conf[3]: Reference "${nope}" not found
/etc/freeradius/clients.conf[3]: Variable expansion missing }

Additional context

Two decisions are involved, and the second one is why I am reporting this rather than opening a pull request.

  1. The first decision is which quoting to use. Double quotes can represent every value: a backslash as \\, a quote as \", and a dollar sign as \044, which is not expanded because the parser expands ${} and $ENV{} before it processes escapes. Single quotes cannot. They stop the expansion and treat only \' specially, but a value ending in a backslash cannot be written in them, and neither can a backslash followed by an apostrophe. For the trailing backslash, the parser reads the backslash and the closing quote together as an escaped quote, finds no closing quote, and fails with Parse error: Unterminated string. Writing the backslash as \\ stores two backslashes, because single quotes keep \\ as two characters. So clients.conf and mods-enabled-sql can keep their double quotes, proxy.conf can add them, and all three can escape \, " and $ in the value. A mask is then optional.

  2. The second decision is what to do about secrets that admins escaped by hand. #1655 was closed with the guidance that admins escape secrets themselves, and with the reason that validation could not be added without breaking setups whose secrets are already escaped. Since these fields have no mask, an admin may have stored an escaped value. For the two double-quoted templates the trade-off is:

    Stored value Today After the template escapes the value
    foo"bar server does not start starts, secret read as foo"bar
    foo\"bar, standing for foo"bar starts, secret read as foo"bar starts, secret read as foo\"bar

    proxy.conf is different. In the unquoted rendering, the server starts with foo"bar as the secret today, and the parser keeps foo\"bar as written, so escaping would not alter how either is read. A secret stored with its own surrounding double quotes, which the unquoted rendering accepts today, would be read with the quotes as part of the secret, where today the parser strips them. #2511 already made a one-time change of this kind for clients.conf in 2021 without a migration.

    The options are: leave the templates as they are; add the escaping and accept that secrets escaped by hand under the #1655 guidance are read differently until the admin re-enters them; or add the escaping together with a migration that unescapes the stored values. That is a decision for the maintainers rather than something to settle in a pull request. I can open one once it is made.

#5678 reports a similar fault in the users template, and #5695 fixes it. That case has no compatibility constraint. The three masks there, password, tunnel_password and fallback_tunnel_password, never permitted \ or ", so no admin has an escaped value stored there. #5695 does not change these three templates because the second decision is still open.

Environment

OPNsense 26.7.3_8 (amd64), FreeBSD 15.1-RELEASE-p3
os-freeradius 1.10.2, FreeRADIUS 3.2.10

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the clients.conf, proxy.conf, and mods-enabled-sql template references, then inspect the Client.xml, Proxy.xml, and General.xml fields named in the report. Run FreeRADIUS configuration checks with representative secrets and review the compatibility history in #1655 and #2511. Done requires a maintainer decision on escaping, existing stored values, and whether migration is needed.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
authentication, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.