Exclude more invalid chars from files UI path #26461

Merged
merged 2 commits into from Oct 25, 2016

Projects

None yet

3 participants

@PVince81
Collaborator

Description

Prevent newlines and zero byte chars to be used in files UI URL and
redirect to root if one is detected.

Added additional hardening in case the request fails with 400 or the
XMLHttpRequest throw a DOMException, both can happen with invalid paths
as well.

Related Issue

https://nextcloud.com/security/advisory/?id=nc-sa-2016-010

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@DeepDiver1975 @Peter-Prochaska

@PVince81 PVince81 added this to the 9.2 milestone Oct 24, 2016
@mention-bot

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @butonic, @icewind1991 and @MorrisJobke to be potential reviewers.

apps/files/js/filelist.js
if (sections[i] === '..') {
return false;
}
}
+ var specialChars = decodeURIComponent('%00%0A');
+ for (i = 0; i < specialChars.length; i++) {
+ if (path.indexOf(specialChars[i]) >= 0) {
@PVince81
PVince81 Oct 24, 2016 Collaborator

I just remembered that this might not work in all browsers, some want `charAt' for strings... will adjust

@PVince81 PVince81 Exclude more invalid chars from files UI path
Prevent newlines and zero byte chars to be used in files UI URL and
redirect to root if one is detected.

Added additional hardening in case the request fails with 400 or the
XMLHttpRequest throw a DOMException, both can happen with invalid paths
as well.
38f8b3b
@PVince81
Collaborator

Please review @DeepDiver1975 @Peter-Prochaska @tomneedham @PhilippSchaffrath @felixheidecke

@PVince81 PVince81 added the app:files label Oct 24, 2016
@@ -1422,11 +1422,18 @@
_isValidPath: function(path) {
var sections = path.split('/');
- for (var i = 0; i < sections.length; i++) {
+ var i;
@phisch
phisch Oct 24, 2016 Contributor

why do you declare a local variable here if you are not using it outside the for loops?

@PVince81
PVince81 Oct 24, 2016 Collaborator

because I can't redeclare it in the other loop below.

I could of course leave the var inside the first loop and remove it in the second one. But then it would be less visible.

@phisch
phisch Oct 24, 2016 Contributor
for(i=0; i< 5; i++) {
    console.log(i);
}

for(i=3; i< 5; i++) {
    console.log(i);
}

works great, creates a variable in the scope of the loop.

@PVince81
PVince81 Oct 24, 2016 Collaborator

but then this variable bleeds into the global scope.

forvarbleed

We need to have var somewhere to limit it to the local function scope.

@PVince81
PVince81 Oct 24, 2016 Collaborator

Additionally if we had "use scrict" at the top of the file (we should do that at some point), not having a var would result in a JS error. (see http://stackoverflow.com/a/5717233)

apps/files/js/filelist.js
if (sections[i] === '..') {
return false;
}
}
+ var specialChars = [decodeURIComponent('%00'), decodeURIComponent('%0A')];
+ for (i = 0; i < specialChars.length; i++) {
+ if (path.indexOf(specialChars[i]) >= 0) {
@phisch
phisch Oct 24, 2016 Contributor

micro optimization but != -1 would be faster.

@PVince81
PVince81 Oct 24, 2016 Collaborator

okay, I didn't know that. Shouldn't it be rather !== -1 ?

@phisch
phisch Oct 24, 2016 Contributor

!== would be better, but doesn't really matter here

apps/files/js/filelist.js
+ }
+ );
+ } catch (e) {
+ console.error(e);
@phisch
phisch Oct 24, 2016 Contributor

Maybe only console.error if it's a DOMException? Like this e might be logged multiple times (depends on the further exception handling).

@PVince81
PVince81 Oct 24, 2016 Collaborator

I wanted to have it for all in case people submit bug reports where unknown exceptions appear.

But now I remember that I decided to rethrow so might not be needed.

@PVince81 PVince81 Minor fixes in valid path check
faba991
@PVince81
Collaborator

Fixed 2 of 3 comments. Please recheck.

@PVince81
Collaborator

In the light of my justifications, is this PR now acceptable ? @PhilippSchaffrath @DeepDiver1975

@phisch
Contributor
phisch commented Oct 25, 2016

👍

@PVince81 PVince81 merged commit c9cff46 into master Oct 25, 2016

4 checks passed

Scrutinizer 1 new issues
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the filelist-catchinvalidchars branch Oct 25, 2016
@PVince81
Collaborator

stable9.1: #26474

@PVince81
Collaborator

stable9: #26475

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment