ETH-01-cure fixes #1414

frozeman · 2016-11-14T10:10:21Z

Fixes currently:

ETH-01-002
ETH-01-003
ETH-01-005

Questions here:
The question is do we want to avoid data:// URIs and what threat do they bring.

Note that these URIs are only prevent in the browserbar and as a possible full page load. Not as assets in a webpage

mention-bot · 2016-11-14T10:10:22Z

@frozeman, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexvandesande and @evertonfraga to be potential reviewers.

holiman · 2016-11-14T14:01:29Z

interface/client/lib/helpers/helperFunctions.js

+        url = url.replace(/^(javascript:)?(data:)?\/\//i, 'http:');
+    }
+
+    return url;


blacklist is not sufficient. javaScript:// is also valid, and maybe special characters such as %0A .

Also, why only handle strings?
If you expect anything other than strings, such as location-objects, you should filter them. If you expect string input, then cast the url to string and do the sanitation unconditionally. Otherwise, something like /javascript/ (regexp) or ["javascript://foo"] (array) or random object can be used to bypass this (depending on where the input comes from)

Sorry, I missed the /i . But javascript can still be broken up with tab or %0A. I'd rather see:

Robust URL parsing, either using DOM anchor element to parse URL (example: https://gist.github.com/jlong/2428561) or a proper URL parsing library (example: https://nodejs.org/api/url.html) .

Whitelist, only allowing the protocols we want to allow, instead of blacklisting a few.

I will look into a proper JS library. Currently the URL is never a location object, but i can add checks for them too.

Whitelisting would make sense if we fully know what people will use and currently we don't want to restrict unknown URIs. E.g. ftp, etc.
As it looks only JavaScript is semi-dangerous as it can spawn ui actions, but this js has also not added privileges than the webpage itself.
So removing JavaScript seems to be the most dangerous one, can you think of any other?

@holiman i changed it now to use the parser object form your gist, but concerning the whitelisting im not yet convinced if we are not blocking uses cases. This current sanatizer should prevent spoofing. We can discuss in the future if we want to add more

evertonfraga · 2016-11-14T19:53:45Z

Testing

ETH-01-003
ETH-01-002
ETH-01-005

holiman · 2016-11-15T07:08:20Z

interface/client/lib/helpers/helperFunctions.js

+        parser = url;
+    }
+
+    parser.protocol = parser.protocol.replace(/^(javascript:)?(data:)?(:)?/i, 'http:');


See playground example. The code as it is written function will replace "https://" urls with "http://"

This is (I think) because the matches all, due to there only being optional capture groups. See here for a more robust example.

And I still think that any input not explicitly supported, should be coerced into a string. Otherwise it does not get filtered.

Here's an example which is even more robust, although I'd still prefer whitelisting protos instead of blacklisting

I tested you version and they didn't cover everything.
I found the best way which is:

