yuezk / GlobalProtect-openconnect

A GlobalProtect VPN client for Linux, written in Rust, based on OpenConnect and Tauri, supports SSO with MFA, Yubikey, and client certificate authentication, etc.
GNU General Public License v3.0
1.29k stars 147 forks source link

gpclient hangs and does not connect #34

Closed jochristian closed 3 years ago

jochristian commented 3 years ago

Hi, gpclient does not connect, but hangs on "Start parsing the priority rules..." Complete log follows

020-10-12 21:10:11.452 INFO  [15995] [main@22] GlobalProtect started, version: v1.2.5
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-root'
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-root'
2020-10-12 21:10:11.755 INFO  [15995] [GPClient::populateGatewayMenu@100] Populating the Switch Gateway menu...
2020-10-12 21:10:18.109 INFO  [15995] [GPClient::doConnect@205] Start connecting...
2020-10-12 21:10:18.109 INFO  [15995] [GPClient::doConnect@226] Start portal login...
2020-10-12 21:10:18.129 INFO  [15995] [PortalAuthenticator::authenticate@29] Preform portal prelogin at https://portal.test.com/global-protect/prelogin.esp?tmp=tmp&kerberos-support=yes&ipv6-support=yes&clientVer=4100&clientos=Linux
2020-10-12 21:10:18.137 INFO  [15995] [GPClient::populateGatewayMenu@100] Populating the Switch Gateway menu...
2020-10-12 21:10:18.555 INFO  [15995] [PortalAuthenticator::onPreloginFinished@46] Portal prelogin succeeded.
2020-10-12 21:10:18.555 INFO  [15995] [PreloginResponse::parse@26] Start parsing the prelogin response...
2020-10-12 21:10:18.556 INFO  [15995] [PortalAuthenticator::onPreloginFinished@50] Finished parsing the prelogin response. The region field is: 192.168.0.0-192.168.255.255
2020-10-12 21:10:18.556 INFO  [15995] [PortalAuthenticator::normalAuth@82] Trying to launch the normal login window...
2020-10-12 21:10:18.629 INFO  [15995] [GPClient::populateGatewayMenu@100] Populating the Switch Gateway menu...
2020-10-12 21:11:03.606 INFO  [15995] [PortalAuthenticator::fetchConfig@157] Fetching the portal config from https://portal.test.com/global-protect/getconfig.esp for user: jochristian
2020-10-12 21:11:05.190 INFO  [15995] [PortalAuthenticator::onFetchConfigFinished@183] Fetch the portal config succeeded.
2020-10-12 21:11:05.190 INFO  [15995] [PortalConfigResponse::parse@20] Start parsing the portal configuration...
2020-10-12 21:11:05.190 INFO  [15995] [PortalConfigResponse::parseGateways@64] Start parsing the gateways from portal configuration...
2020-10-12 21:11:05.191 INFO  [15995] [PortalConfigResponse::parsePriorityRules@88] Start parsing the priority rules...

Let me know if you need any more information. Palo Alto firewall is running 10.0.1. But I have seen the same issue on 9.1.4.

BTW, openconnect --protocol=gp works without any issues.

Thanks

/Jo Christian

rsantanna commented 3 years ago

So, the same happened to me and I just found that the parsing of Priority Rules and Gateways lead to an infinite loop in my case. I'm not an expert in Qt (actually that was my first time debugging a C++ code) but my workaround did work. Here is what I've done to fix. I'll improve the code later to open a PR.

tl;dr: the XML parsing seems to make assumptions that fail. I've added safety checks to guarantee that the parsing occurs as expected.

XML

The gateways section returned in my case looks exactly like this:

...
<gateways>
    <cutoff-time>5</cutoff-time>
    <internal>  
        <list>
            <entry name="my-vpn-int.example.com">
                <description>Internal_Gateway</description>
            </entry>
        </list>
    </internal>
    <external>
        <list>
            <entry name="my-vpn.example.com">
                <priority-rule>
                    <entry name="Any">
                        <priority>1</priority>
                    </entry>
                </priority-rule>
                <priority>1</priority>
                <description>External_Gateway</description>
            </entry>
        </list>
    </external>
</gateways>
...

Priority Rules

The problem:

This is the original code:

QMap<QString, int> PortalConfigResponse::parsePriorityRules(QXmlStreamReader &xmlReader)
{
    PLOGI << "Start parsing the priority rules...";

    QMap<QString, int> priorityRules;

    while (xmlReader.name() != "priority-rule" || !xmlReader.isEndElement()) {
        xmlReader.readNext();

        if (xmlReader.name() == "entry" && xmlReader.isStartElement()) {
            QString ruleName = xmlReader.attributes().value("name").toString();
            // Read the priority tag
            xmlReader.readNextStartElement();
            int ruleValue = xmlReader.readElementText().toUInt(); // <-- It fails here!
            priorityRules.insert(ruleName, ruleValue);
        }
    }

    PLOGI << "Finished parsing the priority rules.";

    return priorityRules;
}

