| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 4 Sep 2022 17:44:26 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Emeel Hakim <ehakim@...dia.com>
Cc: dsahern@...nel.org, netdev@...r.kernel.org, raeds@...dia.com,
tariqt@...dia.com
Subject: Re: [PATCH main v3 1/2] macsec: add Extended Packet Number support
2022-09-04, 10:47:28 +0300, Emeel Hakim wrote:
> @@ -174,14 +186,36 @@ static int parse_sa_args(int *argcp, char ***argvp, struct sa_desc *sa)
>
> while (argc > 0) {
> if (strcmp(*argv, "pn") == 0) {
> - if (sa->pn != 0)
> + if (sa->pn.pn64 != 0)
> duparg2("pn", "pn");
> NEXT_ARG();
> - ret = get_u32(&sa->pn, *argv, 0);
> + ret = get_u32(&sa->pn.pn32, *argv, 0);
> + if (ret)
> + invarg("expected pn", *argv);
> + if (sa->pn.pn32 == 0)
> + invarg("expected pn != 0", *argv);
> + } else if (strcmp(*argv, "xpn") == 0) {
> + if (sa->pn.pn64 != 0)
> + duparg2("xpn", "xpn");
> + NEXT_ARG();
> + ret = get_u64(&sa->pn.pn64, *argv, 0);
> if (ret)
> invarg("expected pn", *argv);
> - if (sa->pn == 0)
> + if (sa->pn.pn64 == 0)
> invarg("expected pn != 0", *argv);
> + sa->xpn = true;
> + } else if (strcmp(*argv, "salt") == 0) {
> + unsigned int len;
> +
> + NEXT_ARG();
That should have a duparg check.
> + if (!hexstring_a2n(*argv, sa->salt, MACSEC_SALT_LEN,
> + &len))
> + invarg("expected salt", *argv);
> + } else if (strcmp(*argv, "ssci") == 0) {
> + NEXT_ARG();
Also worth a duparg check.
> + ret = get_ssci(&sa->ssci, *argv);
> + if (ret)
> + invarg("expected ssci", *argv);
> } else if (strcmp(*argv, "key") == 0) {
> unsigned int len;
>
> @@ -392,9 +426,22 @@ static int do_modify_nl(enum cmd c, enum macsec_nl_commands cmd, int ifindex,
> addattr8(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_AN, sa->an);
>
> if (c != CMD_DEL) {
> - if (sa->pn)
> + if (sa->xpn) {
> + if (sa->pn.pn64)
> + addattr64(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_PN,
> + sa->pn.pn64);
> + if (sa->salt[0] != '\0')
Does the specification say the salt can't start with a 0 byte, or is
it just a "salt was set" test? If that's coming from the spec, the
kernel should also check this.
> + addattr_l(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_SALT,
> + sa->salt, MACSEC_SALT_LEN);
> + if (sa->ssci != 0)
Same question as for the salt.
For both, I'd just add a salt_set/ssci_set flag to sa_desc, and use it
for duparg as well.
> + addattr32(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_SSCI,
> + sa->ssci);
> + }
> +
> + if (sa->pn.pn32 && !sa->xpn) {
Nit: combine this with the previous if:
} else if (sa->pn.pn32) {
> addattr32(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_PN,
> - sa->pn);
> + sa->pn.pn32);
> + }
>
> if (sa->key_len) {
> addattr_l(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_KEYID,
> @@ -426,10 +473,11 @@ static bool check_sa_args(enum cmd c, struct sa_desc *sa)
> return -1;
> }
>
> - if (sa->pn == 0) {
> + if (sa->pn.pn64 == 0) {
> fprintf(stderr, "must specify a packet number != 0\n");
> return -1;
> }
> +
Remove that blank line.
> } else if (c == CMD_UPD) {
> if (sa->key_len) {
> fprintf(stderr, "cannot change key on SA\n");
[...]
> @@ -875,8 +930,16 @@ static void print_tx_sc(const char *prefix, __u64 sci, __u8 encoding_sa,
> print_string(PRINT_FP, NULL, "%s", prefix);
> print_uint(PRINT_ANY, "an", "%d:",
> rta_getattr_u8(sa_attr[MACSEC_SA_ATTR_AN]));
> - print_uint(PRINT_ANY, "pn", " PN %u,",
> - rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> + if (!is_xpn) {
Nit: I'd flip those branches so that the test is "if (is_xpn)"
> + print_uint(PRINT_ANY, "pn", " PN %u,",
> + rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> + } else {
> + print_uint(PRINT_ANY, "pn", " PN %u,",
> + rta_getattr_u64(sa_attr[MACSEC_SA_ATTR_PN]));
> + print_0xhex(PRINT_ANY, "ssci",
> + "SSCI %08x",
> + ntohl(rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_SSCI])));
> + }
>
> print_bool(PRINT_JSON, "active", NULL, state);
> print_string(PRINT_FP, NULL,
[...]
> @@ -943,8 +1007,16 @@ static void print_rx_sc(const char *prefix, __be64 sci, __u8 active,
> print_string(PRINT_FP, NULL, "%s", prefix);
> print_uint(PRINT_ANY, "an", "%u:",
> rta_getattr_u8(sa_attr[MACSEC_SA_ATTR_AN]));
> - print_uint(PRINT_ANY, "pn", " PN %u,",
> - rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> + if (!is_xpn) {
Same nit, flip the test.
> + print_uint(PRINT_ANY, "pn", " PN %u,",
> + rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> + } else {
> + print_uint(PRINT_ANY, "pn", " PN %u,",
> + rta_getattr_u64(sa_attr[MACSEC_SA_ATTR_PN]));
> + print_0xhex(PRINT_ANY, "ssci",
> + "SSCI %08x",
> + ntohl(rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_SSCI])));
> + }
>
> print_bool(PRINT_JSON, "active", NULL, state);
> print_string(PRINT_FP, NULL, " state %s,",
[...]
> @@ -989,6 +1062,8 @@ static int process(struct nlmsghdr *n, void *arg)
> int ifindex;
> __u64 sci;
> __u8 encoding_sa;
> + __u64 cid;
> + bool is_xpn = false;
>
> if (n->nlmsg_type != genl_family)
> return -1;
> @@ -1032,13 +1107,16 @@ static int process(struct nlmsghdr *n, void *arg)
>
> print_attrs(attrs_secy);
>
> - print_tx_sc(" ", sci, encoding_sa,
> + cid = rta_getattr_u64(attrs_secy[MACSEC_SECY_ATTR_CIPHER_SUITE]);
> + if (cid == MACSEC_CIPHER_ID_GCM_AES_XPN_128 || cid == MACSEC_CIPHER_ID_GCM_AES_XPN_256)
I'd stuff that in a ciphersuite_is_xpn(cid) helper, and then it's just
is_xpn = ciphersuite_is_xpn(cid);
> + is_xpn = true;
> + print_tx_sc(" ", sci, encoding_sa, is_xpn,
> attrs[MACSEC_ATTR_TXSC_STATS],
> attrs[MACSEC_ATTR_SECY_STATS],
> attrs[MACSEC_ATTR_TXSA_LIST]);
[...]
> @@ -1268,7 +1347,7 @@ static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
> enum macsec_offload offload;
> struct cipher_args cipher = {0};
> enum macsec_validation_type validate;
> - bool es = false, scb = false, send_sci = false;
> + bool es = false, scb = false, send_sci = false, xpn = false;
> int replay_protect = -1;
> struct sci sci = { 0 };
>
> @@ -1388,6 +1467,11 @@ static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
> return ret;
> addattr8(n, MACSEC_BUFLEN,
> IFLA_MACSEC_OFFLOAD, offload);
> + } else if (strcmp(*argv, "xpn") == 0) {
> + NEXT_ARG();
> + xpn = parse_on_off("xpn", *argv, &ret);
I'm not convinced the "xpn on/off" flag is the best API here. How
about just letting the admin pass the full cipher suite name
(GCM-AES-XPN-128 or GCM-AES-XPN-256)?
This xpn flag is not consistent with the dump side (which uses the
cipher suite only), and it's not consistent with the kernel API, which
also doesn't use an XPN flag but the cipher suite ID.
So I'd just add 2 options to "cipher" argument parsing (and extract
that to a new cs_name_to_id helper, since we'd now have 4 options),
and get rid of the "xpn on/off" option (and the cipher.id flip to XPN
below).
> + if (ret != 0)
> + return ret;
> } else {
> fprintf(stderr, "macsec: unknown command \"%s\"?\n",
> *argv);
> @@ -1415,6 +1499,13 @@ static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
> return -1;
> }
>
> + if (xpn) {
> + if (cipher.id == MACSEC_CIPHER_ID_GCM_AES_256)
> + cipher.id = MACSEC_CIPHER_ID_GCM_AES_XPN_256;
> + else
> + cipher.id = MACSEC_CIPHER_ID_GCM_AES_XPN_128;
> + }
> +
> if (cipher.id)
> addattr_l(n, MACSEC_BUFLEN, IFLA_MACSEC_CIPHER_SUITE,
> &cipher.id, sizeof(cipher.id));
> --
> 2.21.3
>
--
Sabrina
Powered by blists - more mailing lists