* [RFC PATCH v3 17/19] calipso: Add validation of CALIPSO option.
@ 2016-02-17 13:22 ` Huw Davies
0 siblings, 0 replies; 6+ messages in thread
From: Huw Davies @ 2016-02-17 13:22 UTC (permalink / raw)
To: netdev, linux-security-module, selinux; +Cc: Paul Moore
We check lengths, checksum and the DOI. We leave checking of the
level and categories for the socket layer.
Signed-off-by: Huw Davies <huw@codeweavers.com>
---
include/net/calipso.h | 6 ++++++
net/ipv6/calipso.c | 42 ++++++++++++++++++++++++++++++++++++++++++
net/ipv6/exthdrs.c | 27 +++++++++++++++++++++++++++
3 files changed, 75 insertions(+)
diff --git a/include/net/calipso.h b/include/net/calipso.h
index 38dbb47..85404e2 100644
--- a/include/net/calipso.h
+++ b/include/net/calipso.h
@@ -65,6 +65,7 @@ struct calipso_doi {
#ifdef CONFIG_NETLABEL
int __init calipso_init(void);
void calipso_exit(void);
+bool calipso_validate(const struct sk_buff *skb, const unsigned char *option);
#else
static inline int __init calipso_init(void)
{
@@ -74,6 +75,11 @@ static inline int __init calipso_init(void)
static inline void calipso_exit(void)
{
}
+static inline bool calipso_validate(const struct sk_buff *skb,
+ const unsigned char *option)
+{
+ return true;
+}
#endif /* CONFIG_NETLABEL */
#endif /* _CALIPSO_H */
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index fa371a8..b8bcf9f 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -321,6 +321,48 @@ doi_walk_return:
}
/**
+ * calipso_validate - Validate a CALIPSO option
+ * @skb: the packet
+ * @option: the start of the option
+ *
+ * Description:
+ * This routine is called to validate a CALIPSO option.
+ * If the option is valid then a zero value is returned. If the
+ * option is invalid then a non-zero value is returned and
+ * representing the offset to the offending portion of the option.
+ *
+ * The caller should have already checked that the length of the
+ * option (including the TLV header) is >= 10 and that the catmap
+ * length is consistent with the option length.
+ *
+ * We leave checks on the level and categories to the socket layer.
+ */
+bool calipso_validate(const struct sk_buff *skb, const unsigned char *option)
+{
+ struct calipso_doi *doi_def;
+ int ret_val;
+ u16 crc, len = option[1] + 2;
+ static const u8 zero[2];
+
+ /* The original CRC runs over the option including the TLV header
+ * with the CRC-16 field (at offset 8) zeroed out. */
+ crc = crc_ccitt(0xffff, option, 8);
+ crc = crc_ccitt(crc, zero, sizeof(zero));
+ if (len > 10)
+ crc = crc_ccitt(crc, option + 10, len - 10);
+ crc = ~crc;
+ if (option[8] != (crc & 0xff) || option[9] != ((crc >> 8) & 0xff))
+ return false;
+
+ rcu_read_lock();
+ doi_def = calipso_doi_search(get_unaligned_be32(option + 2));
+ ret_val = !!doi_def;
+ rcu_read_unlock();
+
+ return ret_val;
+}
+
+/**
* calipso_map_cat_hton - Perform a category mapping from host to network
* @doi_def: the DOI definition
* @secattr: the security attributes
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index d5fd3e7..0f69cab 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -43,6 +43,7 @@
#include <net/ndisc.h>
#include <net/ip6_route.h>
#include <net/addrconf.h>
+#include <net/calipso.h>
#if IS_ENABLED(CONFIG_IPV6_MIP6)
#include <net/xfrm.h>
#endif
@@ -603,6 +604,28 @@ drop:
return false;
}
+/* CALIPSO RFC 5570 */
+
+static bool ipv6_hop_calipso(struct sk_buff *skb, int optoff)
+{
+ const unsigned char *nh = skb_network_header(skb);
+
+ if (nh[optoff + 1] < 8)
+ goto drop;
+
+ if (nh[optoff + 6] * 4 + 8 > nh[optoff + 1])
+ goto drop;
+
+ if (!calipso_validate(skb, nh + optoff))
+ goto drop;
+
+ return true;
+
+drop:
+ kfree_skb(skb);
+ return false;
+}
+
static const struct tlvtype_proc tlvprochopopt_lst[] = {
{
.type = IPV6_TLV_ROUTERALERT,
@@ -612,6 +635,10 @@ static const struct tlvtype_proc tlvprochopopt_lst[] = {
.type = IPV6_TLV_JUMBO,
.func = ipv6_hop_jumbo,
},
+ {
+ .type = IPV6_TLV_CALIPSO,
+ .func = ipv6_hop_calipso,
+ },
{ -1, }
};
--
2.7.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC PATCH v3 17/19] calipso: Add validation of CALIPSO option.
@ 2016-02-17 13:22 ` Huw Davies
0 siblings, 0 replies; 6+ messages in thread
From: Huw Davies @ 2016-02-17 13:22 UTC (permalink / raw)
To: netdev, linux-security-module, selinux
We check lengths, checksum and the DOI. We leave checking of the
level and categories for the socket layer.
Signed-off-by: Huw Davies <huw@codeweavers.com>
---
include/net/calipso.h | 6 ++++++
net/ipv6/calipso.c | 42 ++++++++++++++++++++++++++++++++++++++++++
net/ipv6/exthdrs.c | 27 +++++++++++++++++++++++++++
3 files changed, 75 insertions(+)
diff --git a/include/net/calipso.h b/include/net/calipso.h
index 38dbb47..85404e2 100644
--- a/include/net/calipso.h
+++ b/include/net/calipso.h
@@ -65,6 +65,7 @@ struct calipso_doi {
#ifdef CONFIG_NETLABEL
int __init calipso_init(void);
void calipso_exit(void);
+bool calipso_validate(const struct sk_buff *skb, const unsigned char *option);
#else
static inline int __init calipso_init(void)
{
@@ -74,6 +75,11 @@ static inline int __init calipso_init(void)
static inline void calipso_exit(void)
{
}
+static inline bool calipso_validate(const struct sk_buff *skb,
+ const unsigned char *option)
+{
+ return true;
+}
#endif /* CONFIG_NETLABEL */
#endif /* _CALIPSO_H */
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index fa371a8..b8bcf9f 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -321,6 +321,48 @@ doi_walk_return:
}
/**
+ * calipso_validate - Validate a CALIPSO option
+ * @skb: the packet
+ * @option: the start of the option
+ *
+ * Description:
+ * This routine is called to validate a CALIPSO option.
+ * If the option is valid then a zero value is returned. If the
+ * option is invalid then a non-zero value is returned and
+ * representing the offset to the offending portion of the option.
+ *
+ * The caller should have already checked that the length of the
+ * option (including the TLV header) is >= 10 and that the catmap
+ * length is consistent with the option length.
+ *
+ * We leave checks on the level and categories to the socket layer.
+ */
+bool calipso_validate(const struct sk_buff *skb, const unsigned char *option)
+{
+ struct calipso_doi *doi_def;
+ int ret_val;
+ u16 crc, len = option[1] + 2;
+ static const u8 zero[2];
+
+ /* The original CRC runs over the option including the TLV header
+ * with the CRC-16 field (at offset 8) zeroed out. */
+ crc = crc_ccitt(0xffff, option, 8);
+ crc = crc_ccitt(crc, zero, sizeof(zero));
+ if (len > 10)
+ crc = crc_ccitt(crc, option + 10, len - 10);
+ crc = ~crc;
+ if (option[8] != (crc & 0xff) || option[9] != ((crc >> 8) & 0xff))
+ return false;
+
+ rcu_read_lock();
+ doi_def = calipso_doi_search(get_unaligned_be32(option + 2));
+ ret_val = !!doi_def;
+ rcu_read_unlock();
+
+ return ret_val;
+}
+
+/**
* calipso_map_cat_hton - Perform a category mapping from host to network
* @doi_def: the DOI definition
* @secattr: the security attributes
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index d5fd3e7..0f69cab 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -43,6 +43,7 @@
#include <net/ndisc.h>
#include <net/ip6_route.h>
#include <net/addrconf.h>
+#include <net/calipso.h>
#if IS_ENABLED(CONFIG_IPV6_MIP6)
#include <net/xfrm.h>
#endif
@@ -603,6 +604,28 @@ drop:
return false;
}
+/* CALIPSO RFC 5570 */
+
+static bool ipv6_hop_calipso(struct sk_buff *skb, int optoff)
+{
+ const unsigned char *nh = skb_network_header(skb);
+
+ if (nh[optoff + 1] < 8)
+ goto drop;
+
+ if (nh[optoff + 6] * 4 + 8 > nh[optoff + 1])
+ goto drop;
+
+ if (!calipso_validate(skb, nh + optoff))
+ goto drop;
+
+ return true;
+
+drop:
+ kfree_skb(skb);
+ return false;
+}
+
static const struct tlvtype_proc tlvprochopopt_lst[] = {
{
.type = IPV6_TLV_ROUTERALERT,
@@ -612,6 +635,10 @@ static const struct tlvtype_proc tlvprochopopt_lst[] = {
.type = IPV6_TLV_JUMBO,
.func = ipv6_hop_jumbo,
},
+ {
+ .type = IPV6_TLV_CALIPSO,
+ .func = ipv6_hop_calipso,
+ },
{ -1, }
};
--
2.7.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v3 17/19] calipso: Add validation of CALIPSO option.
2016-02-17 13:22 ` Huw Davies
(?)
@ 2016-05-06 22:59 ` Paul Moore
[not found] ` <CAHC9VhTRp8bHgwxZ2YNj=pa2a4PtO=8jJT1siG99GiFYnwm53Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
-1 siblings, 1 reply; 6+ messages in thread
From: Paul Moore @ 2016-05-06 22:59 UTC (permalink / raw)
To: Huw Davies; +Cc: netdev, linux-security-module, selinux, Paul Moore
On Wed, Feb 17, 2016 at 8:22 AM, Huw Davies <huw@codeweavers.com> wrote:
> We check lengths, checksum and the DOI. We leave checking of the
> level and categories for the socket layer.
>
> Signed-off-by: Huw Davies <huw@codeweavers.com>
> ---
> include/net/calipso.h | 6 ++++++
> net/ipv6/calipso.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> net/ipv6/exthdrs.c | 27 +++++++++++++++++++++++++++
> 3 files changed, 75 insertions(+)
>
> diff --git a/include/net/calipso.h b/include/net/calipso.h
> index 38dbb47..85404e2 100644
> --- a/include/net/calipso.h
> +++ b/include/net/calipso.h
> @@ -65,6 +65,7 @@ struct calipso_doi {
> #ifdef CONFIG_NETLABEL
> int __init calipso_init(void);
> void calipso_exit(void);
> +bool calipso_validate(const struct sk_buff *skb, const unsigned char *option);
> #else
> static inline int __init calipso_init(void)
> {
> @@ -74,6 +75,11 @@ static inline int __init calipso_init(void)
> static inline void calipso_exit(void)
> {
> }
> +static inline bool calipso_validate(const struct sk_buff *skb,
> + const unsigned char *option)
> +{
> + return true;
> +}
> #endif /* CONFIG_NETLABEL */
>
> #endif /* _CALIPSO_H */
> diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
> index fa371a8..b8bcf9f 100644
> --- a/net/ipv6/calipso.c
> +++ b/net/ipv6/calipso.c
> @@ -321,6 +321,48 @@ doi_walk_return:
> }
>
> /**
> + * calipso_validate - Validate a CALIPSO option
> + * @skb: the packet
> + * @option: the start of the option
> + *
> + * Description:
> + * This routine is called to validate a CALIPSO option.
> + * If the option is valid then a zero value is returned. If the
> + * option is invalid then a non-zero value is returned and
> + * representing the offset to the offending portion of the option.
> + *
> + * The caller should have already checked that the length of the
> + * option (including the TLV header) is >= 10 and that the catmap
> + * length is consistent with the option length.
> + *
> + * We leave checks on the level and categories to the socket layer.
> + */
> +bool calipso_validate(const struct sk_buff *skb, const unsigned char *option)
> +{
> + struct calipso_doi *doi_def;
> + int ret_val;
> + u16 crc, len = option[1] + 2;
> + static const u8 zero[2];
> +
> + /* The original CRC runs over the option including the TLV header
> + * with the CRC-16 field (at offset 8) zeroed out. */
> + crc = crc_ccitt(0xffff, option, 8);
> + crc = crc_ccitt(crc, zero, sizeof(zero));
> + if (len > 10)
> + crc = crc_ccitt(crc, option + 10, len - 10);
> + crc = ~crc;
I should have caught this in the v2 patchset when I mentioned it with
respect to the CRC generation, but why not simply do 'crc =
~crc_cccitt(...);'?
Also, while I'm looking at this, why not do the CRC verification in
ipv6_hop_calipso()? The only thing we should need to do here is the
DOI lookup/verification so that we still work correctly when
CONFIG_NETLABEL=n; all the core protocol stuff, e.g. length and
checksum validation, should be done in the core stack functions, e.g.
ipv6_hop_calipso().
> + if (option[8] != (crc & 0xff) || option[9] != ((crc >> 8) & 0xff))
> + return false;
> +
> + rcu_read_lock();
> + doi_def = calipso_doi_search(get_unaligned_be32(option + 2));
> + ret_val = !!doi_def;
> + rcu_read_unlock();
> +
> + return ret_val;
> +}
> +
> +/**
> * calipso_map_cat_hton - Perform a category mapping from host to network
> * @doi_def: the DOI definition
> * @secattr: the security attributes
> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> index d5fd3e7..0f69cab 100644
> --- a/net/ipv6/exthdrs.c
> +++ b/net/ipv6/exthdrs.c
> @@ -43,6 +43,7 @@
> #include <net/ndisc.h>
> #include <net/ip6_route.h>
> #include <net/addrconf.h>
> +#include <net/calipso.h>
> #if IS_ENABLED(CONFIG_IPV6_MIP6)
> #include <net/xfrm.h>
> #endif
> @@ -603,6 +604,28 @@ drop:
> return false;
> }
>
> +/* CALIPSO RFC 5570 */
> +
> +static bool ipv6_hop_calipso(struct sk_buff *skb, int optoff)
> +{
> + const unsigned char *nh = skb_network_header(skb);
> +
> + if (nh[optoff + 1] < 8)
> + goto drop;
> +
> + if (nh[optoff + 6] * 4 + 8 > nh[optoff + 1])
> + goto drop;
> +
> + if (!calipso_validate(skb, nh + optoff))
> + goto drop;
> +
> + return true;
> +
> +drop:
> + kfree_skb(skb);
> + return false;
> +}
> +
> static const struct tlvtype_proc tlvprochopopt_lst[] = {
> {
> .type = IPV6_TLV_ROUTERALERT,
> @@ -612,6 +635,10 @@ static const struct tlvtype_proc tlvprochopopt_lst[] = {
> .type = IPV6_TLV_JUMBO,
> .func = ipv6_hop_jumbo,
> },
> + {
> + .type = IPV6_TLV_CALIPSO,
> + .func = ipv6_hop_calipso,
> + },
> { -1, }
> };
>
> --
> 2.7.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v3 17/19] calipso: Add validation of CALIPSO option.
2016-05-06 22:59 ` Paul Moore
@ 2016-05-09 10:39 ` Huw Davies
0 siblings, 0 replies; 6+ messages in thread
From: Huw Davies @ 2016-05-09 10:39 UTC (permalink / raw)
To: Paul Moore
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
selinux-+05T5uksL2qpZYMLLGbcSA
On Fri, May 06, 2016 at 06:59:32PM -0400, Paul Moore wrote:
> On Wed, Feb 17, 2016 at 8:22 AM, Huw Davies <huw-PJ7GQ4yptbsswetKESUqMA@public.gmane.org> wrote:
> > We check lengths, checksum and the DOI. We leave checking of the
> > level and categories for the socket layer.
> >
> > Signed-off-by: Huw Davies <huw-PJ7GQ4yptbsswetKESUqMA@public.gmane.org>
> > ---
> > include/net/calipso.h | 6 ++++++
> > net/ipv6/calipso.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > net/ipv6/exthdrs.c | 27 +++++++++++++++++++++++++++
> > 3 files changed, 75 insertions(+)
> >
> > diff --git a/include/net/calipso.h b/include/net/calipso.h
> > index 38dbb47..85404e2 100644
> > --- a/include/net/calipso.h
> > +++ b/include/net/calipso.h
> > @@ -65,6 +65,7 @@ struct calipso_doi {
> > #ifdef CONFIG_NETLABEL
> > int __init calipso_init(void);
> > void calipso_exit(void);
> > +bool calipso_validate(const struct sk_buff *skb, const unsigned char *option);
> > #else
> > static inline int __init calipso_init(void)
> > {
> > @@ -74,6 +75,11 @@ static inline int __init calipso_init(void)
> > static inline void calipso_exit(void)
> > {
> > }
> > +static inline bool calipso_validate(const struct sk_buff *skb,
> > + const unsigned char *option)
> > +{
> > + return true;
> > +}
> > #endif /* CONFIG_NETLABEL */
> >
> > #endif /* _CALIPSO_H */
> > diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
> > index fa371a8..b8bcf9f 100644
> > --- a/net/ipv6/calipso.c
> > +++ b/net/ipv6/calipso.c
> > @@ -321,6 +321,48 @@ doi_walk_return:
> > }
> >
> > /**
> > + * calipso_validate - Validate a CALIPSO option
> > + * @skb: the packet
> > + * @option: the start of the option
> > + *
> > + * Description:
> > + * This routine is called to validate a CALIPSO option.
> > + * If the option is valid then a zero value is returned. If the
> > + * option is invalid then a non-zero value is returned and
> > + * representing the offset to the offending portion of the option.
> > + *
> > + * The caller should have already checked that the length of the
> > + * option (including the TLV header) is >= 10 and that the catmap
> > + * length is consistent with the option length.
> > + *
> > + * We leave checks on the level and categories to the socket layer.
> > + */
> > +bool calipso_validate(const struct sk_buff *skb, const unsigned char *option)
> > +{
> > + struct calipso_doi *doi_def;
> > + int ret_val;
> > + u16 crc, len = option[1] + 2;
> > + static const u8 zero[2];
> > +
> > + /* The original CRC runs over the option including the TLV header
> > + * with the CRC-16 field (at offset 8) zeroed out. */
> > + crc = crc_ccitt(0xffff, option, 8);
> > + crc = crc_ccitt(crc, zero, sizeof(zero));
> > + if (len > 10)
> > + crc = crc_ccitt(crc, option + 10, len - 10);
> > + crc = ~crc;
>
> I should have caught this in the v2 patchset when I mentioned it with
> respect to the CRC generation, but why not simply do 'crc =
> ~crc_cccitt(...);'?
Simply because the final crc_ccitt() is inside an if statement.
Since len is guaranteed to be >= 10, I could dispense with the if and
have crc_ccitt() handle having its final argument equal to zero (which
it does just fine). Then I could do as you suggest.
> Also, while I'm looking at this, why not do the CRC verification in
> ipv6_hop_calipso()? The only thing we should need to do here is the
> DOI lookup/verification so that we still work correctly when
> CONFIG_NETLABEL=n; all the core protocol stuff, e.g. length and
> checksum validation, should be done in the core stack functions, e.g.
> ipv6_hop_calipso().
The only reason was to not bring in CONFIG_CRC_CCITT if CONFIG_NETLABEL=n
(this is currently selected in net/netlabel/Kconfig).
If folks are happy for me to do so, I could change this to:
select CONFIG_CRC_CCITT
under menuconfig IPV6
Then the crc check could move to exthdrs.c:ipv6_hop_calipso(). Would
that be ok?
Thanks,
Huw.
> > + if (option[8] != (crc & 0xff) || option[9] != ((crc >> 8) & 0xff))
> > + return false;
> > +
> > + rcu_read_lock();
> > + doi_def = calipso_doi_search(get_unaligned_be32(option + 2));
> > + ret_val = !!doi_def;
> > + rcu_read_unlock();
> > +
> > + return ret_val;
> > +}
> > +
> > +/**
> > * calipso_map_cat_hton - Perform a category mapping from host to network
> > * @doi_def: the DOI definition
> > * @secattr: the security attributes
> > diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> > index d5fd3e7..0f69cab 100644
> > --- a/net/ipv6/exthdrs.c
> > +++ b/net/ipv6/exthdrs.c
> > @@ -43,6 +43,7 @@
> > #include <net/ndisc.h>
> > #include <net/ip6_route.h>
> > #include <net/addrconf.h>
> > +#include <net/calipso.h>
> > #if IS_ENABLED(CONFIG_IPV6_MIP6)
> > #include <net/xfrm.h>
> > #endif
> > @@ -603,6 +604,28 @@ drop:
> > return false;
> > }
> >
> > +/* CALIPSO RFC 5570 */
> > +
> > +static bool ipv6_hop_calipso(struct sk_buff *skb, int optoff)
> > +{
> > + const unsigned char *nh = skb_network_header(skb);
> > +
> > + if (nh[optoff + 1] < 8)
> > + goto drop;
> > +
> > + if (nh[optoff + 6] * 4 + 8 > nh[optoff + 1])
> > + goto drop;
> > +
> > + if (!calipso_validate(skb, nh + optoff))
> > + goto drop;
> > +
> > + return true;
> > +
> > +drop:
> > + kfree_skb(skb);
> > + return false;
> > +}
> > +
> > static const struct tlvtype_proc tlvprochopopt_lst[] = {
> > {
> > .type = IPV6_TLV_ROUTERALERT,
> > @@ -612,6 +635,10 @@ static const struct tlvtype_proc tlvprochopopt_lst[] = {
> > .type = IPV6_TLV_JUMBO,
> > .func = ipv6_hop_jumbo,
> > },
> > + {
> > + .type = IPV6_TLV_CALIPSO,
> > + .func = ipv6_hop_calipso,
> > + },
> > { -1, }
> > };
> >
> > --
> > 2.7.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> paul moore
> www.paul-moore.com
_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v3 17/19] calipso: Add validation of CALIPSO option.
@ 2016-05-09 10:39 ` Huw Davies
0 siblings, 0 replies; 6+ messages in thread
From: Huw Davies @ 2016-05-09 10:39 UTC (permalink / raw)
To: Paul Moore; +Cc: netdev, linux-security-module, selinux, Paul Moore
On Fri, May 06, 2016 at 06:59:32PM -0400, Paul Moore wrote:
> On Wed, Feb 17, 2016 at 8:22 AM, Huw Davies <huw@codeweavers.com> wrote:
> > We check lengths, checksum and the DOI. We leave checking of the
> > level and categories for the socket layer.
> >
> > Signed-off-by: Huw Davies <huw@codeweavers.com>
> > ---
> > include/net/calipso.h | 6 ++++++
> > net/ipv6/calipso.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > net/ipv6/exthdrs.c | 27 +++++++++++++++++++++++++++
> > 3 files changed, 75 insertions(+)
> >
> > diff --git a/include/net/calipso.h b/include/net/calipso.h
> > index 38dbb47..85404e2 100644
> > --- a/include/net/calipso.h
> > +++ b/include/net/calipso.h
> > @@ -65,6 +65,7 @@ struct calipso_doi {
> > #ifdef CONFIG_NETLABEL
> > int __init calipso_init(void);
> > void calipso_exit(void);
> > +bool calipso_validate(const struct sk_buff *skb, const unsigned char *option);
> > #else
> > static inline int __init calipso_init(void)
> > {
> > @@ -74,6 +75,11 @@ static inline int __init calipso_init(void)
> > static inline void calipso_exit(void)
> > {
> > }
> > +static inline bool calipso_validate(const struct sk_buff *skb,
> > + const unsigned char *option)
> > +{
> > + return true;
> > +}
> > #endif /* CONFIG_NETLABEL */
> >
> > #endif /* _CALIPSO_H */
> > diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
> > index fa371a8..b8bcf9f 100644
> > --- a/net/ipv6/calipso.c
> > +++ b/net/ipv6/calipso.c
> > @@ -321,6 +321,48 @@ doi_walk_return:
> > }
> >
> > /**
> > + * calipso_validate - Validate a CALIPSO option
> > + * @skb: the packet
> > + * @option: the start of the option
> > + *
> > + * Description:
> > + * This routine is called to validate a CALIPSO option.
> > + * If the option is valid then a zero value is returned. If the
> > + * option is invalid then a non-zero value is returned and
> > + * representing the offset to the offending portion of the option.
> > + *
> > + * The caller should have already checked that the length of the
> > + * option (including the TLV header) is >= 10 and that the catmap
> > + * length is consistent with the option length.
> > + *
> > + * We leave checks on the level and categories to the socket layer.
> > + */
> > +bool calipso_validate(const struct sk_buff *skb, const unsigned char *option)
> > +{
> > + struct calipso_doi *doi_def;
> > + int ret_val;
> > + u16 crc, len = option[1] + 2;
> > + static const u8 zero[2];
> > +
> > + /* The original CRC runs over the option including the TLV header
> > + * with the CRC-16 field (at offset 8) zeroed out. */
> > + crc = crc_ccitt(0xffff, option, 8);
> > + crc = crc_ccitt(crc, zero, sizeof(zero));
> > + if (len > 10)
> > + crc = crc_ccitt(crc, option + 10, len - 10);
> > + crc = ~crc;
>
> I should have caught this in the v2 patchset when I mentioned it with
> respect to the CRC generation, but why not simply do 'crc =
> ~crc_cccitt(...);'?
Simply because the final crc_ccitt() is inside an if statement.
Since len is guaranteed to be >= 10, I could dispense with the if and
have crc_ccitt() handle having its final argument equal to zero (which
it does just fine). Then I could do as you suggest.
> Also, while I'm looking at this, why not do the CRC verification in
> ipv6_hop_calipso()? The only thing we should need to do here is the
> DOI lookup/verification so that we still work correctly when
> CONFIG_NETLABEL=n; all the core protocol stuff, e.g. length and
> checksum validation, should be done in the core stack functions, e.g.
> ipv6_hop_calipso().
The only reason was to not bring in CONFIG_CRC_CCITT if CONFIG_NETLABEL=n
(this is currently selected in net/netlabel/Kconfig).
If folks are happy for me to do so, I could change this to:
select CONFIG_CRC_CCITT
under menuconfig IPV6
Then the crc check could move to exthdrs.c:ipv6_hop_calipso(). Would
that be ok?
Thanks,
Huw.
> > + if (option[8] != (crc & 0xff) || option[9] != ((crc >> 8) & 0xff))
> > + return false;
> > +
> > + rcu_read_lock();
> > + doi_def = calipso_doi_search(get_unaligned_be32(option + 2));
> > + ret_val = !!doi_def;
> > + rcu_read_unlock();
> > +
> > + return ret_val;
> > +}
> > +
> > +/**
> > * calipso_map_cat_hton - Perform a category mapping from host to network
> > * @doi_def: the DOI definition
> > * @secattr: the security attributes
> > diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> > index d5fd3e7..0f69cab 100644
> > --- a/net/ipv6/exthdrs.c
> > +++ b/net/ipv6/exthdrs.c
> > @@ -43,6 +43,7 @@
> > #include <net/ndisc.h>
> > #include <net/ip6_route.h>
> > #include <net/addrconf.h>
> > +#include <net/calipso.h>
> > #if IS_ENABLED(CONFIG_IPV6_MIP6)
> > #include <net/xfrm.h>
> > #endif
> > @@ -603,6 +604,28 @@ drop:
> > return false;
> > }
> >
> > +/* CALIPSO RFC 5570 */
> > +
> > +static bool ipv6_hop_calipso(struct sk_buff *skb, int optoff)
> > +{
> > + const unsigned char *nh = skb_network_header(skb);
> > +
> > + if (nh[optoff + 1] < 8)
> > + goto drop;
> > +
> > + if (nh[optoff + 6] * 4 + 8 > nh[optoff + 1])
> > + goto drop;
> > +
> > + if (!calipso_validate(skb, nh + optoff))
> > + goto drop;
> > +
> > + return true;
> > +
> > +drop:
> > + kfree_skb(skb);
> > + return false;
> > +}
> > +
> > static const struct tlvtype_proc tlvprochopopt_lst[] = {
> > {
> > .type = IPV6_TLV_ROUTERALERT,
> > @@ -612,6 +635,10 @@ static const struct tlvtype_proc tlvprochopopt_lst[] = {
> > .type = IPV6_TLV_JUMBO,
> > .func = ipv6_hop_jumbo,
> > },
> > + {
> > + .type = IPV6_TLV_CALIPSO,
> > + .func = ipv6_hop_calipso,
> > + },
> > { -1, }
> > };
> >
> > --
> > 2.7.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> paul moore
> www.paul-moore.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v3 17/19] calipso: Add validation of CALIPSO option.
2016-05-09 10:39 ` Huw Davies
(?)
@ 2016-05-09 15:30 ` Paul Moore
-1 siblings, 0 replies; 6+ messages in thread
From: Paul Moore @ 2016-05-09 15:30 UTC (permalink / raw)
To: Huw Davies, netdev; +Cc: linux-security-module, selinux, Paul Moore
On Mon, May 9, 2016 at 6:39 AM, Huw Davies <huw@codeweavers.com> wrote:
> On Fri, May 06, 2016 at 06:59:32PM -0400, Paul Moore wrote:
>> On Wed, Feb 17, 2016 at 8:22 AM, Huw Davies <huw@codeweavers.com> wrote:
>> > We check lengths, checksum and the DOI. We leave checking of the
>> > level and categories for the socket layer.
>> >
>> > Signed-off-by: Huw Davies <huw@codeweavers.com>
>> > ---
>> > include/net/calipso.h | 6 ++++++
>> > net/ipv6/calipso.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>> > net/ipv6/exthdrs.c | 27 +++++++++++++++++++++++++++
>> > 3 files changed, 75 insertions(+)
>> >
>> > diff --git a/include/net/calipso.h b/include/net/calipso.h
>> > index 38dbb47..85404e2 100644
>> > --- a/include/net/calipso.h
>> > +++ b/include/net/calipso.h
>> > @@ -65,6 +65,7 @@ struct calipso_doi {
>> > #ifdef CONFIG_NETLABEL
>> > int __init calipso_init(void);
>> > void calipso_exit(void);
>> > +bool calipso_validate(const struct sk_buff *skb, const unsigned char *option);
>> > #else
>> > static inline int __init calipso_init(void)
>> > {
>> > @@ -74,6 +75,11 @@ static inline int __init calipso_init(void)
>> > static inline void calipso_exit(void)
>> > {
>> > }
>> > +static inline bool calipso_validate(const struct sk_buff *skb,
>> > + const unsigned char *option)
>> > +{
>> > + return true;
>> > +}
>> > #endif /* CONFIG_NETLABEL */
>> >
>> > #endif /* _CALIPSO_H */
>> > diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
>> > index fa371a8..b8bcf9f 100644
>> > --- a/net/ipv6/calipso.c
>> > +++ b/net/ipv6/calipso.c
>> > @@ -321,6 +321,48 @@ doi_walk_return:
>> > }
>> >
>> > /**
>> > + * calipso_validate - Validate a CALIPSO option
>> > + * @skb: the packet
>> > + * @option: the start of the option
>> > + *
>> > + * Description:
>> > + * This routine is called to validate a CALIPSO option.
>> > + * If the option is valid then a zero value is returned. If the
>> > + * option is invalid then a non-zero value is returned and
>> > + * representing the offset to the offending portion of the option.
>> > + *
>> > + * The caller should have already checked that the length of the
>> > + * option (including the TLV header) is >= 10 and that the catmap
>> > + * length is consistent with the option length.
>> > + *
>> > + * We leave checks on the level and categories to the socket layer.
>> > + */
>> > +bool calipso_validate(const struct sk_buff *skb, const unsigned char *option)
>> > +{
>> > + struct calipso_doi *doi_def;
>> > + int ret_val;
>> > + u16 crc, len = option[1] + 2;
>> > + static const u8 zero[2];
>> > +
>> > + /* The original CRC runs over the option including the TLV header
>> > + * with the CRC-16 field (at offset 8) zeroed out. */
>> > + crc = crc_ccitt(0xffff, option, 8);
>> > + crc = crc_ccitt(crc, zero, sizeof(zero));
>> > + if (len > 10)
>> > + crc = crc_ccitt(crc, option + 10, len - 10);
>> > + crc = ~crc;
>>
>> I should have caught this in the v2 patchset when I mentioned it with
>> respect to the CRC generation, but why not simply do 'crc =
>> ~crc_cccitt(...);'?
>
> Simply because the final crc_ccitt() is inside an if statement.
> Since len is guaranteed to be >= 10, I could dispense with the if and
> have crc_ccitt() handle having its final argument equal to zero (which
> it does just fine). Then I could do as you suggest.
Yes, I get that it is in an if-conditional; I probably should have
been more specific. I was thinking of something more like this:
if (len > 10)
crc = ~crc(crc, option + 10, ...);
else
crc = ~crc;
... although now that I've typed it out, I'm not sure it's an
improvement. I would just ignore me ;)
>> Also, while I'm looking at this, why not do the CRC verification in
>> ipv6_hop_calipso()? The only thing we should need to do here is the
>> DOI lookup/verification so that we still work correctly when
>> CONFIG_NETLABEL=n; all the core protocol stuff, e.g. length and
>> checksum validation, should be done in the core stack functions, e.g.
>> ipv6_hop_calipso().
>
> The only reason was to not bring in CONFIG_CRC_CCITT if CONFIG_NETLABEL=n
> (this is currently selected in net/netlabel/Kconfig).
>
> If folks are happy for me to do so, I could change this to:
> select CONFIG_CRC_CCITT
> under menuconfig IPV6
>
> Then the crc check could move to exthdrs.c:ipv6_hop_calipso(). Would
> that be ok?
Thanks, that explains it.
I think there is value in verifying the checksum, but I completely
understand if the netdev folks don't want to pull in the CRC_CCITT
code, especially since currently only a few network drivers/protocols
pull it into the build.
DaveM, your thoughts?
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-09 15:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-17 13:22 [RFC PATCH v3 17/19] calipso: Add validation of CALIPSO option Huw Davies
2016-02-17 13:22 ` Huw Davies
2016-05-06 22:59 ` Paul Moore
[not found] ` <CAHC9VhTRp8bHgwxZ2YNj=pa2a4PtO=8jJT1siG99GiFYnwm53Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-05-09 10:39 ` Huw Davies
2016-05-09 10:39 ` Huw Davies
2016-05-09 15:30 ` Paul Moore
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.