[PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands

When handling SHDLC I-Frame commands "pipe" field used for indexing
into an array should be checked before usage. If left unchecked it
might access memory outside of the array of size NFC_HCI_MAX_PIPES(127).

Malformed NFC HCI frames could be injected by a malicious NFC device
communicating with the device being attacked (remote attack vector),
or even by an attacker with physical access to the I2C bus such that
they could influence the data transfers on that bus (local attack vector).
skb->data is controlled by the attacker and has only been sanitized in
the most trivial ways (CRC check), therefore we can consider the
create_info struct and all of its members to tainted. 'create_info->pipe'
with max value of 255 (uint8) is used to take an offset of the
hdev->pipes array of 127 elements which can lead to OOB write.

Suggested-by: Kevin Deus <redacted>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 net/nfc/hci/core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/nfc/hci/core.c b/net/nfc/hci/core.c
index ac8030c4bcf8..19cb2e473ea6 100644
--- a/net/nfc/hci/core.c
+++ b/net/nfc/hci/core.c

@@ -209,6 +209,11 @@ void nfc_hci_cmd_received(struct nfc_hci_dev *hdev, u8 pipe, u8 cmd,
 		}
 		create_info = (struct hci_create_pipe_resp *)skb->data;
 
+		if (create_info->pipe >= NFC_HCI_MAX_PIPES) {
+			status = NFC_HCI_ANY_E_NOK;
+			goto exit;
+		}
+
 		/* Save the new created pipe and bind with local gate,
 		 * the description for skb->data[3] is destination gate id
 		 * but since we received this cmd from host controller, we

@@ -232,6 +237,11 @@ void nfc_hci_cmd_received(struct nfc_hci_dev *hdev, u8 pipe, u8 cmd,
 		}
 		delete_info = (struct hci_delete_pipe_noti *)skb->data;
 
+		if (delete_info->pipe >= NFC_HCI_MAX_PIPES) {
+			status = NFC_HCI_ANY_E_NOK;
+			goto exit;
+		}
+
 		hdev->pipes[delete_info->pipe].gate = NFC_HCI_INVALID_GATE;
 		hdev->pipes[delete_info->pipe].dest_host = NFC_HCI_INVALID_HOST;
 		break;

-- 
2.18.0.597.ga71716f1ad-goog

On Mon, Aug 13, 2018 at 3:39 PM, Suren Baghdasaryan [off-list ref] wrote:

When handling SHDLC I-Frame commands "pipe" field used for indexing
into an array should be checked before usage. If left unchecked it
might access memory outside of the array of size NFC_HCI_MAX_PIPES(127).

Malformed NFC HCI frames could be injected by a malicious NFC device
communicating with the device being attacked (remote attack vector),
or even by an attacker with physical access to the I2C bus such that
they could influence the data transfers on that bus (local attack vector).
skb->data is controlled by the attacker and has only been sanitized in
the most trivial ways (CRC check), therefore we can consider the
create_info struct and all of its members to tainted. 'create_info->pipe'
with max value of 255 (uint8) is used to take an offset of the
hdev->pipes array of 127 elements which can lead to OOB write.

Suggested-by: Kevin Deus <redacted>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Nice find!

Acked-by: Kees Cook <redacted>

-- 
Kees Cook
Pixel Security

On Mon, Aug 13, 2018 at 03:39:08PM -0700, Suren Baghdasaryan wrote:

When handling SHDLC I-Frame commands "pipe" field used for indexing
into an array should be checked before usage. If left unchecked it
might access memory outside of the array of size NFC_HCI_MAX_PIPES(127).

Malformed NFC HCI frames could be injected by a malicious NFC device
communicating with the device being attacked (remote attack vector),
or even by an attacker with physical access to the I2C bus such that
they could influence the data transfers on that bus (local attack vector).
skb->data is controlled by the attacker and has only been sanitized in
the most trivial ways (CRC check), therefore we can consider the
create_info struct and all of its members to tainted. 'create_info->pipe'
with max value of 255 (uint8) is used to take an offset of the
hdev->pipes array of 127 elements which can lead to OOB write.

Suggested-by: Kevin Deus <redacted>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks.  This is great.  I'm so glad these are finally getting fixed.

Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
well?  In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
(0x7f) so that's one element beyond the end of the array and the
NFC_HCI_HCP_RESPONSE isn't checked.

Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
NCI_HCP_MSG_GET_PIPE() so those could be off by one.

regards,
dan carpenter

On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter [off-list ref] wrote:

Thanks.  This is great.  I'm so glad these are finally getting fixed.

Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
well?  In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
(0x7f) so that's one element beyond the end of the array and the
NFC_HCI_HCP_RESPONSE isn't checked.

Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
NCI_HCP_MSG_GET_PIPE() so those could be off by one.

Good point. From hci.h:

/*
 * According to specification 102 622 chapter 4.4 Pipes,
 * the pipe identifier is 7 bits long.
 */
#define NFC_HCI_MAX_PIPES 127

And then:

struct nfc_hci_dev {
  ...
  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
  ...
}

I think the correct fix would be to change it to:

  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];

