iked: segfault on invalid transformation

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

iked: segfault on invalid transformation

Klemens Nanni-2
Latest -CURRENT iked(8) dumps core on a seemingly invalid transformation,
although this specific value is listed in iked.conf(8).  Using "aes-256"
for example works, turns out this effects only those that are specified
with "[ESP only]" in the manual. ESP is the default and manually
specifying it to ensure the transformation is valid does not help.

Minimal reproducer:

        # cat /etc/iked.conf
        ikev2 from any to any ikesa enc aes-256-gcm
        # ./obj/iked -n
        /etc/iked.conf: 1: aes-256-gcm not a valid transform
        Segmentation fault (core dumped)

Backtrace of above invocation:

[New process 464271]
Core was generated by `iked'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000003dfe6c5946d in copy_transforms (type=1, xfs=0x3e285cf7fd0, nxfs=1, dst=0x7f7ffffca448, ndst=0x7f7ffffca43c, src=0x3dfe6c605a0 <ikev2_default_ike_transforms>, nsrc=11) at /s/sbin/iked/parse.y:2647
2647 b->xform_id = xf->id;
#0  0x000003dfe6c5946d in copy_transforms (type=1, xfs=0x3e285cf7fd0, nxfs=1, dst=0x7f7ffffca448, ndst=0x7f7ffffca43c, src=0x3dfe6c605a0 <ikev2_default_ike_transforms>, nsrc=11) at /s/sbin/iked/parse.y:2647
#1  0x000003dfe6c5a0c3 in create_ike (name=0x0, af=0, ipproto=0 '\000', hosts=0x3e2a547d360, peers=0x3e200e4d490, ike_sa=0x3e215d80d10, ipsec_sa=0x0, saproto=3 '\003', flags=0 '\000', srcid=0x0, dstid=0x0, ikelifetime=0, lt=0x3e200e4e8e0, authtype=0x3e200e4ecf0, filter=0x0, ikecfg=0x0) at /s/sbin/iked/parse.y:2832
#2  0x000003dfe6c53e65 in yyparse () at /s/sbin/iked/parse.y:482
#3  0x000003dfe6c52a4b in parse_config (filename=0x3e23aca9560 "/etc/iked.conf", x_env=0x3e23aca9560) at /s/sbin/iked/parse.y:1596
#4  0x000003dfe6c12a79 in parent_configure (env=0x3e23aca9560) at /s/sbin/iked/iked.c:198
#5  0x000003dfe6c1267c in main (argc=0, argv=0x7f7ffffcd598) at /s/sbin/iked/iked.c:183

`xf' is NULL but gets dereferenced.

I have not tried older versions of iked(8) and can only take a closer
look at it this weekend, so pardon the scarse report.

Reply | Threaded
Open this post in threaded view
|

Re: iked: segfault on invalid transformation

Tobias Heider-2
Unfortunately as it turns out segfaulting is a common iked reaction to
invalid configurations (at least for invalid transforms), so what you found
is a rather systematic problem (and has been on my list of things to fix for
some time).

As to why those with [ESP only] trigger this behaviour:
[ESP only] means the transform type can only be used for ESP SAs
(meaning Child SAs), what you're specifying in ikesa is the IKE SA.
Try using AES-256-GCM in the childsa option and it will work as intended.

Maybe we should also change the man page to make this clearer?

Reply | Threaded
Open this post in threaded view
|

Re: iked: segfault on invalid transformation

Klemens Nanni-2
On Wed, Aug 14, 2019 at 07:15:14PM +0200, Tobias Heider wrote:
> Unfortunately as it turns out segfaulting is a common iked reaction to
> invalid configurations (at least for invalid transforms), so what you found
> is a rather systematic problem (and has been on my list of things to fix for
> some time).
If that's already on your list, feel free to beat me to it ;-)

> As to why those with [ESP only] trigger this behaviour:
> [ESP only] means the transform type can only be used for ESP SAs
> (meaning Child SAs), what you're specifying in ikesa is the IKE SA.
> Try using AES-256-GCM in the childsa option and it will work as intended.
That makes sense, yes.

> Maybe we should also change the man page to make this clearer?
I agree;  writing down an iked.conf(5) initially to match a very
specific endpoint, this detail was (easily) missed/skipped.

Reply | Threaded
Open this post in threaded view
|

Re: iked: segfault on invalid transformation

Tobias Heider-2
> If that's already on your list, feel free to beat me to it ;-)

Sure, if you will test it for me. Here's a fix for the segfault.

It seems the initial mistake was that yyerror() does not exit.
Instead one has to use YYERROR (or err() as the check above does).
I opted for YYERROR, but i don't think it makes a difference as both
are used throughout the file.

