Re: pf.conf macro with space

[Stuart Henderson]

On 2016/06/21 08:57, sven falempin wrote:
> A parsing tool is not like hacking into an advanced kernel
> feature with unexpected side effect

That is incorrect.

I'm not implying anything about diffs proposed in this thread, but this
parser *is* sensitive, it's more complex than you think, and incorrect
changes made here certainly could have an unexpected side effect:
people getting access to systems they shouldn't.

Re: pf.conf macro with space

[Stefan Sperling]

On Tue, Jun 21, 2016 at 08:57:44AM -0400, sven falempin wrote:
> I have explain the use of spaced macro,
> a config file that is self explanatory.

I suggest you use underscores instead of spaces in macro names.

Re: pf.conf macro with space

[Mike Belopuhov]

On 21 June 2016 at 14:57, Sebastian Benoit  wrote:
> Henning Brauer(hb-openbsdt...@ml.bsws.de) on 2016.06.21 13:11:16 +0200:
>> * Stefan Sperling  [2016-06-21 11:15]:
>> > Generally, I would appreciate more detailed error messages from the pf.conf
>> > parser. I recall several occasions where pfctl threw "syntax error" and 
>> > more
>> > specific error reporting would have saved me some time with finding the
>> > silly mistake I made. And in this case the ruleset loads successfully even
>> > though, while parsing, we already know it's not going to work as 
>> > intended...
>>
>> true, that's shared by all yacc-style parsers, if the grammar doesn't
>> match you just get syntax error without much of a hint what's wrong.
>
> the quality of the error message depends both on the language (some make
> it harder to error out early) and on the parser.
>
> fail early is better.
>
>> however, the ruleset in this case does NOT load.
>>
>>   $ echo '"a macro with spaces"="foo"\npass from $a\ macro\ 
>> with\ spaces"' | pfctl -nvf -
>> a macro with spaces = "foo"
>> stdin:2: macro 'a' not defined
>> stdin:2: syntax error
>>
>
> sure, thats what the op reported
>
>>
>> > Only as long as it doesn't make the parser code overly complex, of course.
>> > But currently the balance is tilted too much towards terse error messages
>> > for my taste. So I liked benno's first diff.
>>
>> it's just a tiny check indeed, which swings the "cost" (not in
>> financial terms) vs benefit pendulum towards the benefit side, yes.
>
> thx.
>
> here is a diff for all daemons.
>
> ok for this?
>
>

Looks good to me. OK mikeb FWIW

Re: pf.conf macro with space

[Otto Moerbeek]

On Tue, Jun 21, 2016 at 01:11:16PM +0200, Henning Brauer wrote:

> * Stefan Sperling  [2016-06-21 11:15]:
> > Generally, I would appreciate more detailed error messages from the pf.conf
> > parser. I recall several occasions where pfctl threw "syntax error" and more
> > specific error reporting would have saved me some time with finding the
> > silly mistake I made. And in this case the ruleset loads successfully even
> > though, while parsing, we already know it's not going to work as intended...
> 
> true, that's shared by all yacc-style parsers, if the grammar doesn't
> match you just get syntax error without much of a hint what's wrong.

The reason pfctl is not giving much details about the error is also
determined by the way it is written.

Take for example bc(1), also using a yacc parser.
$ bc
1=a
bc: stdin:1: syntax error: = unexpected
a = b c
bc: stdin:2: syntax error: c unexpected
a =&9
bc: stdin:3: illegal character: & unexpected

Which is more helpfull than what pfctl produces in similar cases.

-Otto

> 
> however, the ruleset in this case does NOT load.
> 
>   $ echo '"a macro with spaces"="foo"\npass from $a\ macro\ 
> with\ spaces"' | pfctl -nvf - 
> a macro with spaces = "foo"
> stdin:2: macro 'a' not defined
> stdin:2: syntax error
> 
> > Only as long as it doesn't make the parser code overly complex, of course.
> > But currently the balance is tilted too much towards terse error messages
> > for my taste. So I liked benno's first diff.
> 
> it's just a tiny check indeed, which swings the "cost" (not in
> financial terms) vs benefit pendulum towards the benefit side, yes.
> 
> -- 
> Henning Brauer, h...@bsws.de, henn...@openbsd.org
> BS Web Services GmbH, http://bsws.de, Full-Service ISP
> Secure Hosting, Mail and DNS. Virtual & Dedicated Servers, Root to Fully 
> Managed
> Henning Brauer Consulting, http://henningbrauer.com/

