* [PATCH 1/5] auto-t: end process_io on HUP signal, detect process crash
@ 2024-02-29 17:07 James Prestwood
2024-02-29 17:07 ` [PATCH 2/5] auto-t: Add frame fuzzing test James Prestwood
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: James Prestwood @ 2024-02-29 17:07 UTC (permalink / raw
To: iwd; +Cc: James Prestwood
When HUP is received the IO read callback was never completing which
caused it to block indefinitely until waited for. This didn't matter
for most transient processes but for IWD, hostapd, wpa_supplicant
it would cause test-runner to hang if the process crashed.
Detecting a crash is somewhat hacky because we have no process
management like systemd and the return code isn't reliable as some
processes return non-zero under normal circumstances. So to detect
a crash the process output is being checked for the string:
"++++++++ backtrace ++++++++". This isn't 100% reliable obviously
since its dependent on how the binary is compiled, but even if the
crash itself isn't detected any test should still fail if written
correctly.
Doing this allows auto-tests to handle IWD crashes gracefully by
failing the test, printing the exception (event without debugging)
and continue with other tests.
---
tools/utils.py | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/utils.py b/tools/utils.py
index 5984fc69..d5445ea7 100644
--- a/tools/utils.py
+++ b/tools/utils.py
@@ -175,23 +175,27 @@ class Process(subprocess.Popen):
def process_io(self, source, condition):
if condition & GLib.IO_HUP:
self.hup = True
+ self.wait()
+ bt = self.out.partition("++++++++ backtrace ++++++++")
+ if bt[1]:
+ raise Exception(f"Process {self.args[0]} crashed!\n{bt[1] + bt[2]}")
data = source.read()
if not data:
- return True
+ return not self.hup
try:
data = data.decode('utf-8')
except:
- return True
+ return not self.hup
# Save data away in case the caller needs it (e.g. list_sta)
self.out += data
self._write_io(self, data)
- return True
+ return not self.hup
def _append_outfile(self, file, append=True):
gid = int(os.environ.get('SUDO_GID', os.getgid()))
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/5] auto-t: Add frame fuzzing test
2024-02-29 17:07 [PATCH 1/5] auto-t: end process_io on HUP signal, detect process crash James Prestwood
@ 2024-02-29 17:07 ` James Prestwood
2024-02-29 17:07 ` [PATCH 3/5] p2putil: fix crash/remove side effect parsing adv service info James Prestwood
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: James Prestwood @ 2024-02-29 17:07 UTC (permalink / raw
To: iwd; +Cc: James Prestwood, Alex Radocea
Add a test to validate a crash found by Alex Radocea when sending
a fuzzed beacon frame.
Co-authored-by: Alex Radocea <alex@supernetworks.org>
---
autotests/testFrameFuzzing/fake_ap.py | 72 +++++++++++++++++++
autotests/testFrameFuzzing/hw.conf | 7 ++
.../testFrameFuzzing/test_frame_fuzzing.py | 37 ++++++++++
3 files changed, 116 insertions(+)
create mode 100644 autotests/testFrameFuzzing/fake_ap.py
create mode 100644 autotests/testFrameFuzzing/hw.conf
create mode 100644 autotests/testFrameFuzzing/test_frame_fuzzing.py
diff --git a/autotests/testFrameFuzzing/fake_ap.py b/autotests/testFrameFuzzing/fake_ap.py
new file mode 100644
index 00000000..8ee369de
--- /dev/null
+++ b/autotests/testFrameFuzzing/fake_ap.py
@@ -0,0 +1,72 @@
+import unittest
+import sys
+import sys
+import os
+from scapy.layers.dot11 import *
+from scapy.arch import str2mac, get_if_raw_hwaddr
+from time import time, sleep
+from threading import Thread
+
+def if_hwaddr(iff):
+ return str2mac(get_if_raw_hwaddr(iff)[1])
+
+def config_mon(iface, channel):
+ """set the interface in monitor mode and then change channel using iw"""
+ os.system("ip link set dev %s down" % iface)
+ os.system("iw dev %s set type monitor" % iface)
+ os.system("ip link set dev %s up" % iface)
+ os.system("iw dev %s set channel %d" % (iface, channel))
+
+class AP:
+ def __init__(self, ssid, psk, mac=None, mode="stdio", iface="wlan0", channel=1):
+ self.channel = channel
+ self.iface = iface
+ self.mode = mode
+ if self.mode == "iface":
+ if not mac:
+ mac = if_hwaddr(iface)
+ config_mon(iface, channel)
+ if not mac:
+ raise Exception("Need a mac")
+ else:
+ self.mac = mac
+ self.boottime = time()
+
+ def get_radiotap_header(self):
+ return RadioTap()
+
+ def dot11_beacon(self, contents):
+ evil_packet = (
+ self.get_radiotap_header()
+ / Dot11(
+ subtype=8, addr1="ff:ff:ff:ff:ff:ff", addr2=self.mac, addr3=self.mac
+ )
+ / Dot11Beacon(cap=0x3101)
+ / contents
+ )
+ self.sendp(evil_packet)
+
+ def run(self, contents):
+ interval = 0.05
+ num_beacons = 100
+
+ while num_beacons:
+ self.dot11_beacon(contents)
+ sleep(interval)
+ num_beacons -= 1
+
+ def start(self, contents):
+ self.thread = Thread(target=self.run, args=(contents,))
+ self.thread.start()
+
+ def stop(self):
+ self.thread.join()
+
+ def sendp(self, packet, verbose=False):
+ if self.mode == "stdio":
+ x = packet.build()
+ sys.stdout.buffer.write(struct.pack("<L", len(x)) + x)
+ sys.stdout.buffer.flush()
+ return
+ assert self.mode == "iface"
+ sendp(packet, iface=self.iface, verbose=False)
diff --git a/autotests/testFrameFuzzing/hw.conf b/autotests/testFrameFuzzing/hw.conf
new file mode 100644
index 00000000..0500b51d
--- /dev/null
+++ b/autotests/testFrameFuzzing/hw.conf
@@ -0,0 +1,7 @@
+[SETUP]
+num_radios=2
+start_iwd=0
+hwsim_medium=yes
+
+[rad1]
+reserve=true
diff --git a/autotests/testFrameFuzzing/test_frame_fuzzing.py b/autotests/testFrameFuzzing/test_frame_fuzzing.py
new file mode 100644
index 00000000..9def7684
--- /dev/null
+++ b/autotests/testFrameFuzzing/test_frame_fuzzing.py
@@ -0,0 +1,37 @@
+#! /usr/bin/python3
+
+import unittest
+import sys
+import sys
+import os
+from fake_ap import AP
+
+sys.path.append('../util')
+from iwd import IWD
+
+# Probe frame that causes IWD to crash
+beacon=b'\xdd\nPo\x9a\t\x0e\x00\x00\x19\x10\x00\xdd/Po\x9a\t\x0c\x02\x00\x00\xdd\x05\x03\x03\x03Po\x9a\x10\x00\x0b\x05\x0e\x00\x00\x00\x00\x0b\x05\x00\x00\x00\xdd\x05\x00\x03\x03\x03\x03\x00\x00\x00\xdd\x05\x03\x03\x03\x03\x03'
+
+class Test(unittest.TestCase):
+ def test_beacon_crash(self):
+ wd = IWD(True)
+
+ devs = wd.list_devices()
+
+ self.assertEqual(len(devs), 1)
+
+ devs[0].autoconnect = True
+
+ os.system("iw phy rad1 interface add wlan1 type managed")
+
+ ap = AP("evilAP", "password1234", mode="iface", iface="wlan1", channel=4)
+ ap.start(beacon)
+
+ condition = "obj.scanning == True"
+ wd.wait_for_object_condition(devs[0], condition)
+
+ condition = "obj.scanning == False"
+ wd.wait_for_object_condition(devs[0], condition)
+
+if __name__ == '__main__':
+ unittest.main(exit=True)
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/5] p2putil: fix crash/remove side effect parsing adv service info
2024-02-29 17:07 [PATCH 1/5] auto-t: end process_io on HUP signal, detect process crash James Prestwood
2024-02-29 17:07 ` [PATCH 2/5] auto-t: Add frame fuzzing test James Prestwood
@ 2024-02-29 17:07 ` James Prestwood
2024-02-29 17:07 ` [PATCH 4/5] p2putil: initialize all parsing structures to zero James Prestwood
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: James Prestwood @ 2024-02-29 17:07 UTC (permalink / raw
To: iwd; +Cc: James Prestwood, Alex Radocea
The input queue pointer was being initialized unconditionally so if
parsing fails the out pointer is still set after the queue is
destroyed. This causes a crash during cleanup.
Instead use a temporary pointer while parsing and only after parsing
has finished do we set the out pointer.
Reported-By: Alex Radocea <alex@supernetworks.org>
---
src/p2putil.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/p2putil.c b/src/p2putil.c
index 5313b34c..faf151a5 100644
--- a/src/p2putil.c
+++ b/src/p2putil.c
@@ -541,7 +541,8 @@ static void p2p_clear_advertised_service_descriptor(void *data)
static bool extract_p2p_advertised_service_info(const uint8_t *attr, size_t len,
void *data)
{
- struct l_queue **out = data;
+ struct l_queue **q = data;
+ struct l_queue *out = NULL;
while (len) {
struct p2p_advertised_service_descriptor *desc;
@@ -557,11 +558,11 @@ static bool extract_p2p_advertised_service_info(const uint8_t *attr, size_t len,
if (!l_utf8_validate((const char *) attr + 7, name_len, NULL))
goto error;
- if (!*out)
- *out = l_queue_new();
+ if (!out)
+ out = l_queue_new();
desc = l_new(struct p2p_advertised_service_descriptor, 1);
- l_queue_push_tail(*out, desc);
+ l_queue_push_tail(out, desc);
desc->advertisement_id = l_get_le32(attr + 0);
desc->wsc_config_methods = l_get_be16(attr + 4);
@@ -572,10 +573,12 @@ static bool extract_p2p_advertised_service_info(const uint8_t *attr, size_t len,
len -= 7 + name_len;
}
+ *q = out;
+
return true;
error:
- l_queue_destroy(*out, p2p_clear_advertised_service_descriptor);
+ l_queue_destroy(out, p2p_clear_advertised_service_descriptor);
return false;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 4/5] p2putil: initialize all parsing structures to zero
2024-02-29 17:07 [PATCH 1/5] auto-t: end process_io on HUP signal, detect process crash James Prestwood
2024-02-29 17:07 ` [PATCH 2/5] auto-t: Add frame fuzzing test James Prestwood
2024-02-29 17:07 ` [PATCH 3/5] p2putil: fix crash/remove side effect parsing adv service info James Prestwood
@ 2024-02-29 17:07 ` James Prestwood
2024-02-29 17:07 ` [PATCH 5/5] p2putil: check length of client info description James Prestwood
2024-02-29 20:36 ` [PATCH 1/5] auto-t: end process_io on HUP signal, detect process crash Denis Kenzior
4 siblings, 0 replies; 6+ messages in thread
From: James Prestwood @ 2024-02-29 17:07 UTC (permalink / raw
To: iwd; +Cc: James Prestwood
Since these are all stack variables they are not zero initialized.
If parsing fails there may be invalid pointers within the structures
which can get dereferenced by p2p_clear_*
---
src/p2putil.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/p2putil.c b/src/p2putil.c
index faf151a5..c90810e5 100644
--- a/src/p2putil.c
+++ b/src/p2putil.c
@@ -776,7 +776,7 @@ done:
/* Section 4.2.1 */
int p2p_parse_beacon(const uint8_t *pdu, size_t len, struct p2p_beacon *out)
{
- struct p2p_beacon d = {};
+ struct p2p_beacon d = {0};
int r;
r = p2p_parse_attrs(pdu, len,
@@ -797,7 +797,7 @@ int p2p_parse_beacon(const uint8_t *pdu, size_t len, struct p2p_beacon *out)
int p2p_parse_probe_req(const uint8_t *pdu, size_t len,
struct p2p_probe_req *out)
{
- struct p2p_probe_req d = {};
+ struct p2p_probe_req d = {0};
int r;
r = p2p_parse_attrs(pdu, len,
@@ -828,7 +828,7 @@ int p2p_parse_probe_req(const uint8_t *pdu, size_t len,
int p2p_parse_probe_resp(const uint8_t *pdu, size_t len,
struct p2p_probe_resp *out)
{
- struct p2p_probe_resp d = {};
+ struct p2p_probe_resp d = {0};
int r;
r = p2p_parse_attrs(pdu, len,
@@ -853,7 +853,7 @@ int p2p_parse_probe_resp(const uint8_t *pdu, size_t len,
int p2p_parse_association_req(const uint8_t *pdu, size_t len,
struct p2p_association_req *out)
{
- struct p2p_association_req d = {};
+ struct p2p_association_req d = {0};
int r;
r = p2p_parse_attrs(pdu, len,
@@ -876,7 +876,7 @@ int p2p_parse_association_req(const uint8_t *pdu, size_t len,
int p2p_parse_association_resp(const uint8_t *pdu, size_t len,
struct p2p_association_resp *out)
{
- struct p2p_association_resp d = {};
+ struct p2p_association_resp d = {0};
int r;
r = p2p_parse_attrs(pdu, len,
@@ -939,7 +939,7 @@ int p2p_parse_disassociation(const uint8_t *pdu, size_t len,
int p2p_parse_go_negotiation_req(const uint8_t *pdu, size_t len,
struct p2p_go_negotiation_req *out)
{
- struct p2p_go_negotiation_req d = {};
+ struct p2p_go_negotiation_req d = {0};
int r;
struct p2p_go_intent_attr go_intent;
uint8_t *wsc_data;
@@ -1001,7 +1001,7 @@ error:
int p2p_parse_go_negotiation_resp(const uint8_t *pdu, size_t len,
struct p2p_go_negotiation_resp *out)
{
- struct p2p_go_negotiation_resp d = {};
+ struct p2p_go_negotiation_resp d = {0};
int r;
struct p2p_go_intent_attr go_intent;
uint8_t *wsc_data;
@@ -1062,7 +1062,7 @@ error:
int p2p_parse_go_negotiation_confirmation(const uint8_t *pdu, size_t len,
struct p2p_go_negotiation_confirmation *out)
{
- struct p2p_go_negotiation_confirmation d = {};
+ struct p2p_go_negotiation_confirmation d = {0};
int r;
if (len < 1)
@@ -1096,7 +1096,7 @@ error:
int p2p_parse_invitation_req(const uint8_t *pdu, size_t len,
struct p2p_invitation_req *out)
{
- struct p2p_invitation_req d = {};
+ struct p2p_invitation_req d = {0};
int r;
uint8_t *wsc_data;
ssize_t wsc_len;
@@ -1151,7 +1151,7 @@ error:
int p2p_parse_invitation_resp(const uint8_t *pdu, size_t len,
struct p2p_invitation_resp *out)
{
- struct p2p_invitation_resp d = {};
+ struct p2p_invitation_resp d = {0};
int r;
if (len < 1)
@@ -1185,7 +1185,7 @@ error:
int p2p_parse_device_disc_req(const uint8_t *pdu, size_t len,
struct p2p_device_discoverability_req *out)
{
- struct p2p_device_discoverability_req d = {};
+ struct p2p_device_discoverability_req d = {0};
int r;
if (len < 1)
@@ -1210,7 +1210,7 @@ int p2p_parse_device_disc_req(const uint8_t *pdu, size_t len,
int p2p_parse_device_disc_resp(const uint8_t *pdu, size_t len,
struct p2p_device_discoverability_resp *out)
{
- struct p2p_device_discoverability_resp d = {};
+ struct p2p_device_discoverability_resp d = {0};
int r;
if (len < 1)
@@ -1234,7 +1234,7 @@ int p2p_parse_device_disc_resp(const uint8_t *pdu, size_t len,
int p2p_parse_provision_disc_req(const uint8_t *pdu, size_t len,
struct p2p_provision_discovery_req *out)
{
- struct p2p_provision_discovery_req d = {};
+ struct p2p_provision_discovery_req d = {0};
int r;
uint8_t *wsc_data;
ssize_t wsc_len;
@@ -1309,7 +1309,7 @@ error:
int p2p_parse_provision_disc_resp(const uint8_t *pdu, size_t len,
struct p2p_provision_discovery_resp *out)
{
- struct p2p_provision_discovery_resp d = {};
+ struct p2p_provision_discovery_resp d = {0};
int r;
uint8_t *wsc_data;
ssize_t wsc_len;
@@ -1389,7 +1389,7 @@ error:
int p2p_parse_notice_of_absence(const uint8_t *pdu, size_t len,
struct p2p_notice_of_absence *out)
{
- struct p2p_notice_of_absence d = {};
+ struct p2p_notice_of_absence d = {0};
int r;
if (len < 1)
@@ -1411,7 +1411,7 @@ int p2p_parse_notice_of_absence(const uint8_t *pdu, size_t len,
int p2p_parse_presence_req(const uint8_t *pdu, size_t len,
struct p2p_presence_req *out)
{
- struct p2p_presence_req d = {};
+ struct p2p_presence_req d = {0};
int r;
if (len < 1)
@@ -1437,7 +1437,7 @@ int p2p_parse_presence_req(const uint8_t *pdu, size_t len,
int p2p_parse_presence_resp(const uint8_t *pdu, size_t len,
struct p2p_presence_resp *out)
{
- struct p2p_presence_resp d = {};
+ struct p2p_presence_resp d = {0};
int r;
if (len < 1)
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 5/5] p2putil: check length of client info description
2024-02-29 17:07 [PATCH 1/5] auto-t: end process_io on HUP signal, detect process crash James Prestwood
` (2 preceding siblings ...)
2024-02-29 17:07 ` [PATCH 4/5] p2putil: initialize all parsing structures to zero James Prestwood
@ 2024-02-29 17:07 ` James Prestwood
2024-02-29 20:36 ` [PATCH 1/5] auto-t: end process_io on HUP signal, detect process crash Denis Kenzior
4 siblings, 0 replies; 6+ messages in thread
From: James Prestwood @ 2024-02-29 17:07 UTC (permalink / raw
To: iwd; +Cc: James Prestwood, Alex Radocea
A length check was missing which could cause a out of bounds read.
Co-authored-by: Alex Radocea <alex@supernetworks.org>
---
src/p2putil.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/p2putil.c b/src/p2putil.c
index c90810e5..d1f114d0 100644
--- a/src/p2putil.c
+++ b/src/p2putil.c
@@ -376,6 +376,9 @@ static bool extract_p2p_group_info(const uint8_t *attr, size_t len,
desc = l_new(struct p2p_client_info_descriptor, 1);
l_queue_push_tail(*out, desc);
+ if (desc_len < 24)
+ goto error;
+
memcpy(desc->device_addr, attr + 0, 6);
memcpy(desc->interface_addr, attr + 6, 6);
desc->device_caps = attr[12];
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/5] auto-t: end process_io on HUP signal, detect process crash
2024-02-29 17:07 [PATCH 1/5] auto-t: end process_io on HUP signal, detect process crash James Prestwood
` (3 preceding siblings ...)
2024-02-29 17:07 ` [PATCH 5/5] p2putil: check length of client info description James Prestwood
@ 2024-02-29 20:36 ` Denis Kenzior
4 siblings, 0 replies; 6+ messages in thread
From: Denis Kenzior @ 2024-02-29 20:36 UTC (permalink / raw
To: James Prestwood, iwd
Hi James,
On 2/29/24 11:07, James Prestwood wrote:
> When HUP is received the IO read callback was never completing which
> caused it to block indefinitely until waited for. This didn't matter
> for most transient processes but for IWD, hostapd, wpa_supplicant
> it would cause test-runner to hang if the process crashed.
>
> Detecting a crash is somewhat hacky because we have no process
> management like systemd and the return code isn't reliable as some
> processes return non-zero under normal circumstances. So to detect
> a crash the process output is being checked for the string:
> "++++++++ backtrace ++++++++". This isn't 100% reliable obviously
> since its dependent on how the binary is compiled, but even if the
> crash itself isn't detected any test should still fail if written
> correctly.
>
> Doing this allows auto-tests to handle IWD crashes gracefully by
> failing the test, printing the exception (event without debugging)
> and continue with other tests.
> ---
> tools/utils.py | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
All applied, thanks.
Regards,
-Denis
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-29 20:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-29 17:07 [PATCH 1/5] auto-t: end process_io on HUP signal, detect process crash James Prestwood
2024-02-29 17:07 ` [PATCH 2/5] auto-t: Add frame fuzzing test James Prestwood
2024-02-29 17:07 ` [PATCH 3/5] p2putil: fix crash/remove side effect parsing adv service info James Prestwood
2024-02-29 17:07 ` [PATCH 4/5] p2putil: initialize all parsing structures to zero James Prestwood
2024-02-29 17:07 ` [PATCH 5/5] p2putil: check length of client info description James Prestwood
2024-02-29 20:36 ` [PATCH 1/5] auto-t: end process_io on HUP signal, detect process crash Denis Kenzior
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.