* SMP Key distribution @ 2011-12-05 9:02 Ganir, Chen 2011-12-05 15:39 ` Brian Gix 0 siblings, 1 reply; 7+ messages in thread From: Ganir, Chen @ 2011-12-05 9:02 UTC (permalink / raw) To: linux-bluetooth Hi. According to the SMP spec (Vol3, Part H, Appendix 5.3.4), keys are distributed in a specific order, where the slave first sends its LTK,EDIV, RAND , IRK, ADDR TYPE and signature key (according to the key distribution parameter. Only when the slave completes its key distribution, the master then starts distributing its own keys (same order, according to the master key distribution options). In the current implementation in the smp.c, it seems that we start distributing our keys too early (after the MASTER IDENTIFICATION message, ignoring the possibility of Addr type and signature keys which may come from the slave. This may break the key distribution phase. Has anyone seen this or has anything to comment on that ? Thanks, Chen Ganir ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: SMP Key distribution 2011-12-05 9:02 SMP Key distribution Ganir, Chen @ 2011-12-05 15:39 ` Brian Gix 2011-12-05 15:48 ` Ganir, Chen 0 siblings, 1 reply; 7+ messages in thread From: Brian Gix @ 2011-12-05 15:39 UTC (permalink / raw) To: Ganir, Chen; +Cc: linux-bluetooth Hi Chen, On 12/5/2011 1:02 AM, Ganir, Chen wrote: > Hi. > > According to the SMP spec (Vol3, Part H, Appendix 5.3.4), keys are distributed in a specific order, where the slave first sends its LTK,EDIV, RAND , IRK, ADDR TYPE and signature key (according to the key distribution parameter. Only when the slave completes its key distribution, the master then starts distributing its own keys (same order, according to the master key distribution options). In the current implementation in the smp.c, it seems that we start distributing our keys too early (after the MASTER IDENTIFICATION message, ignoring the possibility of Addr type and signature keys which may come from the slave. This may break the key distribution phase. > > Has anyone seen this or has anything to comment on that ? > Currently the SMP code is hard coded to only support a single key, which is the LTK+MID from the Slave to the Master. You are correct that we will need to ensure that all of the keys are handled correctly, including ensuring proper order, once we add support for additional key distribution. At the moment though, that is a moot question. -- Brian Gix bgix@codeaurora.org Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: SMP Key distribution 2011-12-05 15:39 ` Brian Gix @ 2011-12-05 15:48 ` Ganir, Chen 2011-12-05 17:44 ` Brian Gix 0 siblings, 1 reply; 7+ messages in thread From: Ganir, Chen @ 2011-12-05 15:48 UTC (permalink / raw) To: Brian Gix; +Cc: linux-bluetooth Brian, > -----Original Message----- > From: Brian Gix [mailto:bgix@codeaurora.org] > Sent: Monday, December 05, 2011 5:39 PM > To: Ganir, Chen > Cc: linux-bluetooth@vger.kernel.org > Subject: Re: SMP Key distribution > > Hi Chen, > > On 12/5/2011 1:02 AM, Ganir, Chen wrote: > > Hi. > > > > According to the SMP spec (Vol3, Part H, Appendix 5.3.4), keys are > distributed in a specific order, where the slave first sends its > LTK,EDIV, RAND , IRK, ADDR TYPE and signature key (according to the key > distribution parameter. Only when the slave completes its key > distribution, the master then starts distributing its own keys (same > order, according to the master key distribution options). In the > current implementation in the smp.c, it seems that we start > distributing our keys too early (after the MASTER IDENTIFICATION > message, ignoring the possibility of Addr type and signature keys which > may come from the slave. This may break the key distribution phase. > > > > Has anyone seen this or has anything to comment on that ? > > > > Currently the SMP code is hard coded to only support a single key, > which > is the LTK+MID from the Slave to the Master. > > You are correct that we will need to ensure that all of the keys are > handled correctly, including ensuring proper order, once we add support > for additional key distribution. At the moment though, that is a moot > question. > > > -- > Brian Gix > bgix@codeaurora.org > Employee of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum I understand that we support only those keys, but I don't think that we can break the convention just because of our lack of support. We can ignore any other keys, but still we need to start distributing our keys only after the slave has finished sending all of his keys, and not do it automatically after the Master identification sent from the slave. In addition, I believe there is a problem in the SMP code. In smp_cmd_master_ident, we do hci_add_ltk() with the conn->src instead of conn->dst, as in other places we call the hci_add_ltk(): static int smp_cmd_master_ident(struct l2cap_conn *conn, struct sk_buff *skb) { struct smp_cmd_master_ident *rp = (void *) skb->data; struct smp_chan *smp = conn->smp_chan; skb_pull(skb, sizeof(*rp)); hci_add_ltk(conn->hcon->hdev, 1, conn->src, smp->smp_key_size, rp->ediv, rp->rand, smp->tk); smp_distribute_keys(conn, 1); return 0; } In addition, we call the hci_add_ltk twice in this case - once from this function, and once from the smp_distribute_keys. Is this intentional or is this a bug ? Thanks, Chen Ganir. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: SMP Key distribution 2011-12-05 15:48 ` Ganir, Chen @ 2011-12-05 17:44 ` Brian Gix 2011-12-06 7:43 ` Ganir, Chen 0 siblings, 1 reply; 7+ messages in thread From: Brian Gix @ 2011-12-05 17:44 UTC (permalink / raw) To: Ganir, Chen; +Cc: linux-bluetooth Hi Chen, On 12/5/2011 7:48 AM, Ganir, Chen wrote: > Brian, > > I understand that we support only those keys, but I don't think that we can break the convention just because of our lack of support. We can ignore any other keys, but still we need to start distributing our keys only after the slave has finished sending all of his keys, and not do it automatically after the Master identification sent from the slave. > You are correct, and this is definitely one of the things I hope to address (if someone else doesn't beat me to it) once I get the MITM changes that I have submitted accepted into bluetooth-next. > In addition, I believe there is a problem in the SMP code. In smp_cmd_master_ident, we do hci_add_ltk() with the conn->src instead of conn->dst, as in other places we call the hci_add_ltk(): > This may or may not be a bug, depending on how you look at things. When "Privacy" is implemented, it will be possible for a remote device to "change addresses" on us (either via the "Address Resolution" key distribution, or via the GAP primary service) and it becomes "more interesting" to keep track of who is who. Saving the LTK with the SRC (local) address might work, because LTK resolution is suppose to be done by matching on the EDIV and Randomizer, not by address. Having said that, however, If we are the Master (which most Dual mode devices will generally be) we still need to know the remote devices BDADDR, so that we can establish the connection, and thereafter kick off link encryption. So yes, I would still consider that the Bug: The Master gets an LTK+MID from the Slave, which will be used to encrypt links as long as those roles do not change. Therefore, the Master should be storing the LTK+MID pair under that Slaves Identity (BDADDR). One of the catches being however, that if we get Address Resolution keys, or the slaves GAP Primary Service tells us to use a different Address, this storage "key" will need to be adjusted. > static int smp_cmd_master_ident(struct l2cap_conn *conn, struct sk_buff *skb) > { > struct smp_cmd_master_ident *rp = (void *) skb->data; > struct smp_chan *smp = conn->smp_chan; > > skb_pull(skb, sizeof(*rp)); > > hci_add_ltk(conn->hcon->hdev, 1, conn->src, smp->smp_key_size, > rp->ediv, rp->rand, smp->tk); > > smp_distribute_keys(conn, 1); > > return 0; > } > > In addition, we call the hci_add_ltk twice in this case - once from this function, and once from the smp_distribute_keys. Is this intentional or is this a bug ? This is not actually a bug. Or at least not what I would consider a serious bug. The LTK key that the Slave sends to the Master, is distinct from the key the Master sends to the Slave. However, as I indicated above, an LTK+MID pair that is distributed "outbound" should not be associated with the remote devices BDADDR. It should be stored using a lookup key that is the EDIV, which is authenticated with the Randomizer. > > Thanks, > Chen Ganir. -- Brian Gix bgix@codeaurora.org Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: SMP Key distribution 2011-12-05 17:44 ` Brian Gix @ 2011-12-06 7:43 ` Ganir, Chen 2011-12-06 18:16 ` Brian Gix 0 siblings, 1 reply; 7+ messages in thread From: Ganir, Chen @ 2011-12-06 7:43 UTC (permalink / raw) To: Brian Gix; +Cc: linux-bluetooth Brian, > -----Original Message----- > From: Brian Gix [mailto:bgix@codeaurora.org] > Sent: Monday, December 05, 2011 7:45 PM > To: Ganir, Chen > Cc: linux-bluetooth@vger.kernel.org > Subject: Re: SMP Key distribution > > Hi Chen, > > On 12/5/2011 7:48 AM, Ganir, Chen wrote: > > Brian, > > > > I understand that we support only those keys, but I don't think that > we can break the convention just because of our lack of support. We can > ignore any other keys, but still we need to start distributing our keys > only after the slave has finished sending all of his keys, and not do > it automatically after the Master identification sent from the slave. > > > > You are correct, and this is definitely one of the things I hope to > address (if someone else doesn't beat me to it) once I get the MITM > changes that I have submitted accepted into bluetooth-next. > So basically what we need to do is listen for the SMP_CMD_MASTER_IDENT, SMP_CMD_IDENT_INFO, SMP_CMD_IDENT_ADDR_INFO and SMP_CMD_SIGN_INFO, and check the slave key distribution options to see how many keys he is sending. If we get one of these and it is actually the last key from the slave, and then distribute the master keys with the smp_distribute_keys (simply call a new function like smp_check_last_slave_key() which will do this work). Am I correct ? > > In addition, I believe there is a problem in the SMP code. In > smp_cmd_master_ident, we do hci_add_ltk() with the conn->src instead of > conn->dst, as in other places we call the hci_add_ltk(): > > > > This may or may not be a bug, depending on how you look at things. > When > "Privacy" is implemented, it will be possible for a remote device to > "change addresses" on us (either via the "Address Resolution" key > distribution, or via the GAP primary service) and it becomes "more > interesting" to keep track of who is who. Saving the LTK with the SRC > (local) address might work, because LTK resolution is suppose to be > done > by matching on the EDIV and Randomizer, not by address. > > Having said that, however, If we are the Master (which most Dual mode > devices will generally be) we still need to know the remote devices > BDADDR, so that we can establish the connection, and thereafter kick > off > link encryption. So yes, I would still consider that the Bug: The > Master gets an LTK+MID from the Slave, which will be used to encrypt > links as long as those roles do not change. Therefore, the Master > should be storing the LTK+MID pair under that Slaves Identity (BDADDR). > > One of the catches being however, that if we get Address Resolution > keys, or the slaves GAP Primary Service tells us to use a different > Address, this storage "key" will need to be adjusted. > Calling the hci_add_ltk with the conn->src actually causes the code to call mgmt_new_link_key with the local and remote addresses to be the same, which then causes the btd_event_link_key_notify(...) -> get_adapter_and_device(...) -> adapter_get_device(...) -> adapter_create_device(...) to create a new device, with the local address instead of the remote one. This is mostly due to the fact that get_adapter_and_device is called with the create flag hard coded as TRUE, even for this problematic key. What do you think about this ? > > static int smp_cmd_master_ident(struct l2cap_conn *conn, struct > sk_buff *skb) > > { > > struct smp_cmd_master_ident *rp = (void *) skb->data; > > struct smp_chan *smp = conn->smp_chan; > > > > skb_pull(skb, sizeof(*rp)); > > > > hci_add_ltk(conn->hcon->hdev, 1, conn->src, smp->smp_key_size, > > rp->ediv, rp->rand, smp->tk); > > > > smp_distribute_keys(conn, 1); > > > > return 0; > > } > > > > In addition, we call the hci_add_ltk twice in this case - once from > this function, and once from the smp_distribute_keys. Is this > intentional or is this a bug ? > > This is not actually a bug. Or at least not what I would consider a > serious bug. The LTK key that the Slave sends to the Master, is > distinct from the key the Master sends to the Slave. > > However, as I indicated above, an LTK+MID pair that is distributed > "outbound" should not be associated with the remote devices BDADDR. It > should be stored using a lookup key that is the EDIV, which is > authenticated with the Randomizer. > > > > > Thanks, > > Chen Ganir. > > > -- > Brian Gix > bgix@codeaurora.org > Employee of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum Thanks, Chen Ganir. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: SMP Key distribution 2011-12-06 7:43 ` Ganir, Chen @ 2011-12-06 18:16 ` Brian Gix 2011-12-07 0:54 ` Vinicius Costa Gomes 0 siblings, 1 reply; 7+ messages in thread From: Brian Gix @ 2011-12-06 18:16 UTC (permalink / raw) To: Ganir, Chen; +Cc: linux-bluetooth Hi Chen, On 12/5/2011 11:43 PM, Ganir, Chen wrote: > Brian, > [...] >> You are correct, and this is definitely one of the things I hope to >> address (if someone else doesn't beat me to it) once I get the MITM >> changes that I have submitted accepted into bluetooth-next. >> > So basically what we need to do is listen for the SMP_CMD_MASTER_IDENT, SMP_CMD_IDENT_INFO, SMP_CMD_IDENT_ADDR_INFO and SMP_CMD_SIGN_INFO, and check the slave key distribution options to see how many keys he is sending. If we get one of these and it is actually the last key from the slave, and then distribute the master keys with the smp_distribute_keys (simply call a new function like smp_check_last_slave_key() which will do this work). Am I correct ? Basically, Yes. This will be necessary once support is added for those key types. Until then, it is desirable, but not critical for correct "over-the-air" operation. >>> In addition, I believe there is a problem in the SMP code. In >> smp_cmd_master_ident, we do hci_add_ltk() with the conn->src instead of >> conn->dst, as in other places we call the hci_add_ltk(): >>> >> >> This may or may not be a bug, depending on how you look at things. >> When >> "Privacy" is implemented, it will be possible for a remote device to >> "change addresses" on us (either via the "Address Resolution" key >> distribution, or via the GAP primary service) and it becomes "more >> interesting" to keep track of who is who. Saving the LTK with the SRC >> (local) address might work, because LTK resolution is suppose to be >> done >> by matching on the EDIV and Randomizer, not by address. >> >> Having said that, however, If we are the Master (which most Dual mode >> devices will generally be) we still need to know the remote devices >> BDADDR, so that we can establish the connection, and thereafter kick >> off >> link encryption. So yes, I would still consider that the Bug: The >> Master gets an LTK+MID from the Slave, which will be used to encrypt >> links as long as those roles do not change. Therefore, the Master >> should be storing the LTK+MID pair under that Slaves Identity (BDADDR). >> >> One of the catches being however, that if we get Address Resolution >> keys, or the slaves GAP Primary Service tells us to use a different >> Address, this storage "key" will need to be adjusted. >> > Calling the hci_add_ltk with the conn->src actually causes the code to call mgmt_new_link_key with the local and remote addresses to be the same, which then causes the btd_event_link_key_notify(...) -> get_adapter_and_device(...) -> adapter_get_device(...) -> adapter_create_device(...) to create a new device, with the local address instead of the remote one. This is mostly due to the fact that get_adapter_and_device is called with the create flag hard coded as TRUE, even for this problematic key. What do you think about this ? I'm sure that this needs to be worked on, and I am not suprised if this is not currently working correctly. Our code over on codeauroraforum.org already has made that equivilent change (to use the dst addr instead of the local), but I have not submitted it here yet, because I am trying to single-thread the changes versus fixes, to simplify the upstreaming process (and for my own personal sanity). If you were to submit it as a patch, you would have my full support :-) -- Brian Gix bgix@codeaurora.org Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: SMP Key distribution 2011-12-06 18:16 ` Brian Gix @ 2011-12-07 0:54 ` Vinicius Costa Gomes 0 siblings, 0 replies; 7+ messages in thread From: Vinicius Costa Gomes @ 2011-12-07 0:54 UTC (permalink / raw) To: Brian Gix; +Cc: Ganir, Chen, linux-bluetooth On 10:16 Tue 06 Dec, Brian Gix wrote: > Hi Chen, > > On 12/5/2011 11:43 PM, Ganir, Chen wrote: > >Brian, > > > [...] > >>You are correct, and this is definitely one of the things I hope to > >>address (if someone else doesn't beat me to it) once I get the MITM > >>changes that I have submitted accepted into bluetooth-next. > >> > >So basically what we need to do is listen for the SMP_CMD_MASTER_IDENT, SMP_CMD_IDENT_INFO, SMP_CMD_IDENT_ADDR_INFO and SMP_CMD_SIGN_INFO, and check the slave key distribution options to see how many keys he is sending. If we get one of these and it is actually the last key from the slave, and then distribute the master keys with the smp_distribute_keys (simply call a new function like smp_check_last_slave_key() which will do this work). Am I correct ? > > Basically, Yes. > > This will be necessary once support is added for those key types. > Until then, it is desirable, but not critical for correct > "over-the-air" operation. > > >>>In addition, I believe there is a problem in the SMP code. In > >>smp_cmd_master_ident, we do hci_add_ltk() with the conn->src instead of > >>conn->dst, as in other places we call the hci_add_ltk(): > >>> > >> > >>This may or may not be a bug, depending on how you look at things. > >>When > >>"Privacy" is implemented, it will be possible for a remote device to > >>"change addresses" on us (either via the "Address Resolution" key > >>distribution, or via the GAP primary service) and it becomes "more > >>interesting" to keep track of who is who. Saving the LTK with the SRC > >>(local) address might work, because LTK resolution is suppose to be > >>done > >>by matching on the EDIV and Randomizer, not by address. > >> > >>Having said that, however, If we are the Master (which most Dual mode > >>devices will generally be) we still need to know the remote devices > >>BDADDR, so that we can establish the connection, and thereafter kick > >>off > >>link encryption. So yes, I would still consider that the Bug: The > >>Master gets an LTK+MID from the Slave, which will be used to encrypt > >>links as long as those roles do not change. Therefore, the Master > >>should be storing the LTK+MID pair under that Slaves Identity (BDADDR). > >> > >>One of the catches being however, that if we get Address Resolution > >>keys, or the slaves GAP Primary Service tells us to use a different > >>Address, this storage "key" will need to be adjusted. > >> > >Calling the hci_add_ltk with the conn->src actually causes the code to call mgmt_new_link_key with the local and remote addresses to be the same, which then causes the btd_event_link_key_notify(...) -> get_adapter_and_device(...) -> adapter_get_device(...) -> adapter_create_device(...) to create a new device, with the local address instead of the remote one. This is mostly due to the fact that get_adapter_and_device is called with the create flag hard coded as TRUE, even for this problematic key. What do you think about this ? > > > I'm sure that this needs to be worked on, and I am not suprised if > this is not currently working correctly. Our code over on > codeauroraforum.org already has made that equivilent change (to use > the dst addr instead of the local), but I have not submitted it here > yet, because I am trying to single-thread the changes versus fixes, > to simplify the upstreaming process (and for my own personal > sanity). If you were to submit it as a patch, you would have my full > support :-) I agree that it should work better. When implementing it I was trying to avoid creating many custom types for each key, seems that it ended complicating things more than necessary. Having more specific type for each type of key seems to solve the problems, and will make userspace life easier, please take a look at the RFC that I just sent, and see what do you think. > > > > > -- > Brian Gix > bgix@codeaurora.org > Employee of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, -- Vinicius ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-12-07 0:54 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-12-05 9:02 SMP Key distribution Ganir, Chen 2011-12-05 15:39 ` Brian Gix 2011-12-05 15:48 ` Ganir, Chen 2011-12-05 17:44 ` Brian Gix 2011-12-06 7:43 ` Ganir, Chen 2011-12-06 18:16 ` Brian Gix 2011-12-07 0:54 ` Vinicius Costa Gomes
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.