Skip to content

Commit

Permalink
Merge pull request #4739: Backport "FIX(client): Only allow "http"/"h…
Browse files Browse the repository at this point in the history
…ttps" for URLs in ConnectDialog"

Our public server list registration script doesn't have an URL scheme
whitelist for the website field.

Turns out a malicious server can register itself with a dangerous URL in
an attempt to attack a user's machine.

User interaction is required, as the URL has to be opened by
right-clicking on the server entry and clicking on "Open Webpage".

This commit introduces a client-side whitelist, which only allows "http"
and "https" schemes. We will also implement it in our public list.

In future we should probably add a warning QMessageBox informing the
user that there's no guarantee the URL is safe (regardless of the
scheme).

Thanks a lot to https://positive.security for reporting the RCE
vulnerability to us privately.

This is a backport of #4733
  • Loading branch information
Krzmbrzl authored Feb 6, 2021
2 parents 8ac07a3 + fb28df2 commit 6b54dbc
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 3 deletions.
20 changes: 17 additions & 3 deletions src/mumble/ConnectDialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1265,11 +1265,25 @@ void ConnectDialog::on_qaFavoritePaste_triggered() {
}

void ConnectDialog::on_qaUrl_triggered() {
ServerItem *si = static_cast<ServerItem *>(qtwServers->currentItem());
if (! si || si->qsUrl.isEmpty())
auto *si = static_cast< const ServerItem * >(qtwServers->currentItem());
if (!si || si->qsUrl.isEmpty()) {
return;
}

const QStringList allowedSchemes = { QLatin1String("http"), QLatin1String("https") };

QDesktopServices::openUrl(QUrl(si->qsUrl));
const auto url = QUrl(si->qsUrl);
if (allowedSchemes.contains(url.scheme())) {
QDesktopServices::openUrl(url);
} else {
// Inform user that the requested URL has been blocked
QMessageBox msgBox;
msgBox.setText(QObject::tr("<b>Blocked URL scheme \"%1\"</b>").arg(url.scheme()));
msgBox.setInformativeText(QObject::tr("The URL uses a scheme that has been blocked for security reasons."));
msgBox.setDetailedText(QObject::tr("Blocked URL: \"%1\"").arg(url.toString()));
msgBox.setIcon(QMessageBox::Warning);
msgBox.exec();
}
}

void ConnectDialog::onFiltersTriggered(QAction *act) {
Expand Down
14 changes: 14 additions & 0 deletions src/mumble/mumble_en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7925,6 +7925,20 @@ To upgrade these files to their latest versions, click the button below.</source
<translation type="unfinished"></translation>
</message>
</context>
<context>
<message>
<source>&lt;b&gt;Blocked URL scheme &quot;%1&quot;&lt;/b&gt;</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>The URL uses a scheme that has been blocked for security reasons.</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Blocked URL: &quot;%1&quot;</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>RichTextEditor</name>
<message>
Expand Down

0 comments on commit 6b54dbc

Please sign in to comment.