Re: pf.conf macro with space

[sven falempin]

On Tue, Jun 21, 2016 at 8:30 AM, Stefan Sperling  wrote:

> On Tue, Jun 21, 2016 at 01:11:16PM +0200, Henning Brauer wrote:
> > however, the ruleset in this case does NOT load.
> >   $ echo '"a macro with spaces"="foo"\npass from $a\
> macro\ with\ spaces"' | pfctl -nvf -
> > a macro with spaces = "foo"
> > stdin:2: macro 'a' not defined
> > stdin:2: syntax error
>
> Ah, good. I tested with just a macro definition, but not use.
>
>
I have explain the use of spaced macro,
a config file that is self explanatory.

I've been around a while, and i saw a lot of effort
making configuration file clean and meaningful.
A recent example is the doas.conf file.

A parsing tool is not like hacking into an advanced kernel
feature with unexpected side effect, so the feature of fully
handling macro is
 * not an hassle
 * a plus, as you can define element in a more clear way
 * not a risk


BTW,
There s better way to change the parse.y to kill the space,
not using STRING.
It s in the samples of the lexer, for c code.

I hope someone will help the case for spaces in macro.

Cheers.

-- 
-
() ascii ribbon campaign - against html e-mail
/\

Re: pf.conf macro with space

[Sebastian Benoit]

Henning Brauer(hb-openbsdt...@ml.bsws.de) on 2016.06.21 13:11:16 +0200:
> * Stefan Sperling  [2016-06-21 11:15]:
> > Generally, I would appreciate more detailed error messages from the pf.conf
> > parser. I recall several occasions where pfctl threw "syntax error" and more
> > specific error reporting would have saved me some time with finding the
> > silly mistake I made. And in this case the ruleset loads successfully even
> > though, while parsing, we already know it's not going to work as intended...
> 
> true, that's shared by all yacc-style parsers, if the grammar doesn't
> match you just get syntax error without much of a hint what's wrong.

the quality of the error message depends both on the language (some make
it harder to error out early) and on the parser.

fail early is better.

> however, the ruleset in this case does NOT load.
> 
>   $ echo '"a macro with spaces"="foo"\npass from $a\ macro\ 
> with\ spaces"' | pfctl -nvf - 
> a macro with spaces = "foo"
> stdin:2: macro 'a' not defined
> stdin:2: syntax error
>

sure, thats what the op reported

>
> > Only as long as it doesn't make the parser code overly complex, of course.
> > But currently the balance is tilted too much towards terse error messages
> > for my taste. So I liked benno's first diff.
> 
> it's just a tiny check indeed, which swings the "cost" (not in
> financial terms) vs benefit pendulum towards the benefit side, yes.

thx.

here is a diff for all daemons.

ok for this?


diff --git sbin/iked/parse.y sbin/iked/parse.y
index 958e51a..6455a00 100644
--- sbin/iked/parse.y
+++ sbin/iked/parse.y
@@ -73,6 +73,7 @@ intlookup(char *);
 int lgetc(int);
 int lungetc(int);
 int findeol(void);