The method assumes that the tag <priority> comes immediately after <entry>, which in my case is not true. So, it fails at the line marked leading to an QXmlStreamReader::UnexpectedElementError. This cause the xmlReader to stop reading more tokens and since the while loop has no check for errors, it gets stuck and the code doesn't finish.

The workaround:

QMap<QString, int> PortalConfigResponse::parsePriorityRules(QXmlStreamReader &xmlReader)
{
    PLOGI << "Start parsing the priority rules...";

    QMap<QString, int> priorityRules;

    while ((xmlReader.name() != "priority-rule" || !xmlReader.isEndElement()) && !xmlReader.hasError()) {
        xmlReader.readNext();

        if (xmlReader.name() == "entry" && xmlReader.isStartElement()) {
            QString ruleName = xmlReader.attributes().value("name").toString();
            // Read the priority tag
            while (xmlReader.name() != "priority"){
                xmlReader.readNext();
            }
            //xmlReader.readNextStartElement();
            int ruleValue = xmlReader.readElementText().toUInt();
            priorityRules.insert(ruleName, ruleValue);
        }
    }

    PLOGI << "Finished parsing the priority rules.";

    return priorityRules;
}

I put an error check to prevent infinite loop and added a nested while to search for the tag <priority> before calling readElementText().

Gateways

The problem:

This is the original code:

QList<GPGateway> PortalConfigResponse::parseGateways(QXmlStreamReader &xmlReader)
{
    PLOGI << "Start parsing the gateways from portal configuration...";

    QList<GPGateway> gateways;

    while (xmlReader.name() != xmlGateways || !xmlReader.isEndElement()) {
        xmlReader.readNext();
        // Parse the gateways -> external -> list -> entry
        if (xmlReader.name() == "entry" && xmlReader.isStartElement()) {
            GPGateway g;
            QString address = xmlReader.attributes().value("name").toString();
            g.setAddress(address);
            g.setPriorityRules(parsePriorityRules(xmlReader));
            g.setName(parseGatewayName(xmlReader));
            gateways.append(g);
        }
    }

    PLOGI << "Finished parsing the gateways.";

    return gateways;
}

Here the code executes without problem, but instead of getting the external gateway it takes the internal first and fails to connect since the internal gateway is unreachable.

The (very ugly) workaround:

QList<GPGateway> PortalConfigResponse::parseGateways(QXmlStreamReader &xmlReader)
{
    PLOGI << "Start parsing the gateways from portal configuration...";

    QList<GPGateway> gateways;

    while (xmlReader.name() != "external"){
        xmlReader.readNext();
    }

    while (xmlReader.name() != "list"){
        xmlReader.readNext();
    }

    while (xmlReader.name() != xmlGateways || !xmlReader.isEndElement()) {
        xmlReader.readNext();
        // Parse the gateways -> external -> list -> entry
        if (xmlReader.name() == "entry" && xmlReader.isStartElement()) {
            GPGateway g;
            QString address = xmlReader.attributes().value("name").toString();
            g.setAddress(address);
            g.setPriorityRules(parsePriorityRules(xmlReader));
            g.setName(parseGatewayName(xmlReader));
            gateways.append(g);
        }
    }

    PLOGI << "Finished parsing the gateways.";

    return gateways;
}

I added two loops before the original one to make sure that the entries read by the method correspond to those of the external list only.

Kind regards,

Rapha

CardboardTurkey commented 3 years ago

I have also encountered this problem and also find that openconnect --protocol=gp works fine.

I'm also a vpn novice and can't make heads or tails of rsantanna's fix 😛

rsantanna commented 3 years ago

I have also encountered this problem and also find that openconnect --protocol=gp works fine.

I'm also a vpn novice and can't make heads or tails of rsantanna's fix

I forked the project to open a PR later. The code is still ugly, but you can check it out and see if it works for you.

https://github.com/rsantanna/GlobalProtect-openconnect/tree/gateways-parsing-fix

mikedld commented 3 years ago

This should probably be closed as the PR got merged and fix released.

betrcode commented 1 year ago

I think I have this problem and I am running this version: 1.4.8+28snapshot.g4a3f74f

gpclient --version
2023-02-07 11:07:38.108 INFO  [24062] [main@24] GlobalProtect started, version: 1.4.8+28snapshot.g4a3f74f
gpclient