Fix TLS verification, four out-of-bounds reads, and the firmware-update hang/silent-failure

AntScope.pro

@@ -267,6 +267,15 @@ unix:!macx {
     SOURCES += analyzer/usbhid/hidapi/linux/hid.c
     LIBS += -lusb-1.0
     DEFINES += _NO_WINDOWS_
+
+    # Without this, analyzer/usbhid/hidapi/linux/hid.c's
+    # libusb_claim_interface() is called without ever detaching the
+    # kernel's usbhid driver first. usbhid auto-binds normal-mode HID
+    # interfaces on Linux, and while libusb can still open/claim the
+    # interface in that state, an interrupt-OUT hid_write() to it can fail
+    # outright (observed: hid_write() returning -1 for the firmware-update
+    # RESET report on a real device the kernel had already claimed).
+    DEFINES += DETACH_KERNEL_DRIVER
 }
 
 macx {

README.md

@@ -1,7 +1,16 @@
 The AntScope2 software is designed to support various models of RigExpert analyzers under various operating systems.
 
-Windows: QT5, QT Creator 2 and higher
+Windows: Qt6, Qt Creator 2 and higher
 
-Linux: to do
+Linux: Qt6, qmake. Build with:
+
+```
+qmake6 CONFIG+=release AntScope.pro
+make
+```
+
+Depends on `qt6-base`, `qt6-serialport`, `qt6-connectivity`, and `libusb` (used via the vendored HIDAPI backend for HID-connected analyzers). Packaged for Arch Linux/AUR as
+[`antscope2`](https://aur.archlinux.org/packages/antscope2) (pinned to a known-good commit, patched)
+and [`antscope2-git`](https://aur.archlinux.org/packages/antscope2-git) (tracks a patched branch's tip - see either package's `PKGBUILD` for the udev rule, desktop file, and data-path packaging details this repo's own build doesn't set up on its own).
 
 Mac OS: to do

analyzer/ble_analyzer.cpp

@@ -474,6 +474,13 @@ void BleAnalyzer::dataReceived(const QLowEnergyCharacteristic &c, const QByteArr
 {
     if (c.uuid() != QBluetoothUuid(uuidRead))
         return;
+    if (value.size() < BLE_PACKET_SIZE) {
+        // checkCRC()/returnCRC() index data[BLE_PACKET_SIZE-1] with no
+        // bounds check of their own; a malformed or malicious peripheral
+        // sending a short notification would otherwise read out of bounds.
+        qInfo() << "dataReceived: short packet, ignoring" << value.size();
+        return;
+    }
     //qInfo() << trace("dataReceived: ", const_cast<QByteArray&>(value));
     m_lastReadTimeMS = QDateTime::currentMSecsSinceEpoch();
     if (!checkCRC(value)) {

analyzer/hid_analyzer.cpp

@@ -2,6 +2,7 @@
 #include "customanalyzer.h"
 #include <QtConcurrent/QtConcurrentRun>
 #include <QThread>
+#include <QElapsedTimer>
 #include "analyzerpro.h"
 
 extern bool g_usbOnly;
@@ -453,14 +454,20 @@ void HidAnalyzer::hidRead (void)
     {
         return;
     }
-    unsigned char readBuff[64];
+    unsigned char readBuff[64] = {0};
     int read = hid_read(m_hidDevice, readBuff, 64);
     m_mutexRead.lock();
     if(read > 0)
     {
         if(readBuff[0] == ANTSCOPE_REPORT)
         {
-            for(int i = 0; i < readBuff[1]; i++)
+            // readBuff[1] is a payload length supplied by the device
+            // (0-255); clamp it to both the fixed-size buffer and to how
+            // many bytes hid_read() actually returned, so a device
+            // reporting more than 62 payload bytes - or a short read -
+            // cannot walk past the end of readBuff.
+            int len = qMin<int>(readBuff[1], qMax(0, read - 2));
+            for(int i = 0; i < len; i++)
             {
                 m_incomingBuffer.append(readBuff[i+2]);
             }
@@ -680,9 +687,59 @@ void HidAnalyzer::preUpdate ()
 {
     hid_close(m_hidDevice);
     m_hidDevice = nullptr;
+    // Leaving boot mode (whichever way update() ended, success or
+    // failure) - without this, a second update() call would skip the
+    // boot-mode entry block above and immediately try to write firmware
+    // commands to the now-null m_hidDevice.
+    m_bootMode = false;
     searchAnalyzer(true);
 }
 
+bool HidAnalyzer::waitForBootDevice(qint64 timeoutMs)
+{
+    QElapsedTimer elapsed;
+    elapsed.start();
+    while (!elapsed.hasExpired(timeoutMs))
+    {
+        struct hid_device_info* devs = hid_enumerate(RE_BOOT_VID, RE_BOOT_PID);
+        hid_device* newDevice = nullptr;
+        for (struct hid_device_info* cur = devs; cur != nullptr; cur = cur->next)
+        {
+            // The serial number is preserved across the mode switch
+            // (verified against a real capture). If for some reason it's
+            // not known (m_serialNumber empty), fall back to matching on
+            // VID:PID alone rather than refusing to proceed.
+            QString serial = QString::fromWCharArray(cur->serial_number);
+            if (!m_serialNumber.isEmpty() && serial != m_serialNumber)
+            {
+                continue;
+            }
+            newDevice = hid_open(RE_BOOT_VID, RE_BOOT_PID, cur->serial_number);
+            if (newDevice != nullptr)
+            {
+                break;
+            }
+        }
+        hid_free_enumeration(devs);
+
+        if (newDevice != nullptr)
+        {
+            if (m_hidDevice != nullptr)
+            {
+                hid_close(m_hidDevice);
+            }
+            m_hidDevice = newDevice;
+            hid_set_nonblocking(m_hidDevice, 1);
+            m_bootMode = true;
+            return true;
+        }
+
+        QCoreApplication::processEvents();
+        QThread::msleep(100);
+    }
+    return false;
+}
+
 bool HidAnalyzer::update (QIODevice *fw)
 {
     m_hidReadTimer->stop();
@@ -697,16 +754,17 @@ bool HidAnalyzer::update (QIODevice *fw)
         hid_write(m_hidDevice, buff, sizeof(buff));
         qDebug() << "RESET: " << hidError(m_hidDevice);
 
-        QTimer::singleShot(5000, this, [this]() {
-            this->preUpdate();
-        });
-        while(1)//for(int i = 0; i < 565535; ++i)
-        {
-            if(m_bootMode)
-                break;
-            QCoreApplication::processEvents();
-        }
-        if(!m_bootMode)
+        // The device closes its normal-mode connection and re-enumerates
+        // under RE_BOOT_VID:RE_BOOT_PID as its DFU bootloader, keeping the
+        // same USB serial number (verified against a real capture of a
+        // successful update - see docs/firmware-update-protocol-findings.md
+        // in this tree). The app's normal hot-plug detection
+        // (searchAnalyzer()/m_devices) is gated behind g_usbOnly and, even
+        // if it were reachable, only ever looks for the application-mode
+        // VID:PID - it can never find the device in this state. Poll for it
+        // directly instead. 15s is a generous bound; the one real session
+        // this was verified against re-enumerated within about 1.3s.
+        if (!waitForBootDevice(15000))
         {
             g_showMessageBox(nullptr, QMessageBox::Warning,tr("Warning"),tr("Can't enter to boot mode!"));
             return false;
@@ -752,16 +810,24 @@ bool HidAnalyzer::update (QIODevice *fw)
         emit updatePercentChanged(i*100/totsize);
         QCoreApplication::processEvents();
 
-        if (firstWrite)
+        // Read and check the response to every chunk, not just the first.
+        // hidapi's background read thread queues unread reports and drops
+        // the oldest once the queue is full (see
+        // docs/firmware-update-protocol-findings.md); previously only the
+        // first chunk's response was ever consumed, so by the time the
+        // final BL_CMD_CHECK was sent, the queue held nothing but stale
+        // BL_CMD_OK reports left over from earlier chunks - the "checksum
+        // verification" was reading one of those, not a real answer to
+        // CHECK, and so reported success unconditionally. Reading (and
+        // pacing on) every response fixes that as a side effect, and
+        // means a real BL_CMD_ERROR from the device is no longer silently
+        // ignored.
+        firstWrite = false;
+        res = waitAnswer();
+        if (!res)
         {
-            res = waitAnswer();
-            firstWrite = false;
-            if (!res)
-            {
-                emit updatePercentChanged(100);
-                return false;
-                break;
-            }
+            emit updatePercentChanged(100);
+            return false;
         }
     }
     emit updatePercentChanged(100);

analyzer/hid_analyzer.h

@@ -61,6 +61,20 @@ class HidAnalyzer : public BaseAnalyzer
 
     bool update(QIODevice *fw);
 
+#ifdef ANTSCOPE2_UNIT_TEST
+    // Test-only seam: inject an already-open device handle and serial
+    // number, bypassing the normal UI-dialog-driven connection
+    // establishment (SelectDeviceDialog / AnalyzerPro::createDevice),
+    // so update() can be exercised in isolation against a mock hidapi
+    // backend. Compiled out entirely unless ANTSCOPE2_UNIT_TEST is
+    // defined; has no effect on and is not reachable from the real app.
+    void setTestHidDevice(hid_device* dev, const QString& serial)
+    {
+        m_hidDevice = dev;
+        m_serialNumber = serial;
+    }
+#endif
+
     void nonblocking (int nonblock);
     void preUpdate();
     QString hidError(hid_device* _device);
@@ -104,6 +118,7 @@ private slots:
     bool disconnectHid(void);
     qint32 parse (QByteArray arr);
     bool waitAnswer();
+    bool waitForBootDevice(qint64 timeoutMs);
     QFuture<struct hid_device_info*> *m_futureRefresh;
     QFutureWatcher<struct hid_device_info*> *m_watcherRefresh;
 };

analyzer/hidanalyzer.cpp

@@ -2,6 +2,7 @@
 #include "customanalyzer.h"
 #include <QtConcurrent/QtConcurrentRun>
 #include <QThread>
+#include <QElapsedTimer>
 #include "analyzer.h"
 extern int g_showMessageBox(QWidget* parent, QMessageBox::Icon icon,
                             QString title, QString text,
@@ -462,14 +463,20 @@ void hidAnalyzer::hidRead (void)
     {
         return;
     }
-    unsigned char readBuff[64];
+    unsigned char readBuff[64] = {0};
     int read = hid_read(m_hidDevice, readBuff, 64);
     m_mutexRead.lock();
     if(read > 0)
     {
         if(readBuff[0] == ANTSCOPE_REPORT)
         {
-            for(int i = 0; i < readBuff[1]; i++)
+            // readBuff[1] is a payload length supplied by the device
+            // (0-255); clamp it to both the fixed-size buffer and to how
+            // many bytes hid_read() actually returned, so a device
+            // reporting more than 62 payload bytes - or a short read -
+            // cannot walk past the end of readBuff.
+            int len = qMin<int>(readBuff[1], qMax(0, read - 2));
+            for(int i = 0; i < len; i++)
             {
                 m_incomingBuffer.append(readBuff[i+2]);
             }
@@ -755,10 +762,22 @@ bool hidAnalyzer::update (QIODevice *fw)
         QTimer::singleShot(5000, this, [this]() {
             this->preUpdate();
         });
-        while(1)//for(int i = 0; i < 565535; ++i)
+        // m_bootMode is only ever set by the device re-arrival path, which
+        // does not currently work (the feature this belongs to is
+        // incomplete and unreachable from the UI). Previously this was an
+        // unbounded while(1) with no way out but m_bootMode becoming true,
+        // so a call that could never detect the device coming back would
+        // spin forever instead of ever reaching the failure handling right
+        // below. Bound the wait so that path is actually reachable.
+        QElapsedTimer bootModeWait;
+        bootModeWait.start();
+        const qint64 bootModeTimeoutMs = 15000;
+        while(1)
         {
             if(m_bootMode)
                 break;
+            if(bootModeWait.hasExpired(bootModeTimeoutMs))
+                break;
             QCoreApplication::processEvents();
         }
         if(!m_bootMode)

analyzer/updater/aa230firmwareupdater.cpp

@@ -124,7 +124,10 @@ AA230FirmwareUpdater::FirmwareInfo AA230FirmwareUpdater::firmwareInfo(const ReDe
         arr = m_port.readAll();
     }
 
-    if (arr.isEmpty()) {
+    // The probe loop above gives up after 10 timeouts and falls through
+    // regardless of how many bytes actually arrived, so arr can be
+    // shorter than FirmwareInfo on a slow/short/malicious response.
+    if (arr.size() < static_cast<int>(sizeof(FirmwareInfo))) {
         return info;
     }
 

analyzer/updater/downloader.cpp

@@ -29,9 +29,6 @@ Downloader::State Downloader::startDownloadInfo(QUrl url)
     QNetworkRequest request(url);
 
     m_mng.clearAccessCache();
-    QSslConfiguration conf = request.sslConfiguration();
-    conf.setPeerVerifyMode(QSslSocket::VerifyNone);
-    request.setSslConfiguration(conf);
 
     m_mng.get(request);
 
@@ -61,9 +58,6 @@ Downloader::State Downloader::startDownloadFw()
     QNetworkReply *reply;
 
     m_mng.clearAccessCache();
-    QSslConfiguration conf = request.sslConfiguration();
-    conf.setPeerVerifyMode(QSslSocket::VerifyNone);
-    request.setSslConfiguration(conf);
 
     reply = m_mng.get(request);
     connect(reply, SIGNAL(downloadProgress(qint64,qint64)),

analyzer/updater/hidfirmwareupdater.cpp

@@ -167,7 +167,15 @@ HidFirmwareUpdater::FirmwareInfo HidFirmwareUpdater::firmwareInfo(const ReDevice
     ret = hid_read_timeout(m_handleDev, buff, sizeof(buff), 100);
 
     if (ret > 0) {
-        memcpy((char*)&info, &buff[6], 60);
+        // buff is 65 bytes (valid indices 0-64); copying 60 bytes
+        // starting at offset 6 would read buff[65], one byte past the
+        // end. Clamp to both the buffer's real size and to how many
+        // bytes were actually received.
+        int avail = qMin<int>(ret, sizeof(buff));
+        int len = qMin(60, avail - 6);
+        if (len > 0) {
+            memcpy((char*)&info, &buff[6], len);
+        }
     }
 
     closeDevice();

licenseagent.cpp

@@ -62,9 +62,6 @@ void LicenseAgent::requestEmailStatus()
     request.setTransferTimeout(REPLY_TIMEOUT);
 
     m_mng.clearAccessCache();
-    QSslConfiguration conf = request.sslConfiguration();
-    conf.setPeerVerifyMode(QSslSocket::VerifyNone);
-    request.setSslConfiguration(conf);
 
     setState(WaitEmailStatusWeb);
     m_mng.get(request);
@@ -149,9 +146,6 @@ void LicenseAgent::requestLicense(QString key)
     request.setTransferTimeout(REPLY_TIMEOUT);
 
     m_mng.clearAccessCache();
-    QSslConfiguration conf = request.sslConfiguration();
-    conf.setPeerVerifyMode(QSslSocket::VerifyNone);
-    request.setSslConfiguration(conf);
 
     setState(WaitLicense);
     m_canceled = false;
@@ -296,9 +290,6 @@ void LicenseAgent::requestInfo()
     request.setTransferTimeout(REPLY_TIMEOUT);
 
     m_mng.clearAccessCache();
-    QSslConfiguration conf = request.sslConfiguration();
-    conf.setPeerVerifyMode(QSslSocket::VerifyNone);
-    request.setSslConfiguration(conf);
 
     setState(WaitInfoWeb);
     //showModeless(tr("Register device"),tr("Registration..."), tr("Cancel"));
@@ -362,9 +353,6 @@ void LicenseAgent::requestUnit()
     request.setTransferTimeout(REPLY_TIMEOUT);
 
     m_mng.clearAccessCache();
-    QSslConfiguration conf = request.sslConfiguration();
-    conf.setPeerVerifyMode(QSslSocket::VerifyNone);
-    request.setSslConfiguration(conf);
 
     setState(WaitUnitWeb);
     m_mng.get(request);
@@ -432,9 +420,6 @@ void LicenseAgent::requestStatus_B16(QByteArray data)
     request.setTransferTimeout(REPLY_TIMEOUT);
 
     m_mng.clearAccessCache();
-    QSslConfiguration conf = request.sslConfiguration();
-    conf.setPeerVerifyMode(QSslSocket::VerifyNone);
-    request.setSslConfiguration(conf);
 
     setState(WaitProfileB16);
     m_mng.get(request);
@@ -453,9 +438,6 @@ void LicenseAgent::requestInfo_B16(QByteArray data)
     request.setTransferTimeout(REPLY_TIMEOUT);
 
     m_mng.clearAccessCache();
-    QSslConfiguration conf = request.sslConfiguration();
-    conf.setPeerVerifyMode(QSslSocket::VerifyNone);
-    request.setSslConfiguration(conf);
 
     setState(WaitInfoB16);
     m_mng.get(request);