sanatizeUrl = function(url){ // make sure location objects are string too url = String(url); // remove whitespaces and tabs url = url.replace(/[\t\n\r\s]+/g, ''); // replace javascript:// data:// and // url = url.replace(/^((?:javascript)?(?:data)?[:\/\/]{1,3})/i, 'http://'); return url; };

Test yourself

What specifically did not work with the DOM-anchor method ?
I like that you now force it to be a string, but dislike that it's gone back to regexp. URL parsing is by no means trivial, so I'd still advocate a proper URL parsing library if the DOM is insufficient.

alexvandesande · 2016-11-15T13:02:41Z

interface/client/lib/helpers/helperFunctions.js

+    url = String(url);
+
+    url = url.replace(/[\t\n\r\s]+/g, '');
+    url = url.replace(/^((?:javascript)?(?:data)?[:\/\/]{1,3})/i, 'http://');


Shouldn't we do the opposite? Have only some allowed prefixes and remove all the rest?

There was a discussion above already. My point is that there are a lot of URI types and we don't want to restrict use cases. "Javascript:" is the most dangerous one. But even this cannot do any harm, besides annoy people or mislead them with alert windows.

alexvandesande · 2016-11-15T13:12:27Z

interface/client/lib/helpers/helperFunctions.js

+@method sanatizeUrl
+@param {String} url
+**/
+Helpers.sanatizeUrl = function(url){


It's "san_i_tize"

alexvandesande · 2016-11-15T13:15:13Z

interface/client/templates/views/tab.js

+        var titleFull = webview.getTitle(),
+            title;
+
+        if(titleFull && titleFull.length > 40) {


Why are you cropping it on javascript? This could be accomplished with css, using overflow:hidden; text-overflow: ellipsis; white-space:nowrap;. Word width is not the same as character count.

Because this text is used in a lot of places and it fixes: https://github.com/ethereum/security-issues/issues/14

The point is we don't want to add the tex overflow on any element where we use that.
Titles more than 40 characters are uncommon and for that i added the property "nameFull"

alexvandesande · 2016-11-15T13:17:06Z

modules/preloader/mistUI.js

@@ -5,7 +5,7 @@
 require('./include/common')('mist');
 const { ipcRenderer: ipc, remote, webFrame } = require('electron');
 const { Menu, MenuItem } = remote;
-const syncDb = require('../syncDb.js');
+const dbSync = require('../dbSync.js');


what's the difference between syncDB and dbSync?

Its just a name refactor. The reason is that now the files db.js and dbSync.js are next to each other, which helps with mental grouping.
Also we calle the nodeSync: nodeSync ;) so consistency.

This has actually northie todo with the vu.

alexvandesande · 2016-11-15T13:18:02Z

modules/ipc/ipcProviderBackend.js

@@ -337,14 +337,14 @@ class IpcProviderBackend {
        })
        .catch((err) => {

-            err = this._makeErrorResponsePayload(payload || {}, {
+            var returnErr = this._makeErrorResponsePayload(payload || {}, {


use let instead?

But thats actually part of another PR: #1421 i will change it there

holiman · 2016-11-15T13:54:27Z

interface/client/lib/helpers/helperFunctions.js

+    url = String(url);
+
+    url = url.replace(/[\t\n\r\s]+/g, '');
+    url = url.replace(/^((?:javascript)?(?:data)?[:\/\/]{1,3})/i, 'http://');


As I've said, I dislike using naive regexps for parsing URLs. I don't have a bypass, but here's a simple false positive:

sanitizeUrl("javascript/jquery.js"); -> http://jquery.js

So if it's ever used to filter/sanitize relative URLs, it will fail if for data and javascript folders.

lock · 2018-03-31T12:23:14Z

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

frozeman added 6 commits November 4, 2016 16:31

updated package elemeents

79ba4ee

Merge branch 'develop' of github.com:ethereum/mist into develop

4139564

Merge branch 'develop' of github.com:ethereum/mist into develop

4c79f8a

Merge branch 'develop' of github.com:ethereum/mist into develop

0135208

Merge branch 'develop' of github.com:ethereum/mist into develop

93d76fd

fixes ETH-01-002

f45a08a

frozeman added Status: In progress Status: Please review labels Nov 14, 2016

fixes ETH-01-003

611d370

frozeman force-pushed the ETH-01-cure branch from 2a2199c to 611d370 Compare November 14, 2016 12:53

fixes ETH-01-005

669051b

frozeman force-pushed the ETH-01-cure branch from eb4d5d7 to 669051b Compare November 14, 2016 13:24

holiman reviewed Nov 14, 2016

View reviewed changes

changes to 002, also parse location object if passed in

0144c16

evertonfraga mentioned this pull request Nov 14, 2016

i18n small fixes #1417

Closed

frozeman added 2 commits November 14, 2016 21:12

Merge branch 'develop' of github.com:ethereum/mist into develop

7fddec7

merged develop

46f4ad2

holiman suggested changes Nov 15, 2016

View reviewed changes

frozeman added 2 commits November 15, 2016 09:42

improved sanatizer

0b82be7

fixed max callstack error

828cd97

alexvandesande reviewed Nov 15, 2016

View reviewed changes

sanatizeUrl -> sanitizeUrl

b058d00

holiman reviewed Nov 15, 2016

View reviewed changes

alexvandesande approved these changes Nov 15, 2016

View reviewed changes

frozeman merged commit f58730e into develop Nov 15, 2016

luclu mentioned this pull request Nov 16, 2016

[i18n] add i18n strings for 'connect popup' #1424

Merged

evertonfraga removed Status: In progress Status: Please review labels Nov 16, 2016

evertonfraga added this to the 0.8.8 milestone Nov 16, 2016

lock bot locked and limited conversation to collaborators Mar 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ETH-01-cure fixes #1414

ETH-01-cure fixes #1414

frozeman commented Nov 14, 2016 •

edited by evertonfraga

Loading

mention-bot commented Nov 14, 2016

holiman Nov 14, 2016 •

edited

Loading

holiman Nov 14, 2016 •

edited

Loading

frozeman Nov 14, 2016

frozeman Nov 14, 2016

evertonfraga commented Nov 14, 2016 •

edited by alexvandesande

Loading

holiman Nov 15, 2016 •

edited

Loading

holiman Nov 15, 2016

frozeman Nov 15, 2016 •

edited

Loading

holiman Nov 15, 2016

alexvandesande Nov 15, 2016

frozeman Nov 15, 2016

alexvandesande Nov 15, 2016

frozeman Nov 15, 2016

alexvandesande Nov 15, 2016

frozeman Nov 15, 2016

frozeman Nov 15, 2016

alexvandesande Nov 15, 2016

frozeman Nov 15, 2016

alexvandesande Nov 15, 2016

frozeman Nov 15, 2016

frozeman Nov 15, 2016

holiman Nov 15, 2016

lock bot commented Mar 31, 2018

ETH-01-cure fixes #1414

ETH-01-cure fixes #1414

Conversation

frozeman commented Nov 14, 2016 • edited by evertonfraga Loading

mention-bot commented Nov 14, 2016

holiman Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

holiman Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evertonfraga commented Nov 14, 2016 • edited by alexvandesande Loading

holiman Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frozeman Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lock bot commented Mar 31, 2018

frozeman commented Nov 14, 2016 •

edited by evertonfraga

Loading

holiman Nov 14, 2016 •

edited

Loading

holiman Nov 14, 2016 •

edited

Loading

evertonfraga commented Nov 14, 2016 •

edited by alexvandesande

Loading

holiman Nov 15, 2016 •

edited

Loading

frozeman Nov 15, 2016 •

edited

Loading