Skip to content

vuln(NSWG-ECO-464): exceljs#395

Merged
lirantal merged 3 commits into
nodejs:masterfrom
lirantal:vuln/464
Sep 7, 2018
Merged

vuln(NSWG-ECO-464): exceljs#395
lirantal merged 3 commits into
nodejs:masterfrom
lirantal:vuln/464

Conversation

@lirantal

@lirantal lirantal commented Sep 1, 2018

Copy link
Copy Markdown
Member

No description provided.

@lirantal lirantal self-assigned this Sep 1, 2018
@yonjah

yonjah commented Sep 3, 2018

Copy link
Copy Markdown
Contributor

@lirantal I'm wondering what is the reason for marking this as an issue with exceljs.
I think @guyonroche was kind to include a minor mitigation for this issue exceljs/exceljs@9066cd8 (which only applies to XSS and can miss a few edge cases depending on the context it's being used).

But in the end it is the responsibility of whoever getting a user input to sanitize and validate it with regard to how it's being used and excel file or JSON I can't really see the difference.

In the same respect exceljs has a sql injection vulnerability -

sqlConnection.query(`SELECT * FROM user WHERE user_id=${workbook.getCell('A1').value}`);

nosql vulnerability

User.find(JSON.parse(workbook.getCell('A1').value))

And probably a few dozen other vulnerabilities in code that is careless about inputs.

@lirantal

lirantal commented Sep 3, 2018

Copy link
Copy Markdown
Member Author

@yonjah true that it isn't an inherent issue with the library itself and I couldn't agree more about context being key as with so many other vulnerabilities handling related to user input.

To this end, we indeed resolved the issue without requiring any fix to .value() or how it returns data, but instead added a helper method to draw security concern of users to that.

+@bl4de

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants