Re: [PATCH v5 3/4] dpp: add support for configuration protocol

James Prestwood
2021-12-17T21:21:04Z

[-- Attachment #1: Type: text/plain, Size: 8624 bytes --]

On Fri, 2021-12-17 at 14:22 -0600, Denis Kenzior wrote:
> Hi James,
>
> On 12/17/21 13:14, James Prestwood wrote:
> > This is a minimal implementation only supporting legacy network
> > configuration, i.e. only SSID and PSK/passphrase are supported.
> >
> > Missing features include:
> >   - Fragmentation/comeback delay support
> >   - DPP AKM support
> >   - 8021x/PKEX support
> > ---
> >   src/dpp.c | 328
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 328 insertions(+)
> >
> > v5:
> >   * Updated use of wrap/unwrap APIs
> >   * Added comments about how AAD is used with configuration
> >
>
> <snip>
>
> > +static void dpp_handle_config_response_frame(const struct
> > mmpdu_header *frame,
> > +                               const void *body, size_t body_len,
> > +                               int rssi, void *user_data)
> > +{
> > +       struct dpp_sm *dpp = user_data;
> > +       const uint8_t *ptr = body;
> > +       uint16_t status;
> > +       uint16_t fragmented; /* Fragmented/Comeback delay field */
> > +       uint8_t adv_protocol_element[] = { 0x6C, 0x08, 0x7F };
> > +       uint8_t adv_protocol_id[] = { 0xDD, 0x05, 0x50, 0x6F,
> > +                                       0x9A, 0x1A, 0x01 };
> > +       uint16_t query_len;
> > +       struct dpp_attr_iter iter;
> > +       enum dpp_attribute_type type;
> > +       size_t len;
> > +       const uint8_t *data;
> > +       const char *json = NULL;
> > +       size_t json_len = 0;
> > +       int dstatus = -1;
> > +       const uint8_t *wrapped = NULL;
> > +       const uint8_t *e_nonce = NULL;
> > +       size_t wrapped_len = 0;
> > +       _auto_(l_free) uint8_t *unwrapped = NULL;
> > +       struct dpp_configuration *config;
> > +       uint8_t ad0[] = { 0x00, 0x10, 0x01, 0x00, 0x05 };
> > +
> > +       if (dpp->state != DPP_STATE_CONFIGURING)
> > +               return;
> > +
> > +       ptr += 2;
> > +
> > +       /*
> > +        * Can a configuration request come from someone other than
> > who you
> > +        * authenticated to?
> > +        */
> > +       if (memcmp(dpp->auth_addr, frame->address_2, 6))
> > +               return;
>
> Hmm, I'm not seeing any length checking?

Yeah, I will add a check for this entire header since its pretty
static.

>
> > +
> > +       if (*ptr++ != dpp->diag_token)
> > +               return;
> > +
> > +       status = l_get_le16(ptr);
> > +       ptr += 2;
> > +
> > +       if (status != 0) {
> > +               l_debug("Bad configuration status %u", status);
> > +               return;
> > +       }
> > +
> > +       fragmented = l_get_le16(ptr);
> > +       ptr += 2;
> > +
> > +       /*
> > +        * TODO: handle 0x0001 (fragmented), as well as comeback
> > delay.
> > +        */
> > +       if (fragmented != 0) {
> > +               l_debug("Fragmented messages not currently
> > supported");
> > +               return;
> > +       }
> > +
> > +       if (memcmp(ptr, adv_protocol_element,
> > sizeof(adv_protocol_element))) {
> > +               l_debug("Invalid Advertisement protocol element");
> > +               return;
> > +       }
> > +
> > +       ptr += sizeof(adv_protocol_element);
> > +
> > +       if (memcmp(ptr, adv_protocol_id, sizeof(adv_protocol_id)))
> > {
> > +               l_debug("Invalid Advertisement protocol ID");
> > +               return;
> > +       }
> > +
> > +       ptr += sizeof(adv_protocol_id);
> > +
> > +       query_len = l_get_le16(ptr);
> > +       ptr += 2;
> > +
>
> How do we know query_len is valid for
>
> > +       dpp_attr_iter_init(&iter, ptr, query_len);
>
> this invocation?

yeah, that was the idea. But body_len needs to come into play here
since query_len could just as well be 0xFFFF.

>
> > +
> > +       while (dpp_attr_iter_next(&iter, &type, &len, &data)) {
> > +               switch (type) {
> > +               case DPP_ATTR_STATUS:
> > +                       dstatus = l_get_u8(data);
> > +                       break;
> > +               case DPP_ATTR_WRAPPED_DATA:
> > +                       wrapped = data;
> > +                       wrapped_len = len;
> > +                       break;
> > +               default:
> > +                       /*
> > +                        * TODO: CSR Attribute
> > +                        */
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (dstatus != DPP_STATUS_OK || !wrapped) {
> > +               l_debug("Bad status or missing attributes");
> > +               return;
> > +       }
> > +
> > +       unwrapped = dpp_unwrap_attr(ad0, sizeof(ad0), NULL, 0, dpp-
> > >ke,
> > +                                       dpp->key_len, wrapped,
> > wrapped_len,
> > +                                       &wrapped_len);
> > +       if (!unwrapped) {
> > +               l_debug("Failed to unwrap");
> > +               return;
> > +       }
> > +
> > +       dpp_attr_iter_init(&iter, unwrapped, wrapped_len);
> > +
> > +       while (dpp_attr_iter_next(&iter, &type, &len, &data)) {
> > +               switch (type) {
> > +               case DPP_ATTR_ENROLLEE_NONCE:
> > +                       if (len != dpp->nonce_len)
> > +                               break;
> > +
> > +                       if (memcmp(data, dpp->e_nonce, dpp-
> > >nonce_len))
> > +                               break;
> > +
> > +                       e_nonce = data;
> > +                       break;
> > +               case DPP_ATTR_CONFIGURATION_OBJECT:
> > +                       json = (const char *)data;
> > +                       json_len = len;
> > +                       break;
> > +               default:
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (!json || !e_nonce) {
> > +               l_debug("No configuration object in response");
> > +               return;
> > +       }
> > +
> > +       config = dpp_parse_configuration_object(json, json_len);
> > +       if (!config) {
> > +               l_error("Configuration object did not parse");
> > +               return;
> > +       }
> > +
> > +       dpp_write_config(config);
> > +       /*
> > +        * TODO: Depending on the info included in the
> > configuration object a
> > +        * limited scan could be issued to get autoconnect to
> > trigger faster.
> > +        * In addition this network may already be in past scan
> > results and
> > +        * could be joined immediately.
> > +        *
> > +        * For now just wait for autoconnect.
> > +        */
> > +
> > +       dpp_configuration_free(config);
> > +
> > +       send_config_result(dpp, dpp->auth_addr);
> > +}
> > +
> >   /*
> >    * The Authentication protocol has a consistent use of AD
> > components, and this
> >    * use is defined in 6.3.1.4:
>
> Regards,
> -Denis