What do you think?

regards,
dan carpenter

On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan [off-list ref] wrote:

On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter [off-list ref] wrote:

quoted

Thanks.  This is great.  I'm so glad these are finally getting fixed.

Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
well?  In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
(0x7f) so that's one element beyond the end of the array and the
NFC_HCI_HCP_RESPONSE isn't checked.

Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
NCI_HCP_MSG_GET_PIPE() so those could be off by one.

Good point. From hci.h:

/*
 * According to specification 102 622 chapter 4.4 Pipes,
 * the pipe identifier is 7 bits long.
 */
#define NFC_HCI_MAX_PIPES 127

And then:

struct nfc_hci_dev {
  ...
  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
  ...
}

I think the correct fix would be to change it to:

  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];

What do you think?

Just to be clear, this would fix the problem Dan described in his
reply and it should be implemented in a separate patch. The original
fix is still valid.

quoted

regards,
dan carpenter

On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan [off-list ref] wrote:

On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter [off-list ref] wrote:

quoted

Thanks.  This is great.  I'm so glad these are finally getting fixed.

Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
well?  In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
(0x7f) so that's one element beyond the end of the array and the
NFC_HCI_HCP_RESPONSE isn't checked.

Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
NCI_HCP_MSG_GET_PIPE() so those could be off by one.

Good point. From hci.h:

/*
 * According to specification 102 622 chapter 4.4 Pipes,
 * the pipe identifier is 7 bits long.
 */
#define NFC_HCI_MAX_PIPES 127

And then:

struct nfc_hci_dev {
  ...
  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
  ...
}

I think the correct fix would be to change it to:

  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];

What do you think?

Just to be clear, this would fix the problem Dan described in his
reply and it should be implemented in a separate patch. The original
fix is still valid.

I think you could merge the fixes into a single patch.

-Kees

-- 
Kees Cook
Pixel Security

On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook [off-list ref] wrote:

On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter [off-list ref] wrote:

quoted

Thanks.  This is great.  I'm so glad these are finally getting fixed.

Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
well?  In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
(0x7f) so that's one element beyond the end of the array and the
NFC_HCI_HCP_RESPONSE isn't checked.

Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
NCI_HCP_MSG_GET_PIPE() so those could be off by one.

Good point. From hci.h:

/*
 * According to specification 102 622 chapter 4.4 Pipes,
 * the pipe identifier is 7 bits long.
 */
#define NFC_HCI_MAX_PIPES 127

And then:

struct nfc_hci_dev {
  ...
  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
  ...
}

I think the correct fix would be to change it to:

  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];

What do you think?

Just to be clear, this would fix the problem Dan described in his
reply and it should be implemented in a separate patch. The original
fix is still valid.