+int has_whitespace(const char *);
 
 TAILQ_HEAD(symhead, sym)symhead = TAILQ_HEAD_INITIALIZER(symhead);
 struct sym {
@@ -1006,6 +1007,10 @@ string   : string STRING
 varset : STRING '=' string
{
log_debug("%s = \"%s\"\n", $1, $3);
+   if (has_whitespace($1)) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable");
free($1);
@@ -1234,6 +1239,16 @@ findeol(void)
 }
 
 int
+has_whitespace(const char *s) {
+   while (*s != '\0') {
+   if (isspace((unsigned char)*s))
+   return 1;
+   s++;
+   }
+   return 0;
+}
+
+int
 yylex(void)
 {
unsigned charbuf[8096];
diff --git sbin/ipsecctl/parse.y sbin/ipsecctl/parse.y
index fe9cee0..67fbd72 100644
--- sbin/ipsecctl/parse.y
+++ sbin/ipsecctl/parse.y
@@ -71,6 +71,7 @@ intlookup(char *);
 int lgetc(int);
 int lungetc(int);
 int findeol(void);
+int has_whitespace(const char *);
 
 TAILQ_HEAD(symhead, sym)symhead = TAILQ_HEAD_INITIALIZER(symhead);
 struct sym {
@@ -883,6 +884,10 @@ varset : STRING '=' string
{
if (ipsec->opts & IPSECCTL_OPT_VERBOSE)
printf("%s = \"%s\"\n", $1, $3);
+   if (has_whitespace($1)) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable");
free($1);
@@ -1092,6 +1097,16 @@ findeol(void)
 }
 
 int
+has_whitespace(const char *s) {
+   while (*s != '\0') {
+   if (isspace((unsigned char)*s))
+   return 1;
+   s++;
+   }
+   return 0;
+}
+
+int
 yylex(void)
 {
u_char   buf[8096];
diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index 934438c..9846063 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -85,6 +85,7 @@ intlookup(char *);
 int lgetc(int);
 int lungetc(int);
 int findeol(void);
+int has_whitespace(const char *);
 
 TAILQ_HEAD(symhead, sym)symhead = TAILQ_HEAD_INITIALIZER(symhead);
 struct sym {
@@ -714,6 +715,10 @@ numberstring   : NUMBER
{
 varset : STRING '=' varstring  {
if (pf->opts & PF_OPT_VERBOSE)
printf("%s = \"%s\"\n", $1, $3);
+   if (has_whitespace($1)) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable %s", $1);

Re: pf.conf macro with space

[Stefan Sperling]

On Tue, Jun 21, 2016 at 01:11:16PM +0200, Henning Brauer wrote:
> however, the ruleset in this case does NOT load.
>   $ echo '"a macro with spaces"="foo"\npass from $a\ macro\ 
> with\ spaces"' | pfctl -nvf - 
> a macro with spaces = "foo"
> stdin:2: macro 'a' not defined
> stdin:2: syntax error

Ah, good. I tested with just a macro definition, but not use.

Re: pf.conf macro with space

[Stefan Sperling]

On Tue, Jun 21, 2016 at 10:37:45AM +0200, Henning Brauer wrote:
> * Sebastian Benoit  [2016-06-21 10:15]:
> > same thing without a stupid helper function, pointed out by henning.
> 
> I'm actually not quite sure we need or want this. From my PoV, making
> the tools too much of a nanny is against unix spirit. Macros with
> spaces don't actually cause harm, they just don't work. Not too
> unexpected apparently given that, afair at least, nobody spoke up on it
> in more than a decade.
> 
> So, do we really want this extra check? I'm unsure.
> If not, short mention in the manpage or just leave things as they are?

Generally, I would appreciate more detailed error messages from the pf.conf
parser. I recall several occasions where pfctl threw "syntax error" and more
specific error reporting would have saved me some time with finding the
silly mistake I made. And in this case the ruleset loads successfully even
though, while parsing, we already know it's not going to work as intended...

Only as long as it doesn't make the parser code overly complex, of course.
But currently the balance is tilted too much towards terse error messages
for my taste. So I liked benno's first diff.

Re: pf.conf macro with space

[Henning Brauer]

* Sebastian Benoit  [2016-06-21 10:15]:
> same thing without a stupid helper function, pointed out by henning.

I'm actually not quite sure we need or want this. From my PoV, making
the tools too much of a nanny is against unix spirit. Macros with
spaces don't actually cause harm, they just don't work. Not too
unexpected apparently given that, afair at least, nobody spoke up on it
in more than a decade.

So, do we really want this extra check? I'm unsure.
If not, short mention in the manpage or just leave things as they are?

-- 
Henning Brauer, h...@bsws.de, henn...@openbsd.org
BS Web Services GmbH, http://bsws.de, Full-Service ISP
Secure Hosting, Mail and DNS. Virtual & Dedicated Servers, Root to Fully Managed
Henning Brauer Consulting, http://henningbrauer.com/

Re: pf.conf macro with space

[Sebastian Benoit]

Stefan Sperling(s...@stsp.name) on 2016.06.21 10:23:13 +0200:
> On Tue, Jun 21, 2016 at 10:14:52AM +0200, Sebastian Benoit wrote:
> > 
> > same thing without a stupid helper function, pointed out by henning.
> > 
> > diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
> > index 934438c..426cd93 100644
> > --- sbin/pfctl/parse.y
> > +++ sbin/pfctl/parse.y
> > @@ -714,6 +714,10 @@ numberstring   : NUMBER
> > {
> >  varset : STRING '=' varstring  {
> > if (pf->opts & PF_OPT_VERBOSE)
> > printf("%s = \"%s\"\n", $1, $3);
> > +   if (strchr($1, ' ') != NULL) {
> 
> The previous version used isspace(3). Now, what about tabs? Do we not care?

*sigh*

i need coffee. two. big.
 
> > +   yyerror("macro name cannot contain whitespace");
> > +   YYERROR;
> > +   }
> > if (symset($1, $3, 0) == -1)
> > err(1, "cannot store variable %s", $1);
> > free($1);
> 

-- 

Re: pf.conf macro with space

[Stefan Sperling]

On Tue, Jun 21, 2016 at 10:14:52AM +0200, Sebastian Benoit wrote:
> 
> same thing without a stupid helper function, pointed out by henning.
> 
> diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
> index 934438c..426cd93 100644
> --- sbin/pfctl/parse.y
> +++ sbin/pfctl/parse.y
> @@ -714,6 +714,10 @@ numberstring : NUMBER
> {
>  varset   : STRING '=' varstring  {
>   if (pf->opts & PF_OPT_VERBOSE)
>   printf("%s = \"%s\"\n", $1, $3);
> + if (strchr($1, ' ') != NULL) {

The previous version used isspace(3). Now, what about tabs? Do we not care?

> + yyerror("macro name cannot contain whitespace");
> + YYERROR;
> + }
>   if (symset($1, $3, 0) == -1)
>   err(1, "cannot store variable %s", $1);
>   free($1);

Re: pf.conf macro with space

[Florian Obser]

On Tue, Jun 21, 2016 at 10:14:52AM +0200, Sebastian Benoit wrote:
> 
> same thing without a stupid helper function, pointed out by henning.

OK florian@ (for all parse.y instances we have, oh and as usual you
forgot cwm in your list :) )

> 
> diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
> index 934438c..426cd93 100644
> --- sbin/pfctl/parse.y
> +++ sbin/pfctl/parse.y
> @@ -714,6 +714,10 @@ numberstring : NUMBER
> {
>  varset   : STRING '=' varstring  {
>   if (pf->opts & PF_OPT_VERBOSE)
>   printf("%s = \"%s\"\n", $1, $3);
> + if (strchr($1, ' ') != NULL) {
> + yyerror("macro name cannot contain whitespace");
> + YYERROR;
> + }
>   if (symset($1, $3, 0) == -1)
>   err(1, "cannot store variable %s", $1);
>   free($1);
> 
> 
> Sebastian Benoit(be...@openbsd.org) on 2016.06.21 01:18:33 +0200:
> > sven falempin(sven.falem...@gmail.com) on 2016.06.20 17:38:40 -0400:
> > > Dear Tech Readers,
> > > 
> > > in a pf.conf file one can do
> > > "silly things" = egress
> > 
> > Thanks for your diff, but
> > 
> > one, i dont think spaces in macros are useful in pf.conf.
> > 
> > second, we want to keep this consistent across all the parse.y in our code,
> > so we have to think how this affects these(*)
> > 
> > Below is a diff that disallows "silly things".
> > 
> > I thinks it's easier to check that spaces in macros can be done without.
> > 
> 

-- 
I'm not entirely sure you are real.

Re: pf.conf macro with space

[Sebastian Benoit]

same thing without a stupid helper function, pointed out by henning.

diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index 934438c..426cd93 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -714,6 +714,10 @@ numberstring   : NUMBER
{
 varset : STRING '=' varstring  {
if (pf->opts & PF_OPT_VERBOSE)
printf("%s = \"%s\"\n", $1, $3);
+   if (strchr($1, ' ') != NULL) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable %s", $1);
free($1);


Sebastian Benoit(be...@openbsd.org) on 2016.06.21 01:18:33 +0200:
> sven falempin(sven.falem...@gmail.com) on 2016.06.20 17:38:40 -0400:
> > Dear Tech Readers,
> > 
> > in a pf.conf file one can do
> > "silly things" = egress
> 
> Thanks for your diff, but
> 
> one, i dont think spaces in macros are useful in pf.conf.
> 
> second, we want to keep this consistent across all the parse.y in our code,
> so we have to think how this affects these(*)
> 
> Below is a diff that disallows "silly things".
> 
> I thinks it's easier to check that spaces in macros can be done without.
> 

Re: pf.conf macro with space

[Sebastian Benoit]

sven falempin(sven.falem...@gmail.com) on 2016.06.20 17:38:40 -0400:
> Dear Tech Readers,
> 
> in a pf.conf file one can do
> "silly things" = egress

Thanks for your diff, but

one, i dont think spaces in macros are useful in pf.conf.

second, we want to keep this consistent across all the parse.y in our code,
so we have to think how this affects these(*)

Below is a diff that disallows "silly things".

I thinks it's easier to check that spaces in macros can be done without.

diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index 934438c..341d1af 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -94,6 +94,7 @@ struct sym {
char*nam;
char*val;
 };
+int has_whitespace(const char *);
 int symset(const char *, const char *, int);
 char   *symget(const char *);
 
@@ -714,6 +715,10 @@ numberstring   : NUMBER
{
 varset : STRING '=' varstring  {
if (pf->opts & PF_OPT_VERBOSE)
printf("%s = \"%s\"\n", $1, $3);
+   if (has_whitespace($1)) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable %s", $1);
free($1);
@@ -5455,6 +5460,16 @@ parse_config(char *filename, struct pfctl *xpf)
 }
 
 int
+has_whitespace(const char *s) {
+   while (*s != '\0') {
+   if (isspace(*s))
+   return 1;
+   s++;
+   }
+   return 0;
+}
+
+int
 symset(const char *nam, const char *val, int persist)
 {
struct sym  *sym;



(*)
./bin/chio/parse.y
./sbin/iked/parse.y
./sbin/ipsecctl/parse.y
./sbin/pfctl/parse.y
./usr.bin/doas/parse.y
./usr.sbin/bgpd/parse.y
./usr.sbin/dvmrpd/parse.y
./usr.sbin/eigrpd/parse.y
./usr.sbin/hostapd/parse.y
./usr.sbin/httpd/parse.y
./usr.sbin/ifstated/parse.y
./usr.sbin/iscsictl/parse.y
./usr.sbin/ldapd/parse.y
./usr.sbin/ldomctl/parse.y
./usr.sbin/ldpd/parse.y
./usr.sbin/npppd/npppd/parse.y
./usr.sbin/ntpd/parse.y
./usr.sbin/ospf6d/parse.y
./usr.sbin/ospfd/parse.y
./usr.sbin/radiusd/parse.y
./usr.sbin/relayd/parse.y
./usr.sbin/ripd/parse.y
./usr.sbin/smtpd/parse.y
./usr.sbin/snmpd/parse.y
./usr.sbin/vmd/parse.y
./usr.sbin/ypldap/parse.y
 
> as defined in parse.y like
> 
> varset  : STRING '=' varstring  {
> if (pf->opts & PF_OPT_VERBOSE)
> printf("%s = \"%s\"\n", $1, $3);
> if (symset($1, $3, 0) == -1)
> err(1, "cannot store variable %s", $1);
> free($1);
> free($3);
> }
> 
> and because it s better to triple check
> $ cat /tmp/pf.lol.conf
> karate = egress
> "kar ra tei" = egress
> pass on $kar\ ra\ tei
> $ pfctl -nf /tmp/pf.lol.conf
> /tmp/pf.lol.conf:3: macro 'kar' not defined
> /tmp/pf.lol.conf:3: syntax error
> 
> i also tried the bash ${hope} or makefile $(madness) and even $"sillyness"
> 
> I also remember that being able to read a config file with ease can save a
> lot of time
> 
> "Third Floor Network Switch" = em8
> pass quick on $"Third Floor Network Switch" from www.openbsd.org to
> ($"Third Floor Network Switch":network) set prio (7,7)
> 
> I was wondering why it refused such and read the code,
> fount out a lgetc function is used to read string, and first argument is
> explicit and is a switch to manage quoted string,
> so why not using it after the $macro ?
> 
> --- ./parse.y   Tue Apr 21 12:34:59 2015
> +++ /tmp/1  Mon Jun 20 17:04:08 2016
> @@ -5179,7 +5179,7 @@
> ; /* nothing */
> if (c == '$' && parsebuf == NULL) {
> while (1) {
> -   if ((c = lgetc(0)) == EOF)
> +   if ((c = lgetc(1)) == EOF)
> return (0);
> 
> if (p + 1 >= buf + sizeof(buf) - 1) {
> 
> of course it s not that simple as the code below show, this one works,
> the previous does not.
>  :
> 
> Index: parse.y
> ===
> RCS file: /cvs/src/sbin/pfctl/parse.y,v
> retrieving revision 1.648
> diff -u -r1.648 parse.y
> --- parse.y 21 Apr 2015 16:34:59 -  1.648
> +++ parse.y 20 Jun 2016 21:36:29 -
> @@ -5178,22 +5178,55 @@
> while ((c = lgetc(0)) != '\n' && c != EOF)
> ; /* nothing */
> if (c == '$' && parsebuf == NULL) {
> -   while (1) {
> -   if ((c = lgetc(0)) == EOF)
> -   return (0);
> -
> -   if (p + 1 >= buf + sizeof(buf) - 1) {
> -   yyerror("string too long");
> 

pf.conf macro with space

[sven falempin]

Dear Tech Readers,

in a pf.conf file one can do
"silly things" = egress

as defined in parse.y like

varset  : STRING '=' varstring  {
if (pf->opts & PF_OPT_VERBOSE)
printf("%s = \"%s\"\n", $1, $3);
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable %s", $1);
free($1);
free($3);
}

and because it s better to triple check
$ cat /tmp/pf.lol.conf
karate = egress
"kar ra tei" = egress
pass on $kar\ ra\ tei
$ pfctl -nf /tmp/pf.lol.conf
/tmp/pf.lol.conf:3: macro 'kar' not defined
/tmp/pf.lol.conf:3: syntax error

i also tried the bash ${hope} or makefile $(madness) and even $"sillyness"

I also remember that being able to read a config file with ease can save a
lot of time

"Third Floor Network Switch" = em8
pass quick on $"Third Floor Network Switch" from www.openbsd.org to
($"Third Floor Network Switch":network) set prio (7,7)

I was wondering why it refused such and read the code,
fount out a lgetc function is used to read string, and first argument is
explicit and is a switch to manage quoted string,
so why not using it after the $macro ?

--- ./parse.y   Tue Apr 21 12:34:59 2015
+++ /tmp/1  Mon Jun 20 17:04:08 2016
@@ -5179,7 +5179,7 @@
; /* nothing */
if (c == '$' && parsebuf == NULL) {
while (1) {
-   if ((c = lgetc(0)) == EOF)
+   if ((c = lgetc(1)) == EOF)
return (0);

if (p + 1 >= buf + sizeof(buf) - 1) {

of course it s not that simple as the code below show, this one works,
the previous does not.
 :

Index: parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.648
diff -u -r1.648 parse.y
--- parse.y 21 Apr 2015 16:34:59 -  1.648
+++ parse.y 20 Jun 2016 21:36:29 -
@@ -5178,22 +5178,55 @@
while ((c = lgetc(0)) != '\n' && c != EOF)
; /* nothing */
if (c == '$' && parsebuf == NULL) {
-   while (1) {
-   if ((c = lgetc(0)) == EOF)
-   return (0);
-
-   if (p + 1 >= buf + sizeof(buf) - 1) {
-   yyerror("string too long");
-   return (findeol());
-   }
-   if (isalnum(c) || c == '_') {
+   if ((c = lgetc(0)) == '"') {
+   quotec = c;
+   while (1) {
+   if ((c = lgetc(quotec)) == EOF)
+   return (0);
+   if (c == '\n') {
+   file->lineno++;
+   continue;
+   } else if (c == '\\') {
+   if ((next = lgetc(quotec)) == EOF)
+   return (0);
+   if (next == quotec || c == ' ' || c
== '\t')
+   c = next;
+   else if (next == '\n') {
+   file->lineno++;
+   continue;
+   } else
+   lungetc(next);
+   } else if (c == quotec) {
+   *p = '\0';
+   break;
+   } else if (c == '\0') {
+   yyerror("syntax error");
+   return (findeol());
+   }
+   if (p + 1 >= buf + sizeof(buf) - 1) {
+   yyerror("string too long");
+   return (findeol());
+   }
*p++ = c;
-   continue;
}
-   *p = '\0';
-   lungetc(c);
-   break;
-   }
+   } else
+   while (1) {
+   if ((c = lgetc(0)) == EOF)
+   return (0);
+
+   if (p + 1 >= buf + sizeof(buf) - 1) {
+   yyerror("string too long");
+   return (findeol());
+   }
+
+   if (isalnum(c) || c == '_') {
+   *p++ = c;
+