You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2021/04/15 10:23:55 UTC
[GitHub] [mynewt-nimble] prasad-alatkar opened a new pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
prasad-alatkar opened a new pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971
Issue: In case of secure connection pairing and Paskkey Entry method, there is chance that MITM attacker can reply with same public key as received from vulnerable BLE device. This can potentially authenticate MITM attacker. This PR fixes this potential vulnerability.
Solution: Check if the peer public key is same as our generated public key. Return fail if the public keys match to avoid 'Impersonation in Passkey entry protocol' vulnerability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot removed a comment on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-820930665
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] dhrishi commented on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
dhrishi commented on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-833418571
Hi @rymanluk Can you please review the unit test and merge if it looks okay? Post that, we will pull this into Espressif's NimBLE repository.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot commented on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-820315413
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot commented on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-831340410
<!-- style-bot -->
## Style check summary
### Our coding style is [here!](https://github.com/apache/mynewt-core/blob/master/CODING_STANDARDS.md)
#### nimble/host/test/src/ble_sm_sc_test.c
<details>
```diff
@@ -2298,8 +2292,7 @@
* Responder key distribution: 5
* Peer responds with same public key
*/
-TEST_CASE_SELF(ble_sm_sc_us_pk_iio2_rio4_b1_iat0_rat0_ik7_rk5_peer_same_pk)
-{
+TEST_CASE_SELF(ble_sm_sc_us_pk_iio2_rio4_b1_iat0_rat0_ik7_rk5_peer_same_pk) {
struct ble_sm_test_params params;
params = (struct ble_sm_test_params) {
```
</details>
#### nimble/host/test/src/ble_sm_test_util.c
<details>
```diff
@@ -2597,11 +2584,10 @@
}
static void
-ble_sm_test_util_us_sc_bad_once_no_init(
- struct ble_sm_test_params *params,
- struct ble_hs_conn *conn,
- struct ble_sm_test_util_entity *our_entity,
- struct ble_sm_test_util_entity *peer_entity)
+ble_sm_test_util_us_sc_bad_once_no_init(struct ble_sm_test_params *params,
+ struct ble_hs_conn *conn,
+ struct ble_sm_test_util_entity *our_entity,
+ struct ble_sm_test_util_entity *peer_entity)
{
int num_iters;
int rc;
@@ -2714,11 +2700,10 @@
}
static void
-ble_sm_test_util_peer_sc_good_once_no_init(
- struct ble_sm_test_params *params,
- struct ble_hs_conn *conn,
- struct ble_sm_test_util_entity *our_entity,
- struct ble_sm_test_util_entity *peer_entity)
+ble_sm_test_util_peer_sc_good_once_no_init(struct ble_sm_test_params *params,
+ struct ble_hs_conn *conn,
+ struct ble_sm_test_util_entity *our_entity,
+ struct ble_sm_test_util_entity *peer_entity)
{
int num_iters;
int rc;
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] prasad-alatkar commented on a change in pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
prasad-alatkar commented on a change in pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#discussion_r617547801
##########
File path: nimble/host/src/ble_sm_sc.c
##########
@@ -612,6 +612,14 @@ ble_sm_sc_public_key_rx(uint16_t conn_handle, struct os_mbuf **om,
}
cmd = (struct ble_sm_public_key *)(*om)->om_data;
+ /* Check if the peer public key is same as our generated public key.
+ * Return fail if the public keys match. */
+ if (ble_sm_sc_keys_generated) {
Review comment:
Yes, the check is not needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot commented on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-820930665
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] sjanc commented on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
sjanc commented on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-820383758
do I get it right that without this patch pairing would end with success even though remote peer is not knowing our private key?
How would it calculate valid DHKey and then mackey and LTK?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot commented on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-832442568
<!-- style-bot -->
## Style check summary
### Our coding style is [here!](https://github.com/apache/mynewt-core/blob/master/CODING_STANDARDS.md)
#### nimble/host/test/src/ble_sm_sc_test.c
<details>
```diff
@@ -2298,8 +2292,7 @@
* Responder key distribution: 5
* Peer responds with same public key
*/
-TEST_CASE_SELF(ble_sm_sc_us_pk_iio2_rio4_b1_iat0_rat0_ik7_rk5_peer_same_pk)
-{
+TEST_CASE_SELF(ble_sm_sc_us_pk_iio2_rio4_b1_iat0_rat0_ik7_rk5_peer_same_pk) {
struct ble_sm_test_params params;
params = (struct ble_sm_test_params) {
```
</details>
#### nimble/host/test/src/ble_sm_test_util.c
<details>
```diff
@@ -2597,11 +2584,10 @@
}
static void
-ble_sm_test_util_us_sc_bad_once_no_init(
- struct ble_sm_test_params *params,
- struct ble_hs_conn *conn,
- struct ble_sm_test_util_entity *our_entity,
- struct ble_sm_test_util_entity *peer_entity)
+ble_sm_test_util_us_sc_bad_once_no_init(struct ble_sm_test_params *params,
+ struct ble_hs_conn *conn,
+ struct ble_sm_test_util_entity *our_entity,
+ struct ble_sm_test_util_entity *peer_entity)
{
int rc;
@@ -2709,11 +2695,10 @@
}
static void
-ble_sm_test_util_peer_sc_good_once_no_init(
- struct ble_sm_test_params *params,
- struct ble_hs_conn *conn,
- struct ble_sm_test_util_entity *our_entity,
- struct ble_sm_test_util_entity *peer_entity)
+ble_sm_test_util_peer_sc_good_once_no_init(struct ble_sm_test_params *params,
+ struct ble_hs_conn *conn,
+ struct ble_sm_test_util_entity *our_entity,
+ struct ble_sm_test_util_entity *peer_entity)
{
int num_iters;
int rc;
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot removed a comment on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-831340410
<!-- style-bot -->
## Style check summary
### Our coding style is [here!](https://github.com/apache/mynewt-core/blob/master/CODING_STANDARDS.md)
#### nimble/host/test/src/ble_sm_sc_test.c
<details>
```diff
@@ -2298,8 +2292,7 @@
* Responder key distribution: 5
* Peer responds with same public key
*/
-TEST_CASE_SELF(ble_sm_sc_us_pk_iio2_rio4_b1_iat0_rat0_ik7_rk5_peer_same_pk)
-{
+TEST_CASE_SELF(ble_sm_sc_us_pk_iio2_rio4_b1_iat0_rat0_ik7_rk5_peer_same_pk) {
struct ble_sm_test_params params;
params = (struct ble_sm_test_params) {
```
</details>
#### nimble/host/test/src/ble_sm_test_util.c
<details>
```diff
@@ -2597,11 +2584,10 @@
}
static void
-ble_sm_test_util_us_sc_bad_once_no_init(
- struct ble_sm_test_params *params,
- struct ble_hs_conn *conn,
- struct ble_sm_test_util_entity *our_entity,
- struct ble_sm_test_util_entity *peer_entity)
+ble_sm_test_util_us_sc_bad_once_no_init(struct ble_sm_test_params *params,
+ struct ble_hs_conn *conn,
+ struct ble_sm_test_util_entity *our_entity,
+ struct ble_sm_test_util_entity *peer_entity)
{
int num_iters;
int rc;
@@ -2714,11 +2700,10 @@
}
static void
-ble_sm_test_util_peer_sc_good_once_no_init(
- struct ble_sm_test_params *params,
- struct ble_hs_conn *conn,
- struct ble_sm_test_util_entity *our_entity,
- struct ble_sm_test_util_entity *peer_entity)
+ble_sm_test_util_peer_sc_good_once_no_init(struct ble_sm_test_params *params,
+ struct ble_hs_conn *conn,
+ struct ble_sm_test_util_entity *our_entity,
+ struct ble_sm_test_util_entity *peer_entity)
{
int num_iters;
int rc;
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot commented on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-832454524
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot commented on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-824173296
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] prasad-alatkar commented on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
prasad-alatkar commented on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-820314794
@sjanc @rymanluk Could you guys take a look ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] sjanc commented on a change in pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
sjanc commented on a change in pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#discussion_r615662464
##########
File path: nimble/host/src/ble_sm_sc.c
##########
@@ -612,6 +612,14 @@ ble_sm_sc_public_key_rx(uint16_t conn_handle, struct os_mbuf **om,
}
cmd = (struct ble_sm_public_key *)(*om)->om_data;
+ /* Check if the peer public key is same as our generated public key.
+ * Return fail if the public keys match. */
+ if (ble_sm_sc_keys_generated) {
Review comment:
I think this check is not needed as ble_sm_sc_ensure_keys_generated() is called before
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] prasad-alatkar commented on a change in pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
prasad-alatkar commented on a change in pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#discussion_r617676406
##########
File path: nimble/host/src/ble_sm_sc.c
##########
@@ -612,6 +612,14 @@ ble_sm_sc_public_key_rx(uint16_t conn_handle, struct os_mbuf **om,
}
cmd = (struct ble_sm_public_key *)(*om)->om_data;
+ /* Check if the peer public key is same as our generated public key.
+ * Return fail if the public keys match. */
+ if (ble_sm_sc_keys_generated) {
+ if (memcmp(cmd, ble_sm_sc_pub_key, 64) == 0) {
+ res->enc_cb = 1;
+ res->sm_err = BLE_SM_ERR_AUTHREQ;
Review comment:
Added return. I will create unit test for the same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] prasad-alatkar commented on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
prasad-alatkar commented on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-831327944
> > Looks good. Any chance for the unit test for that ?:)
>
> @rymanluk yes, I will add the unit test by this weekend :)
@rymanluk, I have tried to add unit test to verify the scenario. PTAL and suggest if any improvements can be done...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] rymanluk merged pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
rymanluk merged pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot removed a comment on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-832442568
<!-- style-bot -->
## Style check summary
### Our coding style is [here!](https://github.com/apache/mynewt-core/blob/master/CODING_STANDARDS.md)
#### nimble/host/test/src/ble_sm_sc_test.c
<details>
```diff
@@ -2298,8 +2292,7 @@
* Responder key distribution: 5
* Peer responds with same public key
*/
-TEST_CASE_SELF(ble_sm_sc_us_pk_iio2_rio4_b1_iat0_rat0_ik7_rk5_peer_same_pk)
-{
+TEST_CASE_SELF(ble_sm_sc_us_pk_iio2_rio4_b1_iat0_rat0_ik7_rk5_peer_same_pk) {
struct ble_sm_test_params params;
params = (struct ble_sm_test_params) {
```
</details>
#### nimble/host/test/src/ble_sm_test_util.c
<details>
```diff
@@ -2597,11 +2584,10 @@
}
static void
-ble_sm_test_util_us_sc_bad_once_no_init(
- struct ble_sm_test_params *params,
- struct ble_hs_conn *conn,
- struct ble_sm_test_util_entity *our_entity,
- struct ble_sm_test_util_entity *peer_entity)
+ble_sm_test_util_us_sc_bad_once_no_init(struct ble_sm_test_params *params,
+ struct ble_hs_conn *conn,
+ struct ble_sm_test_util_entity *our_entity,
+ struct ble_sm_test_util_entity *peer_entity)
{
int rc;
@@ -2709,11 +2695,10 @@
}
static void
-ble_sm_test_util_peer_sc_good_once_no_init(
- struct ble_sm_test_params *params,
- struct ble_hs_conn *conn,
- struct ble_sm_test_util_entity *our_entity,
- struct ble_sm_test_util_entity *peer_entity)
+ble_sm_test_util_peer_sc_good_once_no_init(struct ble_sm_test_params *params,
+ struct ble_hs_conn *conn,
+ struct ble_sm_test_util_entity *our_entity,
+ struct ble_sm_test_util_entity *peer_entity)
{
int num_iters;
int rc;
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot removed a comment on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-824173296
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] rymanluk commented on a change in pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
rymanluk commented on a change in pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#discussion_r615166928
##########
File path: nimble/host/src/ble_sm_sc.c
##########
@@ -612,6 +612,14 @@ ble_sm_sc_public_key_rx(uint16_t conn_handle, struct os_mbuf **om,
}
cmd = (struct ble_sm_public_key *)(*om)->om_data;
+ /* Check if the peer public key is same as our generated public key.
+ * Return fail if the public keys match. */
+ if (ble_sm_sc_keys_generated) {
+ if (memcmp(cmd, ble_sm_sc_pub_key, 64) == 0) {
+ res->enc_cb = 1;
+ res->sm_err = BLE_SM_ERR_AUTHREQ;
Review comment:
shouldn't be here a return?
Could you please create a unit test for that in nimble/host/test/src/ble_sm_sc_test.c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot removed a comment on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-820315413
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] prasad-alatkar commented on pull request #971: nimble/host: Fix potential MITM vulnerability in case of secure connection pairing
Posted by GitBox <gi...@apache.org>.
prasad-alatkar commented on pull request #971:
URL: https://github.com/apache/mynewt-nimble/pull/971#issuecomment-828253869
> Looks good. Any chance for the unit test for that ?:)
@rymanluk yes, I will add the unit test by this weekend :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org