diff mbox

[3/7] TDLS: allow extra erroneous IEs in all packets

Message ID 1424226915-1100-3-git-send-email-ilan.peer@intel.com
State Accepted
Headers show

Commit Message

Peer, Ilan Feb. 18, 2015, 2:35 a.m. UTC 
From: Arik Nemtsov <arik@wizery.com>

Some APs (Cisco 1260) sometimes add invalid IEs to the end of various
tdls management packets. This was allowed on M3 and discovery packets, but
not in others. Allow it for the other packets as well, since required IEs
are verified in the code anyway.

Signed-off-by: Arik Nemtsov <arikx.nemtsov@intel.com>
---
 src/rsn_supp/tdls.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

Comments

Jouni Malinen Feb. 18, 2015, 4:01 p.m. UTC | #1 
On Tue, Feb 17, 2015 at 09:35:11PM -0500, Ilan Peer wrote:
> Some APs (Cisco 1260) sometimes add invalid IEs to the end of various
> tdls management packets. This was allowed on M3 and discovery packets, but
> not in others. Allow it for the other packets as well, since required IEs
> are verified in the code anyway.

Would you be able to share a sniffer capture showing such a case? This
seemed to be needed when the forwarded packet was shorter than minimum
Ethernet frame (even if not really going out on Ethernet which is the
somewhat strange part here and likely specific to some APs), but I'm not
sure in which cases the other frames would be short enough to trigger
this issue.

Furthermore, these are not really supposed to be "extra erroneous IEs",
but some arbitrary padding at the end of the "Ethernet" frame.
Arik Nemtsov Feb. 23, 2015, 1:56 p.m. UTC | #2 
On Wed, Feb 18, 2015 at 6:01 PM, Jouni Malinen <j@w1.fi> wrote:
> On Tue, Feb 17, 2015 at 09:35:11PM -0500, Ilan Peer wrote:
>> Some APs (Cisco 1260) sometimes add invalid IEs to the end of various
>> tdls management packets. This was allowed on M3 and discovery packets, but
>> not in others. Allow it for the other packets as well, since required IEs
>> are verified in the code anyway.

I don't have a sniffer capture (or event the wpa_s log anymore), but
we encountered this on an incoming TDLS teardown packet.

>
> Would you be able to share a sniffer capture showing such a case? This
> seemed to be needed when the forwarded packet was shorter than minimum
> Ethernet frame (even if not really going out on Ethernet which is the
> somewhat strange part here and likely specific to some APs), but I'm not
> sure in which cases the other frames would be short enough to trigger
> this issue.
>
> Furthermore, these are not really supposed to be "extra erroneous IEs",
> but some arbitrary padding at the end of the "Ethernet" frame.

For this case I'm not sure what it was, but for previous cases, the
extra data looked like an IE from the CCX protocol (which I don't
really know).
But does it matter what it really is? IMO we should be lenient in what
we accept, as long as the MIC is not damaged.

For the case in hand, we had a peer that was out of sync in its TDLS
state with us, when talking through the cisco 1260, which is a pretty
unpleasant experience.

Arik
Jouni Malinen Feb. 23, 2015, 5:42 p.m. UTC | #3 
On Mon, Feb 23, 2015 at 03:56:48PM +0200, Arik Nemtsov wrote:
> I don't have a sniffer capture (or event the wpa_s log anymore), but
> we encountered this on an incoming TDLS teardown packet.

I can understand teardown as a relevant case. It is the other messages
that I was more curious about since they should be long enough to avoid
hitting this issue.

> For this case I'm not sure what it was, but for previous cases, the
> extra data looked like an IE from the CCX protocol (which I don't
> really know).
> But does it matter what it really is? IMO we should be lenient in what
> we accept, as long as the MIC is not damaged.

It does not matter as far as the actual implementation is concerned. It
does matter as far as the commit message and code comments are
concerned. As far as I can tell, the real issue here is in the AP
padding IEEE 802.11 Data frames that are shorter than minimum Ethernet
frames with some arbitrary data. If you are seeing things like IEs from
CCX (etc.), that would be a potential sign of information disclosure
vulnerability, i.e., maybe some arbitrary AP memory getting used as
padding.

> For the case in hand, we had a peer that was out of sync in its TDLS
> state with us, when talking through the cisco 1260, which is a pretty
> unpleasant experience.