I think you could merge the fixes into a single patch.

Couple reasons I would prefer to keep them separate:
- I feel that descriptions for these two issues should be different
and it's easier if we don't mix them up
- This one is already merged into Android kernels, so would be easier
to introduce the second fix separately
- I would like to give credit to people who noticed the problem (in
this case those are different people)

However if more people think we should fix both issues in the same
patch I'll happily do that.
Thanks!

-Kees

--
Kees Cook
Pixel Security

On Tue, Aug 14, 2018 at 1:55 PM, Suren Baghdasaryan [off-list ref] wrote:

On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter [off-list ref] wrote:

quoted

Thanks.  This is great.  I'm so glad these are finally getting fixed.

Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
well?  In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
(0x7f) so that's one element beyond the end of the array and the
NFC_HCI_HCP_RESPONSE isn't checked.

Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
NCI_HCP_MSG_GET_PIPE() so those could be off by one.

Good point. From hci.h:

/*
 * According to specification 102 622 chapter 4.4 Pipes,
 * the pipe identifier is 7 bits long.
 */
#define NFC_HCI_MAX_PIPES 127

And then:

struct nfc_hci_dev {
  ...
  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
  ...
}

I think the correct fix would be to change it to:

  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];

What do you think?

Just to be clear, this would fix the problem Dan described in his
reply and it should be implemented in a separate patch. The original
fix is still valid.

I think you could merge the fixes into a single patch.

Couple reasons I would prefer to keep them separate:
- I feel that descriptions for these two issues should be different
and it's easier if we don't mix them up
- This one is already merged into Android kernels, so would be easier
to introduce the second fix separately
- I would like to give credit to people who noticed the problem (in
this case those are different people)

However if more people think we should fix both issues in the same
patch I'll happily do that.
Thanks!

If it's already landed separately somewhere else, then yeah, 2 patches
sounds good. No objection either way from me!

-Kees

-- 
Kees Cook
Pixel Security

On Tue, Aug 14, 2018 at 2:49 PM, Kees Cook [off-list ref] wrote:

On Tue, Aug 14, 2018 at 1:55 PM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter [off-list ref] wrote:

quoted

Thanks.  This is great.  I'm so glad these are finally getting fixed.

Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
well?  In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
(0x7f) so that's one element beyond the end of the array and the
NFC_HCI_HCP_RESPONSE isn't checked.

Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
NCI_HCP_MSG_GET_PIPE() so those could be off by one.

Good point. From hci.h:

/*
 * According to specification 102 622 chapter 4.4 Pipes,
 * the pipe identifier is 7 bits long.
 */
#define NFC_HCI_MAX_PIPES 127

And then:

struct nfc_hci_dev {
  ...
  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
  ...
}

I think the correct fix would be to change it to:

  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];

What do you think?

Actually, after looking more closely, NFC_HCI_MAX_PIPES is always used
as the number of supported pipes and not as the max pipe ID, so the
right fix would be:

-#define NFC_HCI_MAX_PIPES 127
+#define NFC_HCI_MAX_PIPES 128

I would prefer to rename it into NFC_HCI_PIPE_COUNT but don't want to
introduce unnecessary churn for one-line change, so will keep the
name. Will post a separate fix for this shortly.

quoted

quoted

quoted

Just to be clear, this would fix the problem Dan described in his
reply and it should be implemented in a separate patch. The original
fix is still valid.

I think you could merge the fixes into a single patch.

Couple reasons I would prefer to keep them separate:
- I feel that descriptions for these two issues should be different
and it's easier if we don't mix them up
- This one is already merged into Android kernels, so would be easier
to introduce the second fix separately
- I would like to give credit to people who noticed the problem (in
this case those are different people)

However if more people think we should fix both issues in the same
patch I'll happily do that.
Thanks!

If it's already landed separately somewhere else, then yeah, 2 patches
sounds good. No objection either way from me!

-Kees