Ok?

Index: parse.y
===================================================================
RCS file: /cvs/src/sbin/iked/parse.y,v
retrieving revision 1.81
diff -u -p -u -r1.81 parse.y
--- parse.y 28 Jun 2019 13:32:44 -0000 1.81
+++ parse.y 15 Aug 2019 21:35:16 -0000
@@ -750,8 +750,10 @@ transform : AUTHXF STRING {
     sizeof(struct ipsec_xf *));
  if (xfs == NULL)
  err(1, "transform: recallocarray");
- if ((xfs[nxfs] = parse_xf($2, 0, authxfs)) == NULL)
+ if ((xfs[nxfs] = parse_xf($2, 0, authxfs)) == NULL) {
  yyerror("%s not a valid transform", $2);
+ YYERROR;
+ }
  ipsec_transforms->authxf = xfs;
  ipsec_transforms->nauthxf++;
  }
@@ -762,8 +764,10 @@ transform : AUTHXF STRING {
     sizeof(struct ipsec_xf *));
  if (xfs == NULL)
  err(1, "transform: recallocarray");
- if ((xfs[nxfs] = parse_xf($2, 0, encxfs)) == NULL)
+ if ((xfs[nxfs] = parse_xf($2, 0, encxfs)) == NULL) {
  yyerror("%s not a valid transform", $2);
+ YYERROR;
+ }
  ipsec_transforms->encxf = xfs;
  ipsec_transforms->nencxf++;
  }
@@ -774,8 +778,10 @@ transform : AUTHXF STRING {
     sizeof(struct ipsec_xf *));
  if (xfs == NULL)
  err(1, "transform: recallocarray");
- if ((xfs[nxfs] = parse_xf($2, 0, prfxfs)) == NULL)
+ if ((xfs[nxfs] = parse_xf($2, 0, prfxfs)) == NULL) {
  yyerror("%s not a valid transform", $2);
+ YYERROR;
+ }
  ipsec_transforms->prfxf = xfs;
  ipsec_transforms->nprfxf++;
  }
@@ -786,8 +792,10 @@ transform : AUTHXF STRING {
     sizeof(struct ipsec_xf *));
  if (xfs == NULL)
  err(1, "transform: recallocarray");
- if ((xfs[nxfs] = parse_xf($2, 0, groupxfs)) == NULL)
+ if ((xfs[nxfs] = parse_xf($2, 0, groupxfs)) == NULL) {
  yyerror("%s not a valid transform", $2);
+ YYERROR;
+ }
  ipsec_transforms->groupxf = xfs;
  ipsec_transforms->ngroupxf++;
  }

Reply | Threaded
Open this post in threaded view
|

Re: iked: segfault on invalid transformation

Sebastian Benoit-3
Tobias Heider([hidden email]) on 2019.08.16 00:06:59 +0200:

> > If that's already on your list, feel free to beat me to it ;-)
>
> Sure, if you will test it for me. Here's a fix for the segfault.
>
> It seems the initial mistake was that yyerror() does not exit.
> Instead one has to use YYERROR (or err() as the check above does).
> I opted for YYERROR, but i don't think it makes a difference as both
> are used throughout the file.
>
> Ok?

yes, this is ok benno@


/Benno


>
> Index: parse.y
> ===================================================================
> RCS file: /cvs/src/sbin/iked/parse.y,v
> retrieving revision 1.81
> diff -u -p -u -r1.81 parse.y
> --- parse.y 28 Jun 2019 13:32:44 -0000 1.81
> +++ parse.y 15 Aug 2019 21:35:16 -0000
> @@ -750,8 +750,10 @@ transform : AUTHXF STRING {
>      sizeof(struct ipsec_xf *));
>   if (xfs == NULL)
>   err(1, "transform: recallocarray");
> - if ((xfs[nxfs] = parse_xf($2, 0, authxfs)) == NULL)
> + if ((xfs[nxfs] = parse_xf($2, 0, authxfs)) == NULL) {
>   yyerror("%s not a valid transform", $2);
> + YYERROR;
> + }
>   ipsec_transforms->authxf = xfs;
>   ipsec_transforms->nauthxf++;
>   }
> @@ -762,8 +764,10 @@ transform : AUTHXF STRING {
>      sizeof(struct ipsec_xf *));
>   if (xfs == NULL)
>   err(1, "transform: recallocarray");
> - if ((xfs[nxfs] = parse_xf($2, 0, encxfs)) == NULL)
> + if ((xfs[nxfs] = parse_xf($2, 0, encxfs)) == NULL) {
>   yyerror("%s not a valid transform", $2);
> + YYERROR;
> + }
>   ipsec_transforms->encxf = xfs;
>   ipsec_transforms->nencxf++;
>   }
> @@ -774,8 +778,10 @@ transform : AUTHXF STRING {
>      sizeof(struct ipsec_xf *));
>   if (xfs == NULL)
>   err(1, "transform: recallocarray");
> - if ((xfs[nxfs] = parse_xf($2, 0, prfxfs)) == NULL)
> + if ((xfs[nxfs] = parse_xf($2, 0, prfxfs)) == NULL) {
>   yyerror("%s not a valid transform", $2);
> + YYERROR;
> + }
>   ipsec_transforms->prfxf = xfs;
>   ipsec_transforms->nprfxf++;
>   }
> @@ -786,8 +792,10 @@ transform : AUTHXF STRING {
>      sizeof(struct ipsec_xf *));
>   if (xfs == NULL)
>   err(1, "transform: recallocarray");
> - if ((xfs[nxfs] = parse_xf($2, 0, groupxfs)) == NULL)
> + if ((xfs[nxfs] = parse_xf($2, 0, groupxfs)) == NULL) {
>   yyerror("%s not a valid transform", $2);
> + YYERROR;
> + }
>   ipsec_transforms->groupxf = xfs;
>   ipsec_transforms->ngroupxf++;
>   }
>