Understood and agreed (I did already apply the patch). I just want to
make sure there is now new interop issue reported here and the main
reason for the issue can still be assumed to be in that incorrect
padding of IEEE 802.11 Data frames when going through the stack
somewhere within the AP.
Arik Nemtsov Feb. 24, 2015, 4:42 p.m. UTC | #4 
On Mon, Feb 23, 2015 at 7:42 PM, Jouni Malinen <j@w1.fi> wrote:
> > For the case in hand, we had a peer that was out of sync in its TDLS
> > state with us, when talking through the cisco 1260, which is a pretty
> > unpleasant experience.
>
> Understood and agreed (I did already apply the patch). I just want to
> make sure there is now new interop issue reported here and the main
> reason for the issue can still be assumed to be in that incorrect
> padding of IEEE 802.11 Data frames when going through the stack
> somewhere within the AP.

I think we can still work under this assumption, yea.

Arik
diff mbox

Patch

diff --git a/src/rsn_supp/tdls.c b/src/rsn_supp/tdls.c
index 10413ed..7e2f852 100644
--- a/src/rsn_supp/tdls.c
+++ b/src/rsn_supp/tdls.c
@@ -939,10 +939,16 @@  static int wpa_tdls_recv_teardown(struct wpa_sm *sm, const u8 *src_addr,
 		   " (reason code %u)", MAC2STR(src_addr), reason_code);
 
 	ielen = len - (pos - buf); /* start of IE in buf */
-	if (wpa_supplicant_parse_ies((const u8 *) pos, ielen, &kde) < 0) {
-		wpa_printf(MSG_INFO, "TDLS: Failed to parse IEs in Teardown");
-		return -1;
-	}
+
+	/*
+	 * Don't reject the message if failing to parse IEs. The IEs we need are
+	 * explicitly checked below. Some APs piggy-back broken IEs to the end
+	 * of a TDLS Confirm packet, which will fail the link if we don't ignore
+	 * this error.
+	 */
+	if (wpa_supplicant_parse_ies((const u8 *) pos, ielen, &kde) < 0)
+		wpa_printf(MSG_INFO,
+			   "TDLS: Failed to parse IEs in Teardown - ignore as an interop workaround");
 
 	if (kde.lnkid == NULL || kde.lnkid_len < 3 * ETH_ALEN) {
 		wpa_printf(MSG_INFO, "TDLS: No Link Identifier IE in TDLS "
@@ -1823,10 +1829,16 @@  static int wpa_tdls_process_tpk_m1(struct wpa_sm *sm, const u8 *src_addr,
 	cpos += 2;
 
 	ielen = len - (cpos - buf); /* start of IE in buf */
-	if (wpa_supplicant_parse_ies(cpos, ielen, &kde) < 0) {
-		wpa_printf(MSG_INFO, "TDLS: Failed to parse IEs in TPK M1");
-		goto error;
-	}
+
+	/*
+	 * Don't reject the message if failing to parse IEs. The IEs we need are
+	 * explicitly checked below. Some APs piggy-back broken IEs to the end
+	 * of a TDLS Confirm packet, which will fail the link if we don't ignore
+	 * this error.
+	 */
+	if (wpa_supplicant_parse_ies(cpos, ielen, &kde) < 0)
+		wpa_printf(MSG_INFO,
+			   "TDLS: Failed to parse IEs in TPK M1 - ignore as an interop workaround");
 
 	if (kde.lnkid == NULL || kde.lnkid_len < 3 * ETH_ALEN) {
 		wpa_printf(MSG_INFO, "TDLS: No valid Link Identifier IE in "
@@ -2199,10 +2211,16 @@  static int wpa_tdls_process_tpk_m2(struct wpa_sm *sm, const u8 *src_addr,
 	pos += 2;
 
 	ielen = len - (pos - buf); /* start of IE in buf */
-	if (wpa_supplicant_parse_ies(pos, ielen, &kde) < 0) {
-		wpa_printf(MSG_INFO, "TDLS: Failed to parse IEs in TPK M2");
-		goto error;
-	}
+
+	/*
+	 * Don't reject the message if failing to parse IEs. The IEs we need are
+	 * explicitly checked below. Some APs piggy-back broken IEs to the end
+	 * of a TDLS Confirm packet, which will fail the link if we don't ignore
+	 * this error.
+	 */
+	if (wpa_supplicant_parse_ies(pos, ielen, &kde) < 0)
+		wpa_printf(MSG_INFO,
+			   "TDLS: Failed to parse IEs in TPK M2 - ignore as an interop workaround");
 
 #ifdef CONFIG_TDLS_TESTING
 	if (tdls_testing & TDLS_TESTING_DECLINE_RESP) {