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? bluhm
