On Fri, May 12, 2017 at 01:53:12PM +0200, Alexander Bluhm wrote: > On Fri, May 12, 2017 at 07:30:28AM +0100, Tom Cosgrove wrote: > > >>> Alexander Bluhm 11-May-17 23:25 >>> > > > Instead of printing a debug message at the end, panic early if the > > > IPsec security protocol is unknown. > > > > Is this before or after we have decrypted and checked MAC? TBH, even if > > it's after, would this mean a compromise of a remote machine could be used > > to crash the running system's kernel? > > Of course it is risky to put a panic() into the IP input path. But > to find bugs and to understand the code it is better than a DPRINTF() > nobody has cared about for 15 years. I think the default case with > the panic() can only be triggered by programming errors. > > The variable sproto is passed as parmeter to ipsec_common_input(). > It gets called by ah4_input(), esp4_input(), ipcomp4_input(), > ah6_input(), esp6_input(), ipcomp6_input() where sproto comes from > the proto parameter. These are only called by the pr_input callbacks > where the protocol is one of IPPROTO_ESP, IPPROTO_AH, IPPROTO_IPCOMP. > > In ipsec_common_input_cb() sproto comes from tdbp->tdb_sproto. It > is set in reserve_spi() which gets its sproto from sa1->tdb_sproto > in pfkeyv2_send(). This field is set by pfkeyv2_get_proto_alg(), > possible values are IPPROTO_AH, IPPROTO_ESP, IPPROTO_IPIP, > IPPROTO_IPCOMP, IPPROTO_TCP. > > So why is ipsec_common_input_cb() not called with IPPROTO_IPIP or > IPPROTO_TCP? > > ipsec_common_input_cb() is called by ah_input_cb(), esp_input_cb(), > ipcomp_input_cb() with the tdb coming from gettdb(). gettdb() is > called with tc->tc_proto and only returns a tdb with matching > tdb_sproto. The input callbacks are registered in ah_input(), > esp_input(), ipcomp_input(), there tc->tc_proto is set to tdb->tdb_sproto > or IPPROTO_IPCOMP. tdb is passed to ah_input(), esp_input() as > parameter, they are called via the xf_input pointer. This is called > from ipsec_common_input() and bridge_ipsec(). In ipsec_common_input() > tdbp comes from gettdb() called with sproto passed as parameter. > This has been discussed before. In bridge_ipsec() tdb comes from > gettdb() called with proto. There we goto skiplookup if proto != > IPPROTO_ESP && proto != IPPROTO_AH && proto != IPPROTO_IPCOMP. > > The question is, do we trust this analysis? >
I came to a similar conclusion and I think now it is the right time to take this risk. We may decide to make this a non fatal error return later on. OK claudio@ -- :wq Claudio