--
Kees Cook
Pixel Security

The separate fix for the size of pipes[] array is posted here:
https://lkml.org/lkml/2018/8/14/1034
Thanks!

On Tue, Aug 14, 2018 at 3:19 PM, Suren Baghdasaryan [off-list ref] wrote:

On Tue, Aug 14, 2018 at 2:49 PM, Kees Cook [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 1:55 PM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter [off-list ref] wrote:

quoted

Thanks.  This is great.  I'm so glad these are finally getting fixed.

Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as
well?  In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT
(0x7f) so that's one element beyond the end of the array and the
NFC_HCI_HCP_RESPONSE isn't checked.

Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use
NCI_HCP_MSG_GET_PIPE() so those could be off by one.

Good point. From hci.h:

/*
 * According to specification 102 622 chapter 4.4 Pipes,
 * the pipe identifier is 7 bits long.
 */
#define NFC_HCI_MAX_PIPES 127

And then:

struct nfc_hci_dev {
  ...
  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES];
  ...
}

I think the correct fix would be to change it to:

  struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1];

What do you think?

Actually, after looking more closely, NFC_HCI_MAX_PIPES is always used
as the number of supported pipes and not as the max pipe ID, so the
right fix would be:

-#define NFC_HCI_MAX_PIPES 127
+#define NFC_HCI_MAX_PIPES 128

I would prefer to rename it into NFC_HCI_PIPE_COUNT but don't want to
introduce unnecessary churn for one-line change, so will keep the
name. Will post a separate fix for this shortly.

quoted

quoted

quoted

quoted

Just to be clear, this would fix the problem Dan described in his
reply and it should be implemented in a separate patch. The original
fix is still valid.

I think you could merge the fixes into a single patch.

Couple reasons I would prefer to keep them separate:
- I feel that descriptions for these two issues should be different
and it's easier if we don't mix them up
- This one is already merged into Android kernels, so would be easier
to introduce the second fix separately
- I would like to give credit to people who noticed the problem (in
this case those are different people)

However if more people think we should fix both issues in the same
patch I'll happily do that.
Thanks!

If it's already landed separately somewhere else, then yeah, 2 patches
sounds good. No objection either way from me!

-Kees

--
Kees Cook
Pixel Security

On Tue, Aug 14, 2018 at 03:38:14PM -0700, Suren Baghdasaryan wrote:

The separate fix for the size of pipes[] array is posted here:
https://lkml.org/lkml/2018/8/14/1034
Thanks!

That's great!  Let's add some bounds checking to nfc_hci_msg_rx_work()
and nfc_hci_recv_from_llc() as well and then we can close the chapter on
these bugs.

regards,
dan carpenter

On Wed, Aug 15, 2018 at 1:29 AM, Dan Carpenter [off-list ref] wrote:

On Tue, Aug 14, 2018 at 03:38:14PM -0700, Suren Baghdasaryan wrote:

quoted

The separate fix for the size of pipes[] array is posted here:
https://lkml.org/lkml/2018/8/14/1034
Thanks!

That's great!  Let's add some bounds checking to nfc_hci_msg_rx_work()
and nfc_hci_recv_from_llc() as well and then we can close the chapter on
these bugs.

Dan, I don't think we need additional checks there. Here are the
relevant parts of the code in nfc_hci_recv_from_llc():

