Skip to content

Commit

Permalink
[security] fix ethereum#1359, improve txData validation (ethereum#2092)
Browse files Browse the repository at this point in the history 
* fix ethereum#1359

* stricter validation

* add test case "shouldn't allow RegExp (possible XSS)"

* shorten test-case

* remove unnecessary regex flags

* improve error msg
  • Loading branch information
@luclu @alexvansande
luclu authored and alexvansande committed Apr 17, 2017
1 parent e528dbf commit 2575feb
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 32 deletions.
2 changes: 1 addition & 1 deletion modules/ipc/ipcProviderBackend.js
View file
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
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/, '')}`;

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
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;
};

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

0 comments on commit 2575feb

Please sign in to comment.