Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate sensitive URLs to onle allow http(s) schemes. #2906

Merged
merged 2 commits into from
Feb 9, 2021
Merged

Validate sensitive URLs to onle allow http(s) schemes. #2906

merged 2 commits into from
Feb 9, 2021

Conversation

allexzander
Copy link
Contributor

Signed-off-by: allexzander blackslayer4@gmail.com

@allexzander
Copy link
Contributor Author

I only validate those URLs coming from the server or user input. Please go through the rest of QDesktopServices::openUrl calls to see why I am not validating those. Non-validate URLs are either hard-coded or point to local files/folders. As @er-vin suggested, I did the revision of all the QDesktopServices::openUrl cases and only validated those risky-ones.

er-vin
er-vin reviewed Feb 5, 2021
View reviewed changes
src/gui/guiutility.cpp Outdated
@@ -23,12 +23,24 @@

#include "common/asserts.h"

namespace {
const QStringList whitelistedUrlSchemes = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid global variables in general and global static non-POD types in particular. It is probably fine to have it as a local const in openBrowser, it's not on any hot path really.

src/gui/tray/UserModel.cpp Outdated
url = "https://" + _users[_currentUserId]->server(false);
}
QDesktopServices::openUrl(QUrl(url));
Utility::openBrowser(QUrl(url), nullptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed for that one because of the check in the block just before. You are right that this use of "contains" is surprising and "startsWith" would be more appropriate.

src/gui/guiutility.cpp
"https"
};
};

using namespace OCC;

Q_LOGGING_CATEGORY(lcUtility, "nextcloud.gui.utility", QtInfoMsg)

bool Utility::openBrowser(const QUrl &url, QWidget *errorWidgetParent)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's exactly one call of that function which doesn't pass nullptr for the second parameter. What about adding a commit before this one in your PR to have nullptr as default value for errorWidgetParent?

This way you could spare the nullptr everywhere else in your PR when openBrowser is called.

@allexzander allexzander force-pushed the fix-url-scheme-security-issue branch from 04695eb to 5c60812 Compare February 8, 2021 09:51
er-vin
er-vin reviewed Feb 8, 2021
View reviewed changes
Copy link
Member

@er-vin er-vin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one nitpick, looks good otherwise.

src/gui/guiutility.cpp Outdated
using namespace OCC;

Q_LOGGING_CATEGORY(lcUtility, "nextcloud.gui.utility", QtInfoMsg)

bool Utility::openBrowser(const QUrl &url, QWidget *errorWidgetParent)
{
const QStringList whitelistedUrlSchemes = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: rename to allowedUrlSchemes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@er-vin Ok. Will fix CI errors later, before merge...

@allexzander allexzander requested a review from er-vin February 8, 2021 12:12
@camilasan
Copy link
Member

/rebase

@allexzander
Default parameter nullptr widget for openBrowser.
e97b7d8 
Signed-off-by: allexzander <blackslayer4@gmail.com>
@github-actions github-actions bot force-pushed the fix-url-scheme-security-issue branch from b8a7d4b to 1dc98de Compare February 9, 2021 08:40
LukasReschke
LukasReschke reviewed Feb 9, 2021
View reviewed changes
src/gui/tray/UserModel.cpp Outdated
@@ -731,10 +732,11 @@ Q_INVOKABLE void UserModel::openCurrentAccountServer()
return;

QString url = _users[_currentUserId]->server(false);
if (!(url.contains("http://") || url.contains("https://"))) {
if (!url.startsWith("http")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if we can match a full scheme here. (in case someone has local handlers that start with http but may do other things.Unlikely, but you never know.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't @er-vin say something about it before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukasReschke Addressed.

@allexzander
Copy link
Contributor Author

/rebase
@camilasan
I guess we need to discuss the necessity of this command, and I'd vote for not doing it in the GitHub PR unless absolutely sure we are merging it immediately :)

@allexzander
Validate sensitive URLs to onle allow http(s) schemes.
013f3ce 
Signed-off-by: allexzander <blackslayer4@gmail.com>
@allexzander allexzander force-pushed the fix-url-scheme-security-issue branch from 8765134 to 013f3ce Compare February 9, 2021 13:00
@camilasan
Copy link
Member

/rebase
@camilasan
I guess we need to discuss the necessity of this command, and I'd vote for not doing it in the GitHub PR unless absolutely sure we are merging it immediately :)

I did think I was going to merge it right away until Lukas commented on it...

@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-2906-013f3cea70acfe7b701cb73c93744d5ff5c0c213-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@allexzander
Copy link
Contributor Author

/rebase
@camilasan
I guess we need to discuss the necessity of this command, and I'd vote for not doing it in the GitHub PR unless absolutely sure we are merging it immediately :)

I did think I was going to merge it right away until Lukas commented on it...

@camilasan I see. Sorry.

@allexzander allexzander merged commit 7f92d8d into master Feb 9, 2021
@allexzander allexzander deleted the fix-url-scheme-security-issue branch February 9, 2021 14:42
@er-vin
Copy link
Member

er-vin commented Feb 10, 2021

/backport to stable-3.1

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Feb 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukasReschke LukasReschke LukasReschke left review comments

@camilasan camilasan camilasan approved these changes

@FlexW FlexW Awaiting requested review from FlexW

@er-vin er-vin Awaiting requested review from er-vin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@allexzander @camilasan @nextcloud-desktop-bot @er-vin @LukasReschke