Skip to content
This repository has been archived by the owner on Sep 5, 2020. It is now read-only.

[security] fix #1359, improve txData validation #2092

Merged
merged 8 commits into from
Apr 17, 2017
Merged

[security] fix #1359, improve txData validation #2092

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1cb0f5c
fix #1359
Mar 31, 2017
def7fca
stricter validation
Mar 31, 2017
b5bd301
Merge branch 'develop' into luclu_improve-tx-data-input-validation
luclu Apr 3, 2017
65a12ff
add test case "shouldn't allow RegExp (possible XSS)"
Apr 3, 2017
c8a8d14
shorten test-case
Apr 4, 2017
9e00574
remove unnecessary regex flags
Apr 6, 2017
cb2b5ad
improve error msg
Apr 11, 2017
c1336d5
Merge branch 'develop' into luclu_improve-tx-data-input-validation
alexvansande Apr 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/ipc/ipcProviderBackend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const Windows = require('../windows');


const ERRORS = {
INVALID_PAYLOAD: { code: -32600, message: 'Payload, or some of its content properties are invalid. Please check if they are valid HEX.' },
INVALID_PAYLOAD: { code: -32600, message: 'Payload, or some of its content properties are invalid. Please check if they are valid HEX with \'0x\' prefix.' },
METHOD_DENIED: { code: -32601, message: "Method \'__method__\' not allowed." },
METHOD_TIMEOUT: { code: -32603, message: "Request timed out for method \'__method__\'." },
TX_DENIED: { code: -32603, message: 'Transaction denied' },
Expand Down
9 changes: 5 additions & 4 deletions modules/ipc/methods/eth_sendTransaction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,13 @@ module.exports = class extends BaseProcessor {
try {
_.each(payload.params[0], (val, key) => {
// if doesn't have hex then leave
if (_.isString(val)) {

if (!_.isString(val)) {
throw this.ERRORS.INVALID_PAYLOAD;
} else {
// make sure all data is lowercase and has 0x
if (val) val = `0x${val.toLowerCase().replace('0x', '')}`;
if (val) val = `0x${val.toLowerCase().replace(/^0x/igm, '')}`;
Copy link
Member

Choose a reason for hiding this comment

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

in this context, the g and m flags seems to be redundant. As the regexp starts with ^.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point!


if (val.match(/[^0-9a-fx]/igm)) {
if (val.substr(2).match(/[^0-9a-f]/igm)) {
throw this.ERRORS.INVALID_PAYLOAD;
}
}
Expand Down
64 changes: 37 additions & 27 deletions tests/mocha-in-browser/spec/permissions-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,48 +1,48 @@
// implement chai's should interface
var expect = chai.expect;

describe("Permissions", function() {
describe('Permissions', function () {

describe('permissions', function() {
it('should be available', function() {
describe('permissions', function () {
it('should be available', function () {
expect(window.permissions).to.be.an('object', 'The permissions object was not present, please reload the test page');
expect(window.permissions.accounts).to.be.an('array', 'The permissions object was not present, please reload the test page');
});
});

describe('web3.eth.accounts', function() {
describe('web3.eth.accounts', function () {

it('should return an array [sync]', function() {
it('should return an array [sync]', function () {
var accounts = web3.eth.accounts;
expect(accounts).to.be.an('array');
});

it('should return an array [async]', function(done) {
web3.eth.getAccounts(function(e, accounts){
it('should return an array [async]', function (done) {
web3.eth.getAccounts(function (e, accounts) {
expect(e).to.be.null;
expect(accounts).to.be.an('array');

done();
});
});

it('should match the allowed accounts in the tabs permisssions, and don\'t contain coinbase [sync]', function(done) {
it('should match the allowed accounts in the tabs permisssions, and don\'t contain coinbase [sync]', function (done) {
var accounts = web3.eth.accounts;

expect(window.permissions.accounts.length).to.equal(accounts.length);
expect(window.permissions.accounts).to.not.include(window.coinbase);
accounts.forEach(function(account){
accounts.forEach(function (account) {
expect(window.permissions.accounts).to.include(account);
});

done();
});

it('should match the allowed accounts in the tabs permisssions, and don\'t contain coinbase [async]', function(done) {
web3.eth.getAccounts(function(e, accounts){
it('should match the allowed accounts in the tabs permisssions, and don\'t contain coinbase [async]', function (done) {
web3.eth.getAccounts(function (e, accounts) {
expect(window.permissions.accounts.length).to.equal(accounts.length);
expect(window.permissions.accounts).to.not.include(window.coinbase);
accounts.forEach(function(account){
accounts.forEach(function (account) {
expect(window.permissions.accounts).to.include(account);
});

Expand All @@ -51,20 +51,21 @@ describe("Permissions", function() {
});


it('should match the allowed accounts in the tabs permisssions, and don\'t contain coinbase [async, batch request]', function(done) {
it('should match the allowed accounts in the tabs permisssions, and don\'t contain coinbase [async, batch request]', function (done) {
var count = 1;

var callback = function(e, accounts){
var callback = function (e, accounts) {
expect(window.permissions.accounts.length).to.equal(accounts.length);
expect(window.permissions.accounts).to.not.include(window.coinbase);
accounts.forEach(function(account){
accounts.forEach(function (account) {
expect(window.permissions.accounts).to.include(account);
});

if(count === 2)
if (count === 2) {
done();
}

count++;
count += count;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you double count at each round?

};

var batch = web3.createBatch();
Expand All @@ -75,15 +76,15 @@ describe("Permissions", function() {

});

describe('web3.eth.coinbase', function() {
describe('web3.eth.coinbase', function () {

it('should be empty [sync]', function() {
it('should be empty [sync]', function () {
var coinbase = web3.eth.coinbase;
expect(coinbase).to.be.null;
});

it('should be empty [async]', function(done) {
web3.eth.getCoinbase(function(e, coinbase){
it('should be empty [async]', function (done) {
web3.eth.getCoinbase(function (e, coinbase) {
expect(e).to.be.null;
expect(coinbase).to.be.null;

Expand All @@ -92,17 +93,18 @@ describe("Permissions", function() {
});


it('should be empty [async, batch request]', function(done) {
it('should be empty [async, batch request]', function (done) {
var count = 1;

var callback = function(e, coinbase){
var callback = function (e, coinbase) {
expect(e).to.be.null;
expect(coinbase).to.be.null;

if(count === 2)
if (count === 2) {
done();
}

count++;
count += count;
};

var batch = web3.createBatch();
Expand All @@ -113,7 +115,15 @@ describe("Permissions", function() {

});

describe('web3 attributes', function() {
describe('web3 parameter validation', function () {

it('shouldn\'t allow RegExp (possible XSS)', function () {
var add = '0x0000000000000000000000000000000000000000';
expect(function () { web3.eth.sendTransaction({ from: add, to: add, data: new RegExp('') }); }).to.throw('Payload, or some of its content properties are invalid. Please check if they are valid HEX with \'0x\' prefix.');
});
});

describe('web3 attributes', function () {
it('shouldn\'t allow `admin`', function () {
expect(web3.admin).to.be.undefined;
});
Expand All @@ -123,7 +133,7 @@ describe("Permissions", function() {
expect(window.ipcRenderer).to.be.undefined;
});

it('should only contain allowed attributes', function (){
it('should only contain allowed attributes', function () {
var allowedAttributes = [
'_requestManager',
'bzz',
Expand Down