search

Lora-net / SWL2001

LoRa Basics Modem LoRaWAN stack
BSD 3-Clause Clear License
87 stars 50 forks source link

v4.5.0 relay_tx_check_decode_ack() does not calculate MIC according to TS011-1.0.0 #62

Open cjhdev opened 1 month ago

cjhdev commented 1 month ago

I think there is a mistake in v4.5.0 relay_tx_check_decode_ack() where the WOR channel frequency and datarate is used to calculate the ACK MIC, instead of the frequency and datarate extracted from the WOR frame that is being acknowledged.

    const relay_tx_channel_config_t* conf =
        ( infos->last_ch_idx == 0 ) ? &infos->default_ch_config : &( infos->relay_tx_config.second_ch );

    wor_ack_mic_info_t ack_mic_info = {
        .dev_addr     = lorawan_api_devaddr_get( relay_stack_id ),
        .wfcnt        = infos->fcnt,
        .frequency_hz = conf->freq_hz,
        .datarate     = conf->dr,
    };

    // SMTC_MODEM_HAL_TRACE_PRINTF( "frequency_hz = %d, infos->last_ch_idx = %d ,  .dev_addr   = %x, datarate = %d\n",
    // conf->ack_freq_hz,infos->last_ch_idx,lorawan_api_devaddr_get( relay_stack_id ),conf->dr);
    //  Key is set to NULL because it is already save in the crypto element (soft or hard)
    const uint32_t mic_calc    = wor_compute_mic_ack( &ack_mic_info, infos->buffer, NULL );

The values are set in ack_mic_info which is then passed to wor_compute_mic_ack().

Line 793 of TS011-1.0.0 defines the MIC as:

CMAC = aes128_cmac(WorSIntKey, B0 | AckUplinkEnc | WOR | pad16)

Table 20 shows how to make WOR.

Line 797 immediately before table 20 says:

WOR contains the data received in the WOR Relay Class A Uplink and is defined as:

opeyrard commented 1 month ago

Thank you very much for you feedback. After confirmation with LoRa Alliance members, the expected behavior is the one you implemented. Similarly to the layer2 spec evolution, the intend is to add physical layer elements to the MIC. This reduces the risk of replay attack of the same frame on a different frequency and data rate.

cjhdev commented 1 month ago

One more thing to add to this ticket, I also notice that wor_compute_mic_ack() doesn't add the pad16 field when calculating the MIC.

https://github.com/Lora-net/SWL2001/blob/d05dad5b4df9c0e5899008e43bfce48f5498abb5/lbm_lib/smtc_modem_core/lr1mac/src/relay/common/wake_on_radio.c#L279

Perhaps this array should be 16B long and initialized to zero.

opeyrard commented 3 weeks ago

Hi, This will be fixed in the next coming release.