Reply | Threaded
Open this post in threaded view
|

Re: iked: segfault on invalid transformation

Tobias Heider-2
> yes, this is ok benno@

Thanks, added!

Reply | Threaded
Open this post in threaded view
|

Re: iked: segfault on invalid transformation

Klemens Nanni-2
In reply to this post by Tobias Heider-2
On Fri, Aug 16, 2019 at 12:06:59AM +0200, Tobias Heider wrote:
> It seems the initial mistake was that yyerror() does not exit.
> Instead one has to use YYERROR (or err() as the check above does).
> I opted for YYERROR, but i don't think it makes a difference as both
> are used throughout the file.
Yes, please.  `YYERROR' is the common idiom after `yyerror()' if you
want to fail hard.

That was more obvious than I thought.

Reply | Threaded
Open this post in threaded view
|

Re: iked: segfault on invalid transformation

Tobias Heider-2
> Yes, please.  `YYERROR' is the common idiom after `yyerror()' if you
> want to fail hard.

Good to hear! Below is what I would propose to add to the man page to make
the [ESP only] a little clearer. Do you think this would be helpful?

Index: iked.conf.5
===================================================================
RCS file: /cvs/src/sbin/iked/iked.conf.5,v
retrieving revision 1.55
diff -u -p -u -r1.55 iked.conf.5
--- iked.conf.5 11 May 2019 16:30:23 -0000 1.55
+++ iked.conf.5 16 Aug 2019 08:50:52 -0000
@@ -846,6 +846,12 @@ not encryption:
 .It Li null Ta "" Ta "[ESP only]"
 .El
 .Pp
+Transform followed by [IKE only] can only be used with the
+.Ic ikesa
+keyword, transforms with [ESP only] can only be used with the
+.Ic childsa
+keyword.
+.Pp
 3DES requires 24 bytes to form its 168-bit key.
 This is because the most significant bit of each byte is used for parity.
 .Pp

Reply | Threaded
Open this post in threaded view
|

Re: iked: segfault on invalid transformation

Klemens Nanni-2
On Fri, Aug 16, 2019 at 10:57:32AM +0200, Tobias Heider wrote:
> Good to hear! Below is what I would propose to add to the man page to make
> the [ESP only] a little clearer. Do you think this would be helpful?
Reads OK to me, thanks.

> Index: iked.conf.5
> ===================================================================
> RCS file: /cvs/src/sbin/iked/iked.conf.5,v
> retrieving revision 1.55
> diff -u -p -u -r1.55 iked.conf.5
> --- iked.conf.5 11 May 2019 16:30:23 -0000 1.55
> +++ iked.conf.5 16 Aug 2019 08:50:52 -0000
> @@ -846,6 +846,12 @@ not encryption:
>  .It Li null Ta "" Ta "[ESP only]"
>  .El
>  .Pp
> +Transform followed by [IKE only] can only be used with the
I'd do `.Dq [IKE only]' or rather `.Bq IKE only'.

> +.Ic ikesa
> +keyword, transforms with [ESP only] can only be used with the
Same here.

Reply | Threaded
Open this post in threaded view
|

Re: iked: segfault on invalid transformation

Tobias Heider-2
> > +Transform followed by [IKE only] can only be used with the
> I'd do `.Dq [IKE only]' or rather `.Bq IKE only'.

Thanks, added with the brackets changed to `.Bq'!