static void nfc_hci_recv_from_llc(struct nfc_hci_dev *hdev, struct sk_buff *skb)
{
...
packet = (struct hcp_packet *)skb->data;
...

/* it's the last fragment. Does it need re-aggregation? */
if (skb_queue_len(&hdev->rx_hcp_frags)) {
    pipe = packet->header & NFC_HCI_FRAGMENT;
    ...
    hcp_skb = nfc_alloc_recv_skb(NFC_HCI_HCP_PACKET_HEADER_LEN +
         msg_len, GFP_KERNEL);
    ...
    *skb_put(hcp_skb, NFC_HCI_HCP_PACKET_HEADER_LEN) = pipe;
    ...
} else {
    packet->header &= NFC_HCI_FRAGMENT;
    hcp_skb = skb;
}

AFAIU in both cases the pipe field in hcp_skb can't exceed 127 after
we applied NFC_HCI_FRAGMENT(0x7f) mask.

regards,
dan carpenter

On Wed, Aug 15, 2018 at 09:40:13AM -0700, Suren Baghdasaryan wrote:

On Wed, Aug 15, 2018 at 1:29 AM, Dan Carpenter [off-list ref] wrote:

quoted

On Tue, Aug 14, 2018 at 03:38:14PM -0700, Suren Baghdasaryan wrote:

quoted

The separate fix for the size of pipes[] array is posted here:
https://lkml.org/lkml/2018/8/14/1034
Thanks!

That's great!  Let's add some bounds checking to nfc_hci_msg_rx_work()
and nfc_hci_recv_from_llc() as well and then we can close the chapter on
these bugs.

Dan, I don't think we need additional checks there. Here are the
relevant parts of the code in nfc_hci_recv_from_llc():

Sorry, I meant after that at the end of the function:

net/nfc/hci/core.c
   902          /* if this is a response, dispatch immediately to
   903           * unblock waiting cmd context. Otherwise, enqueue to dispatch
   904           * in separate context where handler can also execute command.
   905           */
   906          packet = (struct hcp_packet *)hcp_skb->data;
   907          type = HCP_MSG_GET_TYPE(packet->message.header);
   908          if (type == NFC_HCI_HCP_RESPONSE) {
   909                  pipe = packet->header;
                        ^^^^^^^^^^^^^^^^^^^^^
Pipe can go up to 255.

   910                  instruction = HCP_MSG_GET_CMD(packet->message.header);
   911                  skb_pull(hcp_skb, NFC_HCI_HCP_PACKET_HEADER_LEN +
   912                           NFC_HCI_HCP_MESSAGE_HEADER_LEN);
   913                  nfc_hci_hcp_message_rx(hdev, pipe, type, instruction, hcp_skb);
                                                     ^^^^
Then inside the nfc_hci_hcp_message_rx() function we call
nfc_hci_cmd_received() and nfc_hci_event_received() which use it as an
array index.

   914          } else {
   915                  skb_queue_tail(&hdev->msg_rx_queue, hcp_skb);
   916                  schedule_work(&hdev->msg_rx_work);
   917          }
   918  }

It's the same thing when nfc_hci_hcp_message_rx() is called from
nfc_hci_msg_rx_work():

   138  static void nfc_hci_msg_rx_work(struct work_struct *work)
   139  {
   140          struct nfc_hci_dev *hdev = container_of(work, struct nfc_hci_dev,
   141                                                  msg_rx_work);
   142          struct sk_buff *skb;
   143          struct hcp_message *message;
   144          u8 pipe;
   145          u8 type;
   146          u8 instruction;
   147  
   148          while ((skb = skb_dequeue(&hdev->msg_rx_queue)) != NULL) {
   149                  pipe = skb->data[0];
                        ^^^^^^^^^^^^^^^^^^^
   150                  skb_pull(skb, NFC_HCI_HCP_PACKET_HEADER_LEN);
   151                  message = (struct hcp_message *)skb->data;
   152                  type = HCP_MSG_GET_TYPE(message->header);
   153                  instruction = HCP_MSG_GET_CMD(message->header);
   154                  skb_pull(skb, NFC_HCI_HCP_MESSAGE_HEADER_LEN);
   155  
   156                  nfc_hci_hcp_message_rx(hdev, pipe, type, instruction, skb);
                                                     ^^^^
   157          }
   158  }

regards,
dan carpenter

Never mind.  This code is a bit subtle for static analysis so those
warnings are false positives.  Thanks for taking a look at this, Suren.

regards,
dan carpenter